Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
Jakub Narębski writes: > But I am not against "return 0;" on validation error (would putting > it in separate patch make review easier, or just pointlessly proliferate > patches?). OK. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
On Thu, Dec 5, 2013 at 7:16 PM, Junio C Hamano wrote: > Krzesimir Nowak writes: >> On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote: >>> The only change that needs to be done is replacing >>> >>>return $input; >>> >>> with >>> >>>return 1; >>> >> >> I prefer to use zeros instead of undefs - one might wonder if that undef >> is somewhat special that we can't use 0. > > For Perl speakers, I suspect the code gives a totally opposite > impression. Normal "false" return from a sub, when there is no > special need, is to return an undef from it, and a "return 0" would > make the readers wonder if there is something special about the way > the returned value has to be numeric zero, no? Or even plain "return;" (see explanation for policy in [1])... though for functions returning scalar it is recommended to use explicit "return 0;" (or "return undef;"). Anyway, it is easier to see the change and intention of the change if all that is changed id "return $input;" to "return 1;" But I am not against "return 0;" on validation error (would putting it in separate patch make review easier, or just pointlessly proliferate patches?). [1]: http://search.cpan.org/~thaljef/Perl-Critic-1.121/lib/Perl/Critic/Policy/Subroutines/ProhibitExplicitReturnUndef.pm -- Jakub Narebski -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
Krzesimir Nowak writes: > On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote: >> The only change that needs to be doe is replacing >> >>return $input; >> >> with >> >>return 1; >> > > I prefer to use zeros instead of undefs - one might wonder if that undef > is somewhat special that we can't use 0. For Perl speakers, I suspect the code gives a totally opposite impression. Normal "false" return from a sub, when there is no special need, is to return an undef from it, and a "return 0" would make the readers wonder if there is something special about the way the returned value has to be numeric zero, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
On Wed, 2013-12-04 at 17:07 +0100, Jakub Narębski wrote:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak
> wrote:
>
> > Users of validate_* passing "0" might get failures on correct name
> > because of coercion of "0" to false in code like:
> > die_error(500, "invalid ref") unless (check_ref_format ("0"));
>
> I would say that the problem was that validate_sth() subroutines returned
> value of parameter if it was valid, which could be a problem if said value is
> false-ish (e.g. validate_refname("0"), or validate_pathname("0")).
That's what I meant.
>
> Returning undef on invalid data newer was a problem, using 'return $input;'
> on valid input was, especially that validate_sth() functions were ever used
> in a conditional:
>
> if (!validate_sth($param)) {
> die_error(...)
> }
>
> While at it validate_sth() is not a best name for boolean predicate:
> is_valid_sth() would be better, I think.
Ok, I'll rename those.
>
> > Signed-off-by: Krzesimir Nowak
> > ---
> > gitweb/gitweb.perl | 45 +
> > 1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 67415b9..3434602 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -1419,63 +1419,68 @@ sub href {
> > ## validation, quoting/unquoting and escaping
> >
> > sub validate_action {
> > - my $input = shift || return undef;
> > - return undef unless exists $actions{$input};
> > - return $input;
> > + my $input = shift;
> > +
> > + return 0 unless defined $input;
> > + return 0 unless exists $actions{$input};
> > + return 1;
> > }
>
> The only change that needs to be doe is replacing
>
>return $input;
>
> with
>
>return 1;
>
I prefer to use zeros instead of undefs - one might wonder if that undef
is somewhat special that we can't use 0.
--
Krzesimir Nowak
Software Developer
Endocode AG
krzesi...@endocode.com
--
Endocode AG, Johannisstraße 20, 10117 Berlin
i...@endocode.com | www.endocode.com
Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Karl Beecher, Chris Kühl, Sebastian Sucker
Aufsichtsratsvorsitzende: Jennifer Beecher
Registergericht: Amtsgericht Charlottenburg - HRB 150748 B
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
Jakub Narębski writes:
> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak
> wrote:
>
>> Users of validate_* passing "0" might get failures on correct name
>> because of coercion of "0" to false in code like:
>> die_error(500, "invalid ref") unless (check_ref_format ("0"));
>
> I would say that the problem was that validate_sth() subroutines returned
> value of parameter if it was valid, which could be a problem if said value is
> false-ish (e.g. validate_refname("0"), or validate_pathname("0")).
>
> Returning undef on invalid data newer was a problem, using 'return $input;'
> on valid input was, especially that validate_sth() functions were ever used
> in a conditional:
>
> if (!validate_sth($param)) {
> die_error(...)
> }
Correct, and I think that is exactly what the above three lines in
the proposed log message says.
> While at it validate_sth() is not a best name for boolean predicate:
> is_valid_sth() would be better, I think.
Yeah, I tend to agree. As we are doing a whole-sale API clean-up in
this patch, we might as well do that at the same time.
>> Signed-off-by: Krzesimir Nowak
>> ---
>> gitweb/gitweb.perl | 45 +
>> 1 file changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 67415b9..3434602 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -1419,63 +1419,68 @@ sub href {
>> ## validation, quoting/unquoting and escaping
>>
>> sub validate_action {
>> - my $input = shift || return undef;
>> - return undef unless exists $actions{$input};
>> - return $input;
>> + my $input = shift;
>> +
>> + return 0 unless defined $input;
>> + return 0 unless exists $actions{$input};
>> + return 1;
>> }
>
> The only change that needs to be doe is replacing
>
>return $input;
>
> with
>
>return 1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods
On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak wrote:
> Users of validate_* passing "0" might get failures on correct name
> because of coercion of "0" to false in code like:
> die_error(500, "invalid ref") unless (check_ref_format ("0"));
I would say that the problem was that validate_sth() subroutines returned
value of parameter if it was valid, which could be a problem if said value is
false-ish (e.g. validate_refname("0"), or validate_pathname("0")).
Returning undef on invalid data newer was a problem, using 'return $input;'
on valid input was, especially that validate_sth() functions were ever used
in a conditional:
if (!validate_sth($param)) {
die_error(...)
}
While at it validate_sth() is not a best name for boolean predicate:
is_valid_sth() would be better, I think.
> Signed-off-by: Krzesimir Nowak
> ---
> gitweb/gitweb.perl | 45 +
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 67415b9..3434602 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1419,63 +1419,68 @@ sub href {
> ## validation, quoting/unquoting and escaping
>
> sub validate_action {
> - my $input = shift || return undef;
> - return undef unless exists $actions{$input};
> - return $input;
> + my $input = shift;
> +
> + return 0 unless defined $input;
> + return 0 unless exists $actions{$input};
> + return 1;
> }
The only change that needs to be doe is replacing
return $input;
with
return 1;
--
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] gitweb: Return plain booleans in validation methods
Users of validate_* passing "0" might get failures on correct name
because of coercion of "0" to false in code like:
die_error(500, "invalid ref") unless (check_ref_format ("0"));
Signed-off-by: Krzesimir Nowak
---
gitweb/gitweb.perl | 45 +
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 67415b9..3434602 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1419,63 +1419,68 @@ sub href {
## validation, quoting/unquoting and escaping
sub validate_action {
- my $input = shift || return undef;
- return undef unless exists $actions{$input};
- return $input;
+ my $input = shift;
+
+ return 0 unless defined $input;
+ return 0 unless exists $actions{$input};
+ return 1;
}
sub validate_project {
- my $input = shift || return undef;
+ my $input = shift;
+
+ return 0 unless defined $input;
if (!validate_pathname($input) ||
!(-d "$projectroot/$input") ||
!check_export_ok("$projectroot/$input") ||
($strict_export && !project_in_list($input))) {
- return undef;
+ return 0;
} else {
- return $input;
+ return 1;
}
}
sub validate_pathname {
- my $input = shift || return undef;
+ my $input = shift;
+ return 0 unless defined $input;
# no '.' or '..' as elements of path, i.e. no '.' nor '..'
# at the beginning, at the end, and between slashes.
# also this catches doubled slashes
if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
- return undef;
+ return 0;
}
# no null characters
if ($input =~ m!\0!) {
- return undef;
+ return 0;
}
- return $input;
+ return 1;
}
sub check_ref_format {
- my $input = shift || return undef;
+ my $input = shift;
+ return 0 unless defined $input;
# restrictions on ref name according to git-check-ref-format
if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
- return undef;
+ return 0;
}
- return $input;
+ return 1;
}
sub validate_refname {
- my $input = shift || return undef;
+ my $input = shift;
+ return undef unless defined $input;
# textual hashes are O.K.
if ($input =~ m/^[0-9a-fA-F]{40}$/) {
- return $input;
+ return 1;
}
# it must be correct pathname
- $input = validate_pathname($input)
- or return undef;
+ validate_pathname($input) or return 0;
# check git-check-ref-format restrictions
- check_ref_format($input)
- or return undef;
- return $input;
+ check_ref_format($input) or return 0;
+ return 1;
}
# decode sequences of octets in utf8 into Perl's internal form,
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
