[v2] t9832,t2200: avoid using pipes in git commands

Commit Message

Amanda Shafack Oct. 18, 2020, 2:42 p.m. UTC

From: Amanda Shafack <[e-mail]>

When a git command is upstream in a pipe, an unexpected failure of
the git command will go unnoticed.

Write out the output of the git command to a file, so as to actively
catch a failure of the git command.

Signed-off-by: Amanda Shafack <[e-mail]>
---
    [Outreachy][Patch v2] t9832,t2200: avoid using pipes in git related
    commands
    
    Changes since v1:
    
     * Merged patch [1/2] and [2/2] into a single patch
     * Revised commit message
    
    Signed-off-by: Amanda Shafack [e-mail]
    [[e-mail]]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-885%2Flkmandy%2Favoid-pipes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v2
Pull-Request: https://github.com/git/git/pull/885

Range-diff vs v1:

 1:  fc2da014a6 ! 1:  bca393e69a t9832,t2200: avoid using pipes in git commands
     @@ Metadata
       ## Commit message ##
          t9832,t2200: avoid using pipes in git commands
      
     -    When the upstream of a pipe throws an error, the downstream still
     -    executes normally. This happens because the exit code of the upstream
     -    in a pipe is ignored. This behavior can make debugging very hard
     -    incase a test fails. Also, pipes are prone to deadlocks. If the
     -    upstream gets full, the commands downstream will never start.
     +    When a git command is upstream in a pipe, an unexpected failure of
     +    the git command will go unnoticed.
      
     -    Write out the output of the git command to a file, so as to test the
     -    exit codes of both commands.
     -
     -    Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that
     -    the exit code of upstream in the pipe is ignored, thus using pipes
     -    should be avoided.
     +    Write out the output of the git command to a file, so as to actively
     +    catch a failure of the git command.
      
          Signed-off-by: Amanda Shafack <[e-mail]>
      
     @@ t/t2200-add-update.sh: test_expect_success 'add -u resolves unmerged paths' '
       test_expect_success '"add -u non-existent" should fail' '
       	test_must_fail git add -u non-existent &&
      -	! (git ls-files | grep "non-existent")
     -+	! (
     -+		git ls-files >actual &&
     -+		grep "non-existent" actual
     -+	)
     ++	git ls-files >actual &&
     ++	! grep "non-existent" actual
       '
       
       test_done
 2:  0a1550cb22 < -:  ---------- t2200: modify code syntax


 t/t2200-add-update.sh | 3 ++-
 t/t9832-unshelve.sh   | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)


base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a

Comments

Eric Sunshine Oct. 18, 2020, 7:25 p.m. UTC | #1

On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget
<[e-mail]> wrote:
> t9832,t2200: avoid using pipes in git commands

The subject is a bit confusing since pipes aren't used in Git
commands; instead, Git commands may be components of pipes. However,
even that is too imprecise. The issue this patch is addressing is that
we want to avoid Git commands _upstream_ in a pipe. It's perfectly
acceptable for the Git command to be the final element of a pipe since
the pipe returns the exit code of the final command. So, to be more
precise, the subject could say:

    t2200,t9832: avoid using `git` upstream in a pipe

Nit: It's subjective, but it feels a bit more natural to list the test
numbers in ascending order rather than descending order, which is why
I swapped them around in the example above.

> When a git command is upstream in a pipe, an unexpected failure of
> the git command will go unnoticed.
>
> Write out the output of the git command to a file, so as to actively
> catch a failure of the git command.

It's easy to see from the patch itself that the output of the Git
command is now written to a file, so it's not necessary to say so in
the commit message. Therefore, the entire body of the commit message
could be written more succinctly, perhaps like this:

    Avoid placing `git` upstream in a pipe since doing so throws away
    its exit code, thus an unexpected failure may go unnoticed.

The actual patch itself looks fine, and these comments about the
commit message are quite minor, thus there probably is no need to
re-roll (though feel free to do so if you think the bit of extra
polishing of the commit message is worthwhile).

Thanks.

Junio C Hamano Oct. 18, 2020, 8:04 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> precise, the subject could say:
>
>     t2200,t9832: avoid using `git` upstream in a pipe
>
> Nit: It's subjective, but it feels a bit more natural to list the test
> numbers in ascending order rather than descending order, which is why
> I swapped them around in the example above.

;-)

>> When a git command is upstream in a pipe, an unexpected failure of
>> the git command will go unnoticed.
>>
>> Write out the output of the git command to a file, so as to actively
>> catch a failure of the git command.
>
> It's easy to see from the patch itself that the output of the Git
> command is now written to a file, so it's not necessary to say so in
> the commit message. Therefore, the entire body of the commit message
> could be written more succinctly, perhaps like this:
>
>     Avoid placing `git` upstream in a pipe since doing so throws away
>     its exit code, thus an unexpected failure may go unnoticed.

Yup.

> The actual patch itself looks fine, and these comments about the
> commit message are quite minor, thus there probably is no need to
> re-roll (though feel free to do so if you think the bit of extra
> polishing of the commit message is worthwhile).

IIUC, the microproject experience aims new contributors to get used
to the style of communication that happens during review cycles of a
typical topic, using a trivial dip-the-toes-in-the-water problem as
an example.  I'd rather not to see contributors get into the habit
of leaving loose ends and have somebody else clean after them.

Thanks.

Amanda Shafack Oct. 18, 2020, 8:40 p.m. UTC | #3

On Sun, Oct 18, 2020 at 8:25 PM Eric Sunshine <[e-mail]> wrote:
>
> On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget
> <[e-mail]> wrote:
> > t9832,t2200: avoid using pipes in git commands
>
> The subject is a bit confusing since pipes aren't used in Git
> commands; instead, Git commands may be components of pipes. However,
> even that is too imprecise. The issue this patch is addressing is that
> we want to avoid Git commands _upstream_ in a pipe. It's perfectly
> acceptable for the Git command to be the final element of a pipe since
> the pipe returns the exit code of the final command. So, to be more
> precise, the subject could say:
>
>     t2200,t9832: avoid using `git` upstream in a pipe
>
> Nit: It's subjective, but it feels a bit more natural to list the test
> numbers in ascending order rather than descending order, which is why
> I swapped them around in the example above.
>

I agree it looks more appropriate.

> > When a git command is upstream in a pipe, an unexpected failure of
> > the git command will go unnoticed.
> >
> > Write out the output of the git command to a file, so as to actively
> > catch a failure of the git command.
>
> It's easy to see from the patch itself that the output of the Git
> command is now written to a file, so it's not necessary to say so in
> the commit message. Therefore, the entire body of the commit message
> could be written more succinctly, perhaps like this:
>
>     Avoid placing `git` upstream in a pipe since doing so throws away
>     its exit code, thus an unexpected failure may go unnoticed.
>
> The actual patch itself looks fine, and these comments about the
> commit message are quite minor, thus there probably is no need to
> re-roll (though feel free to do so if you think the bit of extra
> polishing of the commit message is worthwhile).

I believe it's best practice to optimize one's work as much as
possible, so I have included these changes.
Thanks for the detailed explanation.

> Thanks.

Amanda Shafack Oct. 18, 2020, 10:04 p.m. UTC | #4

On Sun, Oct 18, 2020 at 9:04 PM Junio C Hamano <[e-mail]> wrote:
>
> Eric Sunshine <[e-mail]> writes:
>
> > precise, the subject could say:
> >
> >     t2200,t9832: avoid using `git` upstream in a pipe
> >
> > Nit: It's subjective, but it feels a bit more natural to list the test
> > numbers in ascending order rather than descending order, which is why
> > I swapped them around in the example above.
>
> ;-)
>
> >> When a git command is upstream in a pipe, an unexpected failure of
> >> the git command will go unnoticed.
> >>
> >> Write out the output of the git command to a file, so as to actively
> >> catch a failure of the git command.
> >
> > It's easy to see from the patch itself that the output of the Git
> > command is now written to a file, so it's not necessary to say so in
> > the commit message. Therefore, the entire body of the commit message
> > could be written more succinctly, perhaps like this:
> >
> >     Avoid placing `git` upstream in a pipe since doing so throws away
> >     its exit code, thus an unexpected failure may go unnoticed.
>
> Yup.
>
> > The actual patch itself looks fine, and these comments about the
> > commit message are quite minor, thus there probably is no need to
> > re-roll (though feel free to do so if you think the bit of extra
> > polishing of the commit message is worthwhile).
>
> IIUC, the microproject experience aims new contributors to get used
> to the style of communication that happens during review cycles of a
> typical topic, using a trivial dip-the-toes-in-the-water problem as
> an example.  I'd rather not to see contributors get into the habit
> of leaving loose ends and have somebody else clean after them.

Taken very seriously

> Thanks.

Patch

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index f764b7e3f5..7cb7a70382 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -179,7 +179,8 @@  test_expect_success 'add -u resolves unmerged paths' '
 
 test_expect_success '"add -u non-existent" should fail' '
 	test_must_fail git add -u non-existent &&
-	! (git ls-files | grep "non-existent")
+	git ls-files >actual &&
+	! grep "non-existent" actual
 '
 
 test_done
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 7194fb2855..6b3cb0414a 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -68,7 +68,8 @@  EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
+		git show refs/remotes/p4-unshelved/$change >actual &&
+		grep -q "Further description" actual &&
 		git cherry-pick refs/remotes/p4-unshelved/$change &&
 		test_path_is_file file2 &&
 		test_cmp file1 "$cli"/file1 &&