Re: [PATCH] gitweb: fixes to gitweb feature check code

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2008, 21:11 UTC
Message-ID: <7vwsf31ima.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1226759165-6894-1-git-send-email-giuseppe.bilotta@gmail.com>

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and to check if a
> feature was enabled.

Show 10 quoted lines

> +# check if a given feature is enabled or not, returning the first (and only)
> +# value of the feature. Comfort code, allowing the use of
> +#   my $bool_feat = gitweb_check_feature('bool_feat');
> +# or
> +#   gitweb_check_feature('bool_feat') or somecode;
> +# instead of
> +#   my ($bool_feat) = gitweb_git_feature('bool_feat');
> +# or
> +#   (gitweb_check_feature('bool_feat'))[0] or somecode;
> +# respectively

What's "Comfort code"?

I'd agree that introduction of gitweb_get_feature() may help avoiding mistakes at the call sites for Perl illiterates like myself.

Show 7 quoted lines

> @@ -767,7 +785,7 @@ our $git_dir;
>  $git_dir = "$projectroot/$project" if $project;
>  
>  # list of supported snapshot formats
> -our @snapshot_fmts = gitweb_check_feature('snapshot');
> +our @snapshot_fmts = gitweb_get_feature('snapshot');
>  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);

And this may be a good change from that point of view, but...

Show 6 quoted lines

> @@ -810,7 +828,7 @@ sub href (%) {
>  		}
>  	}
>  
> -	my ($use_pathinfo) = gitweb_check_feature('pathinfo');
> +	my $use_pathinfo = gitweb_check_feature('pathinfo');

... I do not think changes like these are warranted. They have been using the function _correctly_ by calling it in the list context and I think they will continue to work with your patch.