| Message ID | pull.1805.v3.git.git.1728230769.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | t3404: avoid losing exit status to pipes | expand |
Hello, Kindly, help take a look if this is okay now. Also, I wanted to change this also to use test_line_count, test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo) But, I tried a different approach and the test kept failing. Similar as git show >output && count=$(grep NEVER output | wc -l) && test 0 = $count && Thank you. On Sun, Oct 6, 2024 at 4:06 PM Usman Akinyemi via GitGitGadget <gitgitgadget@gmail.com> wrote: > > At the beginning of my task, I made the mistake of submitting two patches > for two separate commits instead of one. The first patch addressed the issue > of losing the Git exit status due to pipes. > > After submitting the first patch, I noticed that the output of wc -l was > failing due to trailing whitespace. I attempted to fix this by using tr -d > to remove the whitespace. However, instead of squashing the two patches into > one, I inadvertently created another commit. > > Eric Sunshine sunshine@sunshineco.com provided valuable feedback during the > review process. He explained the details of the patches to me and pointed > out that using tr -d was unnecessary to resolve the whitespace issue. > > The root cause of the whitespace issue was quoting $count in the test > command, which led to the inclusion of whitespace in the comparison. By > removing the quotes around $count, the comparison works as expected without > the need for tr -d. > > Signed-off-by: Usman Akinyemi > > Usman Akinyemi (2): > t3404: avoid losing exit status with focus on `git show` and `git > cat-files` > [Outreachy][Patch v1] t3404: employing test_line_count() to replace > test > > t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++++++------------ > 1 file changed, 50 insertions(+), 24 deletions(-) > > > base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1805%2FUnique-Usman%2Favoid_git_pipes-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1805/Unique-Usman/avoid_git_pipes-v3 > Pull-Request: https://github.com/git/git/pull/1805 > > Range-diff vs v2: > > 1: be5a691e96f ! 1: c9a0cca179b [Outreachy][Patch v1] t3404: avoid losing exit status to pipes > @@ Metadata > Author: Usman Akinyemi <usmanakinyemi202@gmail.com> > > ## Commit message ## > - [Outreachy][Patch v1] t3404: avoid losing exit status to pipes > + t3404: avoid losing exit status with focus on `git show` and `git cat-files` > > The exit code of the preceding command in a pipe is disregarded. So > if that preceding command is a Git command that fails, the test would > not fail. Instead, by saving the output of that Git command to a file, > and removing the pipe, we make sure the test will fail if that Git > - command fails. > + command fails. This particular patch focuses on some of the instances > + which include `git show` and `git cat-files`. > > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com> > > -: ----------- > 2: 37b1411ee2c [Outreachy][Patch v1] t3404: employing test_line_count() to replace test > > -- > gitgitgadget
On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote: > > The root cause of the whitespace issue was quoting $count in the test > command, which led to the inclusion of whitespace in the comparison. By > removing the quotes around $count, the comparison works as expected without > the need for tr -d. > > Signed-off-by: Usman Akinyemi > > Usman Akinyemi (2): > t3404: avoid losing exit status with focus on `git show` and `git > cat-files` > [Outreachy][Patch v1] t3404: employing test_line_count() to replace > test You don’t have to sign off on your cover letter. Just the patches. :) Of course do what you prefer.
Thanks for this. I will definitely take note of this next time. Thank you. On Sun, Oct 6, 2024 at 4:21 PM Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote: > > On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote: > > > > The root cause of the whitespace issue was quoting $count in the test > > command, which led to the inclusion of whitespace in the comparison. By > > removing the quotes around $count, the comparison works as expected without > > the need for tr -d. > > > > Signed-off-by: Usman Akinyemi > > > > Usman Akinyemi (2): > > t3404: avoid losing exit status with focus on `git show` and `git > > cat-files` > > [Outreachy][Patch v1] t3404: employing test_line_count() to replace > > test > > You don’t have to sign off on your cover letter. Just the patches. :) > > Of course do what you prefer.
On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi <usmanakinyemi202@gmail.com> wrote: > Kindly, help take a look if this is okay now. > > Also, I wanted to change this also to use test_line_count, > test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo) > > But, I tried a different approach and the test kept failing. > > Similar as > > git show >output && > count=$(grep NEVER output | wc -l) && > test 0 = $count && What is the actual error you encountered? By the way, we have a handy function, test_must_be_empty(), which can be used if you expect the output to not contain anything. As an example: git show >output && grep NEVER output >actual && test_must_be_empty actual
On Sun, Oct 06, 2024 at 04:26:03PM +0000, Usman Akinyemi wrote: > Thanks for this. I will definitely take note of this next time. Thank you. Please note that we tend to not top-post on this mailing list. Instead, your reply should be either inline in the quoted parts in case you want to reply to individual parts of it, or below the quoted part as others are doing :) Patrick > On Sun, Oct 6, 2024 at 4:21 PM Kristoffer Haugsbakk > <kristofferhaugsbakk@fastmail.com> wrote: > > > > On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote: > > > > > > The root cause of the whitespace issue was quoting $count in the test > > > command, which led to the inclusion of whitespace in the comparison. By > > > removing the quotes around $count, the comparison works as expected without > > > the need for tr -d. > > > > > > Signed-off-by: Usman Akinyemi > > > > > > Usman Akinyemi (2): > > > t3404: avoid losing exit status with focus on `git show` and `git > > > cat-files` > > > [Outreachy][Patch v1] t3404: employing test_line_count() to replace > > > test > > > > You don’t have to sign off on your cover letter. Just the patches. :) > > > > Of course do what you prefer.
On Mon, Oct 7, 2024 at 4:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi > <usmanakinyemi202@gmail.com> wrote: > > Kindly, help take a look if this is okay now. > > > > Also, I wanted to change this also to use test_line_count, > > test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo) > > > > But, I tried a different approach and the test kept failing. > > > > Similar as > > > > git show >output && > > count=$(grep NEVER output | wc -l) && > > test 0 = $count && > > What is the actual error you encountered? > > By the way, we have a handy function, test_must_be_empty(), which can > be used if you expect the output to not contain anything. As an > example: > > git show >output && > grep NEVER output >actual && > test_must_be_empty actual Thanks for your review, I really appreciate it. I tried this approach, but I was getting this particular error for the testing. not ok 32 - multi-fixup does not fire up editor # # git checkout -b multi-fixup E && # base=$(git rev-parse HEAD~4) && # ( # set_fake_editor && # FAKE_COMMIT_AMEND="NEVER" \ # FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \ # git rebase -i $base # ) && # test $base = $(git rev-parse HEAD^) && # git show >output && # grep NEVER output >actual && # test_must_be_empty actual && # git checkout @{-1} && # git branch -D multi-fixup # Below is the particular test case test_expect_success 'multi-fixup does not fire up editor' ' git checkout -b multi-fixup E && base=$(git rev-parse HEAD~4) && ( set_fake_editor && FAKE_COMMIT_AMEND="NEVER" \ FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \ git rebase -i $base ) && test $base = $(git rev-parse HEAD^) && git show >output && grep NEVER output >actual && test_must_be_empty actual && git checkout @{-1} && git branch -D multi-fixup '
On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote: > On Mon, Oct 7, 2024 at 4:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > > On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi > > <usmanakinyemi202@gmail.com> wrote: > > > Kindly, help take a look if this is okay now. > > > > > > Also, I wanted to change this also to use test_line_count, > > > test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo) > > > > > > But, I tried a different approach and the test kept failing. > > > > > > Similar as > > > > > > git show >output && > > > count=$(grep NEVER output | wc -l) && > > > test 0 = $count && > > > > What is the actual error you encountered? > > > > By the way, we have a handy function, test_must_be_empty(), which can > > be used if you expect the output to not contain anything. As an > > example: > > > > git show >output && > > grep NEVER output >actual && > > test_must_be_empty actual > > Thanks for your review, I really appreciate it. I tried this approach, > but I was getting this particular error for the testing. > > not ok 32 - multi-fixup does not fire up editor > # > # git checkout -b multi-fixup E && > # base=$(git rev-parse HEAD~4) && > # ( > # set_fake_editor && > # FAKE_COMMIT_AMEND="NEVER" \ > # FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \ > # git rebase -i $base > # ) && > # test $base = $(git rev-parse HEAD^) && > # git show >output && > # grep NEVER output >actual && > # test_must_be_empty actual && > # git checkout @{-1} && > # git branch -D multi-fixup > # > Below is the particular test case > > test_expect_success 'multi-fixup does not fire up editor' ' > git checkout -b multi-fixup E && > base=$(git rev-parse HEAD~4) && > ( > set_fake_editor && > FAKE_COMMIT_AMEND="NEVER" \ > FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \ > git rebase -i $base > ) && > test $base = $(git rev-parse HEAD^) && > git show >output && > grep NEVER output >actual && > test_must_be_empty actual && > git checkout @{-1} && > git branch -D multi-fixup > ' That makes sense. The expectation here is that `output` shouldn't contain the string "NEVER" at all. And as grep(1) would fail when it doesn't find a match the whole test would fail like this. So the below would likely be the best solution. Patrick diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f171af3061..978fdfc2f1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -410,7 +410,8 @@ test_expect_success 'multi-fixup does not fire up editor' ' git rebase -i $base ) && test $base = $(git rev-parse HEAD^) && - test 0 = $(git show | grep NEVER | wc -l) && + git show >output && + ! grep NEVER output && git checkout @{-1} && git branch -D multi-fixup '
On Mon, Oct 7, 2024 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote: > On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote: > > test $base = $(git rev-parse HEAD^) && > > git show >output && > > grep NEVER output >actual && > > test_must_be_empty actual && > > That makes sense. The expectation here is that `output` shouldn't > contain the string "NEVER" at all. And as grep(1) would fail when it > doesn't find a match the whole test would fail like this. So the below > would likely be the best solution. Thanks. I was just about to respond with the same answer. As a bit of extra explanation, the &&-chaining means that every command in the chain must return "success" (status 0), but the return code of `grep` depends upon whether or not it matched any lines. In this case, it returned non-zero which caused the test to fail.
On Mon, Oct 7, 2024 at 8:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Mon, Oct 7, 2024 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote: > > > test $base = $(git rev-parse HEAD^) && > > > git show >output && > > > grep NEVER output >actual && > > > test_must_be_empty actual && > > > > That makes sense. The expectation here is that `output` shouldn't > > contain the string "NEVER" at all. And as grep(1) would fail when it > > doesn't find a match the whole test would fail like this. So the below > > would likely be the best solution. > > Thanks. I was just about to respond with the same answer. As a bit of > extra explanation, the &&-chaining means that every command in the > chain must return "success" (status 0), but the return code of `grep` > depends upon whether or not it matched any lines. In this case, it > returned non-zero which caused the test to fail. Thanks Eric and Partrick. Thanks for the explanation. I will update the patch then.