From: Taylor Blau <me@ttaylorr•com>
To: Usman Akinyemi via GitGitGadget <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, Usman Akinyemi <usmanakinyemi202@gmail•com>
Subject: Re: [PATCH 2/3] t3404: replace test with test_line_count()
Date: Mon, 14 Oct 2024 17:35:45 -0400 [thread overview]
Message-ID: <Zw2OsWorAVL4hCB9@nand.local> (raw)
In-Reply-To: <e2cae7f3a510027864303fe91dcf447f63eb0873.1728774574.git.gitgitgadget@gmail.com>
On Sat, Oct 12, 2024 at 11:09:33PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail•com>
>
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail•com>
> ---
> t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a65783c47..2ab660ef30f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
> test_cmp expect2 file1 &&
> test "$(git diff --name-status |
> sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> - test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> - test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> + grep -v "^#" <.git/rebase-merge/done >actual &&
> + test_line_count = 4 actual &&
> + test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
You use 'test_line_count' in one instance here, but 'test 0 =' below.
You could use 'test_must_be_empty' to stick with our test_-helper
functions.
But I like that you used 'grep -c' here, so it may be better to match
the two like so:
test 4 $(grep -c -v "^#" <.git/rebase-merge/done) &&
test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
> @@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
> ) &&
> test $base = $(git rev-parse HEAD^) &&
> git show >output &&
> - count=$(grep NEVER output | wc -l) &&
> - test 0 = $count &&
> + ! grep NEVER output &&
> git checkout @{-1} &&
> git branch -D multi-fixup
> '
Hmm. Wasn't this modified by the previous step as well? Is there a
reason that these can't be combined to avoid a new intermediate state
that will be thrown away in the next step?
> @@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
> ) &&
> test $base = $(git rev-parse HEAD^) &&
> git show >output &&
> - count=$(grep ONCE output | wc -l) &&
> - test 1 = $count &&
> + grep ONCE output >actual &&
> + test_line_count = 1 actual &&
> git checkout @{-1} &&
> git branch -D conflict-fixup
I am not sure what the benefit of using test_line_count here is over
bare 'test'. Can you explain why you chose to use it here?
In the body of your patch above, you appear to suggest that using
test_line_count is more in the style of Git's current test practices. I
think that's true for cases like writing:
test_line_count = 1 actual
as opposed to:
test 1 = $(wc -l <actual)
Since the former doesn't have the gotcha that you must remember redirect
the input of 'wc -l' to avoid having the filename appear in the output,
and the former also ensures that the file exists, has better error
messages, etc.
But in the case where we're running 'grep -c' directly, it seems cleaner
to use bare test, since we're not writing the matches to a file on disk.
Thanks,
Taylor
next prev parent reply other threads:[~2024-10-14 21:35 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-12 23:09 [PATCH 0/3] R atoi Usman Akinyemi via GitGitGadget
2024-10-12 23:09 ` [PATCH 1/3] t3404: avoid losing exit status with focus on `git show` and `git cat-file` Usman Akinyemi via GitGitGadget
2024-10-14 21:29 ` Taylor Blau
2024-10-12 23:09 ` [PATCH 2/3] t3404: replace test with test_line_count() Usman Akinyemi via GitGitGadget
2024-10-14 21:35 ` Taylor Blau [this message]
2024-10-12 23:09 ` [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-13 9:42 ` Usman Akinyemi
2024-10-14 9:00 ` Phillip Wood
2024-10-14 15:56 ` Usman Akinyemi
2024-10-14 10:53 ` Patrick Steinhardt
2024-10-14 13:57 ` Phillip Wood
2024-10-14 14:00 ` Patrick Steinhardt
2024-10-14 14:55 ` Phillip Wood
2024-10-14 16:13 ` Usman Akinyemi
2024-10-14 16:26 ` Usman Akinyemi
2024-10-14 18:36 ` phillip.wood123
2024-10-15 15:17 ` Usman Akinyemi
2024-10-15 16:19 ` Taylor Blau
2024-10-16 17:58 ` Usman Akinyemi
2024-10-15 18:28 ` phillip.wood123
2024-10-16 9:20 ` Phillip Wood
2024-10-16 18:00 ` Usman Akinyemi
2024-10-17 11:56 ` Usman Akinyemi
2024-10-17 12:02 ` Patrick Steinhardt
2024-10-17 12:13 ` Usman Akinyemi
2024-10-14 16:03 ` Usman Akinyemi
2024-10-14 9:49 ` Phillip Wood
2024-10-14 10:06 ` Kristoffer Haugsbakk
2024-10-14 13:48 ` Phillip Wood
2024-10-14 18:20 ` Usman Akinyemi
2024-10-14 18:30 ` phillip.wood123
2024-10-17 11:16 ` Usman Akinyemi
2024-10-18 13:52 ` [PATCH v2 0/3] " Usman Akinyemi via GitGitGadget
2024-10-18 13:52 ` [PATCH v2 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-21 12:20 ` Patrick Steinhardt
2024-10-21 13:43 ` Usman Akinyemi
2024-10-21 16:24 ` Taylor Blau
2024-10-21 16:34 ` Usman Akinyemi
2024-10-18 13:52 ` [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-21 12:20 ` Patrick Steinhardt
2024-10-21 14:24 ` Usman Akinyemi
2024-10-21 16:34 ` Taylor Blau
2024-10-21 16:39 ` Usman Akinyemi
2024-10-21 18:00 ` Usman Akinyemi
2024-10-21 19:56 ` Taylor Blau
2024-10-30 15:20 ` Phillip Wood
2024-10-30 16:19 ` Usman Akinyemi
2024-10-31 9:58 ` Phillip Wood
2024-10-31 12:21 ` Usman Akinyemi
2024-11-06 6:05 ` Usman Akinyemi
2024-11-06 16:03 ` phillip.wood123
2024-10-18 13:53 ` [PATCH v2 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-21 12:20 ` Patrick Steinhardt
2024-10-21 12:27 ` Usman Akinyemi
2024-10-21 12:34 ` Patrick Steinhardt
2024-10-21 14:38 ` Usman Akinyemi
2024-10-21 16:35 ` Taylor Blau
2024-10-21 16:36 ` Usman Akinyemi
2024-10-22 13:43 ` Usman Akinyemi
2024-10-18 21:21 ` [PATCH v2 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Taylor Blau
2024-10-18 21:29 ` Usman Akinyemi
2024-10-18 21:35 ` Taylor Blau
2024-10-18 21:43 ` Usman Akinyemi
2024-10-22 5:23 ` [PATCH v3 " Usman Akinyemi via GitGitGadget
2024-10-22 5:23 ` [PATCH v3 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-22 16:21 ` Taylor Blau
2024-10-22 22:06 ` Usman Akinyemi
2024-10-22 5:23 ` [PATCH v3 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-22 5:23 ` [PATCH v3 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-22 22:08 ` [PATCH v4 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-22 22:08 ` [PATCH v4 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-22 22:08 ` [PATCH v4 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-22 22:08 ` [PATCH v4 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-23 6:05 ` Patrick Steinhardt
2024-10-23 7:40 ` Usman Akinyemi
2024-10-23 7:40 ` [PATCH v5 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Usman Akinyemi via GitGitGadget
2024-10-23 7:40 ` [PATCH v5 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-23 20:31 ` Taylor Blau
2024-10-24 0:23 ` Usman Akinyemi
2024-10-23 7:40 ` [PATCH v5 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-23 20:32 ` Taylor Blau
2024-10-24 0:23 ` Usman Akinyemi
2024-10-23 7:40 ` [PATCH v5 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-23 8:52 ` [PATCH v5 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Patrick Steinhardt
2024-10-23 20:33 ` Taylor Blau
2024-10-24 0:25 ` Usman Akinyemi
2024-10-24 0:24 ` [PATCH v6 " Usman Akinyemi via GitGitGadget
2024-10-24 0:24 ` [PATCH v6 1/3] daemon: " Usman Akinyemi via GitGitGadget
2024-10-24 0:24 ` [PATCH v6 2/3] merge: replace atoi() with strtol_i() for marker size validation Usman Akinyemi via GitGitGadget
2024-10-24 0:24 ` [PATCH v6 3/3] imap: replace atoi() with strtol_i() for UIDVALIDITY and UIDNEXT parsing Usman Akinyemi via GitGitGadget
2024-10-24 18:03 ` [PATCH v6 0/3] parse: replace atoi() with strtoul_ui() and strtol_i() Taylor Blau
2024-10-25 5:06 ` Patrick Steinhardt
2024-10-25 6:11 ` Usman Akinyemi
2024-10-25 14:44 ` Taylor Blau
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=Zw2OsWorAVL4hCB9@nand.local \
--to=me@ttaylorr$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=usmanakinyemi202@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox