[PATCH 0/3] fixing some random blame corner cases

[PATCH 0/3] fixing some random blame corner cases

[Jeff King]

Here are three fixes for some fairly obscure corner cases. I haven't
actually seen these in the wild. I came up with the final one while
discussing a hypothetical with somebody, then ran across the middle one
while trying to write a test for the third, which made me scratch my
head enough to yield the first one. Classic yak-shaving.

One other thing that surprised me while writing blame tests is that
"--root" is not the default for git-blame (though it has been for many
years in git-log). I'm not sure if it would be a good idea to change it,
or if blame is too plumbing-ish to allow that.

  [1/3]: blame: fix alignment with --abbrev=40
  [2/3]: blame: handle --no-abbrev
  [3/3]: blame: output porcelain "previous" header for each file

 builtin/blame.c             |  27 ++++++----
 t/t8002-blame.sh            |  32 ++++++++++++
 t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+), 10 deletions(-)
 create mode 100755 t/t8011-blame-split-file.sh

-Peff

[PATCH 1/3] blame: fix alignment with --abbrev=40

[Jeff King]

The blame command internally adds 1 to any requested sha1
abbreviation length, and then subtracts it when outputting a
boundary commit. This lets regular and boundary sha1s line
up visually, but it misses one corner case.

When the requested length is 40, we bump the value to 41.
But since we only have 40 characters, that's all we can show
(fortunately the truncation is done by a printf precision
field, so it never tries to read past the end of the
buffer).  So a normal sha1 shows 40 hex characters, and a
boundary sha1 shows "^" plus 40 hex characters. The result
is misaligned.

The "-l" option to show long sha1s gets around this by
skipping the "abbrev" variable entirely and just always
using GIT_SHA1_HEXSZ.  This avoids the "+1" issue, but it
does mean that boundary commits only have 39 characters
printed.  This is somewhat odd, but it does look good
visually: the results are aligned and left-justified. The
alternative would be to allocate an extra column that would
contain either an extra space or the "^" boundary marker.

As this is by definition the human-readable view, it's
probably not that big a deal either way (and of course
--porcelain, etc, correctly produce correct 40-hex sha1s).
But for consistency, this patch teaches --abbrev=40 to
produce the same output as "-l" (always left-aligned, with
40-hex for normal sha1s, and "^" plus 39-hex for
boundaries).

Signed-off-by: Jeff King <peff@peff.net>
---
I mostly didn't explore the extra-column solution out of a sense of
inertia. The "-l" option has behaved this way for years. But it was also
out of laziness, as I doubt anybody cares too much, and it would require
a fair bit of special-casing in the printing routines.

 builtin/blame.c  |  2 +-
 t/t8002-blame.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..1d312542de 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2656,7 +2656,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	} else if (show_progress < 0)
 		show_progress = isatty(2);
 
-	if (0 < abbrev)
+	if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
 
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index ab79de9544..c6347ad8fd 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -86,4 +86,32 @@ test_expect_success 'blame with showEmail config true' '
 	test_cmp expected_n result
 '
 
+test_expect_success 'set up abbrev tests' '
+	test_commit abbrev &&
+	sha1=$(git rev-parse --verify HEAD) &&
+	check_abbrev () {
+		expect=$1; shift
+		echo $sha1 | cut -c 1-$expect >expect &&
+		git blame "$@" abbrev.t >actual &&
+		perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
+		test_cmp expect actual.sha
+	}
+'
+
+test_expect_success 'blame --abbrev=<n> works' '
+	# non-boundary commits get +1 for alignment
+	check_abbrev 31 --abbrev=30 HEAD &&
+	check_abbrev 30 --abbrev=30 ^HEAD
+'
+
+test_expect_success 'blame -l aligns regular and boundary commits' '
+	check_abbrev 40 -l HEAD &&
+	check_abbrev 39 -l ^HEAD
+'
+
+test_expect_success 'blame --abbrev=40 behaves like -l' '
+	check_abbrev 40 --abbrev=40 HEAD &&
+	check_abbrev 39 --abbrev=40 ^HEAD
+'
+
 test_done
-- 
2.11.0.519.g31435224cf

[PATCH 2/3] blame: handle --no-abbrev

[Jeff King]

You can already ask blame for full sha1s with "-l" or with
"--abbrev=40". But for consistency with other parts of Git,
we should support "--no-abbrev".

Worse, blame already accepts --no-abbrev, but it's totally
broken. When we see --no-abbrev, the abbrev variable is set
to 0, which is then used as a printf precision. For regular
sha1s, that means we print nothing at all (which is very
wrong). For boundary commits we decrement it to "-1", which
printf interprets as "no limit" (which is almost correct,
except it misses the 39-length magic explained in the
previous commit).

Let's detect --no-abbrev and behave as if --abbrev=40 was
given.

Signed-off-by: Jeff King <peff@peff.net>
---
I also wondered if we needed to clamp this within MINIMUM_ABBREV, but
that is done for us already by the parseopt handler.

 builtin/blame.c  | 2 ++
 t/t8002-blame.sh | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1d312542de..c6170fed81 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2659,6 +2659,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
 		/* one more abbrev length is needed for the boundary commit */
 		abbrev++;
+	else if (!abbrev)
+		abbrev = GIT_SHA1_HEXSZ;
 
 	if (revs_file && read_ancestry(revs_file))
 		die_errno("reading graft file '%s' failed", revs_file);
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index c6347ad8fd..380e1c1054 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -114,4 +114,8 @@ test_expect_success 'blame --abbrev=40 behaves like -l' '
 	check_abbrev 39 --abbrev=40 ^HEAD
 '
 
+test_expect_success '--no-abbrev works like --abbrev=40' '
+	check_abbrev 40 --no-abbrev
+'
+
 test_done
-- 
2.11.0.519.g31435224cf

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

[Jeff King]

It's possible for content currently found in one file to
have originated in two separate files, each of which may
have been modified in some single older commit.  The
--porcelain output generates an incorrect "previous" header
in this case, whereas --line-porcelain gets it right.  The
problem is that the porcelain output tries to omit repeated
details of commits, and treats "previous" as a property of
the commit, when it is really a property of the blamed block
of lines.

Let's look at an example. In a case like this, you might see
this output from --line-porcelain:

  SOME_SHA1 1 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_one
  filename file_one
          ...some line content...
  SOME_SHA1 2 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_two
  filename file_two
          ...some different content....

The "filename" fields tell us that the two lines are from
two different files. But notice that the filename also
appears in the "previous" field, which tells us where to
start a re-blame. The second content line never appeared in
file_one at all, so we would obviously need to re-blame from
file_two (or possibly even some other file, if had just been
renamed to file_two in SOME_SHA1).

So far so good. Now here's what --porcelain looks like:

  SOME_SHA1 1 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_one
  filename file_one
          ...some line content...
  SOME_SHA1 2 1 1
  filename file_two
          ...some different content....

We've dropped the author and committer fields from the
second line, as they would just be repeats.  But we can't
omit "filename", because it depends on the actual block of
blamed lines, not just the commit. This is handled by
emit_porcelain_details(), which will show the filename
either if it is the first mention of the commit _or_ if the
commit has multiple paths in it.

But we don't give "previous" the same handling. It's written
inside emit_one_suspect_detail(), which bails early if we've
already seen that commit. And so the output above is wrong;
a reader would assume that the correct place to re-blame
line two is from file_one, but that's obviously nonsense.

Let's treat "previous" the same as "filename", and show it
fresh whenever we know we are in a confusing case like this.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm assuming that the parent sha1 for a "previous" field will always be
the same for a given commit. So we really only need to care about
reprinting when we know there are multiple paths, as this patch does
(i.e., treat it exactly the same as "filename"). If I'm wrong, then
there's probably another corner case that this doesn't handle. I
couldn't think of a way to trigger such a setup, though.

 builtin/blame.c             |  23 +++++----
 t/t8011-blame-split-file.sh | 117 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 9 deletions(-)
 create mode 100755 t/t8011-blame-split-file.sh

diff --git a/builtin/blame.c b/builtin/blame.c
index c6170fed81..3aae19a0f9 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1700,13 +1700,23 @@ static void get_commit_info(struct commit *commit,
 }
 
 /*
+ * 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(&prev->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');
 }
 
 /*
@@ -1735,11 +1745,6 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
 	printf("summary %s\n", ci.summary.buf);
 	if (suspect->commit->object.flags & UNINTERESTING)
 		printf("boundary\n");
-	if (suspect->previous) {
-		struct origin *prev = suspect->previous;
-		printf("previous %s ", oid_to_hex(&prev->commit->object.oid));
-		write_name_quoted(prev->path, stdout, '\n');
-	}
 
 	commit_info_destroy(&ci);
 
@@ -1760,7 +1765,7 @@ static void found_guilty_entry(struct blame_entry *ent,
 		       oid_to_hex(&suspect->commit->object.oid),
 		       ent->s_lno + 1, ent->lno + 1, ent->num_lines);
 		emit_one_suspect_detail(suspect, 0);
-		write_filename_info(suspect->path);
+		write_filename_info(suspect);
 		maybe_flush_or_die(stdout, "stdout");
 	}
 	pi->blamed_lines += ent->num_lines;
@@ -1884,7 +1889,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
 {
 	if (emit_one_suspect_detail(suspect, repeat) ||
 	    (suspect->commit->object.flags & MORE_THAN_ONE_PATH))
-		write_filename_info(suspect->path);
+		write_filename_info(suspect);
 }
 
 static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
diff --git a/t/t8011-blame-split-file.sh b/t/t8011-blame-split-file.sh
new file mode 100755
index 0000000000..831125047b
--- /dev/null
+++ b/t/t8011-blame-split-file.sh
@@ -0,0 +1,117 @@
+#!/bin/sh
+
+test_description='
+The general idea is that we have a single file whose lines come from
+multiple other files, and those individual files were modified in the same
+commits. That means that we will see the same commit in multiple contexts,
+and each one should be attributed to the correct file.
+
+Note that we need to use "blame -C" to find the commit for all lines. We will
+not bother testing that the non-C case fails to find it. That is how blame
+behaves now, but it is not a property we want to make sure is retained.
+'
+. ./test-lib.sh
+
+# help avoid typing and reading long strings of similar lines
+# in the tests below
+generate_expect () {
+	while read nr data
+	do
+		i=0
+		while test $i -lt $nr
+		do
+			echo $data
+			i=$((i + 1))
+		done
+	done
+}
+
+test_expect_success 'setup split file case' '
+	# use lines long enough to trigger content detection
+	test_seq 1000 1010 >one &&
+	test_seq 2000 2010 >two &&
+	git add one two &&
+	test_commit base &&
+
+	sed "6s/^/modified /" <one >one.tmp &&
+	mv one.tmp one &&
+	sed "6s/^/modified /" <two >two.tmp &&
+	mv two.tmp two &&
+	git add -u &&
+	test_commit modified &&
+
+	cat one two >combined &&
+	git add combined &&
+	git rm one two &&
+	test_commit combined
+'
+
+test_expect_success 'setup simulated porcelain' '
+	# This just reads porcelain-ish output and tries
+	# to output the value of a given field for each line (either by
+	# reading the field that accompanies this line, or referencing
+	# the information found last time the commit was mentioned).
+	cat >read-porcelain.pl <<-\EOF
+	my $field = shift;
+	while (<>) {
+		if (/^[0-9a-f]{40} /) {
+			flush();
+			$hash = $&;
+		} elsif (/^$field (.*)/) {
+			$cache{$hash} = $1;
+		}
+	}
+	flush();
+
+	sub flush {
+		return unless defined $hash;
+		if (defined $cache{$hash}) {
+			print "$cache{$hash}\n";
+		} else {
+			print "NONE\n";
+		}
+	}
+	EOF
+'
+
+for output in porcelain line-porcelain
+do
+	test_expect_success "generate --$output output" '
+		git blame --root -C --$output combined >output
+	'
+
+	test_expect_success "$output output finds correct commits" '
+		generate_expect >expect <<-\EOF &&
+		5 base
+		1 modified
+		10 base
+		1 modified
+		5 base
+		EOF
+		perl read-porcelain.pl summary <output >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$output output shows correct filenames" '
+		generate_expect >expect <<-\EOF &&
+		11 one
+		11 two
+		EOF
+		perl read-porcelain.pl filename <output >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$output output shows correct previous pointer" '
+		generate_expect >expect <<-EOF &&
+		5 NONE
+		1 $(git rev-parse modified^) one
+		10 NONE
+		1 $(git rev-parse modified^) two
+		5 NONE
+		EOF
+		perl read-porcelain.pl previous <output >actual &&
+		test_cmp expect actual
+	'
+done
+
+test_done
-- 
2.11.0.519.g31435224cf

Re: [PATCH 0/3] fixing some random blame corner cases

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   [1/3]: blame: fix alignment with --abbrev=40
>   [2/3]: blame: handle --no-abbrev
>   [3/3]: blame: output porcelain "previous" header for each file

Thanks. 1 & 2 obviously look correct.  I'd need to look at 3 when I
am not exhausted, even though I expect it to be also fine from a
cursory read.

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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(&prev->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.

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 <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?

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 <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