Re: [RFCv3 1/2] gitweb: add patch view

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

On Thu, Dec 4, 2008 at 2:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:
>> On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>>>> +
>>>> +     # The maximum number of patches in a patchset generated in patch
>>>> +     # view. Set this to 0 or undef to disable patch view, or to a
>>>> +     # negative number to remove any limit.
>>>> +     'patches' => {
>>>> +             'override' => 1,
>>>> +             'default' => [16]},
>
> Errr... you need something like 'sub' => \&feature_patches for override
> to actually work, I think.

Oops, right.

>> I always assumed that the disabled default was related to how invasive
>> the changes would be (to the UI or computationally-wise). As for the
>> overridability, that's actually the only reason why it would make
>> sense to put in the %feature hash ... otherwise a conf-settable
>> $patch_max (as in v2) would have been enough.
>
> Add to that the fact that this patch just adds the new view, like for
> example in the case of 'snapshot' link, which was turned on... but fact,
> it was by default not overridable. I would agree that it can be turned
> on with low limit but not overridable in introductory patch.

Ok, I'll make it non-overridable, and keep this 16 limit for starters.
Or would you suggest even lower?

>>>>  sub git_commitdiff {
>>>>       my $format = shift || 'html';
>>>> +
>>>> +     my $patch_max = gitweb_check_feature('patches');
>>>> +     if ($format eq 'patch') {
>>>> +             die_error(403, "Patch view not allowed") unless $patch_max;
>>>> +     }
>>>> +
>>>
>>> Should you have to pay overhead for the check-feature call even when
>>> the $format is not "patch"?
>>
>> Actually I wasn't sure if I could use my within the if block, and have
>> the value visible outside (it's used further down when picking the
>> options to pass to format-patch). And since it was used in the second
>> patch anyway to choose whether to add the 'patch' link in html view or
>> not, I just put it outside the block.
>
> You have to use _declaration_ ourside block, but assignment can happen
> inside:
>
> +     my $patch_max;
> +     if ($format eq 'patch') {
> +             $patch_max = gitweb_check_feature('patches');
> +             die_error(403, "Patch view not allowed") unless $patch_max;
> +     }

Right, stupid me.

> (Side note: doesn't it uses spaces instead of tabs for align?)

I'll check.

-- 
Giuseppe "Oblomov" Bilotta