patch, 4 partsgitweb: preliminary notes support

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
>
>> Subject: [PATCH 2/4] gitweb: show notes in shortlog view
>
> Is it RFC?

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

>> +# display notes next to a commit
>> +sub format_notes_html {
>> +     my %notes = %{$_[0]};
>
> Why not use 'my $notes = shift;', and later '%$notes'?

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

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

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

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

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

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

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

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

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

> 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