Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King <peff@peff.net>]

On Sat, Mar 04, 2017 at 01:35:04AM +0000, Eric Wong wrote:

> It is disconcerting for users to not notice the behavior
> change in handling alternates from commit cb4d2d35c4622ec2
> ("http: treat http-alternates like redirects")
> 
> Give the user a hint about the config option so they can
> see the URL and decide whether or not they want to enable
> http.followRedirects in their config.

Yeah, I agree it makes sense to notify the user.

> diff --git a/http-walker.c b/http-walker.c
> index b34b6ace7..626badfe6 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -168,6 +168,12 @@ static int is_alternate_allowed(const char *url)
>  	};
>  	int i;
>  
> +	if (http_follow_config != HTTP_FOLLOW_ALWAYS) {
> +		warning("alternate disabled by http.followRedirects!=true: %s",
> +			url);
> +		return 0;
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(protocols); i++) {
>  		const char *end;
>  		if (skip_prefix(url, protocols[i], &end) &&
> @@ -331,9 +337,6 @@ static void fetch_alternates(struct walker *walker, const char *base)
>  	struct alternates_request alt_req;
>  	struct walker_data *cdata = walker->data;
>  
> -	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> -		return;
> -

I was surprised from the description to see not just the addition of a
warning, but a movement of the enforcement code.

I think it's necessary because the original did not bother even fetching
http-alternates if we were not going to respect it. Whereas the new code
will fetch and parse it, and warn only if we actually found something in
it. Which seems reasonable.

The warning itself:

> +		warning("alternate disabled by http.followRedirects!=true: %s",

feels like it could use some whitespace around the "!=", but maybe
that's just me.

-Peff