Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version

[Jeff King <peff@peff.net>]

On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote:

> > Thanks.  Here's another (though I haven't tried bisecting yet):
> > 
> > 	echo url='https://github.com/git/git' |
> > 	GIT_TERMINAL_PROMPT=0 \
> > 	git -c credential.helper= \
> > 		-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \
> > 		credential fill
> 
> gitcredentials(7) says the following:
> 
>   Git considers each credential to have a context defined by a URL.
>   This context is used to look up context-specific configuration, and is
>   passed to any helpers, which may use it as an index into secure
>   storage.
> 
> I'm not sure a hostname qualifies as a URL in this case.  So while my
> patch did break this, I don't believe it's ever been documented to
> actually work and was an artifact of our implementation (along with
> "credential./git/git.helper" and "credential.https://.helper").  I've
> also never seen this syntax used in the wild, but maybe I'm not looking
> in the right places.

I'm pretty sure it was an intended use case, though it is a natural
outcome of the credential_match() strategy of "unspecified things match
anything". I'd suspect that anybody relying on it is doing so
unintentionally, and just forgot to put the protocol field in. Though I
suppose doing so would let you cover http/https in a single block.

At any rate, even in versions _without_ your patch, that became a hard
error in this week's release. In v2.24.3, for example:

  $ echo url=https://anyhost.example.com |
    git -c credential.example.com.username=foo credential fill
  warning: url has no scheme: example.com
  fatal: credential url cannot be parsed: example.com

because we're relying there on credential_from_url() to parse the config
credentials, too. After your patch, we use the http-config machinery,
which simply doesn't match.

> I don't think we can shoehorn it into urlmatch, since that would break
> compatibility with the `http.*` config options, so I think we'd have to
> revert the entire feature if we want to preserve it.  I think I'd prefer
> to leave things as it is since it seems uncommon and there are easy
> alternatives, but if folks prefer, I can send a patch to revert the
> urlmatch feature.

I agree that we should leave it. Aside from the dual http/https thing
(which _hopefully_ is rare these days as https become more of a
standard), I don't think it has a legitimate use case. And I think we
should be pushing users to be a bit more careful with their url config.

-Peff