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

On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski [off-list ref] wrote:

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

quoted

On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski [off-list ref] wrote:

quoted

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.

quoted

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.

[...]

quoted

quoted

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.

[...]

quoted

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.

quoted

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