[PATCH 0/4] gitweb: preliminary notes support

This small patchset introduces notes support in gitweb.

The feature is designed to be powerful and flexible: any amount of notes
ref spaces can be used, and per-namespace styling is possible.

The implementation is not exactly high-performance, needing no less than
one extra git call per commit per notes ref space (two if a note exist),
and is thus disabled by default.

It's quite likely that appropriate C plumbing in git could speed this
considerably; the current implementation, OTOH, has the advantage of
working even when the git core does not support notes itself.

Giuseppe Bilotta (4):
  gitweb: notes feature
  gitweb: show notes in shortlog view
  gitweb: show notes in log
  gitweb: show notes in commit(diff) view

 gitweb/gitweb.css  |   51 +++++++++++++++++++++++
 gitweb/gitweb.perl |  116 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 166 insertions(+), 1 deletions(-)

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).
---
 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.
+
+	# 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;
+	'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 @_;
+}
+
 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") {
+			while (<$fd>) {
+				chomp;
+				push @note_refs, $_ if $_;
+			}
+			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;
+					}
+				}
+			}
+		}
+		$co{'notes'} = \%notes;
 		push @cos, \%co;
 	}
 	close $fd;

-- 
1.7.0.rc1.193.ge8618

The presence of the note is shown by a small icon, hovering on which
reveals the actual note content.
---
 gitweb/gitweb.css  |   29 +++++++++++++++++++++++++++++
 gitweb/gitweb.perl |   30 +++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 50067f2..7d1836b 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -572,3 +572,32 @@ span.match {
 div.binary {
 	font-style: italic;
 }
+
+span.notes {
+	float:right;
+	position:relative;
+}
+
+span.notes span.note-container:before {
+	content: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAgAAAAIAgMAAAC5YVYYAAAAAXNSR0IArs4c6QAAAAlQTFRFAABnybuD//+t5rXizQAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAAGElEQVQI12MIDWXIWglDQHYIQ1YAQ6gDAFWPBrAKFe0fAAAAAElFTkSuQmCC');
+}
+
+span.notes span.note {
+	display:none;
+}
+
+span.notes span.note-container img {
+	content: normal;
+}
+
+span.notes span.note-container:hover span.note {
+	display:block;
+	content:normal;
+	background-color:#ffffad;
+	border:1px solid #c9bb83;
+	padding:4px;margin:0;
+	position:absolute;
+	top:0;right:0;
+	z-index:10;
+	overflow:visible;
+}

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9ba5815..1701ed1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1628,6 +1628,33 @@ sub format_subject_html {
 	}
 }
 
+# display notes next to a commit
+sub format_notes_html {
+	my %notes = %{$_[0]};
+	my $ret = "";
+	while (my ($ref, $text) = each %notes) {
+		# remove 'refs/notes/' and an optional final s
+		$ref =~ s/^refs\/notes\///;
+		$ref =~ s/s$//;
+
+		# double markup is needed to allow pure CSS cross-browser 'popup'
+		# of the note
+		$ret .= "<span title='$ref' class='note-container $ref'>";
+		$ret .= "<span title='$ref' class='note $ref'>";
+		foreach my $line (split /\n/, $text) {
+			$ret .= esc_html($line) . "<br/>";
+		}
+		$ret .= "</span></span>";
+	}
+	if ($ret) {
+		return "<span class='notes'>$ret</span>";
+	} else {
+		return $ret;
+	}
+
+
+}
+
 # Rather than recomputing the url for an email multiple times, we cache it
 # after the first hit. This gives a visible benefit in views where the avatar
 # for the same email is used repeatedly (e.g. shortlog).

@@ -4595,6 +4622,7 @@ sub git_shortlog_body {
 		my %co = %{$commitlist->[$i]};
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
+		my $notes = format_notes_html($co{'notes'});
 		if ($alternate) {
 			print "<tr class=\"dark\">\n";
 		} else {

@@ -4605,7 +4633,7 @@ sub git_shortlog_body {
 		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
 		      format_author_html('td', \%co, 10) . "<td>";
 		print format_subject_html($co{'title'}, $co{'title_short'},
-		                          href(action=>"commit", hash=>$commit), $ref);
+		                          href(action=>"commit", hash=>$commit), $ref . $notes);
 		print "</td>\n" .
 		      "<td class=\"link\">" .
 		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .

-- 
1.7.0.rc1.193.ge8618

The notes are shown in full to the left of the log message.
---
 gitweb/gitweb.css  |   11 +++++++++++
 gitweb/gitweb.perl |   11 +++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 7d1836b..81d66d3 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -601,3 +601,14 @@ span.notes span.note-container:hover span.note {
 	z-index:10;
 	overflow:visible;
 }
+
+div.notes {
+	max-width:150px;
+	float:left;
+}
+
+div.notes div.note {
+	background-color:#ffffad;
+	border:1px solid #c9bb83;
+	padding:4px;margin:0;
+}

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1701ed1..0d0877e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1631,6 +1631,7 @@ sub format_subject_html {
 # display notes next to a commit
 sub format_notes_html {
 	my %notes = %{$_[0]};
+	my $tag = $_[1] || 'span' ;
 	my $ret = "";
 	while (my ($ref, $text) = each %notes) {
 		# remove 'refs/notes/' and an optional final s

@@ -1639,15 +1640,15 @@ sub format_notes_html {
 
 		# double markup is needed to allow pure CSS cross-browser 'popup'
 		# of the note
-		$ret .= "<span title='$ref' class='note-container $ref'>";
-		$ret .= "<span title='$ref' class='note $ref'>";
+		$ret .= "<$tag title='$ref' class='note-container $ref'>";
+		$ret .= "<$tag title='$ref' class='note $ref'>";
 		foreach my $line (split /\n/, $text) {
 			$ret .= esc_html($line) . "<br/>";
 		}
-		$ret .= "</span></span>";
+		$ret .= "</$tag></$tag>";
 	}
 	if ($ret) {
-		return "<span class='notes'>$ret</span>";
+		return "<$tag class='notes'>$ret</$tag>";
 	} else {
 		return $ret;
 	}

@@ -4581,6 +4582,7 @@ sub git_log_body {
 		next if !%co;
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
+		my $notes = format_notes_html($co{'notes'}, 'div');
 		my %ad = parse_date($co{'author_epoch'});
 		git_print_header_div('commit',
 		               "<span class=\"age\">$co{'age_string'}</span>" .

@@ -4598,6 +4600,7 @@ sub git_log_body {
 		      git_print_authorship(\%co, -tag => 'span');
 		      print "<br/>\n</div>\n";
 
+		print "$notes\n";
 		print "<div class=\"log_body\">\n";
 		git_print_log($co{'comment'}, -final_empty_line=> 1);
 		print "</div>\n";

-- 
1.7.0.rc1.193.ge8618

The notes are shown side-by-side along the bottom of the commit message.
---
 gitweb/gitweb.css  |   11 +++++++++++
 gitweb/gitweb.perl |   21 +++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 81d66d3..10acab4 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -145,6 +145,7 @@ div.list_head {
 	border: solid #d9d8d1;
 	border-width: 1px 0px 0px;
 	font-style: italic;
+	clear: both;
 }
 
 .author_date, .author {

@@ -612,3 +613,13 @@ div.notes div.note {
 	border:1px solid #c9bb83;
 	padding:4px;margin:0;
 }
+
+
+div.page_body div.notes {
+	max-width:100%;
+	float:none;
+}
+
+div.page_body div.notes div.note {
+	float:left;
+}

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0d0877e..0d03026 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -2837,12 +2837,31 @@ sub parse_commit {
 	%co = parse_commit_text(<$fd>, 1);
 	close $fd;
 
+	my %notes = ();
+	foreach my $note_ref (get_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;
+				}
+			}
+		}
+	}
+	$co{'notes'} = \%notes;
+
 	return %co;
 }
 
 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+	local $/ = "";
+
 	my @globs = gitweb_get_feature('notes');
 	my @note_refs = ();
 	foreach my $glob (@globs) {

@@ -5875,6 +5894,7 @@ sub git_commit {
 
 	print "<div class=\"page_body\">\n";
 	git_print_log($co{'comment'});
+	print format_notes_html($co{'notes'}, 'div');
 	print "</div>\n";
 
 	git_difftree_body(\@difftree, $hash, @$parents);

@@ -6230,6 +6250,7 @@ sub git_commitdiff {
 			git_print_log($co{'comment'}, -final_empty_line=> 1, -remove_title => 1);
 			print "</div>\n"; # class="log"
 		}
+		print format_notes_html($co{'notes'}, 'div');
 
 	} elsif ($format eq 'plain') {
 		my $refs = git_get_references("tags");

-- 
1.7.0.rc1.193.ge8618

Giuseppe Bilotta [off-list ref] writes:

+		my %notes = () ;
+		foreach my $note_ref (@note_refs) {
+			my $obj = "$note_ref:$co{'id'}";

I think this look-up is wrong (meaning: will stop working anytime in the
future, and needs to be rewritten).

Other parts of this patch looked Ok from a cursory reading, though.

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?

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.

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

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

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

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

Subject: [PATCH 2/4] gitweb: show notes in shortlog view

Is it RFC?

Why it is only for 'shortlog' view, and not also for 'history' which is
also shortlog-like view?  Or is there reason why it is not present for
'history' view?

The presence of the note is shown by a small icon, hovering on which
reveals the actual note content.

Signoff?

Not all web browsers support ':before' pseudo-element, and 'content'
(pseudo-)property.

Not all web browsers support 'data:' URI schema in CSS; also such image
cannot be cached (on the other hand it doesn't require extra TCP 
connection on first access, and CSS file is cached anyway).

On the other hand adding extra images to gitweb would probably require
additional (yet another) build time parameter to tell where static
images are (besides logo and favicon).

So perhaps it is good solution, at least for a first attempt.

[...]

Why not use 'my $notes = shift;', and later '%$notes'?

+	my $ret = "";

Perhaps $return or $result would be a better name, to better distinguish
it from visually similar $ref (see $ref vs $res);

+	while (my ($ref, $text) = each %notes) {
+		# remove 'refs/notes/' and an optional final s
+		$ref =~ s/^refs\/notes\///;

You can use different delimiter than / to avoid 'leaning toothpick'
syndrome, e.g.: $ref =~ s!^refs/notes/!!;

+		$ref =~ s/s$//;
+
+		# double markup is needed to allow pure CSS cross-browser 'popup'
+		# of the note
+		$ret .= "<span title='$ref' class='note-container $ref'>";
+		$ret .= "<span title='$ref' class='note $ref'>";
+		foreach my $line (split /\n/, $text) {
+			$ret .= esc_html($line) . "<br/>";

Probably would want

   			$ret .= esc_html($line) . "<br/>\n";

here.  Or do we want single string here?


Also, do you want/need final <br>?  If not, perhaps

   		join("<br/>", map { esc_html($_) } split(/\n/, $text);

would be a better solution (you can always add final "<br/>" later)?

Nice.

-- 
Jakub Narebski
Poland

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

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

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

[snip]

Signoff?

Y'know, one would figure that, this not being my first contribution
and what, I'd have learned to do this properly 8-/.

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:

quoted

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

As mentioned above, I'd rather use the same syntax deployed on the
command line, either shell-like or PATH-like multiple paths.

Second, perhaps it is time to refactor all those similar feature_xxx
subroutines (just a possible suggestion)?

feature_notes looks remarkably like feature_avatar, indeed.

quoted

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

Good idea, thanks.

quoted

+                     while (<$fd>) {
+                             chomp;
+                             push @note_refs, $_ if $_;
+                     }

Why not simply

               chomp(@note_refs = <$fd>);

Because I didn't know chomp worked on lists. Thanks for the idea.

quoted

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

Hm. The IPC::Open2 doc makes it sound horribly scary, but still doable.

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.

-- 
Giuseppe "Oblomov" Bilotta

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

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted

Subject: [PATCH 2/4] gitweb: show notes in shortlog view

Is it RFC?

See reply to comments on 1/4 8-/

Why it is only for 'shortlog' view, and not also for 'history' which is
also shortlog-like view?  Or is there reason why it is not present for
'history' view?

I always forget about history view, probably because I never use it.

quoted

The presence of the note is shown by a small icon, hovering on which
reveals the actual note content.

Signoff?

A-hem. (whistles innocently)

quoted

+
+span.notes span.note-container:before {
+     content: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAgAAAAIAgMAAAC5YVYYAAAAAXNSR0IArs4c6QAAAAlQTFRFAABnybuD//+t5rXizQAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAAGElEQVQI12MIDWXIWglDQHYIQ1YAQ6gDAFWPBrAKFe0fAAAAAElFTkSuQmCC');
+}

Not all web browsers support ':before' pseudo-element, and 'content'
(pseudo-)property.

I know it's neither good form nor good webdesigner attitude, but I
stopped caring about IE a long time ago. I understand however that
some ancient versions of Mozilla browsers might have the same issue
too.

Not all web browsers support 'data:' URI schema in CSS; also such image
cannot be cached (on the other hand it doesn't require extra TCP
connection on first access, and CSS file is cached anyway).

On the other hand adding extra images to gitweb would probably require
additional (yet another) build time parameter to tell where static
images are (besides logo and favicon).

So perhaps it is good solution, at least for a first attempt.

A possible alternative could maybe do without images and just use
borders and backgrounds of an 8x8 fixed-size element. Wouldn't look as
nice, probably, but should render decently in everything that supports
CSS1.

quoted

+# display notes next to a commit
+sub format_notes_html {
+     my %notes = %{$_[0]};

Why not use 'my $notes = shift;', and later '%$notes'?

No particular reason. I didn't check for syntax preferences regarding
this in gitweb, or I would have noticed there was a preference for the
one you mention.

quoted

+     my $ret = "";

Perhaps $return or $result would be a better name, to better distinguish
it from visually similar $ref (see $ref vs $res);

Yep, good point.

quoted

+     while (my ($ref, $text) = each %notes) {
+             # remove 'refs/notes/' and an optional final s
+             $ref =~ s/^refs\/notes\///;

You can use different delimiter than / to avoid 'leaning toothpick'
syndrome, e.g.: $ref =~ s!^refs/notes/!!;

Indeed I should.

quoted

+             $ref =~ s/s$//;
+
+             # double markup is needed to allow pure CSS cross-browser 'popup'
+             # of the note
+             $ret .= "<span title='$ref' class='note-container $ref'>";
+             $ret .= "<span title='$ref' class='note $ref'>";
+             foreach my $line (split /\n/, $text) {
+                     $ret .= esc_html($line) . "<br/>";

Probably would want

                       $ret .= esc_html($line) . "<br/>\n";

here.  Or do we want single string here?

It's within a span element so I was trying to stick to single line in
the HTML source.

Also, do you want/need final <br>?  If not, perhaps

               join("<br/>", map { esc_html($_) } split(/\n/, $text);

would be a better solution (you can always add final "<br/>" later)?

I did notice that the final br didn't seem to affect the box height,
so I didn't bother looking at ways to do without it, but it's probably
nicer to not have it.

-- 
Giuseppe "Oblomov" Bilotta

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

The notes are shown in full to the left of the log message.

Thats all good if you have wide (high resolution) screen, and your 
project follows common commit message conventions of keeping lines in 
commit message no longer than at most 80 characters, and you don't need 
to use large size fonts.

What happens if screen size is too small to contain both commit message 
and notes?  Does it do the sensible thing of putting notes _below_ 
commit message in such situation?  I do not know CSS+HTML enogh to 
answer this question myself.

BTW. signoff?


P.S. We would probably want some support for notes also in feeds (Atom 
and RSS feed), but this can be left for the future commit.

This could be

   	my $notes = shift;
   	my $tag = shift || 'span' ;

and then use %$notes.

Nice trick, but is this distinction really necessary?

With respect to the question about what happens if the screen is not 
wide enough, shouldn't notes be put in HTML source below body (commit 
message)?

-- 
Jakub Narebski
Poland

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

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted

The notes are shown in full to the left of the log message.

Thats all good if you have wide (high resolution) screen, and your
project follows common commit message conventions of keeping lines in
commit message no longer than at most 80 characters, and you don't need
to use large size fonts.

What happens if screen size is too small to contain both commit message
and notes?  Does it do the sensible thing of putting notes _below_
commit message in such situation?  I do not know CSS+HTML enogh to
answer this question myself.

The CSS forces the width of the notes div at 150px, which is the
amount left to the left of the commit message. This means that notes
will line-wrap, but they will not shift the text.

BTW. signoff?

As usual, I forgot.

P.S. We would probably want some support for notes also in feeds (Atom
and RSS feed), but this can be left for the future commit.

I honestly have absolutely no idea how to do that.

quoted

@@ -1631,6 +1631,7 @@ sub format_subject_html {

 # display notes next to a commit
 sub format_notes_html {
      my %notes = %{$_[0]};
+     my $tag = $_[1] || 'span' ;

This could be

       my $notes = shift;
       my $tag = shift || 'span' ;

and then use %$notes.

Would be much bettere, yes.

quoted

      my $ret = "";
      while (my ($ref, $text) = each %notes) {
              # remove 'refs/notes/' and an optional final s

@@ -1639,15 +1640,15 @@ sub format_notes_html {

              # double markup is needed to allow pure CSS cross-browser 'popup'
              # of the note
-             $ret .= "<span title='$ref' class='note-container $ref'>";
-             $ret .= "<span title='$ref' class='note $ref'>";
+             $ret .= "<$tag title='$ref' class='note-container $ref'>";
+             $ret .= "<$tag title='$ref' class='note $ref'>";
              foreach my $line (split /\n/, $text) {
                      $ret .= esc_html($line) . "<br/>";
              }
-             $ret .= "</span></span>";
+             $ret .= "</$tag></$tag>";
      }
      if ($ret) {
-             return "<span class='notes'>$ret</span>";
+             return "<$tag class='notes'>$ret</$tag>";
      } else {
              return $ret;
      }

Nice trick, but is this distinction really necessary?

I think so.  The distinction is useful both from the structural point
of view (block elements with block elements, inline elements with
inline elements) and for CSS selection (the block case has totally
different styling than the inline case).

quoted

+             print "$notes\n";
              print "<div class=\"log_body\">\n";
              git_print_log($co{'comment'}, -final_empty_line=> 1);
              print "</div>\n";

With respect to the question about what happens if the screen is not
wide enough, shouldn't notes be put in HTML source below body (commit
message)?

As I mentioned, notes width is fixed at the amount of the whitespace
to the left of the log, so this should not be an issue. Additionally,
putting notes below makes it much harder to let them float to the left
of the log body.


-- 
Giuseppe "Oblomov" Bilotta

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

The notes are shown side-by-side along the bottom of the commit
message. 

The same question apply as for previous commit.

What happens if screen size is too small to contain both commit message
and notes?  Does it do the sensible thing of putting notes _below_
commit message in such situation?  I do not know CSS+HTML enogh to
answer this question myself.

Duplicated code.  Please put this code in a separate subroutine, to be
called in those two places.
  

 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+	local $/ = "";
+

Why it is needed here?  Why you want to use empty lines as terminator
(which means reading whole paragraphs), while treating two or more
consecutive empty lines as a single empty line (according to
perlvar(1))?

If you want to slurp whole file, this should be

   	local $/;

or more explicit

   	local $/ = undef;

This of course assumes that we want notes treated exactly (or almost
exactly) the same way for 'log', 'commit' and 'commitdiff' views.
Perhaps it is a good assumption (at least for first step)...

-- 
Jakub Narebski
Poland

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

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

quoted

The notes are shown side-by-side along the bottom of the commit
message.

The same question apply as for previous commit.

What happens if screen size is too small to contain both commit message
and notes?  Does it do the sensible thing of putting notes _below_
commit message in such situation?  I do not know CSS+HTML enogh to
answer this question myself.

In this view the notes are printed side-by-side to each other, but at
the end of the commit message, so there's no interference at all.

quoted

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0d0877e..0d03026 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -2837,12 +2837,31 @@ sub parse_commit {

      %co = parse_commit_text(<$fd>, 1);
      close $fd;

+     my %notes = ();
+     foreach my $note_ref (get_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;
+                             }
+                     }
+             }
+     }
+     $co{'notes'} = \%notes;
+
      return %co;
 }

Duplicated code.  Please put this code in a separate subroutine, to be
called in those two places.

Yup, definitely a good idea.

quoted

 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+     local $/ = "";
+

Why it is needed here?  Why you want to use empty lines as terminator
(which means reading whole paragraphs), while treating two or more
consecutive empty lines as a single empty line (according to
perlvar(1))?

If you want to slurp whole file, this should be

       local $/;

or more explicit

       local $/ = undef;

Ah, sorry, for some reason I thought "" was the default.


-- 
Giuseppe "Oblomov" Bilotta

Giuseppe Bilotta wrote:

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

quoted

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

[...]

quoted

quoted

 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+     local $/ = "";
+

Why it is needed here?  Why you want to use empty lines as terminator
(which means reading whole paragraphs), while treating two or more
consecutive empty lines as a single empty line (according to
perlvar(1))?

If you want to slurp whole file, this should be

       local $/;

or more explicit

       local $/ = undef;

Ah, sorry, for some reason I thought "" was the default.

If you wanted to use default value, why set it at all?

-- 
Jakub Narebski
Poland

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

Giuseppe Bilotta wrote:

quoted

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

quoted

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

[...]

quoted

quoted

quoted

 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+     local $/ = "";
+

Why it is needed here?  Why you want to use empty lines as terminator
(which means reading whole paragraphs), while treating two or more
consecutive empty lines as a single empty line (according to
perlvar(1))?

If you want to slurp whole file, this should be

       local $/;

or more explicit

       local $/ = undef;

Ah, sorry, for some reason I thought "" was the default.

If you wanted to use default value, why set it at all?

Ach, sorry, forgot to reply to the first part of the question. It's
used in a context where $/ is locally set to \0, so it needs to be
reset.

-- 
Giuseppe "Oblomov" Bilotta

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

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

quoted

Giuseppe Bilotta wrote:

quoted

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

quoted

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

[...]

quoted

quoted

quoted

 # return all refs matching refs/notes/<globspecs> where the globspecs
 # are taken from the notes feature content.
 sub get_note_refs {
+     local $/ = "";
+

Why it is needed here?  Why you want to use empty lines as terminator
(which means reading whole paragraphs), while treating two or more
consecutive empty lines as a single empty line (according to
perlvar(1))?

If you want to slurp whole file, this should be

       local $/;

or more explicit

       local $/ = undef;

Ah, sorry, for some reason I thought "" was the default.

If you wanted to use default value, why set it at all?

Ach, sorry, forgot to reply to the first part of the question. It's
used in a context where $/ is locally set to \0, so it needs to be
reset.

Oh, so it should be something like the following, then?

   sub get_note_refs {
  +	# reset to default value (can be called with $/ set to "\0")
  +	local $/ = "\n"; # line by line

-- 
Giuseppe "Oblomov" Bilotta

-- 
Jakub Narebski
Poland

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

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

quoted

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted

The notes are shown in full to the left of the log message.

Thats all good if you have wide (high resolution) screen, and your
project follows common commit message conventions of keeping lines in
commit message no longer than at most 80 characters, and you don't need
to use large size fonts.

What happens if screen size is too small to contain both commit message
and notes?  Does it do the sensible thing of putting notes _below_
commit message in such situation?  I do not know CSS+HTML enogh to
answer this question myself.

The CSS forces the width of the notes div at 150px, which is the
amount left to the left of the commit message. This means that notes
will line-wrap, but they will not shift the text.

[...]

quoted

quoted

+             print "$notes\n";
              print "<div class=\"log_body\">\n";
              git_print_log($co{'comment'}, -final_empty_line=> 1);
              print "</div>\n";

With respect to the question about what happens if the screen is not
wide enough, shouldn't notes be put in HTML source below body (commit
message)?

As I mentioned, notes width is fixed at the amount of the whitespace
to the left of the log, so this should not be an issue. Additionally,
putting notes below makes it much harder to let them float to the left
of the log body.

Perhaps the log body should be floated to the right, instead of notes
being floated to the left, so that when screen width is to narrow for
both commit message and notes, notes would be put below commit message.

A question how to create styles using HTML elements and CSS styling
to get side-by-side with one below other as fallback can be asked
on http://stackoverflow.com, or perhaps even better on http://doctype.com/

-- 
Jakub Narebski
Poland

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

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

quoted

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

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?
 
[...]

quoted

quoted

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

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

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

The identifier of note itself would be either id from tree (e.g. 23da787d...
for note from first line), or note namespace composed with note "pathname",
(e.g. refs/notes/commits:de/adbeef... for note from first line).

Even if you use one git-show per note, it would need only git commands
to discover object to note mapping.


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'}).

-- 
Jakub Narebski
Poland

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

Oh, so it should be something like the following, then?

  sub get_note_refs {
 +     # reset to default value (can be called with $/ set to "\0")
 +     local $/ = "\n"; # line by line

Yes, it's probably worth to mention it in a comment.

-- 
Giuseppe "Oblomov" Bilotta

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

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

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

quoted

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.

Well, fnmatch is what I think git uses for <pattern> e.g. for 
git-for-each-ref.

Colon-separated, fnmatched components is probably the easiest thing to
implement to have multiple refs. I'll go with whatever is chosen for
core. 

I think that having actual list of patterns in $feature{'notes'}{'default'}
might be more clear; you would still need colon separated (or space
separated) list of patterns in per-repo override in gitweb.notes config
variable.

So it would be

  $feature{'notes'}{'default'} = ['commits', '*svn*'];
  $feature{'notes'}{'override'} = 1;

but

  [gitweb]
  	notes = commits:*svn*
 
Note that refs names cannot contain either colon ':' or space ' '
(see git-check-ref-format).

quoted

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.

On the third hand ;-P you propose below a trick to deal with fan-out
schemes, assuming that they use 2-character component breaking.

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

quoted

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.

Right.  This method would be contrary to the goals of fan-out schemes...
well, we could use 'git ls-tree' without '-r' option, or simply 
'git cat-file --batch' to read trees (note that we would get raw, 
unformatted tree, which is parseable with Perl, but it is not that easy),
and go down level-by-level.

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.

Nice trick!  It seems like quite a good idea... but it would absolutely
require using 'git cat-file --batch' rather than one git-show per try.

quoted

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

Errr, I not made myself clear.  

I have added a note to a commit, using "git notes edit d6bbe7f".  Now if
you take a look at gitweb output for this commit (e.g. 'commit' view for
this commit) using gitweb without your changes, you would see that it
flattened notes at the bottom of the commit message (which I think is
intended result by notes implementation).

If you run the command that parse_commit runs, namely

  $ git rev-list --parents --header -z --max-count=1 \
    d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3

you would get (up to invisible NUL characters) the output shown above.
From this output I would like to separate commit message from notes
in parse_commit_text subroutine.

I have set neither GIT_NOTES_REF nor core.notesRef.

-- 
Jakub Narebski
Poland

On Sun, 7 Feb 2010, Jakub Narebski wrote:

Right.  This method would be contrary to the goals of fan-out schemes...
well, we could use 'git ls-tree' without '-r' option, or simply 
'git cat-file --batch' to read trees (note that we would get raw, 
unformatted tree, which is parseable with Perl, but it is not that easy),
and go down level-by-level.

FYI, here is how you can parse raw tree output from 'git cat-file --batch',
assuming that you have plain-ASCII filenames ('use bytes;' would probably
be needed):

-- 8< --
sub decode_tree {
	my $contents = shift;

	# ...

	while (my @entry = decode_tree_entry($contents)) {

		# ...

		my $len = tree_entry_len(@entry);
		contents = substr($contents, $len);
		last unless $contents;
	}

	# ...
}

sub tree_entry_len {
	my ($mode_str, $filename) = @_;

	# length of mode string + separator + 20 bytes of SHA-1
	# + length of filename (in bytes) + terminating NUL ('\0')
	length($mode_str)+1 + length($filename)+1 + 20;
}

sub decode_tree_entry {
	my $buf = shift;

	$buf =~ s/^([0-7]+) //;
	my ($mode_str) = $1;
	my ($filename, $sha1_str) = unpack('Z*H[40]', $buf);

	return ($mode_str, $filename, $sha1_str);
}
-- >8 --

-- 
Jakub Narebski
Poland

On Sunday 07 February 2010, Jakub Narebski wrote:

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

quoted

On the other hand, as mentioned by Junio, this approach is not
future-proof enough for any kind of fan-out schemes.

On the third hand ;-P you propose below a trick to deal with fan-out
schemes, assuming that they use 2-character component breaking.

The current notes code (as it stands in 'pu') use only 2-character component 
breaking, and I don't see any other fanout mechanism being added anytime 
soon.

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

I'd much rather have support for ^{notes} (or similar) in the rev-parse 
machinery, so that you could look up deadbeef's notes by passing 
"deadbeef^{notes}" to 'git cat-file --batch'.

quoted

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.

Right.  This method would be contrary to the goals of fan-out schemes...
well, we could use 'git ls-tree' without '-r' option, or simply
'git cat-file --batch' to read trees (note that we would get raw,
unformatted tree, which is parseable with Perl, but it is not that easy),
and go down level-by-level.

IMHO, it's much better/nicer to re-use the notes code for parsing note 
trees. See above suggestion on deadbeef^{notes}.

quoted

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.

Nice trick!  It seems like quite a good idea... but it would absolutely
require using 'git cat-file --batch' rather than one git-show per try.

Still, I'd still much rather use the notes.c code itself for doing this 
since it should always be the fastest (not to mention future-proof) way of 
making lookups in the notes tree.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski [off-list ref] wrote:

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

quoted

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

quoted

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.

Well, fnmatch is what I think git uses for <pattern> e.g. for
git-for-each-ref.

fnmatch is the function to use to match a single component. The
question is how to group components together in a single spec (aside
from shell globs and []); my personal choice would be brace expansion,
colon-separated was suggested by (IIRC) Junio. Of course, for gitweb
I'll go with whatever is chosen for core.

quoted

Colon-separated, fnmatched components is probably the easiest thing to
implement to have multiple refs. I'll go with whatever is chosen for
core.

I think that having actual list of patterns in $feature{'notes'}{'default'}
might be more clear; you would still need colon separated (or space
separated) list of patterns in per-repo override in gitweb.notes config
variable.

So it would be

 $feature{'notes'}{'default'} = ['commits', '*svn*'];
 $feature{'notes'}{'override'} = 1;

but

 [gitweb]
       notes = commits:*svn*

Note that refs names cannot contain either colon ':' or space ' '
(see git-check-ref-format).

That makes sense. Colon-separated values will probably be allowed in
$feature{'notes'} too, which will keep the code more consistent.

[...]

quoted

quoted

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.

On the third hand ;-P you propose below a trick to deal with fan-out
schemes, assuming that they use 2-character component breaking.

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

There is little doubt that batch note processing should be available
in core, be it with git notes show --batch(-check), git ls-notes, or
both. I also agree with Johan that gitweb should use these mechanisms
as soon as they are available. The approaches we're discussing here
are basically a temporary workaround for the missing core support.

[...]

quoted

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.

Nice trick!  It seems like quite a good idea... but it would absolutely
require using 'git cat-file --batch' rather than one git-show per try.

Oh, that's a given. git-show was just the only approach I could think
of not knowing about the batch processing git commands.

quoted

I'm not getting these in the repo I'm testing this. And I think this
is indeed the behavior of current git next

Errr, I not made myself clear.

I have added a note to a commit, using "git notes edit d6bbe7f".  Now if
you take a look at gitweb output for this commit (e.g. 'commit' view for
this commit) using gitweb without your changes, you would see that it
flattened notes at the bottom of the commit message (which I think is
intended result by notes implementation).

If you run the command that parse_commit runs, namely

 $ git rev-list --parents --header -z --max-count=1 \
   d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3

you would get (up to invisible NUL characters) the output shown above.

And that's what I don't get. git version 1.7.0.rc1.193.ge8618. If I
remember correctly, the behaviour of automatically displaying notes in
git log & friends was changed recently. So git log -1
e8618c52b5f815624251f048609744c9558d92a1 gives me the notes I put
there for testing, git rev-list --parents --header -z --max-count=1
does not.

-- 
Giuseppe "Oblomov" Bilotta

On Sun, 7 February 2010, Johan Herland wrote:

On Sunday 07 February 2010, Jakub Narebski wrote:

quoted

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

quoted

quoted

On the other hand, as mentioned by Junio, this approach is not
future-proof enough for any kind of fan-out schemes.

On the third hand ;-P you propose below a trick to deal with fan-out
schemes, assuming that they use 2-character component breaking.

The current notes code (as it stands in 'pu') use only 2-character component 
breaking, and I don't see any other fanout mechanism being added anytime 
soon.

On one hand checking <notes-ref>:403f3b3e..., then <notes-ref>:40/3f3b3e...,
then <notes-ref>:40/3f/3b3e... etc. feels a bit kludgy...

On the other hand it would allow to support notes in gitweb even if git
binary does not have notes support (and yet notes did get somehow into
repository).
 

quoted

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

I'd much rather have support for ^{notes} (or similar) in the rev-parse 
machinery, so that you could look up deadbeef's notes by passing 
"deadbeef^{notes}" to 'git cat-file --batch'.

+1.  Good idea!

The only caveat is that if/when we support either of:
* allowing to specify multiple notes namespaces (e.g. multiple --notes-ref
  or --notes option, or PATH-like GIT_NOTES_REF with colon-separated list
  of multiple notes namespaces)
* allowing to have multiple notes per object ('tree' notes)
then <commit-ish>^{notes} would mean multiple objects.  But it is not
much different from supported <commit-ish>^! and <commit-ish>^@ syntax.

[...]

quoted

quoted

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.

Nice trick!  It seems like quite a good idea... but it would absolutely
require using 'git cat-file --batch' rather than one git-show per try.

Still, I'd still much rather use the notes.c code itself for doing this 
since it should always be the fastest (not to mention future-proof) way of 
making lookups in the notes tree.

One of Giuseppe goals seems to be to support notes in gitweb even if used
git binary doesn't have support for notes (git-notes command, ^{notes}
extended SHA1 syntax).

-- 
Jakub Narebski
Poland

On Sun, Feb 7, 2010 at 2:48 AM, Johan Herland [off-list ref] wrote:

On Sunday 07 February 2010, Jakub Narebski wrote:

quoted

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

I'd much rather have support for ^{notes} (or similar) in the rev-parse
machinery, so that you could look up deadbeef's notes by passing
"deadbeef^{notes}" to 'git cat-file --batch'.

Maybe something like deadbeef@{notes[:namespace]}? The ability to
embed the notes namespace to use in the call is very useful to be able
to access all the notes with a single git call.

quoted

quoted

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.

Nice trick!  It seems like quite a good idea... but it would absolutely
require using 'git cat-file --batch' rather than one git-show per try.

Still, I'd still much rather use the notes.c code itself for doing this
since it should always be the fastest (not to mention future-proof) way of
making lookups in the notes tree.

I agree with you on this, btw. As I mentioned in the other message,
these would just be workarounds for the current lack of support of
these features in core. I'd probably try and have a go at the thing
myself, too (i.e. first implement the core functionality, and then use
it in gitweb), but I honestly don't feel confident enough to hack at
git core.

-- 
Giuseppe "Oblomov" Bilotta

Giuseppe Bilotta wrote:

On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski [off-list ref] wrote:

quoted

Errr, I not made myself clear.

I have added a note to a commit, using "git notes edit d6bbe7f".  Now if
you take a look at gitweb output for this commit (e.g. 'commit' view for
this commit) using gitweb without your changes, you would see that it
flattened notes at the bottom of the commit message (which I think is
intended result by notes implementation).

If you run the command that parse_commit runs, namely

 $ git rev-list --parents --header -z --max-count=1 \
   d6bbe7fd52058cdf0e48bec00701ae0f4861dcd3

you would get (up to invisible NUL characters) the output shown above.

And that's what I don't get. git version 1.7.0.rc1.193.ge8618. If I
remember correctly, the behaviour of automatically displaying notes in
git log & friends was changed recently. So git log -1
e8618c52b5f815624251f048609744c9558d92a1 gives me the notes I put
there for testing, git rev-list --parents --header -z --max-count=1
does not.

Ah, sorry.  Git version 1.6.6.1 here.

-- 
Jakub Narebski
Poland

On Sun, 7 Feb 2010, Giuseppe Bilotta wrote:

On Sun, Feb 7, 2010 at 2:48 AM, Johan Herland [off-list ref] wrote:

quoted

On Sunday 07 February 2010, Jakub Narebski wrote:

quoted

Also, perhaps "git notes show" should acquire --batch / --batch-check
options, similar to git-cat-file's options of the same name?

I'd much rather have support for ^{notes} (or similar) in the rev-parse
machinery, so that you could look up deadbeef's notes by passing
"deadbeef^{notes}" to 'git cat-file --batch'.

Maybe something like deadbeef@{notes[:namespace]}? The ability to
embed the notes namespace to use in the call is very useful to be able
to access all the notes with a single git call.

That is just bikeshedding, but I'd rather not use '@', which currently
is used only for _reflog_ based revision specifiers: [<ref>]@{<date>},
[<ref>]@{<n>}, @{-<n>}, for notes which are not reflog based.

We can use

  echo <commit>^{notes} | git --notes-ref=<namespace> cat-file --batch

or perhaps

  echo <commit>^{notes:<namespace>} | git cat-file --batch

-- 
Jakub Narebski
Poland

Jakub Narebski [off-list ref] writes:

That is just bikeshedding, but I'd rather not use '@', which currently
is used only for _reflog_ based revision specifiers: [<ref>]@{<date>},
[<ref>]@{<n>}, @{-<n>}, for notes which are not reflog based.

Probably a nicer way to say the same thing is to avoid "reflog based"
which sounds like you are talking about an implementation detail.

A fundamental reason to favor your "bikeshedding" (I don't think it is a
bikeshedding---it is a sound argument against using "@{...}") is that the
at-brace notation applies to a ref, not to an arbitrary commit.  Applying
@{yesterday} to an arbitrary commit does not make any sense.

Notes are fundamenally metainformation about an _object_ [*1*] and are not
metainformation about refs.  Since whatever magic notation to denote notes
we choose wants to be applied to an arbitrary commit, it shouldn't be the
at-brace syntax.

[Footnote]

*1* Yes, I am aware of movements to misuse notes to annotate anything
after mapping it to a random SHA-1 value, but I think that is outside the
scope of notes.  Our design decision should be based on supporting the
primary use of annotating an object, and that might still keep such a use
working, in which case that would be an added bonus.  But our design
shouldn't be constrained by such a secondary use.

On Sun, Feb 7, 2010 at 7:38 PM, Junio C Hamano [off-list ref] wrote:

Notes are fundamenally metainformation about an _object_ [*1*] and are not
metainformation about refs.  Since whatever magic notation to denote notes
we choose wants to be applied to an arbitrary commit, it shouldn't be the
at-brace syntax.

Makes sense. ^{note[:namespace]} is ok for me too btw, although maybe
it looks a little off-base when compared with the tag indicator ^{}
which works, in a sense, in the opposite direction.

[Footnote]

*1* Yes, I am aware of movements to misuse notes to annotate anything
after mapping it to a random SHA-1 value, but I think that is outside the
scope of notes.  Our design decision should be based on supporting the
primary use of annotating an object, and that might still keep such a use
working, in which case that would be an added bonus.  But our design
shouldn't be constrained by such a secondary use.

BTW, I still think that notes should be attachable to named refs (not
SHA-1, thus) too.

-- 
Giuseppe "Oblomov" Bilotta

On Sun, 7 Feb 2010, Giuseppe Bilotta wrote:

On Sun, Feb 7, 2010 at 7:38 PM, Junio C Hamano [off-list ref] wrote:

quoted

Notes are fundamenally metainformation about an _object_ [*1*] and are not
metainformation about refs.  Since whatever magic notation to denote notes
we choose wants to be applied to an arbitrary commit, it shouldn't be the
at-brace syntax.

Makes sense. ^{note[:namespace]} is ok for me too btw, although maybe
it looks a little off-base when compared with the tag indicator ^{}
which works, in a sense, in the opposite direction.

Well, notes refer to objects (commits), but the whole idea of notes
was to have easy mapping in the reverse direction, from object to
its annotations.

We could invent yet another syntax, e.g. ^@{} or ^@{<namespace>}
(following ^@ notation for parents, which can also return multiple SHA1s).

quoted

[Footnote]

*1* Yes, I am aware of movements to misuse notes to annotate anything
after mapping it to a random SHA-1 value, but I think that is outside the
scope of notes.  Our design decision should be based on supporting the
primary use of annotating an object, and that might still keep such a use
working, in which case that would be an added bonus.  But our design
shouldn't be constrained by such a secondary use.

BTW, I still think that notes should be attachable to named refs (not
SHA-1, thus) too.

I have just realized that it is totally no-go.  Why?  Because names of
refs are local to repository: what is one 'master' might be other 'origin';
what is one 'for-linus' might be other 'from-alan', what's one 
'refs/heads/next' might be other 'refs/remotes/origin/next'.

Also I think that there would be problem with renaming and deleting refs.

-- 
Jakub Narebski
Poland