Re: [PATCH][Newcomer] t7004-tag: modernize the test script

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2024, 16:29 UTC
Message-ID: <xmqqmslt7eob.fsf@gitster.g>
In-Reply-To: <dcded6a3-e284-4d52-b36f-dacc056bbc5b@gmail.com>

AbdAlRahman Gad <abdobngad@gmail.com> writes:

Show 21 quoted lines

>> It is a good start.  There are other modernization opportunities in
>> this file, though.
>>   - Output from "test-tool ref-store" piped to "sed" means the exit
>>     status from an abnormal exit of "test-tool" is hidden.  They
>>     should be split into two commands.
>>   - Expected output file prepared outside test_expect_success that
>>     uses it.
>>   - Here-doc that does not interpolate leaving the EOF marker
>>     unquoted.
>
> Thanks for the review. I've just sent a follow-up v2 patch fixing the
> things you mentioned. I also found other issues like:
>
> some test_expect_success are seperated from its name like:
>
>    test_expect_success \
>    	'trying to delete tags without params should succeed and do nothing' '
>
> but I preferred to send the patch first as it was getting very long
> and I also wanted to make sure that I am on the right path and not
> just fixing unrelated things.

To deal with "is this getting too long?" you can split this into a series of multiple patches, e.g.

    [PATCH 0/n] t7004: modernize the style
      cover letter that gives an overview of the series

    [PATCH 1/n] t7004: description on the same line as test_expect_success
      the one you pointed out above

    [PATCH 2/n] t7004: redirection operator
      the patch you sent earlier

    [PATCH 3/n] t7004: do not lose exit status to pipe
      split "test-tool ... | sed" pipeline into two commands
      to avoid losing exit status from test-tool

    [PATCH 4/n] t7004: one command per line
      fix lines like these:
	git tag -l >actual && test_cmp expect actual &&
      to
	git tag -l >actual &&
	test_cmp expect actual &&

    [PATCH 5/n] t7004: here-doc modernization
      use <<-EOF or <<-\EOF to indent here-doc
      use \EOF not EOF when not interpolating

    [PATCH 6/n] t7004: do not do things outside test_expect_success
      do not prepare expect and other things outside test_expect_success

would make a thorough series, while keeping each step still reasonably short, I suspect.