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

2010/2/6 Jakub Narebski [off-list ref]:

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

BTW. shouldn't this series be marked as RFC?

[snip]

Signoff?

Y'know, one would figure that, this not being my first contribution
and what, I'd have learned to do this properly 8-/.

quoted

+     # Notes support. When this feature is enabled, the presence of notes
+     # for any commit is signaled, and the note content is made available
+     # in a way appropriate for the current view.
+     # Set this to '*' to enable all notes namespace, or to a shell-glob
+     # specification to enable specific namespaces only.

It is not obvious from this description that you can provide _list_ of
notes namespaces (or list of shell-globs).

I'm starting to think it might make sense to not have a list here, but
rather a single value only. First of all, multiple refs can be
indicated à la shell with {ref1,ref2,ref3}. Or, we can also use the
intended command-line syntax ref1:ref2:ref3, which would help
consistency. It also makes things easier for project overrides, as per
your subsequent comment:

quoted

+
+     # To enable system wide have in $GITWEB_CONFIG
+     # $feature{'notes'}{'default'} = ['*'];
+     # To have project specific config enable override in $GITWEB_CONFIG
+     # $feature{'notes'}{'override'} = 1;
+     # and in project config gitweb.notes = namespace;

How you can provide list of notes here?  Is overriding limited to single
name or shell-glob?

See feature_snapshot for example implementation.

As mentioned above, I'd rather use the same syntax deployed on the
command line, either shell-like or PATH-like multiple paths.

Second, perhaps it is time to refactor all those similar feature_xxx
subroutines (just a possible suggestion)?

feature_notes looks remarkably like feature_avatar, indeed.

quoted

+# return all refs matching refs/notes/<globspecs> where the globspecs
+# are taken from the notes feature content.
+sub get_note_refs {
+     my @globs = gitweb_get_feature('notes');
+     my @note_refs = ();
+     foreach my $glob (@globs) {
+             if (open my $fd, '-|', git_cmd(), 'for-each-ref',
+                 '--format=%(refname)', "refs/notes/$glob") {

               open my $fd, '-|', git_cmd(), 'for-each-ref',
                       '--format=%(refname)', "refs/notes/$glob"
                       or return;

would reduce indent level a bit.

Good idea, thanks.

quoted

+                     while (<$fd>) {
+                             chomp;
+                             push @note_refs, $_ if $_;
+                     }

Why not simply

               chomp(@note_refs = <$fd>);

Because I didn't know chomp worked on lists. Thanks for the idea.

quoted

+             my %notes = () ;
+             foreach my $note_ref (@note_refs) {
+                     my $obj = "$note_ref:$co{'id'}";
+                     if (open my $fd, '-|', git_cmd(), 'rev-parse',
+                             '--verify', '-q', $obj) {
+                             my $exists = <$fd>;
+                             close $fd;
+                             if (defined $exists) {
+                                     if (open $fd, '-|', git_cmd(), 'show', $obj) {
+                                             $notes{$note_ref} = scalar <$fd>;
+                                             close $fd;
+                                     }
+                             }
+                     }
+             }

First, there are '--batch' and '--batch-check' options to git-cat-file.
With these I think you can get all notes with just single git command,
although using it is a bit complicated (requires open2 from IPC::Open2
for bidi communication).

Hm. The IPC::Open2 doc makes it sound horribly scary, but still doable.

Second, if not using 'git cat-file --batch', perhaps it would be easier
to read each $note_ref tree using 'git ls-tree'/'git ls-tree -r', and
parse its output to check for which commits/objects there are notes
available, and only then call 'git show' (or reuse connection to
'git cat-file --batch').

The second solution, with a bit more work, could work even in presence
of fan-out schemes for notes, I think.

An interesting approach. Without fan-out, git ls-tree -r
refs/notes/whatever [hash ...] gives us the blobs we're interested in.
In case of fan-out schemes, the efficiency of this approach probably
depends on the kind of fan-out we have, and would require some
heavy-duty grepping. A git ls-notes plumbing with a similar syntax and
output would be a nice thing to have.

-- 
Giuseppe "Oblomov" Bilotta