Re: [PATCH][Newcomer] t7004-tag: modernize the test script
From: Junio C Hamano <hidden>
Date: 2024-08-03 16:29:47
AbdAlRahman Gad [off-list ref] writes:
quoted
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.