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