[PATCH 0/4] gitweb: preliminary notes support

[PATCH 0/4] gitweb: preliminary notes support

[Giuseppe Bilotta]

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

[PATCH 1/4] gitweb: notes feature

[Giuseppe Bilotta]

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

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

[Giuseppe Bilotta]

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

[PATCH 3/4] gitweb: show notes in log

[Giuseppe Bilotta]

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

[PATCH 4/4] gitweb: show notes in commit(diff) view

[Giuseppe Bilotta]

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

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

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> 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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> 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).

IOW, the code should be reading output from:

    GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}

as the notes tree may not be storing notes in a flat one-level namespace
like you are assuming.

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

[Jakub Narebski]

On Thu, 4 Feb 2010, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> 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).
> 
> IOW, the code should be reading output from:
> 
>     GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
> 
> as the notes tree may not be storing notes in a flat one-level namespace
> like you are assuming.

First, for some mechanism of deployment (IIRC Apache's mod_perl) changes
to environment variables from CGI script are not passed to invoked
commands (I guess for security reasons).  That is why gitweb uses --git-dir
parameter to git wrapper, and not GIT_DIR environment variable since
25691fb (gitweb: Use --git-dir parameter instead of setting $ENV{'GIT_DIR'},
2006-08-28).  So for proper support we would need --notes-ref (or similar)
option to git wrapper

  git --notes-ref=$note_ref show -s --format=%N $co{'id'}


Second, parse_commit / parse_commits use

  git rev-list -z --parents --header --max-count-X

If this command automatically shows notes (or it can be modified to
automatically show notes) after unindented "Notes:" line (as per
git-notes documentation), then the only thing that needs to be
changed to fill %commit{'notes'} is parse_commit_text subroutine.

There would be no need for extra subroutine (and extra command, or
even up to two extra commands per note) to get notes data.

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Thu, Feb 4, 2010 at 6:21 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 4 Feb 2010, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> 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).
>>
>> IOW, the code should be reading output from:
>>
>>     GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
>>
>> as the notes tree may not be storing notes in a flat one-level namespace
>> like you are assuming.
>
> First, for some mechanism of deployment (IIRC Apache's mod_perl) changes
> to environment variables from CGI script are not passed to invoked
> commands (I guess for security reasons).  That is why gitweb uses --git-dir
> parameter to git wrapper, and not GIT_DIR environment variable since
> 25691fb (gitweb: Use --git-dir parameter instead of setting $ENV{'GIT_DIR'},
> 2006-08-28).  So for proper support we would need --notes-ref (or similar)
> option to git wrapper
>
>  git --notes-ref=$note_ref show -s --format=%N $co{'id'}

As I mentioned on the cover letter, I was hoping to be able to make
something that could be deployable without requiring core changes and
thus a specific minimum git version. I do realize however that this is
inherently not robust (unless the code is updated if and when the
notes storage mechanism changes).

The wrapper is probably needed anyway, I would say, so it makes sense
to implement it. However, see below for a caveat about its
implementation.

> Second, parse_commit / parse_commits use
>
>  git rev-list -z --parents --header --max-count-X
>
> If this command automatically shows notes (or it can be modified to
> automatically show notes) after unindented "Notes:" line (as per
> git-notes documentation), then the only thing that needs to be
> changed to fill %commit{'notes'} is parse_commit_text subroutine.
>
> There would be no need for extra subroutine (and extra command, or
> even up to two extra commands per note) to get notes data.

But unless the --notes-ref is made to accept multiple refspecs, this
will only access _one_ note namespace. It is not hard to envision
situations where multiple namespaces are used for the same repo (e.g.
one to store git-svn metadata, one for bugzilla-related entries), in
which case you'd want _all_ of them to be accessible in gitweb.

And if we do support multiple refspecs for --notes-ref, we also need
some way to identify which note belongs to which namespace, of course,
in any of the output formats that include notes.

An alternative approach would be some git notes-list command that
accepts both multiple namespaces and multiple commits, and is
specifically dedicated to display notes-commits relationships
(together with notes content, obviously)

-- 
Giuseppe "Oblomov" Bilotta

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

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> As I mentioned on the cover letter, I was hoping to be able to make
> something that could be deployable without requiring core changes and
> thus a specific minimum git version. I do realize however that this is
> inherently not robust (unless the code is updated if and when the
> notes storage mechanism changes).

AFAIU, the note code on the core side already creates a fan-out structure
when notes tree gets large (see recent "What's cooking"; the series is
parked in 'pu' but that is primarily because we are in feature freeze); it
is not just "inherently not robust" but is much closer to "broken from day
one" ;-).  Otherwise I wouldn't have wasted time to point it out.

Your code is a very good proof-of-concept, though.

Regarding support of multiple notes hierarchies, listing, etc.

See for example:

  http://thread.gmane.org/gmane.comp.version-control.git/138079/focus=138128

I expect more ideas from needs by end-user would come, as we gain
experience with using notes in real projects.  You will certainly find
some other needs of your own, like the "not an environment but a command
line option" which Jakub mentioned, and "multiple hierarchies" like both
you and I found need for.  Share them and let us together make the notes
mechanism nicer to use.

Thanks.

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> IOW, the code should be reading output from:
>> 
>>     GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
>> 
>> as the notes tree may not be storing notes in a flat one-level namespace
>> like you are assuming.
>
> First, for some mechanism of deployment (IIRC Apache's mod_perl) changes
> to environment variables from CGI script are not passed to invoked
> commands (I guess for security reasons).

I do not believe you are unable to spawn

	open $fd, '-|' 'sh', '-c', "GIT_NOTES_REF=$note_ref git show ..." 

and read from it ;-).

For possible enhancement to make notes easier to use, see the other
response.

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

[Jakub Narebski]

Dnia czwartek 4. lutego 2010 22:07, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> IOW, the code should be reading output from:
>>> 
>>>     GIT_NOTES_REF=$note_ref git show -s --format=%N $co{'id'}
>>> 
>>> as the notes tree may not be storing notes in a flat one-level namespace
>>> like you are assuming.
>>
>> First, for some mechanism of deployment (IIRC Apache's mod_perl) changes
>> to environment variables from CGI script are not passed to invoked
>> commands (I guess for security reasons).
> 
> I do not believe you are unable to spawn
> 
> 	open $fd, '-|' 'sh', '-c', "GIT_NOTES_REF=$note_ref git show ..." 
> 
> and read from it ;-).

You meant here

	my $git_command = quote_command(git_cmd(), 'show', ...);
	open my $fd, '-|', 'sh', '-c', "GIT_NOTES_REF=$note_ref $git_command" 

So you need to take care to quote parameters ($note_ref fortunately 
doesn't need to be quoted)... and you have one more process (shell)
spawned.


Therefore I think it would be nice to have --notes-ref option to git 
wrapper,... especially that it should be easy to set it up in such way
that it would be possible to pass --notes-ref multiple times, e.g.:

	git --notes-ref=commits --notes-ref=git-svn show ...

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Thu, Feb 4, 2010 at 10:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> AFAIU, the note code on the core side already creates a fan-out structure
> when notes tree gets large (see recent "What's cooking"; the series is
> parked in 'pu' but that is primarily because we are in feature freeze); it
> is not just "inherently not robust" but is much closer to "broken from day
> one" ;-).  Otherwise I wouldn't have wasted time to point it out.

Ouch, I hadn't considered that, indeed.

> Your code is a very good proof-of-concept, though.

Thanks. I guess at this point a proper implementation can wait for the
necessary core support functions.

> Regarding support of multiple notes hierarchies, listing, etc.
>
> See for example:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/138079/focus=138128
>
> I expect more ideas from needs by end-user would come, as we gain
> experience with using notes in real projects.  You will certainly find
> some other needs of your own, like the "not an environment but a command
> line option" which Jakub mentioned, and "multiple hierarchies" like both
> you and I found need for.  Share them and let us together make the notes
> mechanism nicer to use.

Collecting those ideas together would also help define some sort of
roadmap, or at least have a clear idea of what's needed, to help drive
the design of the features themselves. Maybe we could start a TODO
page on the wiki collecting these ideas?

-- 
Giuseppe "Oblomov" Bilotta

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

[Jakub Narebski]

Jakub Narebski wrote:
> On Thu, 4 Feb 2010, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>>
>>>> +		my %notes = () ;
>>>> +		foreach my $note_ref (@note_refs) {
>>>> +			my $obj = "$note_ref:$co{'id'}";
[...]
 
> Second, parse_commit / parse_commits use
> 
>   git rev-list -z --parents --header --max-count-X
> 
> If this command automatically shows notes (or it can be modified to
> automatically show notes) after unindented "Notes:" line (as per
> git-notes documentation), then the only thing that needs to be
> changed to fill %commit{'notes'} is parse_commit_text subroutine.

This command automatically shows notes, even in absence of GIT_NOTES_REF
environment variable or core.notesRef (if core.notesRef is not unset),
so unless we want gitweb to display notes flattened into commit message
(which I think was intended behavior - design decision for notes 
display), we would need to modify parse_commit_text to gather notes 
into %commin{'notes'} (or something).

 
> There would be no need for extra subroutine (and extra command, or
> even up to two extra commands per note) to get notes data.

You are right that we would need it if we want to display notes from
non-default namespace.

Still, 1 or 2 git commands per commit is a bit too much (with shortlog
displaying 100 commits per page): that is what "git cat-file --batch"
was invented ;-)

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Still, 1 or 2 git commands per commit is a bit too much (with shortlog
> displaying 100 commits per page)

But who said you need to display notes for all commits by default?

It could be a thing that you would get _on demand_, just like patch text
or diffstat is given only on demand by going to the commitdiff page.

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

[Giuseppe Bilotta]

On Fri, Feb 5, 2010 at 1:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Still, 1 or 2 git commands per commit is a bit too much (with shortlog
>> displaying 100 commits per page)
>
> But who said you need to display notes for all commits by default?
>
> It could be a thing that you would get _on demand_, just like patch text
> or diffstat is given only on demand by going to the commitdiff page.

But while diffstat or patch text always make sense for _any_ commit,
since you always have them; notes don't, in the sense that you might
not have notes in most of the commits, and you might actually be
looking for commits with notes.

At the very least, a view like shortlog should give an indicator that
a commit has notes, and possible the namespaces where its notes are
(e.g., you're only browsing for bugzilla notes).

Anyway, since git cat-file --batch, or --batch-check if we only want
the indicator, (didn't know about these options, btw, thanks a bunch)
means that we can get all the notes with a single extra call, what's
the problem with displaying them?  ;-)



-- 
Giuseppe "Oblomov" Bilotta

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

[Johan Herland]

On Friday 05 February 2010, Giuseppe Bilotta wrote:
> On Thu, Feb 4, 2010 at 10:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > I expect more ideas from needs by end-user would come, as we gain
> > experience with using notes in real projects.  You will certainly find
> > some other needs of your own, like the "not an environment but a
> > command line option" which Jakub mentioned, and "multiple hierarchies"
> > like both you and I found need for.  Share them and let us together
> > make the notes mechanism nicer to use.
> 
> Collecting those ideas together would also help define some sort of
> roadmap, or at least have a clear idea of what's needed, to help drive
> the design of the features themselves. Maybe we could start a TODO
> page on the wiki collecting these ideas?

I already maintain a TODO list at the end of the cover letter to the notes 
series. Here is a preview of it (I plan to send the next iteration of 
jh/notes as soon as v1.7.0 is released):


- Suggestion by Matthieu Moy and Sverre Rabbelier:
  Add notes support to git-format-patch, where note contents in
  refs/notes/format-patch are added to the "comments section"
  (i.e. following the '---' separator) of generated patches.

- Better integration with rebase/amend/cherry-pick. Optionally bring
  notes across a commit rewrite. Controlled by command-line options
  and/or config variables. Add "git notes move" and "git notes copy"
  to suit. Junio says:
    I used to fix minor issues (styles, decl-after-stmt, etc.) using
    rebase-i long after running "am" in bulk, but these days I find
    myself going back to my "inbox" and fix them in MUA; this is
    only because I know these notes do not propagate across rebases
    and amends -- adjusting the workflow to the tool's limitation is
    not very good.

- Junio says:
  The interface to tell tools to use which notes ref to use should be
  able to say "these refs", not just "this ref" i.e. GIT_NOTES_REF=a:b
  just like PATH=a:b:c...); I am fairly certain that we would want to
  store different kind of information in separate notes trees and
  aggregate them, as we gain experience with notes.

- Junio says:
  There should be an interface to tell tools to use which notes refs via
  command line options; "!alias" does not TAB-complete, and "git lgm"
  above doesn't, either. "git log --notes=notes/amlog --notes=notes/other"
  would probably be the way to go.

- Add a "git notes grep" subcommand: Junio says:
  While reviewing the "inbox", I sometimes wonder if I applied a message
  to somewhere already, but there is no obvious way to grep in the notes
  tree and get the object name that a note is attached to.  Of course I
  know I can "git grep -c johan@herland.net notes/amlog" and it will give
  me something like:

    notes/amlog:65807ee697a28cb30b8ad38ebb8b84cebd3f255d:1
    notes/amlog:c789176020d6a008821e01af8b65f28abc138d4b:1

  but this won't scale and needs scripting to mechanize, once we start
  rebalancing the notes tree with different fan-outs.  The end user (me
  in this case) is interested in "set of objects that match this grep
  criteria", not "the pathnames the notes tree's implementation happens
  to use to store notes for them in the hierarchy".

- Handle note objects that are not blobs, but trees

(- Rewrite fast-import notes code to use new notes API with non-note 
support)


Have fun! :)

...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I already maintain a TODO list at the end of the cover letter to the notes 
> series. Here is a preview of it (I plan to send the next iteration of 
> jh/notes as soon as v1.7.0 is released):
> ...

Random additional thoughts.

* Futureproofing

We have to admit that our notes support, especially on the output side, is
still in its infancy.  We may want to advertise it as such -- highly
experimental and subject to change.

* format-patch

To add notes to format-patch output, we might want to do something like:

    $ git format-patch --notes-ref=commits --notes-ref=amlog -1

and produce:

    From 8bff7c5383ed833bd1df9c8d85c00a27af3e5b02 Mon Sep 17 00:00:00 2001
    From: Andrew Myrick <amyrick@apple.com>
    Date: Sat, 30 Jan 2010 03:14:22 +0000
    Subject: [PATCH] git-svn: persistent memoization
    X-Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
     from git://git.bogomips.org/git-svn.git/
    X-Notes-amlog: <1264821262-28322-1-git-send-email-amyrick@apple.com>

    Make memoization of the svn:mergeinfo processing functions persistent with
    ...

Points to notice:

 - There is no point forcing users to spell "--notes-ref" parameter
   starting from refs/notes/; we should DWIM if they are missing;

 - We would want to allow more than one notes hierarchy specified. This
   would affect format_note() function---take list of struct notes_tree,
   perhaps;

 - Allow callers of tell format_note() to add the name of the notes
   hierarchy the note came from (or just always add it if it is not the
   default "refs/notes/commits").

 - For format-patch that produces a mbox output, the email header part may
   be a better place to put notes (obeying the usual "indent by one space
   to continue the line" convention).

* "log --format=%N" and "log --show-notes"

Currently %N expands to the hardcoded "log --show-notes" default format.
We can probably keep it that way.  When the user asked for a non default
notes hierarchy (i.e. other than refs/notes/commits), we may want to
adjust "Notes:" string to use "Notes-%s:" to show which hierarchy it came
from, and concatenate them together.

For "log --show-notes" output, we also might want to move the notes to the
header part like I illustrated above in format-patch output, instead of
"start with unindented Notes: and indented body at the end".  I.e. instead
of showing this:

    $ git log --notes-ref=amlog -1 4d0cc22
    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Thu Feb 4 11:10:44 2010 -0800

        fast-import: count --max-pack-size in bytes

        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
        ...
        Acked-by: Nicolas Pitre <nico@fluxnic.net>

    Notes:
        <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>


show it like this:

    $ git log --notes-ref=amlog -1 4d0cc22
    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Thu Feb 4 11:10:44 2010 -0800
    Notes-amlog: <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>

        fast-import: count --max-pack-size in bytes

        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
        ...
        Acked-by: Nicolas Pitre <nico@fluxnic.net>

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

[Giuseppe Bilotta]

On Fri, Feb 5, 2010 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * format-patch
>
> To add notes to format-patch output, we might want to do something like:
>
>    $ git format-patch --notes-ref=commits --notes-ref=amlog -1
>
> and produce:
>
>    From 8bff7c5383ed833bd1df9c8d85c00a27af3e5b02 Mon Sep 17 00:00:00 2001
>    From: Andrew Myrick <amyrick@apple.com>
>    Date: Sat, 30 Jan 2010 03:14:22 +0000
>    Subject: [PATCH] git-svn: persistent memoization
>    X-Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
>     from git://git.bogomips.org/git-svn.git/
>    X-Notes-amlog: <1264821262-28322-1-git-send-email-amyrick@apple.com>

+1 for header style note exporting. X-Git-Notes might be better.

> Points to notice:
>
>  - There is no point forcing users to spell "--notes-ref" parameter
>   starting from refs/notes/; we should DWIM if they are missing;

Agreed.

>  - We would want to allow more than one notes hierarchy specified. This
>   would affect format_note() function---take list of struct notes_tree,
>   perhaps;

I get the impression everybody wants this ;-)

>  - Allow callers of tell format_note() to add the name of the notes
>   hierarchy the note came from (or just always add it if it is not the
>   default "refs/notes/commits").

This is probably the most bothering issue: find, for each output
format that involves notes, the smart way of also outputting the
namespace it came from.

>  - For format-patch that produces a mbox output, the email header part may
>   be a better place to put notes (obeying the usual "indent by one space
>   to continue the line" convention).

How would you cope with multi-line notes? One X-Git-Notes header per line?

> * "log --format=%N" and "log --show-notes"
>
> Currently %N expands to the hardcoded "log --show-notes" default format.
> We can probably keep it that way.  When the user asked for a non default
> notes hierarchy (i.e. other than refs/notes/commits), we may want to
> adjust "Notes:" string to use "Notes-%s:" to show which hierarchy it came
> from, and concatenate them together.

We might want to do without the dash in standard log output: Notes:
and Notes <namespace>:

> For "log --show-notes" output, we also might want to move the notes to the
> header part like I illustrated above in format-patch output, instead of
> "start with unindented Notes: and indented body at the end".  I.e. instead
> of showing this:
>
>    $ git log --notes-ref=amlog -1 4d0cc22
>    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
>    Author: Junio C Hamano <gitster@pobox.com>
>    Date:   Thu Feb 4 11:10:44 2010 -0800
>
>        fast-import: count --max-pack-size in bytes
>
>        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
>        ...
>        Acked-by: Nicolas Pitre <nico@fluxnic.net>
>
>    Notes:
>        <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
>
>
> show it like this:
>
>    $ git log --notes-ref=amlog -1 4d0cc22
>    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
>    Author: Junio C Hamano <gitster@pobox.com>
>    Date:   Thu Feb 4 11:10:44 2010 -0800
>    Notes-amlog: <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
>
>        fast-import: count --max-pack-size in bytes
>
>        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
>        ...
>        Acked-by: Nicolas Pitre <nico@fluxnic.net>

The footer approach has the benefit of allowing multi-line notes to
just be printed the same way as multi-line commit messages, whereas
the header output would require one header line per commit line.

-- 
Giuseppe "Oblomov" Bilotta

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> * "log --format=%N" and "log --show-notes"
> 
> Currently %N expands to the hardcoded "log --show-notes" default format.
> We can probably keep it that way.  When the user asked for a non default
> notes hierarchy (i.e. other than refs/notes/commits), we may want to
> adjust "Notes:" string to use "Notes-%s:" to show which hierarchy it came
> from, and concatenate them together.
>
> For "log --show-notes" output, we also might want to move the notes to the
> header part like I illustrated above in format-patch output, instead of
> "start with unindented Notes: and indented body at the end".  I.e. instead
> of showing this:

Giuseppe was wondering about multi-line thing, so the illustration should
be adjusted to match the "format-patch" example to show a multi-line note,
I think.  Here is what I meant.

Instead of showing:

    $ git log --notes-ref=amlog -1 4d0cc22
    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Thu Feb 4 11:10:44 2010 -0800

        fast-import: count --max-pack-size in bytes

        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
        ...

    Notes:
        pulled on Fri Feb 5 07:36:12 2010 -0800
        from git://git.bogomips.org/git-svn.git/
    Notes-amlog:
        <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>

which is what 1.6.6 added, showing it like this:

    $ git log --notes-ref=amlog -1 4d0cc22
    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Thu Feb 4 11:10:44 2010 -0800
    Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
     from git://git.bogomips.org/git-svn.git/
    Notes-amlog: <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>

        fast-import: count --max-pack-size in bytes

        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
        ...

might be easier to see.  After all, notes are metainformation on commits,
and people who are interested will look at the header, and those who are
not will skim over the block of text at the beginning, knowing that is
where all the metainformation is.

But this is just "might", not "should---I strongly believe".

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

[Jakub Narebski]

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

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

[Jakub Narebski]

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?

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

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.

[...]
> 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]};

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

> +		}
> +		$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") . " | " .

Nice.

-- 
Jakub Narebski
Poland

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

[Giuseppe Bilotta]

On Fri, Feb 5, 2010 at 11:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Giuseppe was wondering about multi-line thing, so the illustration should
> be adjusted to match the "format-patch" example to show a multi-line note,
> I think.  Here is what I meant.
>
> Instead of showing:
>
>    $ git log --notes-ref=amlog -1 4d0cc22
>    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
>    Author: Junio C Hamano <gitster@pobox.com>
>    Date:   Thu Feb 4 11:10:44 2010 -0800
>
>        fast-import: count --max-pack-size in bytes
>
>        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
>        ...
>
>    Notes:
>        pulled on Fri Feb 5 07:36:12 2010 -0800
>        from git://git.bogomips.org/git-svn.git/
>    Notes-amlog:
>        <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
>
> which is what 1.6.6 added, showing it like this:
>
>    $ git log --notes-ref=amlog -1 4d0cc22
>    commit 4d0cc2243778b38c3759c6a08f4f1ed64155a070
>    Author: Junio C Hamano <gitster@pobox.com>
>    Date:   Thu Feb 4 11:10:44 2010 -0800
>    Notes: pulled on Fri Feb 5 07:36:12 2010 -0800
>     from git://git.bogomips.org/git-svn.git/
>    Notes-amlog: <7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org>
>
>        fast-import: count --max-pack-size in bytes
>
>        Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git
>        ...
>
> might be easier to see.  After all, notes are metainformation on commits,
> and people who are interested will look at the header, and those who are
> not will skim over the block of text at the beginning, knowing that is
> where all the metainformation is.
>
> But this is just "might", not "should---I strongly believe".


I'm not convinced. I believe format-patch output should be as close as
possible to RFC2822 compliance, given how it can be used as basis for
actual email.

According to the RFC, lines should be no more than 78 characters and
must be less than 998. The 'one space indent on newline', if I'm not
mistaken, is a way to indicate a _wrapped_ single line header, rather
than a multi-line one.

A scenario such as the following is thus possible: format-patch
creates a single-line header which is longer than 78 characters
(X-Git-Notes-somerefname: 70ish characters line). The patch gets sent
by email, and a properly configured send-email sends the X-Git-Notes*
headers through (BTW, this should probably be added to the
notes/send-email TODO). Some intermediate forwarder or user agent
decides to rewrap the longish line so that it isn't longer than 78
characters. The user saves the raw email and uses git am to apply it
-> oops, the single line note has become a two-liner.

If we use the convention (at least in format-patch, not necessarily in
log) of one header line per notes line we should be more on the safe
side. It would also allow us to keep the headers wrapped at < 78
characters which is nice for legibility on terminals.


-- 
Giuseppe "Oblomov" Bilotta

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

[Giuseppe Bilotta]

2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> 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-/.

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

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

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

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

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

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

[Giuseppe Bilotta]

2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
>
>> 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.

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

A-hem. (whistles innocently)

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

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

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

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

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

Re: [PATCH 3/4] gitweb: show notes in log

[Jakub Narebski]

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.


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

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

> @@ -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";

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

Re: [PATCH 3/4] gitweb: show notes in log

[Giuseppe Bilotta]

2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> 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.

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.

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

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

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

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Jakub Narebski]

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.

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

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

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

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Giuseppe Bilotta]

2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> 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.

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.

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

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

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Jakub Narebski]

Giuseppe Bilotta wrote:
> 2010/2/6 Jakub Narebski <jnareb@gmail.com>:
>> On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

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

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Giuseppe Bilotta]

On Sat, Feb 6, 2010 at 3:34 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> 2010/2/6 Jakub Narebski <jnareb@gmail.com>:
>>> On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:
>
> [...]
>>>>  # 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

Re: [PATCH 3/4] gitweb: show notes in log

[Jakub Narebski]

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
> 2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> > 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.
> 
> 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.
[...]
> > > +             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

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Jakub Narebski]

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
> On Sat, Feb 6, 2010 at 3:34 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Giuseppe Bilotta wrote:
>>> 2010/2/6 Jakub Narebski <jnareb@gmail.com>:
>>>> On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:
>>
>> [...]
>>>>>  # 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

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

[Jakub Narebski]

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

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

-- 
Jakub Narebski
Poland

Re: [PATCH 4/4] gitweb: show notes in commit(diff) view

[Giuseppe Bilotta]

On Sat, Feb 6, 2010 at 10:50 PM, Jakub Narebski <jnareb@gmail.com> 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

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

[Giuseppe Bilotta]

On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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

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

[Jakub Narebski]

On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
> On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> 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.

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

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

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

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

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

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

[Jakub Narebski]

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

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

[Johan Herland]

On Sunday 07 February 2010, Jakub Narebski wrote:
> On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
> > 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'.

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

> > 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, <johan@herland.net>
www.herland.net

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

[Giuseppe Bilotta]

On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:
>> On Sat, Feb 6, 2010 at 11:14 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> 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.

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

[...]

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

[...]

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

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

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

[Jakub Narebski]

On Sun, 7 February 2010, Johan Herland wrote:
> On Sunday 07 February 2010, Jakub Narebski wrote:
> > On Sat, 6 Feb 2010, Giuseppe Bilotta wrote:

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

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

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

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Sun, Feb 7, 2010 at 2:20 AM, Jakub Narebski <jnareb@gmail.com> wrote:

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

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

[Giuseppe Bilotta]

On Sun, Feb 7, 2010 at 2:48 AM, Johan Herland <johan@herland.net> wrote:
> On Sunday 07 February 2010, Jakub Narebski wrote:
>
>> 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.

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

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

[Jakub Narebski]

On Sun, 7 Feb 2010, Giuseppe Bilotta wrote:
> On Sun, Feb 7, 2010 at 2:48 AM, Johan Herland <johan@herland.net> wrote:
> > On Sunday 07 February 2010, Jakub Narebski wrote:
> >
> > > 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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> 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.

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

[Giuseppe Bilotta]

On Sun, Feb 7, 2010 at 7:38 PM, Junio C Hamano <gitster@pobox.com> 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

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

[Jakub Narebski]

On Sun, 7 Feb 2010, Giuseppe Bilotta wrote:
> On Sun, Feb 7, 2010 at 7:38 PM, Junio C Hamano <gitster@pobox.com> 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.

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

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