Hi Junio,
On Wed, 22 Apr 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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.
> 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.
> > + 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
