mirrors/git
0
0
Fork 0
mirror of https://github.com/git/git.git synced 2024-11-23 18:05:29 +08:00
Code Issues Packages Projects Releases Wiki Activity

blame: output porcelain "previous" header for each file

Browse Source 
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Jeff King 2017-01-05 23:20:51 -05:00 committed by Junio C Hamano
parent ed58d8088b
commit 4e76832984
2 changed files with 131 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
builtin/blame.c
View 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,

117
t/t8011-blame-split-file.sh Executable file
View File

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