From: Giuseppe Bilotta Date: Sat, 06 Feb 2010 22:58:54 GMT Subject: Re: [PATCH 1/4] gitweb: notes feature Message-ID: In-Reply-To: <201002062314.00631.jnareb@gmail.com> On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski wrote: > On Sat, 6 Feb 2010, Giuseppe Bilotta wrote: >> 2010/2/6 Jakub Narebski : >>> 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 1265309578 +0100 >  committer Jakub Narebski 1265309578 +0100 > >      This is a commit message > >      Signed-off-by: Jakub Narebski > >  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