# [PATCH] gitweb: make feature_blame return a list 13 messages from 2008-12-15 to 2008-12-17. Participants: Matt Kraai, Junio C Hamano, Giuseppe Bilotta, Petr Baudis. Thread: https://gitlist.dev/t/16736 ## Matt Kraai, 2008-12-15 14:51 Subject: [PATCH] gitweb: make feature_blame return a list Message-ID: <1229352709-4663-1-git-send-email-kraai@ftbfs.org> URL: https://gitlist.dev/e/1229352709-4663-1-git-send-email-kraai%40ftbfs.org ``` 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 --- 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 ``` ## Matt Kraai, 2008-12-15 14:51 Subject: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <1229352709-4663-2-git-send-email-kraai@ftbfs.org> URL: https://gitlist.dev/e/1229352709-4663-2-git-send-email-kraai%40ftbfs.org In-Reply-To: <1229352709-4663-1-git-send-email-kraai@ftbfs.org> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-12-15 22:20 Subject: Re: [PATCH] gitweb: make feature_blame return a list Message-ID: <7v7i61w06k.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7i61w06k.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1229352709-4663-1-git-send-email-kraai@ftbfs.org> ``` Matt Kraai 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 > --- > 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? ``` ## Junio C Hamano, 2008-12-15 22:21 Subject: Re: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <7v3agpw04m.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3agpw04m.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1229352709-4663-2-git-send-email-kraai@ftbfs.org> ``` Matt Kraai 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... ``` ## Giuseppe Bilotta, 2008-12-15 22:52 Subject: Re: [PATCH] gitweb: make feature_blame return a list Message-ID: URL: https://gitlist.dev/e/gi6n3m%24kvc%241%40ger.gmane.org In-Reply-To: <7v7i61w06k.fsf@gitster.siamese.dyndns.org> ``` On Monday 15 December 2008 23:20, Junio C Hamano wrote: > Matt Kraai 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 >> --- >> 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, 2008-12-16 02:46 Subject: Re: [PATCH] gitweb: make feature_blame return a list Message-ID: <20081216024605.GA4529@ftbfs.org> URL: https://gitlist.dev/e/20081216024605.GA4529%40ftbfs.org In-Reply-To: <7v7i61w06k.fsf@gitster.siamese.dyndns.org> ``` On Mon, Dec 15, 2008 at 02:20:03PM -0800, Junio C Hamano wrote: > Matt Kraai 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, 2008-12-16 05:38 Subject: Re: [PATCH] gitweb: make feature_blame return a list Message-ID: <7vabawu1ao.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vabawu1ao.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20081216024605.GA4529@ftbfs.org> ``` Matt Kraai 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, 2008-12-16 06:16 Subject: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <1229408179-7655-1-git-send-email-kraai@ftbfs.org> URL: https://gitlist.dev/e/1229408179-7655-1-git-send-email-kraai%40ftbfs.org In-Reply-To: <7vabawu1ao.fsf@gitster.siamese.dyndns.org> ``` 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 --- 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 ``` ## Matt Kraai, 2008-12-16 07:36 Subject: [PATCH] gitweb: pass the option name to the feature callback Message-ID: <1229412998-8385-1-git-send-email-kraai@ftbfs.org> URL: https://gitlist.dev/e/1229412998-8385-1-git-send-email-kraai%40ftbfs.org In-Reply-To: <1229408179-7655-1-git-send-email-kraai@ftbfs.org> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-12-16 09:03 Subject: Re: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <7vmyewqypk.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmyewqypk.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1229408179-7655-1-git-send-email-kraai@ftbfs.org> ``` 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, 2008-12-16 14:23 Subject: Re: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <20081216142357.GF4529@ftbfs.org> URL: https://gitlist.dev/e/20081216142357.GF4529%40ftbfs.org In-Reply-To: <7vmyewqypk.fsf@gitster.siamese.dyndns.org> ``` 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/ ``` ## Petr Baudis, 2008-12-17 08:10 Subject: Re: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <20081217081028.GA3640@machine.or.cz> URL: https://gitlist.dev/e/20081217081028.GA3640%40machine.or.cz In-Reply-To: <20081216142357.GF4529@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: > > 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, 2008-12-17 08:20 Subject: Re: [PATCH] gitweb: unify boolean feature subroutines Message-ID: <7vabavp60k.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vabavp60k.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20081217081028.GA3640@machine.or.cz> ``` Petr Baudis 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. ```