| Message ID | e2cae7f3a510027864303fe91dcf447f63eb0873.1728774574.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 19c291e5b2427b717869be3732822e33eb5b4f83 |
| Headers | show |
| Series | R atoi | expand |
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
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) ' test_expect_success 'show conflicted patch' ' @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' ' ) && 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 ' test_expect_success 'multi-fixup does not fire up editor' ' @@ -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 ' @@ -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 ' @@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep TWICE output | wc -l) && - test 2 = $count && + grep TWICE output >actual && + test_line_count = 2 actual && git checkout @{-1} && git branch -D conflict-squash ' @@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' ' ) && 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 skip-comments ' @@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' ' ) && 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 skip-blank-lines '