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

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

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

quoted

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

quoted

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted

quoted

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:

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.

quoted

quoted

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.

quoted

quoted

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 [off-list ref] 1265309578 +0100
 committer Jakub Narebski [off-list ref] 1265309578 +0100

     This is a commit message

     Signed-off-by: Jakub Narebski [off-list ref]

 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