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

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Thu, Feb 4, 2010 at 6:21 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

> 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