threads / patch / 16736

patchgitweb: make feature_blame return a list

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

## tl;dr

13 messages between Dec 15, 2008 and Dec 17, 2008. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Matt Kraai· Dec 15, 2008, 14:51 UTC · lore

The feature defaults are expected to be a list, but feature_blame was returning a scalar. This change makes it consistent with the other boolean feature subroutines.

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to gitweb/gitweb.perl +3 −3
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6eb370d..145e712 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -367,12 +367,12 @@ sub feature_blame {
 	my ($val) = git_get_project_config('blame', '--bool');
 
 	if ($val eq 'true') {
-		return 1;
+		return (1);
 	} elsif ($val eq 'false') {
-		return 0;
+		return (0);
 	}
 
-	return $_[0];
+	return ($_[0]);
 }
 
 sub feature_snapshot {

-- 
1.5.6.5
Matt Kraai· Dec 15, 2008, 14:51 UTC · re: Matt Kraai · lore

[PATCH] gitweb: unify boolean feature subroutines

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

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 gitweb/gitweb.perl |   35 ++++++-----------------------------
 1 files changed, 6 insertions(+), 29 deletions(-)
Show changes to gitweb/gitweb.perl +6 −29
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 145e712..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,8 +363,9 @@ 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);
@@ -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
Junio C Hamano· Dec 15, 2008, 22:21 UTC · re: Matt Kraai · lore

Re: [PATCH] gitweb: unify boolean feature subroutines

Matt Kraai <kraai@ftbfs.org> writes:
> The boolean feature subroutines were identical except for the name of
> the configuration option, so make that a parameter and unify them.

I think this makes sense but unfortunately it comes after the other one that I do not know what its point is...

Junio C Hamano· Dec 15, 2008, 22:20 UTC · re: Matt Kraai · lore

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

Matt Kraai <kraai@ftbfs.org> writes:
Show 27 quoted lines
> The feature defaults are expected to be a list, but feature_blame was
> returning a scalar.  This change makes it consistent with the other
> boolean feature subroutines.
>
> Signed-off-by: Matt Kraai <kraai@ftbfs.org>
> ---
>  gitweb/gitweb.perl |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6eb370d..145e712 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -367,12 +367,12 @@ sub feature_blame {
>  	my ($val) = git_get_project_config('blame', '--bool');
>  
>  	if ($val eq 'true') {
> -		return 1;
> +		return (1);
>  	} elsif ($val eq 'false') {
> -		return 0;
> +		return (0);
>  	}
>  
> -	return $_[0];
> +	return ($_[0]);
>  }

My Perl may be getting rusty, but does the above make any difference? How?

Giuseppe Bilotta· Dec 15, 2008, 22:52 UTC · re: Junio C Hamano · lore

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

On Monday 15 December 2008 23:20, Junio C Hamano wrote:
Show 32 quoted lines
> Matt Kraai <kraai@ftbfs.org> writes:
> 
>> The feature defaults are expected to be a list, but feature_blame was
>> returning a scalar.  This change makes it consistent with the other
>> boolean feature subroutines.
>>
>> Signed-off-by: Matt Kraai <kraai@ftbfs.org>
>> ---
>>  gitweb/gitweb.perl |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 6eb370d..145e712 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -367,12 +367,12 @@ sub feature_blame {
>>      my ($val) = git_get_project_config('blame', '--bool');
>>  
>>      if ($val eq 'true') {
>> -            return 1;
>> +            return (1);
>>      } elsif ($val eq 'false') {
>> -            return 0;
>> +            return (0);
>>      }
>>  
>> -    return $_[0];
>> +    return ($_[0]);
>>  }
> 
> My Perl may be getting rusty, but does the above make any difference?
> How?

It's formally more correct and makes the blame feature fit with the general feature framework, although the feature works correctly even without the (). I was actually going to send a similar patch myself, having missed it during the cleanup and get/check splitup patchset.

Matt's patch gets my Ack, for what it's worth.
-- 
Giuseppe "Oblomov" Bilotta
Matt Kraai· Dec 16, 2008, 02:46 UTC · re: Junio C Hamano · lore

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

On Mon, Dec 15, 2008 at 02:20:03PM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> Matt Kraai <kraai@ftbfs.org> writes:
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -367,12 +367,12 @@ sub feature_blame {
> >  	my ($val) = git_get_project_config('blame', '--bool');
> >  
> >  	if ($val eq 'true') {
> > -		return 1;
> > +		return (1);
> >  	} elsif ($val eq 'false') {
> > -		return 0;
> > +		return (0);
> >  	}
> >  
> > -	return $_[0];
> > +	return ($_[0]);
> >  }
> 
> My Perl may be getting rusty, but does the above make any difference?

I'm still relatively new to Perl, but at least in my limited testing, I couldn't generate a case in which these different constructs had different results.

I made this change so that all of the boolean feature subroutines would have the same body, modulo the name of the option; that way, replacing them with a single routine only involves making the option name a parameter.

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.

-- 
Matt                                                 http://ftbfs.org/
Junio C Hamano· Dec 16, 2008, 05:38 UTC · re: Matt Kraai · lore

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

Matt Kraai <kraai@ftbfs.org> 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.
Matt Kraai· Dec 16, 2008, 06:16 UTC · re: Junio C Hamano · lore

[PATCH] gitweb: unify boolean feature subroutines

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 <kraai@ftbfs.org>
---
 gitweb/gitweb.perl |   41 +++++++++--------------------------------
 1 files changed, 9 insertions(+), 32 deletions(-)
Show changes to gitweb/gitweb.perl +9 −32
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
Matt Kraai· Dec 16, 2008, 07:36 UTC · re: Matt Kraai · lore

[PATCH] gitweb: pass the option name to the feature callback

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 <kraai@ftbfs.org>
---
 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.
Show changes to gitweb/gitweb.perl +6 −6
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
Junio C Hamano· Dec 16, 2008, 09:03 UTC · re: Matt Kraai · lore

Re: [PATCH] gitweb: unify boolean feature subroutines

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.
Matt Kraai· Dec 16, 2008, 14:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitweb: unify boolean feature subroutines

On Tue, Dec 16, 2008 at 01:03:03AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> 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/
Petr Baudis· Dec 17, 2008, 08:10 UTC · re: Matt Kraai · lore

Re: [PATCH] gitweb: unify boolean feature subroutines

Hi,
On Tue, Dec 16, 2008 at 06:23:57AM -0800, Matt Kraai wrote:
Show 11 quoted lines
> 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,

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
Junio C Hamano· Dec 17, 2008, 08:20 UTC · re: Petr Baudis · lore

Re: [PATCH] gitweb: unify boolean feature subroutines

Petr Baudis <pasky@suse.cz> writes:
Show 17 quoted lines
> 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:
>> > 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. :-)
I agree.

It should come *first* before other topics that are not in 'master/next' and change the function signature of feature subs of only existing (read: in 'master') ones. This will force gb/gitweb-patch (and anybody else's patch that haven't been submitted, waiting during the -rc period) to be rebased on top of the updated interface, but that's life.

← back to recent threads