Skip to content

Commit 5fbb42a

Browse files
gitstergitster
committed
Merge branch 'jk/blame-fixes' into maint
"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
2 parents b1e4e17 + 4e76832 commit 5fbb42a

File tree

3 files changed

+166
-10
lines changed

3 files changed

+166
-10
lines changed

‎builtin/blame.c

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1700,13 +1700,23 @@ static void get_commit_info(struct commit *commit,
17001700
}
17011701

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

17121722
/*
@@ -1735,11 +1745,6 @@ static int emit_one_suspect_detail(struct origin *suspect, int repeat)
17351745
printf("summary %s\n", ci.summary.buf);
17361746
if (suspect->commit->object.flags & UNINTERESTING)
17371747
printf("boundary\n");
1738-
if (suspect->previous) {
1739-
struct origin *prev = suspect->previous;
1740-
printf("previous %s ", oid_to_hex(&prev->commit->object.oid));
1741-
write_name_quoted(prev->path, stdout, '\n');
1742-
}
17431748

17441749
commit_info_destroy(&ci);
17451750

@@ -1760,7 +1765,7 @@ static void found_guilty_entry(struct blame_entry *ent,
17601765
oid_to_hex(&suspect->commit->object.oid),
17611766
ent->s_lno + 1, ent->lno + 1, ent->num_lines);
17621767
emit_one_suspect_detail(suspect, 0);
1763-
write_filename_info(suspect->path);
1768+
write_filename_info(suspect);
17641769
maybe_flush_or_die(stdout, "stdout");
17651770
}
17661771
pi->blamed_lines += ent->num_lines;
@@ -1884,7 +1889,7 @@ static void emit_porcelain_details(struct origin *suspect, int repeat)
18841889
{
18851890
if (emit_one_suspect_detail(suspect, repeat) ||
18861891
(suspect->commit->object.flags & MORE_THAN_ONE_PATH))
1887-
write_filename_info(suspect->path);
1892+
write_filename_info(suspect);
18881893
}
18891894

18901895
static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
@@ -2656,9 +2661,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
26562661
} else if (show_progress < 0)
26572662
show_progress = isatty(2);
26582663

2659-
if (0 < abbrev)
2664+
if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
26602665
/* one more abbrev length is needed for the boundary commit */
26612666
abbrev++;
2667+
else if (!abbrev)
2668+
abbrev = GIT_SHA1_HEXSZ;
26622669

26632670
if (revs_file && read_ancestry(revs_file))
26642671
die_errno("reading graft file '%s' failed", revs_file);

‎t/t8002-blame.sh

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,36 @@ test_expect_success 'blame with showEmail config true' '
8686
test_cmp expected_n result
8787
'
8888

89+
test_expect_success 'set up abbrev tests' '
90+
test_commit abbrev &&
91+
sha1=$(git rev-parse --verify HEAD) &&
92+
check_abbrev () {
93+
expect=$1; shift
94+
echo $sha1 | cut -c 1-$expect >expect &&
95+
git blame "$@" abbrev.t >actual &&
96+
perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
97+
test_cmp expect actual.sha
98+
}
99+
'
100+
101+
test_expect_success 'blame --abbrev=<n> works' '
102+
# non-boundary commits get +1 for alignment
103+
check_abbrev 31 --abbrev=30 HEAD &&
104+
check_abbrev 30 --abbrev=30 ^HEAD
105+
'
106+
107+
test_expect_success 'blame -l aligns regular and boundary commits' '
108+
check_abbrev 40 -l HEAD &&
109+
check_abbrev 39 -l ^HEAD
110+
'
111+
112+
test_expect_success 'blame --abbrev=40 behaves like -l' '
113+
check_abbrev 40 --abbrev=40 HEAD &&
114+
check_abbrev 39 --abbrev=40 ^HEAD
115+
'
116+
117+
test_expect_success '--no-abbrev works like --abbrev=40' '
118+
check_abbrev 40 --no-abbrev
119+
'
120+
89121
test_done

‎t/t8011-blame-split-file.sh

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
#!/bin/sh
2+
3+
test_description='
4+
The general idea is that we have a single file whose lines come from
5+
multiple other files, and those individual files were modified in the same
6+
commits. That means that we will see the same commit in multiple contexts,
7+
and each one should be attributed to the correct file.
8+
9+
Note that we need to use "blame -C" to find the commit for all lines. We will
10+
not bother testing that the non-C case fails to find it. That is how blame
11+
behaves now, but it is not a property we want to make sure is retained.
12+
'
13+
. ./test-lib.sh
14+
15+
# help avoid typing and reading long strings of similar lines
16+
# in the tests below
17+
generate_expect () {
18+
while read nr data
19+
do
20+
i=0
21+
while test $i -lt $nr
22+
do
23+
echo $data
24+
i=$((i + 1))
25+
done
26+
done
27+
}
28+
29+
test_expect_success 'setup split file case' '
30+
# use lines long enough to trigger content detection
31+
test_seq 1000 1010 >one &&
32+
test_seq 2000 2010 >two &&
33+
git add one two &&
34+
test_commit base &&
35+
36+
sed "6s/^/modified /" <one >one.tmp &&
37+
mv one.tmp one &&
38+
sed "6s/^/modified /" <two >two.tmp &&
39+
mv two.tmp two &&
40+
git add -u &&
41+
test_commit modified &&
42+
43+
cat one two >combined &&
44+
git add combined &&
45+
git rm one two &&
46+
test_commit combined
47+
'
48+
49+
test_expect_success 'setup simulated porcelain' '
50+
# This just reads porcelain-ish output and tries
51+
# to output the value of a given field for each line (either by
52+
# reading the field that accompanies this line, or referencing
53+
# the information found last time the commit was mentioned).
54+
cat >read-porcelain.pl <<-\EOF
55+
my $field = shift;
56+
while (<>) {
57+
if (/^[0-9a-f]{40} /) {
58+
flush();
59+
$hash = $&;
60+
} elsif (/^$field (.*)/) {
61+
$cache{$hash} = $1;
62+
}
63+
}
64+
flush();
65+
66+
sub flush {
67+
return unless defined $hash;
68+
if (defined $cache{$hash}) {
69+
print "$cache{$hash}\n";
70+
} else {
71+
print "NONE\n";
72+
}
73+
}
74+
EOF
75+
'
76+
77+
for output in porcelain line-porcelain
78+
do
79+
test_expect_success "generate --$output output" '
80+
git blame --root -C --$output combined >output
81+
'
82+
83+
test_expect_success "$output output finds correct commits" '
84+
generate_expect >expect <<-\EOF &&
85+
5 base
86+
1 modified
87+
10 base
88+
1 modified
89+
5 base
90+
EOF
91+
perl read-porcelain.pl summary <output >actual &&
92+
test_cmp expect actual
93+
'
94+
95+
test_expect_success "$output output shows correct filenames" '
96+
generate_expect >expect <<-\EOF &&
97+
11 one
98+
11 two
99+
EOF
100+
perl read-porcelain.pl filename <output >actual &&
101+
test_cmp expect actual
102+
'
103+
104+
test_expect_success "$output output shows correct previous pointer" '
105+
generate_expect >expect <<-EOF &&
106+
5 NONE
107+
1 $(git rev-parse modified^) one
108+
10 NONE
109+
1 $(git rev-parse modified^) two
110+
5 NONE
111+
EOF
112+
perl read-porcelain.pl previous <output >actual &&
113+
test_cmp expect actual
114+
'
115+
done
116+
117+
test_done

0 commit comments

Comments
 (0)