git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / 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
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 \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git