Re: [PATCH 1/4] gitweb: notes feature
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Feb 7, 2010, 10:57 UTC
- Message-ID
- <cb7bb73a1002070257g8f1e59dvf124c8966cdf3270@mail.gmail.com>
- In-Reply-To
- <201002070220.36897.jnareb@gmail.com>
On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 10 quoted lines
> 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.
Show 21 quoted lines
>> 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.
[...]
Show 11 quoted lines
>>> 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.
[...]
Show 8 quoted lines
>> 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.
Show 17 quoted lines
>> 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