On 2020-02-16 at 06:13:23, Jeff King wrote: > So I think we're OK here, unless you can come up with any more > obscure case. Yeah, I made this implicit in my patch, but I couldn't think of a situation where we'd hit this case. I'll update the commit message to reflect that we don't intend for this behavior to change. > Some options are: > > - teach urlmatch to pass matching config keys to our callback even > if they're "worse" than a previously-seen key, so that we can > then record all helpers in the order they appear in the config > (retaining the existing behavior) > > - use urlmatch's cmp_matches() to order the list of helper. This > would be a change in behavior, but I wonder if it might be what > people prefer. I suspect it would make some happy (if the > host-specific helper can answer the query above, you'd just as > soon not run the cache helper at all) and others not (if the > host-specific one is expensive or requires user interaction, you > may want to try the cache first). So I'm not sure if it would be > a good idea or not. I think it should be reasonably simple to adjust the logic to do the former. I'd like to avoid making non-backwards compatible changes in this series. I'll add some tests for this case as well, since I think it's going to be important to get right. Thanks for the sanity check. > A few comments on the patch itself: > > > --- a/Documentation/gitcredentials.txt > > +++ b/Documentation/gitcredentials.txt > > @@ -131,7 +131,9 @@ context would not match: > > because the hostnames differ. Nor would it match `foo.example.com`; Git > > compares hostnames exactly, without considering whether two hosts are part of > > the same domain. Likewise, a config entry for `http://example.com` would not > > -match: Git compares the protocols exactly. > > +match: Git compares the protocols exactly. However, you may use wildcards in > > +the domain name and other pattern matching techniques as with the `http.<url>.*` > > +options. > > You'd probably want to review the documentation to accommodate any of > the behavior changes discussed above that we end up with. As mentioned, my hope is to not need to do this. > > + config.section = "credential"; > > + config.key = NULL; > > + config.collect_fn = credential_config_callback; > > + config.cascade_fn = git_default_config; > > + config.cb = c; > > I don't think the old code would ever call git_default_config (we _just_ > want to load values for this specific URL). So I think you'd want to > leave the cascade_fn NULL here? Okay, can do. > > + credential_describe(c, &url); > > + normalized_url = url_normalize(url.buf, &config.url); > > The purpose of credential_describe() so far has been to show the URL to > the user. It won't do any %-encoding that would be required for more > exotic URLs. But I assume we'd want that in whatever we feed to > url_normalize. So for example: > > echo url=https://example.com/%2566 | > GIT_TERMINAL_PROMPT=0 git \ > -c credential.https://example.com/%2566.helper='!echo >&2 run helper' > credential fill > > matches in the current code, but not after your patch (we decode %25 > into just "%", and then feed "%66" to the url normalizer, which decodes > it to "f". Good point. I'll fix this and add a test. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
