On Fri, Feb 1, 2019 at 4:03 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> > On Fri, Feb 1, 2019 at 11:22 AM Jiang Xin <worldhello.net@xxxxxxxxx> wrote:
> >> +# Note: DO NOT run it in a subshell, otherwise the variables will not be set
> >
> > Which variables won't be set? It's not clear what this restriction is about.
>
> >> +# Note: DO NOT run it in a subshell, otherwise the variables will not be set
> >> +create_pack_1 () {
> >> + P1=$(git -C "$master_repo/objects/pack" pack-objects -q pack <<-EOF
> >
> > Which variables? Note that you can capture output of a subshell into a
> > variable, if necessary.
>
> These helper functions set a bunch of variables $P1, $P2, etc. as
> well as variables whose name begin with P and followed by 40-hex.
> The script wants to use them later when preparing expected output,
> and with the most natural way to organize the code, that "later"
> happens in the process that would have spawned a subshell to run
> this function.
>
> It would have been easier for you to grok if the note instead said
> "this function sets two global shell variables" or something,
> perhaps? Such a variable would certainly not be visible if this
> function is called inside a subshell to the main process.
Yes, better function comments would facilitate comprehension both for
the reviewer and those working on the code in the future. For
instance:
# Create commit for each argument [...with blah properties...] and
# assign [...] to shell variable of same name as argument.
# NOTE: Avoid calling this function from a subshell since variable
# assignments will disappear when subshell exits.
or something.
But, looking more closely at the patch, I'm wondering why the various
create_pack_#() functions are defined at all since they are each only
ever called from a single place.
create_pack_4 () {
...
eval P$P4=P4:$P4
}
...
test_expect_success 'create pack 4, 5' '
create_pack_4 && create_pack_5
'
I haven't been able to convince myself that this helps readability --
especially since the function definition is often far removed from the
single point of use -- over merely inlining the function body directly
in the sole test which calls it.
Anyhow, this is all pretty minor, not necessarily worth a re-roll.
