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

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

BTW. shouldn't this series be marked as RFC?

> 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?
> ---
>  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).

> +
> +	# 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.

> +	'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)?

> +
>  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>);

> +			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.

> +		$co{'notes'} = \%notes;
>  		push @cos, \%co;
>  	}
>  	close $fd;
> -- 
> 1.7.0.rc1.193.ge8618
> 
> 

-- 
Jakub Narebski
Poland