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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2009, 08:40 UTC
Message-ID: <7vvdr8yw78.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234926043-7471-1-git-send-email-marcel@oak.homeunix.org>

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

Show 16 quoted lines

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

Show 16 quoted lines

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

Show 7 quoted lines

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