Re: [PATCH 1/4] gitweb: notes feature
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Feb 6, 2010, 22:58 UTC
- Message-ID
- <cb7bb73a1002061458s5b2c1e7ere83111429473d11c@mail.gmail.com>
- In-Reply-To
- <201002062314.00631.jnareb@gmail.com>
On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 22 quoted lines
> 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.
Show 33 quoted lines
>>> 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.
Show 26 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. > > 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.
Show 19 quoted lines
> 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