Re: [PATCH 2/3] gitweb: Cache $parent_commit info in git_blame()

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 10, 2008, 15:15 UTC
Message-ID: <200812101615.55340.jnareb@gmail.com>
In-Reply-To: <182871.96175.qm@web31804.mail.mud.yahoo.com>

On Wed, 10 Dec 2008, Luben Tuikov wrote:

> --- On Tue, 12/9/08, Jakub Narebski <jnareb@gmail.com> wrote:

Show 6 quoted lines

> > This patch attempts to migitate issue [of performance] a bit by
> > caching $parent_commit info in %metainfo, which makes gitweb to
> > call git-rev-parse only once per unique commit in blame output.
> 
> Have you tested this patch that it gives the same commit chain
> as before it?

The only difference between precious version and this patch is that now, if you calculate sha-1 of $long_rev^, it is stored in $metainfo{$long_rev}{'parent'} and not calculated second time.

But I have checked that (at least for single example file) the blame output is identical for before and after this patch.

> > That is what I have noticed during browsing git_blame()
> > code.
> 
> What?

What I have noticed? I have noticed this inefficiency. Why I was browsing git_blame? To write git_blame_incremental...

See also my reply to Nanako Shiraishi with simple benchmark.

> > We can change it to even more effective implementation
> > (like the ones proposed above in the commit message) later.
> 
> Where?

jn> Unfortunately the implementation in 244a70e used one call for jn> git-rev-parse to find parent revision per line in file, instead of jn> using long lived "git cat-file --batch-check" (which might not existed jn> then), or changing validate_refname to validate_revision and made it jn> accept <rev>^, <rev>^^, <rev>^^^ etc. syntax.

One solution mentioned there is to open bidi pipe (like in Git::Repo in gitweb caching by Lea Wiemann) to "git cat-file --batch-check" (the '--batch-check' option was added to git-cat-file by Adam Roben on Apr 23, 2008 in v1.5.6-rc0~8^2~9), feed it $long_rev^, and parse its output of the form:

  926b07e694599d86cec668475071b32147c95034 commit 637

see manpage for git-cat-file(1). Unfortunately it seems like command_bidi_pipe doesn't work as _I_ expected...

Another solution mentioned there is to change validate_refname to validate_revision when checking script parameters (CGI query or path_info), with validate_revision being something like:

  sub validate_revision {
  	my $rev = shift;
	return validate_refname(strip_rev_suffixes($rev));
  }

or something like that, so we don't need to calculate $long_rev^, but can pass "$long_rev^" as 'hb' parameter ($long_rev can in turn also end in '^').

-- 
Jakub Narebski
Poland