Re: [PATCH] gitweb: Support for snapshot

[Jakub Narebski <jnareb@gmail.com>]

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