Re: [PATCH 3/3] credential: handle `credential.<partial-URL>.<key>` again

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 23, 2020, 23:19 UTC
Message-ID: <nycvar.QRO.7.76.6.2004240057220.18039@tvgsbejvaqbjf.bet>
In-Reply-To: <20200422235708.GF140314@google.com>

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

Show 19 quoted lines

> Johannes Schindelin wrote:
>
> [...]
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,
> >  		char *url = xmemdupz(key, dot - key);
> >  		int matched;
> >
> > -		credential_from_url(&want, url);
> > +		if (credential_from_url_gently(&want, url, 0, 0) < 0) {
> > +			warning(_("skipping credential lookup for url: %s"), url);
> > +			credential_clear(c);
>
> Hm, the error message doesn't seem right here, since `url` is a config
> key instead of URL whose credential's are being looked up.
>
> Should the error message include the entirety of `var` to make
> debugging easier?

I suppose you're right.

BTW I just realized a much worse issue: `credential_clear(c);`. This clears the wrong struct. It should clear `want`, not `c`.

Show 14 quoted lines

> [...]
> > --- a/t/t0300-credentials.sh
> > +++ b/t/t0300-credentials.sh
> > @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '
> >  	test_i18ncmp expect stderr
> >  '
> >
> > +test_expect_success 'credential config accepts partial URLs' '
> > +	echo url=https://example.com |
> > +	git -c credential.example.com.username=boo \
> > +		credential fill >actual &&
>
> Can the tests also check the behavior with bad URLs (that we are
> appropriately tolerating and warning about them?

Yes, my bad! The next iteration will have a test for that.

Show 5 quoted lines

> Stepping back: one thing I like about the code in "master" that uses
> urlmatch_config_entry is that it is not treating these config keys
> in the same way as URLs that Git would fetch from.  For example, if
> one of the config keys contains %0a, then that's perfectly fine ---
> we are not going to write them to a credential helper or to libcurl.

That is actually not what the code does, at least not in `maint-2.17`. It very much warns about `%0a` in config keys. The test I am adding to verify that the warning above is exercised correctly looks like this:

	git -c credential.$partial.helper=yep \
                -c credential.with%0anewline.username=uh-oh \
                credential fill >output 2>error &&
        test_i18ngrep "skipping credential lookup for url" error

That is literally the only way I get `credential_from_url_gently()` to return `-1` in the lenient mode.

> The only problem is that the pattern matching syntax doesn't match
> the behavior that users historically expected.  (Keeping in mind
> that we never actually provided the behavior that users expected.
> `credential.example.com.helper` settings applied to all hosts.)

Yes, I fix that, too. It is a bad usability bug, in my eyes, and I think it is better to fix it while I'm in the space anyway.

Show 17 quoted lines

> Would it make sense for parsing these config url patterns to share
> *less* code with credential_from_url?  Ramifications:
>
> - unless we add specific support for it, we'd lose support for
>   patterns that are specific to a user (e.g.
>   "credential.https://user@example.com.helper").
>
> - likewise, we'd lose support for
>   "credential.https://user:pass@example.com.helper".
>
> - we could control what "credential.https:///repo.git.helper"
>   means, for example by rejecting it.  When we reject it, the
>   error message could be specific to this use case instead of
>   shared with other URL handling.
>
> - we wouldn't suport "credential.example.com/repo.git.helper"
>   by mistake.

I think we can have _all_ of this _without_ violating the DRY principle.

Remember: my main motivation for keeping 2/3 apart from 3/3 was so that it
is really easy to verify that 2/3 does not break the callers that _need_
the strict mode.

And since that is the case, we can then enjoy the benefit of the shared code for the one caller that wants to match also partial URLs.

Show 42 quoted lines

> - to sum up, we could specifically define exactly what cases we want
>   to support:
>
> 	[credential "example.com"]
> 		# settings for the host
> 		...
>
> 	[credential "user@example.com"] # maybe
> 		# settings for the host/user combination
> 		...
>
> 	[credential "https://"]
> 		# settings for the scheme
> 		...
>
> 	[credential "https://example.com"]
> 		# settings for the host/scheme combination
> 		...
>
> 	[credential "https://example.com/"]
> 		# likewise
> 		...
>
> 	[credential "https://user@example.com"] # maybe
> 		# settings for the host/scheme/user combination
> 		...
>
> 	[credential "https://user@example.com/"] # maybe
> 		# likewise
> 		...
>
> 	[credential "https://example.com/repo.git"]
> 		# settings for the host/scheme/path combination
> 		...
>
> 	[credential "https://user@example.com/repo.git"] # maybe
> 		# settings for the host/scheme/user/path combination
> 		...
>
>   without accidentally promising support for anything else
>
> What do you think?

I added tests for all of those. They all work as a naive user (like me) would expect them to.

I threw in another test, too:

	[credential "/repo.git"]
		...

And I also threw in negative tests, to verify that non-matching protocol or host name or path mean that the config setting is ignored.

Thank you for your thorough review, it really helps me, Dscho