On 2020-04-22 at 01:28:17, Jonathan Nieder wrote:
> brian m. carlson wrote:
> > On 2020-04-21 at 22:58:37, Jeff King wrote:
> 
> >> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple
> >> reproduction:
> >>
> >>   url=https://git.example.com/my-proj/my-repo.git
> >>   echo url=$url |
> >>   GIT_TERMINAL_PROMPT=0 \
> >>   ./git \
> >>     -c credential.helper= \
> >>     -c credential.$url.helper='!echo username=foo; echo password=bar;:' \
> >>     credential fill
> >> 
> >> which should print a filled credential (with "foo/bar"), but will fail
> >> with recent versions. It bisects to brian's 46fd7b3900 (credential:
> >> allow wildcard patterns when matching config, 2020-02-20).
> >
> > Yeah, I can reproduce this.  It looks like what's happening is that
> > we're percent-encoding the slash in the paths as %2f, which of course
> > isn't going to match in the urlmatch code.  We probably need to tell the
> > percent encoding function not to encode slashes in this case.
> >
> > I'm testing a patch now and hope to have it on the list a little later
> > this evening.  Thanks for reporting and bisecting, and sorry for the
> > breakage.
> 
> 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 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 will likely not be online tomorrow (Wednesday), so if folks decide
they want a revert, I can send that out Thursday afternoon (GMT-05:00).
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204