Re: [PATCH v9 1/6] t5323: test cases for git-pack-redundant

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 1, 2019, 21:49 UTC
Message-ID: <CAPig+cQh5TDKVaDi0gg9LZTo1Og_Qw6S2sH9cPABR9q05gEUfg@mail.gmail.com>
In-Reply-To: <xmqqva23xlun.fsf@gitster-ct.c.googlers.com>

On Fri, Feb 1, 2019 at 4:03 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Fri, Feb 1, 2019 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> 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.