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

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

On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
>> On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> So it is to be single shell-glob / fnmatch (I think) compatible pattern,
>>> isn't it?
>>
>> Sort of. fnmatch doesn't do brace expansion, which is a pity IMO, but
>> that's just my personal preference.
>
> Well, fnmatch is what I think git uses for <pattern> e.g. for
> git-for-each-ref.

fnmatch is the function to use to match a single component. The
question is how to group components together in a single spec (aside
from shell globs and []); my personal choice would be brace expansion,
colon-separated was suggested by (IIRC) Junio. Of course, for gitweb
I'll go with whatever is chosen for core.

>> Colon-separated, fnmatched components is probably the easiest thing to
>> implement to have multiple refs. I'll go with whatever is chosen for
>> core.
>
> I think that having actual list of patterns in $feature{'notes'}{'default'}
> might be more clear; you would still need colon separated (or space
> separated) list of patterns in per-repo override in gitweb.notes config
> variable.
>
> So it would be
>
>  $feature{'notes'}{'default'} = ['commits', '*svn*'];
>  $feature{'notes'}{'override'} = 1;
>
> but
>
>  [gitweb]
>        notes = commits:*svn*
>
> Note that refs names cannot contain either colon ':' or space ' '
> (see git-check-ref-format).

That makes sense. Colon-separated values will probably be allowed in
$feature{'notes'} too, which will keep the code more consistent.

[...]

>>> The above fragment of code is tested that it works.  You would probably
>>> need to replace dies with something less fatal...
>>
>> On the other hand, as mentioned by Junio, this approach is not
>> future-proof enough for any kind of fan-out schemes.
>
> On the third hand ;-P you propose below a trick to deal with fan-out
> schemes, assuming that they use 2-character component breaking.
>
> Also, perhaps "git notes show" should acquire --batch / --batch-check
> options, similar to git-cat-file's options of the same name?

There is little doubt that batch note processing should be available
in core, be it with git notes show --batch(-check), git ls-notes, or
both. I also agree with Johan that gitweb should use these mechanisms
as soon as they are available. The approaches we're discussing here
are basically a temporary workaround for the missing core support.

[...]

>> If we have a guarantee that the fan-outs follow a 2/[2/...] scheme,
>> the open2 approach might still be the best way to go, by just trying
>> not only namespace:xxxxx...xxx but also namespace:xx/xxxxx etc.
>> Horrible, but could still be coalesced in a single call. It mgiht also
>> be optimized to stop at the first successfull hit in a namespace.
>
> Nice trick!  It seems like quite a good idea... but it would absolutely
> require using 'git cat-file --batch' rather than one git-show per try.

Oh, that's a given. git-show was just the only approach I could think
of not knowing about the batch processing git commands.

>> I'm not getting these in the repo I'm testing this. And I think this
>> is indeed the behavior of current git next
>
> Errr, I not made myself clear.
>
> I have added a note to a commit, using "git notes edit d6bbe7f".  Now if
> you take a look at gitweb output for this commit (e.g. 'commit' view for
> this commit) using gitweb without your changes, you would see that it
> flattened notes at the bottom of the commit message (which I think is
> intended result by notes implementation).
>
> If you run the command that parse_commit runs, namely
>
>  $ git rev-list --parents --header -z --max-count=1 \
>    d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3
>
> you would get (up to invisible NUL characters) the output shown above.

And that's what I don't get. git version 1.7.0.rc1.193.ge8618. If I
remember correctly, the behaviour of automatically displaying notes in
git log & friends was changed recently. So git log -1
e8618c52b5f815624251f048609744c9558d92a1 gives me the notes I put
there for testing, git rev-list --parents --header -z --max-count=1
does not.

-- 
Giuseppe "Oblomov" Bilotta