Merge branch 'jk/blame-fixes'
authorJunio C Hamano <gitster@pobox.com>
Wed, 18 Jan 2017 23:12:13 +0000 (15:12 -0800)
committerJunio C Hamano <gitster@pobox.com>
Wed, 18 Jan 2017 23:12:13 +0000 (15:12 -0800)
"git blame --porcelain" misidentified the "previous" <commit, path>
pair (aka "source") when contents came from two or more files.

* jk/blame-fixes:
blame: output porcelain "previous" header for each file
blame: handle --no-abbrev
blame: fix alignment with --abbrev=40

builtin/blame.c
t/t8002-blame.sh
t/t8011-blame-split-file.sh [new file with mode: 0755]
index ab54a5c1f48ffcb87193eafb69d45a0f8d378c5e..126b8c9e5ba627ee5514f5a472bd7ae5cb9f9f67 100644 (file)
@@ -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,
@@ -2655,9 +2660,11 @@ 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++;
+       else if (!abbrev)
+               abbrev = GIT_SHA1_HEXSZ;
 
        if (revs_file && read_ancestry(revs_file))
                die_errno("reading graft file '%s' failed", revs_file);
index ab79de95441f4f474525b48b14313f1d04172fce..380e1c1054de5d55a80cb558fea0791884ad7f38 100755 (executable)
@@ -86,4 +86,36 @@ 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_expect_success '--no-abbrev works like --abbrev=40' '
+       check_abbrev 40 --no-abbrev
+'
+
 test_done
diff --git a/t/t8011-blame-split-file.sh b/t/t8011-blame-split-file.sh
new file mode 100755 (executable)
index 0000000..8311250
--- /dev/null
@@ -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