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 99f71b4..4ba2a9b 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

--

[ message continues ]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4ba2a9b..2edefda 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

--

[ message continues ]

2009/1/3 Matt Kraai <kraai@ftbfs.org>:

Maybe that should be:

           return ($val eq 'true');

as It is not a good idea to use 0 as a replacement for perls false, as
the two have different behaviour.

$perl -wle'my $val=shift; my $x=$val eq "true"; print "<$_>" for $x, 0+$x' false
<>
<0>

Cheers,
yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"
--

[ message continues ]

I don't think Perl has *a* false value, but rather has multiple values
that are treated as false, such as undef, zero, and the empty string.
Personally, I find 0 clearer than the empty string, but that's
probably just my C bias sneaking in.

All of the boolean feature values use 0 or 1, so if this should be
changed, I think it should probably be done as a separate patch.

-- 
Matt                                                 http://ftbfs.org/
--

[ message continues ]

Yes it definitely does have a false value, PL_sv_no, and a true value,
PL_sv_yes (although it is much less important).  It is these values
which are copied to signify true and false in the cases where the
internals need to, such as for boolean operators that must return
false, and things like negation and (in)equality checks.

It is a so called "dual var" SvPVNV, with 0 in the NV (numeric) slot
and the empty string in the PV (string) slot.

You can see one example of its behaviour in my previous mail, and can
see it further here:

$ perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true")'
SV = PVNV(0x952eb10) at 0x952b6f0
  REFCNT = 2147483647
  FLAGS = (IOK,NOK,POK,READONLY,pIOK,pNOK,pPOK)
  IV = 0
  NV = 0
  PV = 0x952eae8 ""\0
  CUR = 0
  LEN = 4

Compare that to:

perl -MDevel::Peek -e'print Dump(shift @ARGV eq "true" ? 1 : 0)'
SV = IV(0x94d8398) at 0x94bd678
  REFCNT = 1
  FLAGS = (PADBUSY,PADTMP,IOK,READONLY,pIOK)

As you think is best. It is after all a nit, and probably one that is
harmless. But I've been bitten by people not using the languages
native booleans before, and well, once bitten twice shy.

cheers,
Yves
-- 
perl -Mre=debug -e "/just|another|perl|hacker/"
--

[ message continues ]

Wow, I had no idea about this.  Thanks for the information.

It seems like using these values would require obfuscating the code,
though.  :(  They only seem to be exposed directly via XS.

-- 
Matt                                                 http://ftbfs.org/
--

[ message continues ]

Depend how you look at it. You have access to them via the negation
and inequality operators. In a way you can think of the 'not' (or  !)
operator as being the "inverted boolean constructor". So for instance
the kind of surprising

  my $bool= !!$val;

can be used to get a copy of the appropriate PL_sv_yes/no. But the
need to do is rare. Luckily. :-)

But yeah, they are not exposed directly at the perl level. There is no
keyword to use to generate them like there is for undef/PL_sv_undef,
although I guess it would be fairly easy to expose them in a similar
fashion via Scalar::Util.

cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"
--

[ message continues ]

I'd rather want to keep our scripts free of deep Perl magic.  Even if
there are SVs that are interpreted as false other than 0, that does not
necessarily mean you have to fear that 0 can sometimes evaluate to true.

As long as you refrain from doing something crazy like "0 but true",
people who are not (and/or are not inclined to become) familiar with the
gory innards of Perl can rely on 0 being false and 1 being true when
calling feature_something subs, no?
--

[ message continues ]

No, thats not the point. The point is that why should the code do more
work to return a value that a perl programmer might find unexpected.
Especially when the function has the word "bool" in it. Its like
writing a function whose name says "int" that returns a double. If the

Why execute more opcodes, and return a slightly surprising false, when
you dont have to?

Is it really deep perl magic to do:

  return $val eq 'true';

instead of

  return $val eq 'true' ? 1 : 0;

or the actual use:

   if ($val eq 'true') {
      return 1
   } else {
      return 0
   }

Isn't the former superior just on pure minimalism metrics? Theres less
code to understand, less code to go wrong, and as a bonus it returns a
true boolean. Isn't that just a win-win-win? I mean most perl
programmers I know would instantly convert the latter two to the first
just on the grounds that the first version is the clearest expression
of the desired intent.

Anyway, leave it or not, its a minor nit.

cheers,
Yves




-- 
perl -Mre=debug -e "/just|another|perl|hacker/"
--

[ message continues ]

I agree that what you suggest is better than the alternatives you
present.  Unfortunately, none of them match the current behavior.
Here's the current code:

	if ($val eq 'true') {
		return 1;
	} elsif ($val eq 'false') {
		return 0;
	}

	return $_[0];

Is there a way to use the form you suggest while falling back to the
default if $val isn't set to 'true' or 'false'?

-- 
Matt                                                 http://ftbfs.org/
--

[ message continues ]

IIRC the return value is actually threestate: 1 for true, 0 for false,
and undef for non-bool config value.

BTW. git_get_project_config currently emulates old one git-config call
per configuration variable, with git-config normalizing boolean values
(returning 'true' or 'false'). But it could return Perl truish or Perl
falsish instead, as we now use "git config -l -z" + caching config
variables, and it is Perl that does normalization of boolean values.

-- 
Jakub Narebski
Poland
ShadeHawk on #git
--

[ message continues ]