Hi Carlo, On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote: > On Thu, Apr 23, 2020 at 03:28:13PM +0200, Johannes Schindelin wrote: > > On Wed, 22 Apr 2020, Carlo Arenas wrote: > > > On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > > > Johannes Schindelin wrote: > > > > > @@ -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? > > > > > > the previous code was ensuring protocol was always at least "" (albeit it > > > might had been easier to understand with a comment) > > > > I fear that my patches did not make it clear that the lenient mode is > > _only_ used in the config parsing, in which case we very much do not want > > to have the unspecified parts be interpreted as empty strings. > > I think the concern raised was that since we are using the same function > in both cases there might be unintended consequences on changing the > semantics for the other case. Indeed, I share that concern. That's why I wanted to be extra careful there to make sure that introducing this lenient mode does _not_ change the non-lenient mode in the least, i.e. it is the reason why I kept 2/3 separate from 3/3. > the argument made by Jonathan to use something else for configuration > (as is done in master) will help in that direction, and might be needed > anyway as your code it otherwise broken for current maint and master, I am in general not a fan of the idea to have two separate parsers for essentially the same thing. In this instance, the difference between the lenient mode and the non-lenient mode is so obvious that I'd rather reuse the same code (think: Don't Repeat Yourself). > but if not possible (maybe later?) something like this could probably > help : > > diff --git a/credential.c b/credential.c > index 88612e583c..f972fcc895 100644 > --- a/credential.c > +++ b/credential.c > @@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url, > > if (proto_end && proto_end - url > 0) > c->protocol = xmemdupz(url, proto_end - url); > - if (slash - url > 0) > + if (strict || slash > url) > c->host = url_decode_mem(host, slash - host); > + > /* Trim leading and trailing slashes from path */ > while (*slash == '/') > slash++; > > changing the condition there as you suggested to Junio would be a plus IMHO > as well as getting some test that would excercise the new warning that was > introduced in credential.c:57 Yes (modulo doing "greater than" comparison on pointers which is IIRC not permitted in C in general). Ciao, Dscho
