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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> ---
> 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.