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

[Derrick Stolee <stolee@gmail.com>]

On 7/20/2018 2:09 PM, Eric Sunshine wrote:
> 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 &&

This approach seems fine to me. I'd hate to be in the case where 
rev-parse reports an error, terminating early, resulting in an incorrect 
expected file, and then having the test pass because the code is 
similarly incorrect. No matter how slim the chances are, I want to avoid 
a false positive there.

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

Since the merge-base algorithms provide the commits in an order that 
depends on the implementation (not the functional contract), we decided 
to sort the output commit ids in the output of 'test-tool reach 
<method>'. Thus, we sort the rev-parse output to match.

Thanks,

-Stolee