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

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

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.

quoted

quoted

 );

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.

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

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

quoted

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