Re: [PATCH 2/7] t: introduce tests for unexpected object types

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 9, 2019, 02:29 UTC
Message-ID: <20190409022936.GE81620@Taylors-MBP.hsd1.wa.comcast.net>
In-Reply-To: <87a7h1a5uk.fsf@evledraar.gmail.com>

Hi Ævar,

On Sun, Apr 07, 2019 at 11:00:19PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 24 quoted lines

>
> On Fri, Apr 05 2019, Jeff King wrote:
>
> > On Fri, Apr 05, 2019 at 08:42:29PM +0200, SZEDER Gábor wrote:
> >
> >> > > Don't run git commands upstream of a pipe, because the pipe hides
> >> > > their exit code.  This applies to several other tests below as well.
> >> >
> >> > I disagree with that rule here. We're not testing "cat-file" in any
> >> > meaningful way, but just getting some stock output from a known-good
> >> > commit.
> >>
> >> It makes auditing harder and sets bad example.
> >
> > I disagree that it's a bad example. Just because a reader might not
> > realize that it is sometimes OK and sometimes not, does not make it bad
> > to do so in the OK case. It means the reader needs to understand the
> > rule in order to correctly apply it.
>
> FWIW I thought the rule was something to the effect of "we're hacking on
> git, any change might segfault in some unexpected test, let's make sure
> that fails right away", hence the blanket rule in t/README against "!
> git <cmd>" in favor of "test_must_fail git <cmd>", and "don't feed the
> output of a git command to a pipe" documented right after that.

I have some loosely-held opinions on this. Certainly knowing if a change to git caused a segfault in some test is something that we would want to know about, though I'm not sure we're loosing anything by putting 'git' on the left-hand side of a pipe here.

  - If someone wrote a change to git that introduced a segfault in 'git
    cat-file', I'm sure that this would not be the only place that in
    the suite that would break because of it. Presumably, at least one
    of those places uses 'test_must_fail git ...' instead

  - At the very least, 'broken-commit' doesn't contain what it needs to,
    the test breaks in another way (albeit further from the actual
    defect), and the developer finds out about their bug that way.

In any case, these above two might be moot anyways, because I'm almost certain that it will be a rarity for someone to _only_ run t6102, unless it is included in a blanket 'make' from within 't'.

> > I am sympathetic to the auditing issue, though.

Just to satisfy my curiosity, I put git on the left-hand side of a pipe to see how many such examples exist today:

  ~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l
      179

I'm not going to claim that changing 179 -> 180 is neutral or bad, but it's interesting nonetheless.

Show 7 quoted lines

> > I dunno. In this case it is not too bad to do:
> >
> >   git cat-file commit $commit >good-commit &&
> >   perl ... <good-commit >broken-commit
> >
> > but I am not sure I am on board with a blanket rule of "git must never
> > be on the left-hand side of a pipe".

I think that I mostly agree with Peff here for the reasons above.

That all said, I don't really want to hold up the series for this alone. Since there don't seem to be many other comments or issues, I would be quite happy to do whatever people are most in favor of.

I basically don't really feel strongly about writing:

  git cat-file commit $commit | sed -e ... >broken-commit

as opposed to:

  git cat-file commit $commit >good-commit &&
  sed -e '' <commit >bad-commit

Show 18 quoted lines

> >> > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl
> >> > > dependency?
> >> >
> >> > Heh, this was actually the subject of much discussion before the patches
> >> > hit the list. If you can write such a one-liner that is both readable
> >> > and portable, please share it. I got disgusted with sed and suggested
> >> > this perl.
> >>
> >> Hm, so the trivial s/// with '\n' in the replacement part is not
> >> portable, then?  Oh, well.
> >
> > I don't think it is, but I could be wrong. POSIX does say that "\n"
> > matches a newline in the pattern space, but nothing about it on the RHS
> > of a substitution. I have a vague feeling of running into problems in
> > the past, but I could just be misremembering.
> >
> > We also tried matching /^tree/ and using "a\" to append a line, but it
> > was both hard to read and hit portability issues with bsd sed.

I think that all of this discussion is addressed elsewhere in thread, but the gist is that Eric provided a suitable sed invocation that I am going to use instead of Peff's Perl.

> > -Peff

Thanks, Taylor