[PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)

[PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)

[Rafael Garcia-Suarez]

git-rev-parse will abort with an error message on stderr when passed a
non-existent revision spec, such as "deadbeef^" where deadbeef has no
parent. Using the --revs-only parameter makes this error go away, while
retaining functionality, keeping the web server error log nice and clean.

Moreover, when there is no parent commit, direct the blame at the first
commit featuring the file, that is itself. This unbreaks the link on the
line number when the corresponding line had never been modified.

Finally, to avoid forking git-rev-parse too many times, cache its
results in a new hash %parent_commits.

Signed-off-by: Rafael Garcia-Suarez <rgarciasuarez@gmail.com>
---
 gitweb/gitweb.perl |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 55fb100..828cf45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4189,6 +4189,7 @@ sub git_blame2 {
 <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
 HTML
 	my %metainfo = ();
+	my %parent_commits = ();
 	while (1) {
 		$_ = <$fd>;
 		last unless defined $_;
@@ -4226,11 +4227,16 @@ HTML
 			              esc_html($rev));
 			print "</td>\n";
 		}
-		open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
-			or die_error(undef, "Open git-rev-parse failed");
-		my $parent_commit = <$dd>;
-		close $dd;
-		chomp($parent_commit);
+		if (!exists $parent_commits{$full_rev}) {
+		    # --revs-only avoids an error message if $full_rev has no parent
+		    open (my $dd, "-|", git_cmd(), "rev-parse", '--revs-only', "$full_rev^")
+			    or die_error(undef, "Open git-rev-parse failed");
+		    # set the $parent_commit to $full_rev if it has no parent
+		    $parent_commits{$full_rev} = <$dd> || $full_rev;
+		    chomp($parent_commits{$full_rev});
+		    close $dd;
+		}
+		my $parent_commit = $parent_commits{$full_rev};
 		my $blamed = href(action => 'blame',
 		                  file_name => $meta->{'filename'},
 		                  hash_base => $parent_commit);
-- 
1.5.6.rc1

Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)

[Lea Wiemann]

Rafael Garcia-Suarez wrote:
> Finally, to avoid forking git-rev-parse too many times, cache its
> results in a new hash %parent_commits.

I'm not too happy with this:

1) Minor point: I'm working on caching for the backend right now (IOW, 
basically what you're doing, just centralized in a separate module), so 
you're essentially duplicating work, and you're making it (a little) 
harder for me to refactor gitweb since I have to rip out your cache 
code.  Those few lines won't hurt, but in general I suggest that nobody 
make any larger efforts to cache stuff in gitweb for the next few weeks.

2) Major point: You're still forking a lot.  The Right Thing is to 
condense everything into a single call -- I believe "git-rev-list 
--parents --no-walk hash hash hash..." is correct and easily parsable. 
Its output seems to be lines of
     hash parent_1 parent_2 ... parent_n
with n >= 0.  Can you implement that?  It would be very useful and also 
reusable for me!

-- Lea

P.S.: I believe that the usual way to post follow-up patches is to label 
them [PATCH vN] for N >= 2 in the subject (since "take 2" shouldn't be 
part of the commit message), and to send them as In-reply-to a message 
in the original thread -- just provide git-send-email with the 
Message-ID of the message you want to reply to.  </nitpick>

Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame (take 2)

[Rafael Garcia-Suarez]

2008/6/3 Lea Wiemann <lewiemann@gmail.com>:
> Rafael Garcia-Suarez wrote:
>>
>> Finally, to avoid forking git-rev-parse too many times, cache its
>> results in a new hash %parent_commits.
>
> I'm not too happy with this:
>
> 1) Minor point: I'm working on caching for the backend right now (IOW,
> basically what you're doing, just centralized in a separate module), so
> you're essentially duplicating work, and you're making it (a little) harder
> for me to refactor gitweb since I have to rip out your cache code.  Those
> few lines won't hurt, but in general I suggest that nobody make any larger
> efforts to cache stuff in gitweb for the next few weeks.

Sorry for that. I failed to consider that your work was applying there as well.

> 2) Major point: You're still forking a lot.  The Right Thing is to condense
> everything into a single call -- I believe "git-rev-list --parents --no-walk
> hash hash hash..." is correct and easily parsable. Its output seems to be
> lines of
>    hash parent_1 parent_2 ... parent_n
> with n >= 0.  Can you implement that?  It would be very useful and also
> reusable for me!

Ok, I see what you mean -- but I suppose that's the kind of
information that would be nicely abstracted out somewhere. At least I
would put it in a sub. And that sub would fit perfectly in your
caching module, too! Looking at the source, a generic git-rev-list
wrapper could be used.

If you want I can produce a patch against your version, but I'm afraid
I'll end up being a bit short on time, so I might be slow to do it.

> P.S.: I believe that the usual way to post follow-up patches is to label
> them [PATCH vN] for N >= 2 in the subject (since "take 2" shouldn't be part
> of the commit message), and to send them as In-reply-to a message in the
> original thread -- just provide git-send-email with the Message-ID of the
> message you want to reply to.  </nitpick>
>

Ok, noted. Also, next time, I'll send smaller patches.