Re: [PATCH] gitweb: Support for snapshot

Aneesh Kumar K.V wrote:

I tested this and added some comments. I also fixed some code. 
I am attaching the full diff. 

Below comments to the patch.

BTW git-repo-config have the below bug. 

$ git repo-config --bool --get gitweb.blame
true
$ git repo-config --get --bool gitweb.blame
$

So i dropped --get from the git_get_project_config

Wouldn't it be better to correct the error in git-repo-config? 
Or (easier) add '--get' last (see comments)?

+# Feature configuration.

Wouldn't it make it easier to understand code to put %feature hash 
and gitweb_check_feature subroutine _before_ subroutines for specific
features?

+# These subs are only called when per repository
+# overrides are allowed.  They take the default options,
+# inspect the repository and return the values from there if
+# the repository wants to override the system default.
+
+# To enable system wide have in $GITWEB_CONFIG
+# $feature{'blame'} =  [\&feature_blame, 0, 1];

This enables system wide, but also disables per-project override.
To enable system wide, while allowing for per project disabling
it should read
# $feature{'blame'} = [\&feature_blame, 1, 1]; # overridable, enabled by default

+# To disbale project wide 

Typo. disbale -> disable.

+# you should have allow-override enabled in  $GITWEB_CONFIG

Example:
# $feature{'blame'} = [\&feature_blame, 1, 1]; # overridable, enabled by default
or just

$feature{'blame'}->[1] = 1;

(See below for comments on that form)

+# and in project config   gitweb.blame = 0;

Example:
# $ git repo-config --bool gitweb.blame false

+# To disable system wide have in $GITWEB_CONFIG
+# $feature{'snapshot'} =  [\&feature_snapshot, 0, undef, undef, undef];

It would be enough to put:
$feature{'snapshot'} =  [\&feature_snapshot, 0, undef];

+# You define site-wide feature defaults here; override them with
+# $GITWEB_CONFIG as necessary.
+our %feature =
+(
+       # feature       => [feature-sub, allow-override, default options...]
+
+       'blame'         => [\&feature_blame, 0, 0],
+       'snapshot'      => [\&feature_snapshot, 0, 'x-gzip', 'gz', 'gzip'],
+);

By the way, wouldn't it be better to use _hash_ for mixed meaning
than _array_? I.e.

our %feature =
(
       # feature       => {'sub' => feature-sub, 'override' => allow-override, 'default' => default options...]

       'blame'         => {'sub' => \&feature_blame, 'override' => 0, 'default' => 0},
   #or 'blame'         => {'sub' => \&feature_blame, 'override' => 0, 'default' => [ 0 ]},
       'snapshot'      => {'sub' => \&feature_snapshot, 'override' => 0, 'default => [ 'x-gzip', 'gz', 'gzip' ]},
);

Then you could enable override, or change default simplier in
$GITWEB_CONFIG, e.g. $feature{'blame'}{'override'} = 1; instead
of $feature{'blame'}[1] = 1;

By the way, it has more sense to have feature by default 
(i.e. in gitweb.perl) with override enabled if it is set to on.

 sub git_get_project_config {

[...]

-       my $val = qx($GIT repo-config --get gitweb.$key);
+       my @x = ($GIT, 'repo-config');
+       if (defined $type) { push @x, $type; }

Just add '--get' as the last argument, _after_ type:
  +       push @x, '--get';

+       push @x, "gitweb.$key";
+       my $val = qx(@x);
+       chomp $val;
        return ($val);
 }

-       die_error('403 Permission denied', "Permission denied") if (!git_get_project_config_bool ('blame'));
+
+       if (!gitweb_check_feature('blame')) {
+               die_error(undef, "Permission denied");
+       }

Why did you drop '403 Permission denied' HTTP return code from call
to die_error? (And not set in other similar cases)?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git