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

> On Fri, Jul 20, 2018 at 1:20 PM Derrick Stolee <dstolee@microsoft.com> 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?