Re: [PATCH v5 10/14] t5520: test single-line files by git with test_cmp

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > In case an invocation of a Git command fails within the subshell, the
> > failure will be masked. Replace the subshell with a file-redirection and
> > a call to test_cmp.
> 
> I.e.
> 
>     test "$(git cmd args)" = "expected-string"
> 
> =>
> 
>     git cmd args >actual && echo "expected-string" >expect &&
>     test_cmp expect actual
> 
> which makes sense.  It may break if expected-string begins with a
> dash or something silly like that, but a quick eyeballing over the
> patch tells me that we are safe there.
> 
> Technically, "$(git cmd args)" used as a command line option of
> another command is called "command substitution", not "subshell".
> The proposed log message may need to be updated.

> 
> > This change was done with the following GNU sed expressions:
> >
> > 	s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
> > 	s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/
> >
> > A future patch will clean up situations where we have multiple duplicate
> > statements within a test case. This is done to keep this patch purely
> > mechanical.
> 
> OK.  One thing that worries me is if the existing tests are not
> expecting (no pun intended) to see 'expect' or 'actual' (e.g. if
> they somehow rely on output of "ls-files -u", we are now adding two
> untracked files in the working tree).

> Another is if the git command
> is expected to produce nothing, possibly after failing, and the test
> is expecting to see an empty string---in such a case, the hiding of
> the exit status would have been intentional ;-)  We'd want to be sure
> that we aren't breaking the tests like that by reading through the
> result of applying this patch.
> 
> Since this is just a single file, I trust you have already done such
> sanity checking ;-)