[v5,0/8,Newcomer] t7004: modernize the style

Message

AbdAlRahman Gad Aug. 7, 2024, 12:58 p.m. UTC

- Remove whitespace after the redirect operators.

- Move number of expect files prepared outside of
  test_expect_success to be inside the tests that use it.

- Split some lines that have two commands into two lines
  one command each.

- Turn some "<<\EOF" into "<<-\EOF" and indent their body.

- Avoid using pipes in the output from "test-tool ref-store"
  and write the output to a file.

- Change test_expect_success that are seperated from its name
  to be on the same line.

- Avoid separating test Description and test body with backslash

- Use single quotes instead of double quotes for test description and
  body.

- Use write_script which takes care of emitting the `#!/bin/sh` line
  and the `chmod +x`.

There are still tests that could lose exit status to pipe. This needs
to be modernized too, I will fix them in another patch series.

AbdAlRahman Gad (8):
  t7004: remove space after redirect operators
  t7004: one command per line
  t7004: do not prepare things outside test_expect_success
  t7004: use indented here-doc
  t7004: description on the same line as test_expect_success
  t7004: test Description and test body seperated with backslash
  t7004: use single quotes instead of double quotes
  t7004: make use of write_script

 t/t7004-tag.sh | 1144 +++++++++++++++++++++++-------------------------
 1 file changed, 549 insertions(+), 595 deletions(-)


base-commit: 406f326d271e0bacecdb00425422c5fa3f314930

Comments

Junio C Hamano Aug. 8, 2024, 3:42 p.m. UTC | #1

AbdAlRahman Gad <[e-mail]> writes:

> - Remove whitespace after the redirect operators.
>
> - Move number of expect files prepared outside of
>   test_expect_success to be inside the tests that use it.
>
> - Split some lines that have two commands into two lines
>   one command each.
>
> - Turn some "<<\EOF" into "<<-\EOF" and indent their body.
>
> - Avoid using pipes in the output from "test-tool ref-store"
>   and write the output to a file.
>
> - Change test_expect_success that are seperated from its name
>   to be on the same line.
>
> - Avoid separating test Description and test body with backslash
>
> - Use single quotes instead of double quotes for test description and
>   body.
>
> - Use write_script which takes care of emitting the `#!/bin/sh` line
>   and the `chmod +x`.

I gave another look and they all looked fine.  Except the title of
one step that said

    t7004: test Description and test body seperated with backslash

was a "Huh?  what does it even mean?".

    t7004: begin the test body on the same line as test_expect_success

or something?  I dunno.

> There are still tests that could lose exit status to pipe. This needs
> to be modernized too, I will fix them in another patch series.

;-)

Another one that I noticed is that we have quite a lot of

	cat >expect <<-EOF &&
	v1.1.3
	v2.0
	v3.0
	EOF

that can be shortend to

	test_write_lines >expect v1.1.3 v2.0 v3.0 &&

To use without extra quoting, test_write_lines is more limited, but
the majority of here-doc used for expect files in this test are
enumeration of tag names that we can write without any extra frills,
and test_write_lines may be a very good fit for these use cases.

Thanks.

AbdAlRahman Gad Aug. 8, 2024, 4:18 p.m. UTC | #2

On 8/8/24 18:42, Junio C Hamano wrote:
> AbdAlRahman Gad <[e-mail]> writes:
> 
>> - Remove whitespace after the redirect operators.
>>
>> - Move number of expect files prepared outside of
>>    test_expect_success to be inside the tests that use it.
>>
>> - Split some lines that have two commands into two lines
>>    one command each.
>>
>> - Turn some "<<\EOF" into "<<-\EOF" and indent their body.
>>
>> - Avoid using pipes in the output from "test-tool ref-store"
>>    and write the output to a file.
>>
>> - Change test_expect_success that are seperated from its name
>>    to be on the same line.
>>
>> - Avoid separating test Description and test body with backslash
>>
>> - Use single quotes instead of double quotes for test description and
>>    body.
>>
>> - Use write_script which takes care of emitting the `#!/bin/sh` line
>>    and the `chmod +x`.
> 
> I gave another look and they all looked fine.  Except the title of
> one step that said
> 
>      t7004: test Description and test body seperated with backslash
> 
> was a "Huh?  what does it even mean?".
> 
>      t7004: begin the test body on the same line as test_expect_success
> 
> or something?  I dunno.
> 

Thanks! I'll send a v6 right away.

>> There are still tests that could lose exit status to pipe. This needs
>> to be modernized too, I will fix them in another patch series.
> 
> ;-)
> 
> Another one that I noticed is that we have quite a lot of
> 
> 	cat >expect <<-EOF &&
> 	v1.1.3
> 	v2.0
> 	v3.0
> 	EOF
> 
> that can be shortend to
> 
> 	test_write_lines >expect v1.1.3 v2.0 v3.0 &&
> 
> To use without extra quoting, test_write_lines is more limited, but
> the majority of here-doc used for expect files in this test are
> enumeration of tag names that we can write without any extra frills,
> and test_write_lines may be a very good fit for these use cases.
> 
> Thanks.

I'll work on them the next patch series.