* [PATCH] gitweb: make feature_blame return a list @ 2008-12-15 14:51 Matt Kraai 2008-12-15 14:51 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai 2008-12-15 22:20 ` [PATCH] gitweb: make feature_blame return a list Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Matt Kraai @ 2008-12-15 14:51 UTC (permalink / raw) To: git, gitster; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] gitweb: unify boolean feature subroutines 2008-12-15 14:51 [PATCH] gitweb: make feature_blame return a list Matt Kraai @ 2008-12-15 14:51 ` Matt Kraai 2008-12-15 22:21 ` Junio C Hamano 2008-12-15 22:20 ` [PATCH] gitweb: make feature_blame return a list Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Matt Kraai @ 2008-12-15 14:51 UTC (permalink / raw) To: git, gitster; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: unify boolean feature subroutines 2008-12-15 14:51 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai @ 2008-12-15 22:21 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2008-12-15 22:21 UTC (permalink / raw) To: Matt Kraai; +Cc: git 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... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: make feature_blame return a list 2008-12-15 14:51 [PATCH] gitweb: make feature_blame return a list Matt Kraai 2008-12-15 14:51 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai @ 2008-12-15 22:20 ` Junio C Hamano 2008-12-15 22:52 ` Giuseppe Bilotta 2008-12-16 2:46 ` Matt Kraai 1 sibling, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2008-12-15 22:20 UTC (permalink / raw) To: Matt Kraai; +Cc: git 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: make feature_blame return a list 2008-12-15 22:20 ` [PATCH] gitweb: make feature_blame return a list Junio C Hamano @ 2008-12-15 22:52 ` Giuseppe Bilotta 2008-12-16 2:46 ` Matt Kraai 1 sibling, 0 replies; 13+ messages in thread From: Giuseppe Bilotta @ 2008-12-15 22:52 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: make feature_blame return a list 2008-12-15 22:20 ` [PATCH] gitweb: make feature_blame return a list Junio C Hamano 2008-12-15 22:52 ` Giuseppe Bilotta @ 2008-12-16 2:46 ` Matt Kraai 2008-12-16 5:38 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Matt Kraai @ 2008-12-16 2:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: make feature_blame return a list 2008-12-16 2:46 ` Matt Kraai @ 2008-12-16 5:38 ` Junio C Hamano 2008-12-16 6:16 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2008-12-16 5:38 UTC (permalink / raw) To: Matt Kraai; +Cc: git 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] gitweb: unify boolean feature subroutines 2008-12-16 5:38 ` Junio C Hamano @ 2008-12-16 6:16 ` Matt Kraai 2008-12-16 7:36 ` [PATCH] gitweb: pass the option name to the feature callback Matt Kraai 2008-12-16 9:03 ` [PATCH] gitweb: unify boolean feature subroutines Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Matt Kraai @ 2008-12-16 6:16 UTC (permalink / raw) To: Junio C Hamano, git; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] gitweb: pass the option name to the feature callback 2008-12-16 6:16 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai @ 2008-12-16 7:36 ` Matt Kraai 2008-12-16 9:03 ` [PATCH] gitweb: unify boolean feature subroutines Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Matt Kraai @ 2008-12-16 7:36 UTC (permalink / raw) To: gitster, git; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: unify boolean feature subroutines 2008-12-16 6:16 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai 2008-12-16 7:36 ` [PATCH] gitweb: pass the option name to the feature callback Matt Kraai @ 2008-12-16 9:03 ` Junio C Hamano 2008-12-16 14:23 ` Matt Kraai 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2008-12-16 9:03 UTC (permalink / raw) To: Matt Kraai; +Cc: git, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: unify boolean feature subroutines 2008-12-16 9:03 ` [PATCH] gitweb: unify boolean feature subroutines Junio C Hamano @ 2008-12-16 14:23 ` Matt Kraai 2008-12-17 8:10 ` Petr Baudis 0 siblings, 1 reply; 13+ messages in thread From: Matt Kraai @ 2008-12-16 14:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jakub Narebski 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: unify boolean feature subroutines 2008-12-16 14:23 ` Matt Kraai @ 2008-12-17 8:10 ` Petr Baudis 2008-12-17 8:20 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Petr Baudis @ 2008-12-17 8:10 UTC (permalink / raw) To: Matt Kraai; +Cc: Junio C Hamano, git, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gitweb: unify boolean feature subroutines 2008-12-17 8:10 ` Petr Baudis @ 2008-12-17 8:20 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2008-12-17 8:20 UTC (permalink / raw) To: Petr Baudis; +Cc: Matt Kraai, git, Jakub Narebski 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-12-17 8:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-12-15 14:51 [PATCH] gitweb: make feature_blame return a list Matt Kraai 2008-12-15 14:51 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai 2008-12-15 22:21 ` Junio C Hamano 2008-12-15 22:20 ` [PATCH] gitweb: make feature_blame return a list Junio C Hamano 2008-12-15 22:52 ` Giuseppe Bilotta 2008-12-16 2:46 ` Matt Kraai 2008-12-16 5:38 ` Junio C Hamano 2008-12-16 6:16 ` [PATCH] gitweb: unify boolean feature subroutines Matt Kraai 2008-12-16 7:36 ` [PATCH] gitweb: pass the option name to the feature callback Matt Kraai 2008-12-16 9:03 ` [PATCH] gitweb: unify boolean feature subroutines Junio C Hamano 2008-12-16 14:23 ` Matt Kraai 2008-12-17 8:10 ` Petr Baudis 2008-12-17 8:20 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.