Thread (1 message) 1 message, 1 author, 2024-08-03

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help