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

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

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