[PATCH] gitweb: merge boolean feature subroutines

[PATCH] gitweb: merge boolean feature subroutines

[Matt Kraai]

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

[PATCH] gitweb: pass the key to the feature subroutines

[Matt Kraai]

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

Re: [PATCH] gitweb: merge boolean feature subroutines

[demerphq]

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
[...]
> -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;

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/"

Re: [PATCH] gitweb: merge boolean feature subroutines

[Matt Kraai]

On Sat, Jan 03, 2009 at 05:18:44PM +0100, demerphq wrote:
> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> [...]
> > -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;
> 
> 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>

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/

Re: [PATCH] gitweb: merge boolean feature subroutines

[demerphq]

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> On Sat, Jan 03, 2009 at 05:18:44PM +0100, demerphq wrote:
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> [...]
>> > -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;
>>
>> 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>
>
> 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.

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)
  IV = 0

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

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/"

Re: [PATCH] gitweb: merge boolean feature subroutines

[Matt Kraai]

On Sat, Jan 03, 2009 at 05:51:50PM +0100, demerphq wrote:
> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> > 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.
> 
> 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)
>   IV = 0

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/

Re: [PATCH] gitweb: merge boolean feature subroutines

[demerphq]

2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> On Sat, Jan 03, 2009 at 05:51:50PM +0100, demerphq wrote:
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> > 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.
>>
>> 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)
>>   IV = 0
>
> 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.

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/"

Re: [PATCH] gitweb: merge boolean feature subroutines

[Junio C Hamano]

demerphq <demerphq@gmail.com> writes:

> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
> [...]
>> -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;
>
> 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.

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?

Re: [PATCH] gitweb: merge boolean feature subroutines

[demerphq]

2009/1/4 Junio C Hamano <gitster@pobox.com>:
> demerphq <demerphq@gmail.com> writes:
>
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> [...]
>>> -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;
>>
>> 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.
>
> 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.

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
routine was not called "bool" then it wouldnt matter at all.

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

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/"

Re: [PATCH] gitweb: merge boolean feature subroutines

[Matt Kraai]

On Sun, Jan 04, 2009 at 12:41:14PM +0100, demerphq wrote:
> 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.

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/

Re: [PATCH] gitweb: merge boolean feature subroutines

[Junio C Hamano]

demerphq <demerphq@gmail.com> writes:

> Is it really deep perl magic to do:
>
>   return $val eq 'true';
>
> instead of
>
>   return $val eq 'true' ? 1 : 0;

No, neither are magicky.  But your argument to favor the former over the
latter that goes down to XS level was all about deep magic, and you wrote
yourself:

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

My point is that any caller that cares about the differences of "Perl's
true false" and 0 when talking about a function that returns a yes/no
value is already soaked too deep in Perl's deep magic.  I would want the
code to be maintainable by people who does not care the deep voodoo, and
for that reason, I do not want the callers to care.

Having said that, I think it is perfectly fine to favor returning "$val eq
'true'" over returning "$val eq 'true ? 1 : 0".  But that is not because
it is truer way to say false from Perl experts' point of view, but because
it is shorter and more to the point.

Re: [PATCH] gitweb: merge boolean feature subroutines

[Jakub Narebski]

Matt Kraai <kraai@ftbfs.org> writes:

> 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'?

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