On Mon, Sep 17, 2018 at 6:30 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore <matvore@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> > ---
> > 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.
