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

[Jeff King]

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

> On Fri, Jan 6, 2017 at 5:20 AM, Jeff King  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

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 6, 2017 at 5:20 AM, Jeff King  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?

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

[Junio C Hamano]

Jeff King  writes:

>  /*
> + * Write out any suspect information which depends on the path. This must be
> + * handled separately from emit_one_suspect_detail(), because a given commit
> + * may have changes in multiple paths. So this needs to appear each time
> + * we mention a new group.
> + *
>   * To allow LF and other nonportable characters in pathnames,
>   * they are c-style quoted as needed.
>   */
> -static void write_filename_info(const char *path)
> +static void write_filename_info(struct origin *suspect)
>  {
> + if (suspect->previous) {
> + struct origin *prev = suspect->previous;
> + printf("previous %s ", oid_to_hex(>commit->object.oid));
> + write_name_quoted(prev->path, stdout, '\n');
> + }
>   printf("filename ");
> - write_name_quoted(path, stdout, '\n');
> + write_name_quoted(suspect->path, stdout, '\n');
>  }

Yes, "previous" is not per commit but per "origin", and "origin" is
(commit, path) pair, so allowing this helper to examine the entire
suspect instead of just suspect->path makes sense.

Thanks for a fix.