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

[Jakub Narebski <jnareb@gmail.com>]

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.

>>>  );
>>
>> Looking at the existing entries in the %feature hash, it seems that it is
>> our tradition that a new feature starts as disabled and not overridable
>> (see 'ctags' in the context above).
> 
> 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.

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

(Side note: doesn't it uses spaces instead of tabs for align?)
 
[...]
>> Other than that the patch seems quite straightforward and was a pleasant
>> read.  Thanks.

BTW. I'll try to review patch (and probably Ack) soon.
-- 
Jakub Narebski
Poland