Re: [PATCH 3/5] gitweb: Return plain booleans in validation methods

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2013, 18:11 UTC
Message-ID: <xmqqeh5slc53.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CANQwDweGgQpnqDqaekBWt-rczjHkJEmFyXW9qDh2En2r=ZXbMw@mail.gmail.com>

Jakub Narębski <jnareb@gmail.com> writes:

Show 17 quoted lines

> On Wed, Dec 4, 2013 at 2:43 PM, Krzesimir Nowak <krzesimir@endocode.com> 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.

Show 30 quoted lines

>> Signed-off-by: Krzesimir Nowak <krzesimir@endocode.com>
>> ---
>>  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;