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

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Taylor Blau" <me@ttaylorr.com>,
	git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH 2/7] t: introduce tests for unexpected object types
Date: Fri, 5 Apr 2019 22:31:15 -0700	[thread overview]
Message-ID: <20190406053115.GB37216@Taylors-MBP.hsd1.wa.comcast.net> (raw)
In-Reply-To: <20190405182412.GC2284@sigill.intra.peff.net>

On Fri, Apr 05, 2019 at 02:24:12PM -0400, Jeff King wrote:
> On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote:
>
> > > +test_expect_failure 'traverse unexpected non-tree entry (seen)' '
> > > +	test_must_fail git rev-list --objects $blob $broken_tree >output 2>&1
> >
> > This test saves standard output and error, but doesn't look at them.
>
> I think we want to be checking for "not a tree" here, which is later
> added with the fix. But either we should have the test_i18ngrep here
> initially, or we should add both the redirect and the grep with the fix.

Right, pointing out that saving the standard output and error of 'git
rev-list' and then doing nothing with it as being redundant is certainly
right.

I think that the 'fix' here is to write instead:

  +test_expect_failure 'traverse unexpected non-tree entry (seen)' '
  +	test_must_fail git rev-list --objects $blob $broken_tree
  +'

And _then_ add '>output 2>&1 &&' to the end, capturing the output, as
well as the appropriate test_i18ngrep. This matches the pattern that
we've been otherwise following in the series so far.

(FWIW, I think that this is also the result of squashing the series down
a few times...)

> > > +test_expect_success 'setup unexpected non-commit parent' '
> > > +	git cat-file commit $commit |
> > > +		perl -lpe "/^author/ && print q(parent $blob)" \
> > > +		>broken-commit &&
> >
> > 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.
>
> > 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.

I admit that this gave me a chuckle, too. When preparing this series to
send it, I did something like:

  $ git rebase -x 'make && cd t && ./t6102-*.sh

but found that it was broken on the BSD sed that ships with macOS
10.14.2. I didn't notice until preparing the series for upstream because
I wrote it on my Debian 9 VM, with GNU sed (where it is not broken ;-)).

It was originally written as:

	test_expect_success 'setup unexpected non-commit parent' '
		git cat-file commit $commit | sed "/^tree/a\
		parent $blob" >broken-commit &&
		broken_commit="$(git hash-object -w --literally -t commit \
			broken-commit)"
	'

> -Peff

Thanks,
Taylor