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

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

On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
>> 2010/2/6 Jakub Narebski <jnareb@gmail.com>:
>>> On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
>
>>>> +     # 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:
>
> 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. Colon-separated, fnmatched
components is probably the easiest thing to implement to have multiple
refs. I'll go with whatever is chosen for core.

>>> 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.
>
> It would look something like the following (fragment of my WIP code):
>
>        use IPC::Open2 qw(open2);
>        use IO::Handle;
>
>        # ...
>
>        unless ($object_stdout) {
>                # Open bidi pipe the first time get_object is called.
>                # open2 raises an exception on error, no need to 'or die'.
>                $object_pid =
>                        open2($object_stdout, $object_stdin,
>                              git_cmd(), 'cat-file', '--batch');
>        }
>        $object_stdin->printflush("$object_id\n") # NB: \n required to avoid deadlocking
>                or die "get_object: cannot write to pipe: $!";
>        my ($sha1, $type, $size) =
>                split ' ', $object_stdout->getline()
>                or die "get_object: cannot read from pipe: $!";
>        die "'$object_id' not found in repository"
>                if $type eq 'missing';
>        $object_stdout->read(my $content, $size);
>        $object_stdout->getline();  # eat trailing newline
>
> 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.

>>> 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.
>
> No grepping, just pass '-r' option to 'git-ls-tree', and use
> parse_ls_tree_line() to parse lines.  Then if we have fanout scheme
> we would get, I guess, something like the following:
>
>  100644 blob 23da787d...       de/adbeef...
>  100644 blob bc10f25f...       c5/31d986...
>  100644 blob c9656ece...       24/d93129...
>
> Now you only need to s!/!!g on filename to get SHA1 of annotated object
> (for which is the note).

What worries me is that you're going to get fan-outs when there are
LOTS of notes, and that's precisely the kind of situation where you
_don't_ want to go through all the notes to pick the ones you're only
interested in.

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.

> P.S. We still would want parse_commit_text to parse notes from default
> namespace.  parse_commit / parse_commits output contains notes from
> default namespace, e.g.:
>
>  d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3 94ac0c7b30a7dc43d926b0ffbe90892c1e19e5f6
>  tree b9ee8876df81b80b13c6b017be993fff8427cfaf
>  parent 94ac0c7b30a7dc43d926b0ffbe90892c1e19e5f6
>  author Jakub Narebski <jnareb@gmail.com> 1265309578 +0100
>  committer Jakub Narebski <jnareb@gmail.com> 1265309578 +0100
>
>      This is a commit message
>
>      Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>
>  Notes:
>      This is just a note for commit d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3
>
> to get commit message lines in $co{'comment'} (as array reference),
> and notes in $co{'note'} (or $co{'notes'}).

I'm not getting these in the repo I'm testing this. And I think this
is indeed the behavior of current git next

-- 
Giuseppe "Oblomov" Bilotta