git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCHv2 03/10] gitweb: Use "previous" header of git-blame -p in 'blame' view

From
Jakub Narebski <jnareb@gmail.com>
Date
Jul 24, 2009, 22:44 UTC
Message-ID
<1248475450-5668-4-git-send-email-jnareb@gmail.com>
In-Reply-To
<1248475450-5668-1-git-send-email-jnareb@gmail.com>
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>
---
Note that it does not change "sha1" link from 'commit' to 'commitdiff'
view, as requested by Junio C Hamano.  This is left for later,
separate commit.
New in v2:
 * we don't use totally unnecessary newly introduced unquote_maybe(),
   as unquote == unquote_maybe (unquotes only when necessary).
 * Added ACK from Luben Tuikov (author of 'data mining' behaviour)
   http://article.gmane.org/gmane.comp.version-control.git/123154
BTW. I think as a side effect this patch makes code a bit cleaner.
 gitweb/gitweb.perl |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3078b92..b8a121b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4827,7 +4827,7 @@ HTML
 			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);
@@ -4852,20 +4852,21 @@ HTML
 			              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" },
-- 
1.6.3.3
Previous: Junio C HamanoNext: Jakub Narebski
Message 7 of 21 in “[PATCHv2 00/10] gitweb: 'blame' view improvements”
  1. Jakub NarebskiJul 24, 2009
  2. 01/10 gitweb: Make .error style genericJakub Narebski, Jul 24, 2009
  3. 02/10 gitweb: Mark boundary commits in 'blame' viewJakub Narebski, Jul 24, 2009
  4. Junio C HamanoJul 25, 2009
  5. Jakub NarebskiJul 25, 2009
  6. Junio C HamanoJul 25, 2009
  7. 03/10 gitweb: Use "previous" header of git-blame -p in 'blame' viewJakub Narebski, Jul 24, 2009
  8. 04/10 gitweb: Mark commits with no "previous" in 'blame' viewJakub Narebski, Jul 24, 2009
  9. 05/10 gitweb: Add author initials in 'blame' view, a la "git gui blame"Jakub Narebski, Jul 24, 2009
  10. 06/10 gitweb: Use light/dark for class names also in 'blame' viewJakub Narebski, Jul 24, 2009
  11. 07/10 gitweb: Add -partial_query option to href() subroutineJakub Narebski, Jul 24, 2009
  12. 08/10 gitweb: Add optional "time to generate page" info in footerJakub Narebski, Jul 24, 2009
  13. 09/10 gitweb: Incremental blame (proof of concept)Jakub Narebski, Jul 24, 2009
  14. Jakub NarebskiJul 25, 2009
  15. 10/10 gitweb: Create links leading to 'blame_incremental' using JavaScriptJakub Narebski, Jul 24, 2009
  16. Martin KoeglerJul 25, 2009
  17. Jakub NarebskiJul 26, 2009
  18. Martin KoeglerJul 27, 2009
  19. Jakub NarebskiJul 27, 2009
  20. Junio C HamanoJul 24, 2009
  21. Jakub NarebskiJul 25, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.