Re: [PATCH v3 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-files`

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Mon, Oct 7, 2024 at 8:52 AM <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> On 06/10/2024 17:06, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >
> > 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.
>
> This is a good description of the reason for making this change.
>
> > This particular patch focuses on some of the instances
> > which include `git show` and `git cat-files`.
>
> This patch seems to fix all of the instances of "git show" rather than
> some. It also fixes a few instances of "git cat-file" (note there is no
> trailing "s" in the command name). It is not immediately clear to me why
> those instances of "git cat-file" were selected for conversion but not
> others. However they are a worthwhile improvement and converting them
> all in this patch would make it bit too big to comfortably review so I'm
> happy with the changes as they are.
>
> The patch itself looks good.
>
> Thanks
>
> Phillip
Since, I am updating a patch. I will update this commit message also.
Thank you for the review.
>
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >   t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
> >   1 file changed, 48 insertions(+), 23 deletions(-)
> >
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index f171af3061d..96a65783c47 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -319,7 +319,8 @@ test_expect_success 'retain authorship' '
> >       GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
> >       git tag twerp &&
> >       git rebase -i --onto primary HEAD^ &&
> > -     git show HEAD | grep "^Author: Twerp Snog"
> > +     git show HEAD >actual &&
> > +     grep "^Author: Twerp Snog" actual
> >   '
> >
> >   test_expect_success 'retain authorship w/ conflicts' '
> > @@ -360,7 +361,8 @@ test_expect_success 'squash' '
> >   '
> >
> >   test_expect_success 'retain authorship when squashing' '
> > -     git show HEAD | grep "^Author: Twerp Snog"
> > +     git show HEAD >actual &&
> > +     grep "^Author: Twerp Snog" actual
> >   '
> >
> >   test_expect_success '--continue tries to commit' '
> > @@ -374,7 +376,8 @@ test_expect_success '--continue tries to commit' '
> >               FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
> >       ) &&
> >       test_cmp_rev HEAD^ new-branch1 &&
> > -     git show HEAD | grep chouette
> > +     git show HEAD >actual &&
> > +     grep chouette actual
> >   '
> >
> >   test_expect_success 'verbose flag is heeded, even after --continue' '
> > @@ -397,7 +400,9 @@ test_expect_success 'multi-squash only fires up editor once' '
> >                       git rebase -i $base
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> > -     test 1 = $(git show | grep ONCE | wc -l)
> > +     git show >output &&
> > +     count=$(grep ONCE output | wc -l) &&
> > +     test 1 = $count
> >   '
> >
> >   test_expect_success 'multi-fixup does not fire up editor' '
> > @@ -410,7 +415,9 @@ 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 &&
> > +     count=$(grep NEVER output | wc -l) &&
> > +     test 0 = $count &&
> >       git checkout @{-1} &&
> >       git branch -D multi-fixup
> >   '
> > @@ -428,7 +435,9 @@ test_expect_success 'commit message used after conflict' '
> >                       git rebase --continue
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> > -     test 1 = $(git show | grep ONCE | wc -l) &&
> > +     git show >output &&
> > +     count=$(grep ONCE output | wc -l) &&
> > +     test 1 = $count &&
> >       git checkout @{-1} &&
> >       git branch -D conflict-fixup
> >   '
> > @@ -446,7 +455,9 @@ test_expect_success 'commit message retained after conflict' '
> >                       git rebase --continue
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> > -     test 2 = $(git show | grep TWICE | wc -l) &&
> > +     git show >output &&
> > +     count=$(grep TWICE output | wc -l) &&
> > +     test 2 = $count &&
> >       git checkout @{-1} &&
> >       git branch -D conflict-squash
> >   '
> > @@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
> >       ) &&
> >       git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
> >       test_cmp expect-squash-fixup actual-squash-fixup &&
> > -     git cat-file commit HEAD@{2} |
> > -             grep "^# This is a combination of 3 commits\."  &&
> > -     git cat-file commit HEAD@{3} |
> > -             grep "^# This is a combination of 2 commits\."  &&
> > +     git cat-file commit HEAD@{2} >actual &&
> > +     grep "^# This is a combination of 3 commits\." actual &&
> > +     git cat-file commit HEAD@{3} >actual &&
> > +     grep "^# This is a combination of 2 commits\." actual  &&
> >       git checkout @{-1} &&
> >       git branch -D squash-fixup
> >   '
> > @@ -489,7 +500,9 @@ test_expect_success 'squash ignores comments' '
> >                       git rebase -i $base
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> > -     test 1 = $(git show | grep ONCE | wc -l) &&
> > +     git show >output &&
> > +     count=$(grep ONCE output | wc -l) &&
> > +     test 1 = $count &&
> >       git checkout @{-1} &&
> >       git branch -D skip-comments
> >   '
> > @@ -505,7 +518,9 @@ test_expect_success 'squash ignores blank lines' '
> >                       git rebase -i $base
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> > -     test 1 = $(git show | grep ONCE | wc -l) &&
> > +     git show >output &&
> > +     count=$(grep ONCE output | wc -l) &&
> > +     test 1 = $count &&
> >       git checkout @{-1} &&
> >       git branch -D skip-blank-lines
> >   '
> > @@ -572,7 +587,8 @@ test_expect_success '--continue tries to commit, even for "edit"' '
> >               FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
> >       ) &&
> >       test edited = $(git show HEAD:file7) &&
> > -     git show HEAD | grep chouette &&
> > +     git show HEAD >actual &&
> > +     grep chouette actual &&
> >       test $parent = $(git rev-parse HEAD^)
> >   '
> >
> > @@ -757,19 +773,23 @@ test_expect_success 'reword' '
> >               set_fake_editor &&
> >               FAKE_LINES="1 2 3 reword 4" FAKE_COMMIT_MESSAGE="E changed" \
> >                       git rebase -i A &&
> > -             git show HEAD | grep "E changed" &&
> > +             git show HEAD >actual &&
> > +             grep "E changed" actual &&
> >               test $(git rev-parse primary) != $(git rev-parse HEAD) &&
> >               test_cmp_rev primary^ HEAD^ &&
> >               FAKE_LINES="1 2 reword 3 4" FAKE_COMMIT_MESSAGE="D changed" \
> >                       git rebase -i A &&
> > -             git show HEAD^ | grep "D changed" &&
> > +             git show HEAD^ >actual &&
> > +             grep "D changed" actual &&
> >               FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" \
> >                       git rebase -i A &&
> > -             git show HEAD~3 | grep "B changed" &&
> > +             git show HEAD~3 >actual &&
> > +             grep "B changed" actual &&
> >               FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" \
> >                       git rebase -i A
> >       ) &&
> > -     git show HEAD~2 | grep "C changed"
> > +     git show HEAD~2 >actual &&
> > +     grep "C changed" actual
> >   '
> >
> >   test_expect_success 'no uncommitted changes when rewording and the todo list is reloaded' '
> > @@ -1003,8 +1023,10 @@ test_expect_success 'rebase -i --root retain root commit author and message' '
> >               set_fake_editor &&
> >               FAKE_LINES="2" git rebase -i --root
> >       ) &&
> > -     git cat-file commit HEAD | grep -q "^author Twerp Snog" &&
> > -     git cat-file commit HEAD | grep -q "^different author$"
> > +     git cat-file commit HEAD >output &&
> > +     grep -q "^author Twerp Snog" output &&
> > +     git cat-file commit HEAD >actual &&
> > +     grep -q "^different author$" actual
> >   '
> >
> >   test_expect_success 'rebase -i --root temporary sentinel commit' '
> > @@ -1013,7 +1035,8 @@ test_expect_success 'rebase -i --root temporary sentinel commit' '
> >               set_fake_editor &&
> >               test_must_fail env FAKE_LINES="2" git rebase -i --root
> >       ) &&
> > -     git cat-file commit HEAD | grep "^tree $EMPTY_TREE" &&
> > +     git cat-file commit HEAD >actual &&
> > +     grep "^tree $EMPTY_TREE" actual &&
> >       git rebase --abort
> >   '
> >
> > @@ -1036,7 +1059,8 @@ test_expect_success 'rebase -i --root reword original root commit' '
> >               FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
> >                       git rebase -i --root
> >       ) &&
> > -     git show HEAD^ | grep "A changed" &&
> > +     git show HEAD^ >actual &&
> > +     grep "A changed" actual &&
> >       test -z "$(git show -s --format=%p HEAD^)"
> >   '
> >
> > @@ -1048,7 +1072,8 @@ test_expect_success 'rebase -i --root reword new root commit' '
> >               FAKE_LINES="reword 3 1" FAKE_COMMIT_MESSAGE="C changed" \
> >               git rebase -i --root
> >       ) &&
> > -     git show HEAD^ | grep "C changed" &&
> > +     git show HEAD^ >actual &&
> > +     grep "C changed" actual &&
> >       test -z "$(git show -s --format=%p HEAD^)"
> >   '
> >
>