Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite

> Thanks for the patch. See comments below...
>
> On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
>> This patch removes the necessity of pipes in git related commands for test suite.
>>
>> Exit code of the upstream in a pipe is ignored so, it's use should be avoided. The fix for this is to write the output of the git command to a file and test the exit codes of both the commands being linked by pipe.
>
> Please wrap commit messages to fit in about 72 columns; this one is
> far too wide.
>
> On the Git project, commit messages are written in imperative mood, as
> if telling the codebase to "do something". So, instead of writing
> "This patch removes...", you could word it "Remove..." or "Avoid...".
>
> It's misleading to say that the patch "removes the _necessity_ of
> pipes" since pipes were not used out of necessity; they were probably
> just a convenience and seemed reasonable at the time, but later
> experience has shown that they can be problematic for the reason you
> give in the second paragraph.
>
> Taking these observations into consideration, perhaps you could
> rewrite the commit message something like this:
>
>     Avoid using pipes downstream of Git commands since the exit codes
>     of commands upstream of pipes get swallowed, thus potentially
>     hiding failure of those commands. Instead, capture Git command
>     output to a file apply the downstream command(s) to that file.
>
> More comments below...

>> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
>> ---
>> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
>> @@ -116,10 +116,10 @@ test_expect_success \
>>  test_expect_success \
>>      'checking the commit' \
>> -    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
>> -     grep "^R100..*path0/COPYING..*path2/COPYING" &&
>> -     git diff-tree -r -M --name-status  HEAD^ HEAD | \
>> -     grep "^R100..*path0/README..*path2/README"'
>> +    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>> +     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
>> +     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
>> +     grep "^R100..*path0/README..*path2/README" actual'
>
> Although this "mechanical" transformation is technically correct, it
> is nevertheless wasteful. The exact same "git diff-tree ..." command
> is run twice, and both times output is captured to file 'actual',
> which makes the second invocation superfluous. Instead, a better
> transformation would be:
>
>     git diff-tree ... >actual &&
>     grep ... actual &&
>     grep ... actual
>
> The same observation applies to other transformations in this patch.

>> diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
>> @@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
>>  test_expect_success "track multi-parent paths" '
>>         svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
>>         git svn multi-fetch &&
>> -       test $(git cat-file commit refs/remotes/glob | \
>> -              grep "^parent " | wc -l) -eq 2
>> +       test $(git cat-file commit refs/remotes/glob >actual &&
>> +              grep "^parent " actual | wc -l) -eq 2
>>         '
>
> This is not a great transformation. If "git cat-file" fails, then
> neither 'grep' nor 'wc' will run, and the result will be as if 'test'
> was called without an argument before "-eq". For example:
>
>     % test $(false >actual && grep "^parent " actual | wc -l) -eq 2
>     test: -eq: unary operator expected
>
> It would be better to run "git cat-file" outside of "test $(...)". For instance:
>
>     git cat-file ... >actual &&
>     test $(grep ... actual | wc -l) -eq 2
>
> Alternately, you could take advantage of the test_line_count() helper function:
>
>     git cat-file ... >actual &&
>     grep ... actual >actual2 &&
>     test_line_count = 2 actual2