Hi Junio, On Tue, Nov 12, 2019 at 02:17:33PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@xxxxxxxxx> 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. Okay, I'll send out a new reroll with some log message cleanup. > > > 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). Do you mean `ls-files -o`? I think `-u` means "unmerged" while `-o` means "others" (or "untracked"). This test doesn't have any instances of `-o` being used. > 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 ;-) Thanks for double-checking on me. I have, indeed, manually verified the changes. > > The mechanical conversion procedure itself looks OK. > > Thanks.
