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

On Thu, Dec 4, 2008 at 2:48 AM, Jakub Narebski [off-list ref] wrote:

On Thu, Dec 4, 2008 at 01:20, Giuseppe Bilotta wrote:

quoted

On Thu, Dec 4, 2008 at 12:55 AM, Junio C Hamano [off-list ref] wrote:

quoted

Giuseppe Bilotta [off-list ref] writes:

quoted

+
+     # 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.

quoted

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?

quoted

quoted

quoted

 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