Re: [PATCH v2 5/6] tests: split up pipes

From: Matthew DeVore <matvore@google.com>
Date: Sep 19, 2018, 02:33 UTC
Message-ID: <CAMfpvh+0_x4UbPHK9jJoeN6rgZOS7F6iq7u_LRMP0YssMOZynw@mail.gmail.com>
In-Reply-To: <CAPig+cQchHoWffobw3iGrsmRRPx2LKNu+EvR=t4c5RcV9Cmi9g@mail.gmail.com>

On Mon, Sep 17, 2018 at 6:30 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 19 quoted lines

>
> On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore <matvore@google.com> wrote:
> > tests: split up pipes
>
> This title explains the mechanical changes the patch is making but not
> the intent. Perhaps reword it to say something like:
>
>     tests: avoid swallowing Git exit code upstream of a pipe
>
> > Some pipes in tests lose the exit code of git processes, which can mask
> > unexpected behavior. Split these pipes up so that git commands are at
> > the end of pipes rather than the beginning or middle.
>
> Can you say something about how you chose which tests to fix in this
> patch? Is this fixing all such cases or only a subset? It looks like
> it's only fixing "ls-files" and "verify-pack" invocations. If that's
> the case, the commit message should explain that.
>
> Also, missing sign-off.

Fixed - here is the new commit message (I changed the wording of your header to fit within 50 chars):

    tests: don't swallow Git errors upstream of pipes

    Some pipes in tests lose the exit code of git processes, which can mask
    unexpected behavior like crashes. Split these pipes up so that git
    commands are only at the end of pipes rather than the beginning or
    middle.

    The violations fixed in this patch were found in the process of fixing
    pipe placement in a prior patch.

    Signed-off-by: Matthew DeVore <matvore@google.com>

Show 14 quoted lines

>
> > ---
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > @@ -51,8 +51,10 @@ pull_to_client () {
> > -                       git symbolic-ref HEAD refs/heads/$(echo $heads |
> > -                       sed -e "s/^\(.\).*$/\1/") &&
> > +                       git symbolic-ref HEAD refs/heads/$(
> > +                               echo $heads |
> > +                               sed -e "s/^\(.\).*$/\1/"
> > +                       ) &&
>
> Why is this change included in the patch? There is no Git invocation
> upstream of a pipe here. While the cleanup itself may be desirable, it
> doesn't belong in this patch.

This actually should have been part of the "tests: standardize pipe placement" patch, but I did "git commit --fixup=SHA" with the wrong SHA. Fixed.