Re: [PATCH 1/4] gitweb: notes feature
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Feb 5, 2010, 23:44 UTC
- Message-ID
- <201002060044.11225.jnareb@gmail.com>
- In-Reply-To
- <1265300338-25021-2-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
BTW. shouldn't this series be marked as RFC?
Show 8 quoted lines
> Introduce support for notes by collecting them when creating commit > lists. The list of noterefs to look into is configurable, and can be a(n > array of) refspec(s), which will be looked for in the refs/notes > namespace. > > The feature is disabled by default because it's presently not very > efficient (one extra git call per configured refspec, plus two extra git > calls per commit per noteref).
Signoff?
Show 17 quoted lines
> --- > gitweb/gitweb.perl | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 62 insertions(+), 0 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index d0c3ff2..9ba5815 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -411,6 +411,22 @@ our %feature = ( > 'override' => 0, > 'default' => [16]}, > > + # 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).
Show 6 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.
Show 21 quoted lines
> + 'notes' => {
> + 'sub' => \&feature_notes,
> + 'override' => 0,
> + 'default' => []},
> +
> # Avatar support. When this feature is enabled, views such as
> # shortlog or commit will display an avatar associated with
> # the email of the committer(s) and/or author(s).
> @@ -513,6 +529,16 @@ sub feature_patches {
> return ($_[0]);
> }
>
> +sub feature_notes {
> + my @val = (git_get_project_config('notes'));
> +
> + if (@val) {
> + return @val;
> + }
> +
> + return @_;
> +}First, this I think limits overriding in repository config to single value.
Second, perhaps it is time to refactor all those similar feature_xxx subroutines (just a possible suggestion)?
Show 16 quoted lines
> +
> sub feature_avatar {
> my @val = (git_get_project_config('avatar'));
>
> @@ -2786,10 +2812,30 @@ sub parse_commit {
> return %co;
> }
>
> +# 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.
> + while (<$fd>) {
> + chomp;
> + push @note_refs, $_ if $_;
> + }Why not simply
chomp(@note_refs = <$fd>);
Show 34 quoted lines
> + close $fd;
> + }
> + }
> + return @note_refs;
> +}
> +
> sub parse_commits {
> my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
> my @cos;
>
> + my @note_refs = get_note_refs();
> +
> $maxcount ||= 1;
> $skip ||= 0;
>
> @@ -2807,6 +2853,22 @@ sub parse_commits {
> or die_error(500, "Open git-rev-list failed");
> while (my $line = <$fd>) {
> my %co = parse_commit_text($line);
> + 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).
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.
Show 8 quoted lines
> + $co{'notes'} = \%notes;
> push @cos, \%co;
> }
> close $fd;
> --
> 1.7.0.rc1.193.ge8618
>
> -- Jakub Narebski Poland