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
