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

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 10 Dec 2008, Luben Tuikov wrote:
> --- On Tue, 12/9/08, Jakub Narebski <jnareb@gmail.com> wrote:

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