All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: AbdAlRahman Gad <abdobngad@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v5 0/8] [Newcomer] t7004: modernize the style
Date: Thu, 08 Aug 2024 08:42:43 -0700	[thread overview]
Message-ID: <xmqq5xsbkolo.fsf@gitster.g> (raw)
In-Reply-To: <20240807130259.28381-1-abdobngad@gmail.com> (AbdAlRahman Gad's message of "Wed, 7 Aug 2024 15:58:36 +0300")

AbdAlRahman Gad <abdobngad@gmail.com> 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.



  parent reply	other threads:[~2024-08-08 15:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-07 12:58 [PATCH v5 0/8] [Newcomer] t7004: modernize the style AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 1/8] t7004: remove space after redirect operators AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 2/8] t7004: one command per line AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 3/8] t7004: do not prepare things outside test_expect_success AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 4/8] t7004: use indented here-doc AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 5/8] t7004: description on the same line as test_expect_success AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 6/8] t7004: test description and test body seperated with backslash AbdAlRahman Gad
2024-08-07 12:58 ` [PATCH v5 7/8] t7004: use single quotes instead of double quotes AbdAlRahman Gad
2024-08-08 15:36   ` Junio C Hamano
2024-08-07 12:58 ` [PATCH v5 8/8] t7004: make use of write_script AbdAlRahman Gad
2024-08-08 15:42 ` Junio C Hamano [this message]
2024-08-08 16:18   ` [PATCH v5 0/8] [Newcomer] t7004: modernize the style AbdAlRahman Gad

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq5xsbkolo.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=abdobngad@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.