git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Shubham Mishra <shivam828787@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 0/2] microproject: avoid using pipes in test
Date: Tue, 22 Feb 2022 15:24:31 +0100	[thread overview]
Message-ID: <220222.86pmnf6ket.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20220222114313.14921-1-shivam828787@gmail.com>


On Tue, Feb 22 2022, Shubham Mishra wrote:

> pipes doesn't care about error codes and ignore them thus we should not use them in tests.

Aside from what Derrick Stolee mentioned in his feedback, all of which I
agree with.

I think these changes are good, but it's not the case that we try to
avoid using pipes at all in our tests.

It's often a hassle, and just not worth it, e.g.:

    oid=$(echo foo | git hash-object --stdin -w) &&

Sure, we can make that:

    echo foo >in &&
    oid=$(git hash-object --stdin -w <in) &&

But in the general case it's not worth worrying about.

What we *do* try to avoid, and what's actually important is to never
invoke "git" or other programs we invoke on the LHS of a pipe, or to
otherwise do so in a way that hides potential errors.

That's not isolated to just pipes, but e.g. calling it within helper
functions that don't &&-chain, but pipes are probably the most common
offender.

The reason we do that is because in hacking git we may make it error,
segfault etc. If it's on the LHS of a pipe that failure becomes
indistinguishable from success.

And if the test is really checking e.g. "when I run git like this, it
produces no output" printing nothing with an exit of 0 will become the
same as a segafault for the purposes of test.

And that's bad.

But just invoking things on the LHS of a pipe? Sometimes it's a good
thing not do do that, because we'll be able to see a failure more
incrementally, and with intermediate files.

But it's generally not a problem, our test suite assumes that the OS is
basically sane. We're not going to call every invocation of "sed",
"grep" etc. with a wrapper like "test_must_fail grep" instead of "!
grep".

The same goes for our own helper utility functions such as "q_to_nul"
etc, as long as (and this is critical) that they're implemented by
shelling out to "sed", "grep", "perl" or whatever, and not to "git" or
"test-tool" etc. Then we need to start being as paranoid about them as
"git" on the LHS of pipes.


  parent reply	other threads:[~2022-02-22 14:33 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-22 11:43 [PATCH 0/2] microproject: avoid using pipes in test Shubham Mishra
2022-02-22 11:43 ` [PATCH 1/2] t0001: remove pipes Shubham Mishra
2022-02-22 13:54   ` Derrick Stolee
2022-02-22 18:16     ` Shubham Mishra
2022-02-22 20:32   ` Christian Couder
2022-02-23 21:07     ` Junio C Hamano
2022-02-24  3:28       ` Christian Couder
2022-02-22 11:43 ` [PATCH 2/2] t0003: " Shubham Mishra
2022-02-22 13:50 ` [PATCH 0/2] microproject: avoid using pipes in test Derrick Stolee
2022-02-22 14:24 ` Ævar Arnfjörð Bjarmason [this message]
2022-02-22 19:12   ` Shubham Mishra
2022-02-22 19:39   ` Taylor Blau
2022-02-23 11:53 ` [PATCH v2 0/2] avoid pipes with Git on LHS Shubham Mishra
2022-02-23 11:53   ` [PATCH v2 1/2] t0001: " Shubham Mishra
2022-02-23 17:53     ` Taylor Blau
2022-02-23 18:01       ` Shubham Mishra
2022-02-23 18:02         ` Taylor Blau
2022-02-23 21:10     ` Junio C Hamano
2022-02-24  5:14       ` Shubham Mishra
2022-02-23 11:53   ` [PATCH v2 2/2] t0003: " Shubham Mishra
2022-02-23 17:54     ` Taylor Blau
2022-02-23 19:59       ` Shubham Mishra
2022-02-23 12:08   ` [PATCH v2 0/2] " Shubham Mishra
2022-02-23 13:19     ` Shaoxuan Yuan
2022-02-23 18:01       ` Taylor Blau
2022-02-23 20:04         ` Shubham Mishra
2022-02-24  3:43 ` [PATCH 0/2] microproject: avoid using pipes in test Christian Couder
2022-02-24  5:22   ` Shubham Mishra
2022-02-24  9:29     ` Christian Couder
2022-02-24 10:13       ` Shubham Mishra
2022-02-24 18:22         ` Christian Couder

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=220222.86pmnf6ket.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=shivam828787@gmail.com \
    --cc=stolee@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).