t9832,t2200: avoid using pipes in git related 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.

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.

Quoted 4 lines · Amanda Shafack via GitGitGadget

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.

To be more precise, we're not worried about difficulty of debugging when a Git command is upstream in a pipe. What we are more concerned with is that an unexpected failure of the Git command will go unnoticed (unless it also happens to produce wrong output which is later caught somewhere downstream). By avoiding placing Git upstream in a pipe, we can actively catch a failure of the Git command. The commit message should focus on that reason instead.

Quoted 2 lines

Also, pipes are prone to deadlocks. If the
upstream gets full, the commands downstream will never start.

I don't think this is ever an issue in Git tests, so talking about it here probably only muddies the waters, thus makes the commit message less precise and more hand-wavy. Dropping this sentence is recommended.

Quoted 3 lines

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.

There are a lot of commits in the project history which perform this sort of transformation, so it's not clear to the reader why this particular commit is called out as being important. If it's not, then I'd recommend dropping this sentence, as well.

t/t2200-add-update.sh test_expect_success 'add -u resolves unmerged paths' ' · line 186

Quoted 9 lines

t/t2200-add-update.sh +4 -1

 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
+       )
 '

This transformation appears to be incorrect. See my review of patch [2/2] for details.

t/t9832-unshelve.sh EOF · line 69

Quoted 5 lines

t/t9832-unshelve.sh +2 -1

@@ -68,7 +68,8 @@ EOF
-               git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
+               git show refs/remotes/p4-unshelved/$change >actual &&
+               grep -q "Further description" actual &&

This transformation looks fine.

Using the logical not operator on both the git and grep command is redundant, since the main check is done at the level of the grep command.

Apply the logical not operator only to the grep command.
---

Quoted 8 lines · Amanda Shafack via GitGitGadget

Using the logical not operator on both the git and grep command is redundant, since the main check is done at the level of the grep command.

Apply the logical not operator only to the grep command.
---

t/t2200-add-update.sh test_expect_success 'add -u resolves unmerged paths' ' · line 180

Quoted 6 lines

t/t2200-add-update.sh +2 -4

@@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '
-       ! (
-               git ls-files >actual &&
-               grep "non-existent" actual
-       )
+       git ls-files >actual &&
+       ! grep "non-existent" actual

The commit message talks only about redundancy of applying the logical NOT operator to the combined subshell content, but what it misses is that transformation of:

! (git ls-files | grep "non-existent")

in patch [1/2] into:

    ! (
        git ls-files >actual &&
        grep "non-existent" actual
    )

is actively wrong. In particular, if git ls-files itself fails, then the grep will never get run, and the subshell will exit with a failure, however, the NOT then turns that failure into a success, which is quite undesirable. The test should fail if either git ls-files fails or if the grep expectation is not met; it should not succeed when git ls-files fails.

So, the correct thing to do is to merge [1/2] and [2/2] into a single patch which directly transforms:

! (git ls-files | grep "non-existent")

into:

git ls-files >actual &&
! grep "non-existent" actual

t9832,t2200: avoid using pipes in git commands

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.

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.

Quoted 5 lines

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).

Quoted 7 lines · Eric Sunshine

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.

;-)

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 1 line · Amanda Shafack via GitGitGadget

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.

Quoted 5 lines

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.

From: Amanda Shafack <[email protected]>

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

Signed-off-by Amanda Shafack <[email protected]>

---

t/t2200-add-update.sh +0 -0

index f764b7e3f5..7cb7a70382 100755