On Fri, Jul 20, 2018 at 1:20 PM Derrick Stolee <dstolee@xxxxxxxxxxxxx> wrote:
> 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?
> } >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
> '
