Hi, On Wed, 22 Apr 2020, Jeff King wrote: > 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. This affects also pre-v2.26.* versions. One fallout is that some GitHub Desktop users reported that they cannot fetch anymore: https://github.com/desktop/desktop/issues/9597 I _think_ I have a good fix for this, and am only waiting for the PR builds at https://github.com/gitgitgadget/git/pull/615 to finish before I will submit that patch series for review. Ideally, I will integrate these patches into some MinGit backports tonight, still, so that GitHub Desktop can roll out another release to avoid many more reports. Therefore, I hope for quick reviews ;-) Ciao, Dscho > > > 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 > >
