Re: [PATCH] gitweb: make feature_blame return a list

Matt Kraai [off-list ref] writes:

If you'd like me to resubmit my second patch, I'm happy to do so.
Just let me know whether you prefer the resulting function to wrap its
return values in parentheses (as is currently done by feature_grep and
feature_pickaxe) or not.

Sure, keeping what feature_grep does is just fine.

Thanks.

The boolean feature subroutines behaved identically except for the
name of the configuration option, so make that a parameter and unify
them.

Signed-off-by: Matt Kraai <redacted>
---
 gitweb/gitweb.perl |   41 +++++++++--------------------------------
 1 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..827e5c5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -203,7 +203,7 @@ our %feature = (
 	# $feature{'blame'}{'override'} = 1;
 	# and in project config gitweb.blame = 0|1;
 	'blame' => {
-		'sub' => \&feature_blame,
+		'sub' => sub { feature_bool('blame', @_) },
 		'override' => 0,
 		'default' => [0]},
 

@@ -241,7 +241,7 @@ our %feature = (
 	# $feature{'grep'}{'override'} = 1;
 	# and in project config gitweb.grep = 0|1;
 	'grep' => {
-		'sub' => \&feature_grep,
+		'sub' => sub { feature_bool('grep', @_) },
 		'override' => 0,
 		'default' => [1]},
 

@@ -255,7 +255,7 @@ our %feature = (
 	# $feature{'pickaxe'}{'override'} = 1;
 	# and in project config gitweb.pickaxe = 0|1;
 	'pickaxe' => {
-		'sub' => \&feature_pickaxe,
+		'sub' => sub { feature_bool('pickaxe', @_) },
 		'override' => 0,
 		'default' => [1]},
 

@@ -363,16 +363,17 @@ sub gitweb_check_feature {
 }
 
 
-sub feature_blame {
-	my ($val) = git_get_project_config('blame', '--bool');
+sub feature_bool {
+	my $key = shift;
+	my ($val) = git_get_project_config($key, '--bool');
 
 	if ($val eq 'true') {
-		return 1;
+		return (1);
 	} elsif ($val eq 'false') {
-		return 0;
+		return (0);
 	}
 
-	return $_[0];
+	return ($_[0]);
 }
 
 sub feature_snapshot {

@@ -387,30 +388,6 @@ sub feature_snapshot {
 	return @fmts;
 }
 
-sub feature_grep {
-	my ($val) = git_get_project_config('grep', '--bool');
-
-	if ($val eq 'true') {
-		return (1);
-	} elsif ($val eq 'false') {
-		return (0);
-	}
-
-	return ($_[0]);
-}
-
-sub feature_pickaxe {
-	my ($val) = git_get_project_config('pickaxe', '--bool');
-
-	if ($val eq 'true') {
-		return (1);
-	} elsif ($val eq 'false') {
-		return (0);
-	}
-
-	return ($_[0]);
-}
-
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.

-- 
1.5.6.5

The feature_bool callback required the option name to be passed to
it.  Make gitweb_get_feature do so instead of constructing an
anonymous subroutine for each overrideable boolean option.

Signed-off-by: Matt Kraai <redacted>
---
 gitweb/gitweb.perl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

   Sorry for the churn; I read the corresponding part of Higher-Order
   Perl *after* sending my previous patches.  :(

   If I should squash this into my previous patch and resubmit it,
   please let me know.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 827e5c5..3459293 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -203,7 +203,7 @@ our %feature = (
 	# $feature{'blame'}{'override'} = 1;
 	# and in project config gitweb.blame = 0|1;
 	'blame' => {
-		'sub' => sub { feature_bool('blame', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [0]},
 

@@ -241,7 +241,7 @@ our %feature = (
 	# $feature{'grep'}{'override'} = 1;
 	# and in project config gitweb.grep = 0|1;
 	'grep' => {
-		'sub' => sub { feature_bool('grep', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [1]},
 

@@ -255,7 +255,7 @@ our %feature = (
 	# $feature{'pickaxe'}{'override'} = 1;
 	# and in project config gitweb.pickaxe = 0|1;
 	'pickaxe' => {
-		'sub' => sub { feature_bool('pickaxe', @_) },
+		'sub' => \&feature_bool,
 		'override' => 0,
 		'default' => [1]},
 

@@ -344,7 +344,7 @@ sub gitweb_get_feature {
 		warn "feature $name is not overrideable";
 		return @defaults;
 	}
-	return $sub->(@defaults);
+	return $sub->($name, @defaults);
 }
 
 # A wrapper to check if a given feature is enabled.

@@ -377,9 +377,9 @@ sub feature_bool {
 }
 
 sub feature_snapshot {
-	my (@fmts) = @_;
+	my ($key, @fmts) = @_;
 
-	my ($val) = git_get_project_config('snapshot');
+	my ($val) = git_get_project_config($key);
 
 	if ($val) {
 		@fmts = ($val eq 'none' ? () : split /\s*[,\s]\s*/, $val);

-- 
1.5.6.5

I'll queue this in 'pu' for now; it has obvious and trivial conflicts with
other gitweb series that introduce new features ;-)

And regarding your follow-up patch you called "churn", I think it is
probably a good idea in the longer term, although I haven't really
looked at all the callers to make sure everybody would be happy.

But a change to the function signature of feature subroutines is not
something I'd like to apply while other series that want to add new
features are still cooking.  How about doing these two patches as the
first thing that goes to 'next' after 1.6.1, and then force other series
rebase on top of your change?  Alternatively, we could make you wait until
other series do settle in 'next' and then apply your change rebased on
them, but I think that is probably less optimal.

Thanks.

On Tue, Dec 16, 2008 at 01:03:03AM -0800, Junio C Hamano wrote:

But a change to the function signature of feature subroutines is not
something I'd like to apply while other series that want to add new
features are still cooking.  How about doing these two patches as the
first thing that goes to 'next' after 1.6.1, and then force other series
rebase on top of your change?  Alternatively, we could make you wait until
other series do settle in 'next' and then apply your change rebased on
them, but I think that is probably less optimal.

OK, I'll resubmit the patches on top of 'next' once 1.6.1 is
released.  Thanks for your help,

-- 
Matt                                                 http://ftbfs.org/

Hi,

On Tue, Dec 16, 2008 at 06:23:57AM -0800, Matt Kraai wrote:

On Tue, Dec 16, 2008 at 01:03:03AM -0800, Junio C Hamano wrote:

quoted

But a change to the function signature of feature subroutines is not
something I'd like to apply while other series that want to add new
features are still cooking.  How about doing these two patches as the
first thing that goes to 'next' after 1.6.1, and then force other series
rebase on top of your change?  Alternatively, we could make you wait until
other series do settle in 'next' and then apply your change rebased on
them, but I think that is probably less optimal.

OK, I'll resubmit the patches on top of 'next' once 1.6.1 is
released.  Thanks for your help,

is it worth keeping them separate? Just a single patch makes more sense
to me, the interface is much nicer in the latter than in the former. :-)

				Petr "Pasky" Baudis