Re: [PATCH 1/4] gitweb: notes feature

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 4, 2010, 17:21 UTC
Message-ID: <201002041821.22864.jnareb@gmail.com>
In-Reply-To: <7v4olxhrti.fsf@alter.siamese.dyndns.org>

On Thu, 4 Feb 2010, Junio C Hamano wrote:

Show 17 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>
>>> +		my %notes = () ;
>>> +		foreach my $note_ref (@note_refs) {
>>> +			my $obj = "$note_ref:$co{'id'}";
>>
>> I think this look-up is wrong (meaning: will stop working anytime in the
>> future, and needs to be rewritten).
> 
> IOW, the code should be reading output from:
> 
>     GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
> 
> as the notes tree may not be storing notes in a flat one-level namespace
> like you are assuming.

First, for some mechanism of deployment (IIRC Apache's mod_perl) changes to environment variables from CGI script are not passed to invoked commands (I guess for security reasons). That is why gitweb uses --git-dir parameter to git wrapper, and not GIT_DIR environment variable since 25691fb (gitweb: Use --git-dir parameter instead of setting $ENV{'GIT_DIR'}, 2006-08-28). So for proper support we would need --notes-ref (or similar) option to git wrapper

  git --notes-ref=$note_ref show -s --format=%N $co{'id'}

Second, parse_commit / parse_commits use

  git rev-list -z --parents --header --max-count-X

If this command automatically shows notes (or it can be modified to automatically show notes) after unindented "Notes:" line (as per git-notes documentation), then the only thing that needs to be changed to fill %commit{'notes'} is parse_commit_text subroutine.

There would be no need for extra subroutine (and extra command, or even up to two extra commands per note) to get notes data.

-- 
Jakub Narebski
Poland