Re: [PATCH RFC 1/2] gitweb: Fix warnings with override permitted but no repo override

[Junio C Hamano <gitster@pobox.com>]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> When a feature like "blame" is permitted to be overridden in the
> repository configuration but it is not actually set in the
> repository, a warning is emitted due to the undefined value
> of the repository configuration, even though it's a perfectly
> normal condition.
>
> The warning is grounds for test failure in the gitweb test script,
> so it causes some new feature tests of mine to fail.
>
> This patch prevents warning and adds a test case to exercise it.
>
> Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
> ---
>
> Here's a small patch I put together while tinkering with bug hyperlinking.
> Does this look reasonable?

To my cursory look, it doesn't, and it is not entirely your fault, but if
we applied this patch, it would not improve things very much.  It just
would shift the same problem around.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7c48181..653f0be 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -402,13 +402,13 @@ sub feature_bool {
>  	my $key = shift;
>  	my ($val) = git_get_project_config($key, '--bool');
>  
> -	if ($val eq 'true') {
> +	if (!defined $val) {
> +		return ($_[0]);
> +	} elsif ($val eq 'true') {
>  		return (1);
>  	} elsif ($val eq 'false') {
>  		return (0);
>  	}

I think the warning you are talking about is to compare $val with 'true'
with 'eq' operator when $val could be undef.  The check to see if $val is
undefined ato avoid that 'eq' comparison is fine, and the intent to return
false is also good, but I think feature_bool is meant to say "yes" or
"no", and existing code for 'false' is returning (0).  I'd rather see your
new codepath normalize incoming undef the same way string 'false' is
normalized to return (0).  Granted, the caller should be prepared to take
the answer as boolean and treat the usual Perl false values (numeric zero,
a string with single "0", or an undef) without barfing, so returning (undef)
from here ought to be safe (otherwise the callers are broken), but I'd
rather see this function play safe.

But it certainly is not my main complaint.

>  sub feature_snapshot {
> @@ -1978,6 +1978,8 @@ sub git_get_project_config {
>  		$config_file = "$git_dir/config";
>  	}
>  
> +	return undef if (!defined $config{"gitweb.$key"});
> +

I think this change is missing a lot of necessary fixes associated with
it.  Have you actually audited all the callers of this function you are
modifying?  For example, feature_bool does this:

        sub feature_bool {
                my $key = shift;
                my ($val) = git_get_project_config($key, '--bool');

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

With your above change, I think a missing configuration variable will
stuff undef in $val, and trigger the same "$val eq 'true'" comparison
warning here.

Granted, without your change the existing code triggers the same error in
another way, by calling config_to_bool sub with undef here:

	# ensure given type
	if (!defined $type) {
		return $config{"gitweb.$key"};
	} elsif ($type eq 'bool') {
		# backward compatibility: 'git config --bool' returns true/false
		return config_to_bool($config{"gitweb.$key"}) ? 'true' : 'false';

and config_to_bool sub is written in the same carelessness like so:

        sub config_to_bool {
                my $val = shift;

                # strip leading and trailing whitespace
                $val =~ s/^\s+//;

and triggers the same error here in the s/// operation.  I think the right
fix for this part would look like this:

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7c48181..2b140cc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1920,6 +1920,8 @@ sub git_parse_project_config {
 sub config_to_bool {
 	my $val = shift;
 
+	return 1 if (!defined $val);
+
 	# strip leading and trailing whitespace
 	$val =~ s/^\s+//;
 	$val =~ s/\s+$//;

Because

	[gitweb]
        	variable

parsed by git_parse_project_config('gitweb') should return a hash that
maps "gitweb.variable" to undef it must be fed as undef to
config_to_bool.  This variable should be reported as "true".

There are tons of undef unsafeness in this file from a very cursory look.

Unrelated to any of these, I think the following is wrong:

        sub feature_patches {
                my @val = (git_get_project_config('patches', '--int'));

                if (@val) {
                        return @val;
                }

                return ($_[0]);
        }

As git_get_project_config() always returns something, hence "if (@val)"
can never be false.