Skip to content

Commit

Permalink
gitweb: Use "previous" header of git-blame -p in 'blame' view
Browse files Browse the repository at this point in the history
Luben Tuikov changed 'lineno' link (line number link) from pointing to
'blame' view at given line at blamed commit, to the one at parent of
blamed commit in
  244a70e (Blame "linenr" link jumps to previous state at
           "orig_lineno", 2007-01-04).
This made it possible to do data mining using 'blame' view, by going
through history of a line using mentioned line number link.

Original implementation called "git rev-parse <commit>^" to find SHA-1
of a parent of a given commit once per each blamed line.  In
  39c19ce (gitweb: cache $parent_commit info in git_blame(),
           2008-12-11)
this was improved so rev-parse was called once per each unique commit
in git-blame output.  Alternate solution would be to relax validation
for 'hb' parameter by allowing extended SHA-1 syntax of the form
<rev>^ (perhaps redirecting to gitweb URL with <rev>^ resolved, in
practice moving call to rev-parse to 'the other side of link').

This solution had a bug that it didn't work for boundary commits.
Boundary commits don't have parents, so "git rev-parse <commit>^"
returned literal "<commit>^" (which didn't exists).  Gitweb didn't
detect this situation and passed this result literally as 'hb'
parameter in 'linenr' link.  Following such link currently gives
  400 - Invalid hash base parameter
error; 'hb' parameter is restricted via validate_refname to correct
refnames and doesn't allow for extended SHA-1 syntax.  This bug could
have been fixed alternatively by checking if commit is boundary commit,
or check if rev-parse result is unchanged (still ends in '^' prefix).

The solution employing rev-parse to find parent of commit had inherent
problem if blamed commit renamed file; then name of file would be
different in its parent.  Solving this outside git-blame would be
difficult and costly (at least cost of additional fork for extra git
command).

Currently gitweb uses information in "previous" header, which was
introduced by Junio C Hamano in
  96e1170 (blame: show "previous" information in
           --porcelain/--incremental format, 2008-06-04)
This (currently undocumented) header has the following format:
  "previous <sha1 of parent commit> <filename at parent>"
Using "previous" header solves both problem of performance and the
problem that blamed commit could have renaming blamed file.

Because "previous" header can be repeated for the same commit when
blamed commit is merge (has more than one parent), and we are
interested usually in _first_ parent, currently we store only first
value if blame header repeats.  Using first parent (first "previous"
line) was what gitweb did before; without this change gitweb would use
last parent instead.

If there is no previous commit 'linenr' link points to blamed commit
and blamed filename, making it work correctly for boundary commits.

Acked-by: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jakub Narebski authored and Junio C Hamano committed Jul 25, 2009
1 parent 6de9433 commit 1193042
Showing 1 changed file with 15 additions and 14 deletions.
29 changes: 15 additions & 14 deletions gitweb/gitweb.perl
Original file line number Diff line number Diff line change
Expand Up @@ -4827,7 +4827,7 @@ sub git_blame {
chomp $data;
last if ($data =~ s/^\t//); # contents of line
if ($data =~ /^(\S+)(?: (.*))?$/) {
$meta->{$1} = $2;
$meta->{$1} = $2 unless exists $meta->{$1};
}
}
my $short_rev = substr($full_rev, 0, 8);
Expand All @@ -4852,20 +4852,21 @@ sub git_blame {
esc_html($short_rev));
print "</td>\n";
}
my $parent_commit;
if (!exists $meta->{'parent'}) {
open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
or die_error(500, "Open git-rev-parse failed");
$parent_commit = <$dd>;
close $dd;
chomp($parent_commit);
$meta->{'parent'} = $parent_commit;
} else {
$parent_commit = $meta->{'parent'};
}
# 'previous' <sha1 of parent commit> <filename at commit>
if (exists $meta->{'previous'} &&
$meta->{'previous'} =~ /^([a-fA-F0-9]{40}) (.*)$/) {
$meta->{'parent'} = $1;
$meta->{'file_parent'} = unquote($2);
}
my $linenr_commit =
exists($meta->{'parent'}) ?
$meta->{'parent'} : $full_rev;
my $linenr_filename =
exists($meta->{'file_parent'}) ?
$meta->{'file_parent'} : unquote($meta->{'filename'});
my $blamed = href(action => 'blame',
file_name => $meta->{'filename'},
hash_base => $parent_commit);
file_name => $linenr_filename,
hash_base => $linenr_commit);
print "<td class=\"linenr\">";
print $cgi->a({ -href => "$blamed#l$orig_lineno",
-class => "linenr" },
Expand Down

0 comments on commit 1193042

Please sign in to comment.