Re: [PATCH 3/3] blame: output porcelain "previous" header for each file

From: Jeff King <peff@peff.net>
Date: Mar 27, 2017, 22:04 UTC
Message-ID: <20170327220429.dsfp5ytq2yddfn33@sigill.intra.peff.net>
In-Reply-To: <CACBZZX5AnG=hpnZCG3qFkLOO0gbgvt79ZYP3-oG5VjE8p284cQ@mail.gmail.com>

On Mon, Mar 27, 2017 at 11:08:08PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

> On Fri, Jan 6, 2017 at 5:20 AM, Jeff King <peff@peff.net> wrote:
> 
> > +for output in porcelain line-porcelain
> > +do
> > +       test_expect_success "generate --$output output" '
> > +               git blame --root -C --$output combined >output
> > +       '
> > +
> 
> The --root option isn't needed here, the tests pass if it's removed.
> 
> Discovered while looking at the sorry state of our blame test suite
> vis-a-vis tests for config, no tests for blame.showroot &
> blame.blankboundary.
> 
> I'll submit that eventually, but meanwhile did you mean to omit --root
> here, or is the test broken in some other way and it /should/ need
> --root but doesn't?

I don't think it's strictly needed, though I think it's worth keeping.

The test is making sure that some lines blame to HEAD and some to HEAD^. But the latter is a root commit, and so it just becomes a boundary commit without --root.

You can see the difference if you interrupt the test here and run:

  git blame -C [--root] combined

And that's what I was doing when I developed the test case. If you were to test the non-porcelain output, you'd need to it (to match the real sha-1 X, and not the boundary "^X").

When the porcelain outputs are used, though, the boundary commits are shown as full sha1s, and just get annotated with a "boundary" line. As the tests just look for subject, filename, and previous lines, they don't notice whether the boundary marker is there or not. And so --root could technically go away.

We care mostly about detecting the values for the second commit, so I don't think it actually matters. But if we wanted to be thorough, we could confirm that the "boundary" lines are as we expect (or alternatively, we could add an uninteresting base commit at the bottom to make the second one non-root).

-Peff