Re: Re: [PATCH] ls-files: update test style

["lilinchao@oschina.cn" <lilinchao@oschina.cn>]

>
>On Thu, Jun 23 2022, Li Linchao via GitGitGadget wrote:
>
>> From: Li Linchao <lilinchao@oschina.cn>
>>
>> Update test style in t/t30[*].sh for uniformity, that's to
>> keep test title the same line with helper function itself.
>
>We have a few of these sorts of old style tests, and it's good to update
>them. 
Yes. Currently there are at least 400+ old style tests :). It not easy for me
to fix them all at once with some magic regex expressions, so I'm not going
to update them all in one patch. But I think, first of all, we can explicitly
document which test style we prefer first.
>
>>     Write test code like this:
>> diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
>> index 48cec4e5f88..76361b92336 100755
>> --- a/t/t3001-ls-files-others-exclude.sh
>> +++ b/t/t3001-ls-files-others-exclude.sh
>> @@ -67,26 +67,26 @@ echo '!*.2
>> 
>>  allignores='.gitignore one/.gitignore one/two/.gitignore'
>> 
>> -test_expect_success \
>> -    'git ls-files --others with various exclude options.' \
>> -    'git ls-files --others \
>> +test_expect_success 'git ls-files --others with various exclude options.' '
>> +	git ls-files --others \
>>         --exclude=\*.6 \
>>         --exclude-per-directory=.gitignore \
>>         --exclude-from=.git/ignore \
>>         >output &&
>
>This though really stops too short, here we end up with:
>
>	<TAB>git-ls-files --others \
>	<7 spaces>--exclude [...]
> 
OK.
>> -     test_cmp expect output'
>> +     test_cmp expect output
>
>And you've space-indented this test_cmp, presumably the below has the
>same issues (I didn't check in detail) 
>
>Instead the argument lists should be <TAB><TAB> indented, and the rest
>should be TAB indented. 
OK.
>
>> +'
>> 
>>  # Test \r\n (MSDOS-like systems)
>>  printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
>> 
>> -test_expect_success \
>> -    'git ls-files --others with \r\n line endings.' \
>> -    'git ls-files --others \
>> +test_expect_success 'git ls-files --others with \r\n line endings.' '
>> +	git ls-files --others \
>>         --exclude=\*.6 \
>>         --exclude-per-directory=.gitignore \
>>         --exclude-from=.git/ignore \
>>         >output &&
>> -     test_cmp expect output'
>> +     test_cmp expect output
>> +'
>
>Aside from the above I think it's also worth incorporating all the
>"printf", "echo", "cat" etc. that we do into the "test_expect_success"
>themselves, and if they're needed by more than one test perhaps make
>them a "setup" helper function (which would test_when_finished "rm -f
>.gitignore" clean up after itself). 
Yes, make sense.
>
>That's obviously bigger than some whitespace changes, so we could punt
>on it for now, but as we're looking at this anyway we could convert
>fully to a more modern style in a follow-up commit...
>
>> -test_expect_success \
>> -    'git ls-files with path restriction with --.' \
>> -    'git ls-files --others -- path0 >output &&
>> +test_expect_success 'git ls-files with path restriction with --.' '
>> +    git ls-files --others -- path0 >output &&
>>  test_cmp output - <<EOF
>>  path0
>>  EOF
>>  '
>
>On the topic of leaving things on the table: here we could use "<<-EOF"
>(or actually better "<<-\EOF") instead, and indent the here-doc, as we
>usually do. 
OK, will do.