AbdAlRahman Gad <abdobngad@xxxxxxxxx> writes:
>> 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.
