Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 22, 2020, 23:38 UTC
Message-ID: <20200422233854.GE140314@google.com>
In-Reply-To: <1081841b16de31693473e72ff817bed5f0064dda.1587588665.git.gitgitgadget@gmail.com>

Johannes Schindelin wrote:

> There was one call site, though, that really needed that leniency: when
> parsing config settings a la `credential.dev.azure.com.useHTTPPath`.

Thanks for tackling this.

Can the commit message say a little more about the semantics and when someone would use this?

Is it a shortcut for

	[credential "http://dev.azure.com"]
		useHttpPath = true

	[credential "https://dev.azure.com"]
		useHttpPath = true

> In preparation for fixing that regression, let's add a parameter called
> `strict` to the `credential_from_url()` function and convert the
> existing callers to enforce that strict mode.

I suspect this would be easier to read squashed with patch 3. That would also mean that the functionality and test coverage come at the same time.

[...]

Show 10 quoted lines

> diff --git a/credential.c b/credential.c
> index 64a841eddca..c73260ac40f 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
>  }
>  
>  int credential_from_url_gently(struct credential *c, const char *url,
> -			       int quiet)
> +			       int strict, int quiet)

The collection of flags makes me wonder whether it's time to use a single "flags" parameter with flags that are |ed together. That way, call sites are easier to read without requiring cross-reference assistance to see which option each boolean parameter represents.

Alternatively, could the non-strict form be a separate public function that uses the same static helper that takes two boolean args? That is, something like

	int credential_from_url_gently(struct credential *c, const char *url,
				       int quiet)
	{
		return parse_credential_url(c, url, 1, quiet);
	}

	int credential_from_url_nonstrict(struct credential *c, const char *url,
					  int quiet)
	{
		return parse_credential_url(c, url, 0, quiet);
	}

[...]

Show 10 quoted lines

> @@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
>  	 *   (3) proto://<user>:<pass>@<host>/...
>  	 */
>  	proto_end = strstr(url, "://");
> -	if (!proto_end || proto_end == url) {
> +	if (strict && (!proto_end || proto_end == url)) {
>  		if (!quiet)
>  			warning(_("url has no scheme: %s"), url);
>  		return -1;
>  	}

When !strict, this means we are not requiring a protocol. No other difference appears to be intended.

[...]

Show 8 quoted lines

> @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
>  		host = at + 1;
>  	}
>  
> -	c->protocol = xmemdupz(url, proto_end - url);
> -	c->host = url_decode_mem(host, slash - host);
> +	if (proto_end && proto_end - url > 0)
> +		c->protocol = xmemdupz(url, proto_end - url);

What should happen when the protocol isn't present? Does this mean callers will need to be audited to make sure they handle NULL?

> +	if (slash - url > 0)
> +		c->host = url_decode_mem(host, slash - host);

What should happen the URL starts with a slash?

Thanks, Jonathan