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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 22, 2020, 22:50 UTC
Message-ID: <nycvar.QRO.7.76.6.2004230042370.18039@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqh7xbi28f.fsf@gitster.c.googlers.com>

Hi Junio,

On Wed, 22 Apr 2020, Junio C Hamano wrote:

Show 25 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > -	if (!proto_end || proto_end == url) {
> > +	if (strict && (!proto_end || proto_end == url)) {
> >  		if (!quiet)
> >  			warning(_("url has no scheme: %s"), url);
> >  		return -1;
> >  	}
> > -	cp = proto_end + 3;
> > +	cp = proto_end ? proto_end + 3 : url;
> >  	at = strchr(cp, '@');
> >  	colon = strchr(cp, ':');
> >  	slash = strchrnul(cp, '/');
> > @@ -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);
>
> Missing "proto://" under non-strict mode would leave c->protocol
> NULL (not "") here, as described in [0/3].

Right. I debated whether to set it to `NULL` explicitly, or whether to leave it alone. At the end, I settled with the version in v2.17.4.

Show 5 quoted lines

> Here, slash would be pointing at one of "/?#" at the end of the host
> and url would be pointing at...?  E.g. for "http:///path", URL
> points at 'h' at the beginning, proto_end points at ':', cp points
> at the last '/' before "path" and slash is the same as cp.  host
> points at cp as there is no '@' at sign.

It is not obvious from the diff: `url` is never changed. It would still point at the first `h`, as you said.

Show 5 quoted lines

> > +	if (slash - url > 0)
> > +		c->host = url_decode_mem(host, slash - host);
>
> This wants to make c->host NULL when host is missing, as described
> in [0/3].

I did not describe well enough in 0/3, my apologies.

Again, I tried to go with the version from v2.17.4 here, i.e. to set `c->host` if we get a value, but not set it to `NULL` otherwise (instead leave as-is).

> Shouldn't the condition based on "slash - host", though?

That would have been my preferred solution, too. But there is some subtlety at work: for a `url` like `/this/is/my/path`, we want to leave `c->host` as-is, but for `cert:///...` we want to set it to "" (t0300.23 and t7416.12 test for this explicitly).

I guess a better condition would be `if (proto_end || slash - host > 0)`. What do you think?

> Other than that, it looks sensible.

Thanks, Dscho