Re: [RFCv4 1/3] gitweb: add patch view

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

On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> +     my $patch_max;
>> +     if ($format eq 'patch') {
>> +             $patch_max = gitweb_check_feature('patches');
>> +             die_error(403, "Patch view not allowed") unless $patch_max;
>> +     }
>> +
>
> Hmmm... gitweb_check_feature

You're right, it's an abuse. I'll make it gitweb_get_feature()[0]

> I am wondering if we could somehow mark (encode) either $hash_parent
> or number of patches in proposed filename... but I don't think it is
> worth it.

Including hash_parent if defined is quite possible. I'm not sure it's
really worth it considering that the typical usage would be to publish
a patchset for a particular feature (in which case the hash/branch
name would be enough).

>> +     } elsif ($format eq 'patch') {
>> +             local $/ = undef;
>> +             print <$fd>;
>> +             close $fd
>> +                     or print "Reading git-format-patch failed\n";
>
> Nice, although... I'd prefer for Perl expert to say if it is better
> to dump file as a whole in such way (it might be quite large), or
> to do it line by line, i.e. without "local $/ = undef;", or using
> "print while <$fd>;" also without "local $/ = undef;".

I'm just sticking to whatever the existing code does :-)

As soon as you finish the patchset review I'll have a new version
taking into consideration all the other suggestions and remarks I
snipped from this reply.

-- 
Giuseppe "Oblomov" Bilotta