[PATCH] gitweb: make feature_blame return a list

[PATCH] gitweb: make feature_blame return a list

[Matt Kraai]

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]);
 }
 
 sub feature_snapshot {
-- 
1.5.6.5

[PATCH] gitweb: unify boolean feature subroutines

[Matt Kraai]

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(-)

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

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

[Junio C Hamano]

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?

Re: [PATCH] gitweb: unify boolean feature subroutines

[Junio C Hamano]

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...

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

[Giuseppe Bilotta]

On Monday 15 December 2008 23:20, Junio C Hamano wrote:

> 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

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

[Matt Kraai]

On Mon, Dec 15, 2008 at 02:20:03PM -0800, Junio C Hamano wrote:
> 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/

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

[Junio C Hamano]

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.

[PATCH] gitweb: unify boolean feature subroutines

[Matt Kraai]

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(-)

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

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

[Matt Kraai]

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.

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

Re: [PATCH] gitweb: unify boolean feature subroutines

[Junio C Hamano]

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.

Re: [PATCH] gitweb: unify boolean feature subroutines

[Matt Kraai]

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/

Re: [PATCH] gitweb: unify boolean feature subroutines

[Petr Baudis]

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. :-)

				Petr "Pasky" Baudis

Re: [PATCH] gitweb: unify boolean feature subroutines

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> 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.