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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Feb 4, 2010, 20:08 UTC
Message-ID: <cb7bb73a1002041208q54ff1f57y3202e88ae2f5f44e@mail.gmail.com>
In-Reply-To: <201002041821.22864.jnareb@gmail.com>

On Thu, Feb 4, 2010 at 6:21 PM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 28 quoted lines

> On Thu, 4 Feb 2010, Junio C Hamano wrote:
>> 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'}

As I mentioned on the cover letter, I was hoping to be able to make something that could be deployable without requiring core changes and thus a specific minimum git version. I do realize however that this is inherently not robust (unless the code is updated if and when the notes storage mechanism changes).

The wrapper is probably needed anyway, I would say, so it makes sense to implement it. However, see below for a caveat about its implementation.

Show 11 quoted lines

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

But unless the --notes-ref is made to accept multiple refspecs, this will only access _one_ note namespace. It is not hard to envision situations where multiple namespaces are used for the same repo (e.g. one to store git-svn metadata, one for bugzilla-related entries), in which case you'd want _all_ of them to be accessible in gitweb.

And if we do support multiple refspecs for --notes-ref, we also need some way to identify which note belongs to which namespace, of course, in any of the output formats that include notes.

An alternative approach would be some git notes-list command that accepts both multiple namespaces and multiple commits, and is specifically dedicated to display notes-commits relationships (together with notes content, obviously)

-- 
Giuseppe "Oblomov" Bilotta