Re: [PATCH v2 00/18] Consolidate reachability logic

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 20, 2018, 18:09 UTC
Message-ID: <CAPig+cRRa9gcvfu9jKy-+DK+Jjj6maCabuOyprVQMeGx4uJrRg@mail.gmail.com>
In-Reply-To: <20180720171836.106091-1-dstolee@microsoft.com>

On Fri, Jul 20, 2018 at 1:20 PM Derrick Stolee <dstolee@microsoft.com> wrote:

Show 18 quoted lines

> Here is the diff between v1 and v2.
>
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> @@ -67,142 +67,176 @@ test_three_modes () {
>  test_expect_success 'get_merge_bases_many' '
>         cat >input <<-\EOF &&
> +       A:commit-5-7
> +       X:commit-4-8
> +       X:commit-6-6
> +       X:commit-8-3
>         EOF
>         {
> -               printf "get_merge_bases_many(A,X):\n" &&
> -               git rev-parse commit-5-6 &&
> -               git rev-parse commit-4-7
> +               echo "get_merge_bases_many(A,X):" &&
> +               git rev-parse commit-5-6 \
> +                             commit-4-7 | sort

Pipes lose the exit code of the all upstream commands. When a Git command is upstream, we'd usually recommend to dump its output to a file, then use the file as input to the rest of the pipe so as not to lose the Git command's exit code:

    {
        ...
        git rev-parse ... >oids &&
        sort <oids
    } >expect &&

One could argue, in this case, that if git-rev-parse crashes, then it won't have the expected output and the test will fail anyhow despite not seeing its failed exit code. However, git-rev-parse might crash _after_ emitting all the normal, expected output, and that crash would be missed altogether, so avoiding git-rev-parse as a pipe upstream is a good idea.

However, one could argue that argument by saying that it isn't the job of this particular test script to check git-rev-parse's behavior, so crashy git-rev-parse ought to be caught elsewhere by some other test script. Nevertheless, you'll likely encounter reviewers who don't want to see git-rev-parse upstream, even with that argument.

Anyhow, why is that 'sort' even there? It wasn't needed in the original. Is git-rev-parse outputting the OID's in random order?

Show 11 quoted lines

>         } >expect &&
>         test_three_modes get_merge_bases_many
>  '
>
>  test_expect_success 'reduce_heads' '
>         [...]
> +               git rev-parse commit-5-1 \
> +                             commit-4-4 \
> +                             commit-3-6 \
> +                             commit-2-8 \
> +                             commit-1-10 | sort

Ditto.

>         } >expect &&
>         test_three_modes reduce_heads
>  '