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

From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Jeff King <peff@peff.net>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Ilya Tretyakov <it@it3xl.ru>
Subject: Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode
Date: Thu, 23 Apr 2020 14:22:12 -0700	[thread overview]
Message-ID: <20200423212212.GA20669@Carlos-MBP> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2004231433060.18039@tvgsbejvaqbjf.bet>

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@gmail.com> 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.

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, 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

Carlo
