Re: [PATCH 1/4] gitweb: notes feature
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Feb 6, 2010, 09:02 UTC
- Message-ID
- <cb7bb73a1002060102n5544f825x3d01860b1228cee@mail.gmail.com>
- In-Reply-To
- <201002060044.11225.jnareb@gmail.com>
2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> 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-/.
Show 8 quoted lines
>> + # 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:
Show 11 quoted lines
>> +
>> + # 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.
Show 14 quoted lines
>> +# 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.
Show 8 quoted lines
>> + 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.
Show 20 quoted lines
>> + 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.
Show 8 quoted lines
> 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