On Sat, Oct 12, 2024 at 11:09:33PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
