git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Amanda Shafack via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Emily Shaffer <emilyshaffer@google.com>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Amanda Shafack <shafack.likhene@gmail.com>
Subject: Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands
Date: Sun, 18 Oct 2020 02:11:47 -0400	[thread overview]
Message-ID: <CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com> (raw)
In-Reply-To: <fc2da014a62c387009e2175435286afef64d6598.1602950552.git.gitgitgadget@gmail.com>

On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

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

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

> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
>  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.

> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
> @@ -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.

  reply	other threads:[~2020-10-18  6:12 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-17 16:02 [PATCH 0/2] [Outreachy][Patch v1] t9832,t2200: avoid using pipes in git related commands Amanda  Shafack  via GitGitGadget
2020-10-17 16:02 ` [PATCH 1/2] t9832,t2200: avoid using pipes in git commands Amanda Shafack via GitGitGadget
2020-10-18  6:11   ` Eric Sunshine [this message]
2020-10-18 13:57     ` Amanda Shafack
2020-10-17 16:02 ` [PATCH 2/2] t2200: modify code syntax Amanda  Shafack via GitGitGadget
2020-10-18  5:55   ` Eric Sunshine
2020-10-18 13:59     ` Amanda Shafack
2020-10-18 14:42 ` [PATCH v2] t9832,t2200: avoid using pipes in git commands Amanda  Shafack  via GitGitGadget
2020-10-18 19:25   ` Eric Sunshine
2020-10-18 20:04     ` Junio C Hamano
2020-10-18 22:04       ` Amanda Shafack
2020-10-18 20:40     ` Amanda Shafack
2020-10-18 20:48   ` [PATCH v3] t2200,t9832: avoid using 'git' upstream in a pipe Amanda  Shafack  via GitGitGadget
2020-10-18 21:01     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cQnfcc9W3qE5XkyLiwNyK8Wm2SsAyjzatDWEVUi3s8APw@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=shafack.likhene@gmail.com \
    --subject='Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).