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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

> 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`.

> [...]
> > --- 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.

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

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

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