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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html