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

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

On Thu, 4 Jan 2010, Giuseppe Bilotta wrote:

quoted

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

The same question apply as for previous commit.

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

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

quoted

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

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

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

+     my %notes = ();
+     foreach my $note_ref (get_note_refs()) {
+             my $obj = "$note_ref:$co{'id'}";
+             if (open my $fd, '-|', git_cmd(), 'rev-parse',
+                     '--verify', '-q', $obj) {
+                     my $exists = <$fd>;
+                     close $fd;
+                     if (defined $exists) {
+                             if (open $fd, '-|', git_cmd(), 'show', $obj) {
+                                     $notes{$note_ref} = scalar <$fd>;
+                                     close $fd;
+                             }
+                     }
+             }
+     }
+     $co{'notes'} = \%notes;
+
      return %co;
 }

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

Yup, definitely a good idea.

quoted

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

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

If you want to slurp whole file, this should be

       local $/;

or more explicit

       local $/ = undef;

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


-- 
Giuseppe "Oblomov" Bilotta