{"thread":{"id":"52808","subject":"[PATCH 1/3] mailmap: add an additional email address for brian m. carlson","startedAt":"2020-02-14T23:08:09Z","lastAt":"2020-02-16T20:53:40Z","messageCount":8,"participants":["brian m. carlson","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"391804","messageId":"20200214225929.541306-2-sandals@crustytoothpaste.net","threadId":"52808","inReplyTo":"20200214225929.541306-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/3] mailmap: add an additional email address for brian m. carlson","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-14T22:59:27Z","receivedAt":"2020-02-14T23:08:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: brian m. carlson \n\nTo more accurately track the provenance of contributions, brian uses a\nwork email address for commits created at work. Add this email address\nto .mailmap so that contributions are properly attributed to the same\nperson.\n\nSigned-off-by: brian m. carlson \n---\n .mailmap | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/.mailmap b/.mailmap\nindex 73fd92e192..bde7aba756 100644\n--- a/.mailmap\n+++ b/.mailmap\n@@ -31,6 +31,7 @@ Brandon Casey \n Brandon Williams \n brian m. carlson \n brian m. carlson \n+brian m. carlson \n Bryan Larsen \n Bryan Larsen \n Cheng Renquan \n"},{"id":"391805","messageId":"20200214225929.541306-1-sandals@crustytoothpaste.net","threadId":"52808","inReplyTo":null,"subject":"[PATCH 0/3] Wildcard matching for credentials","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-14T22:59:26Z","receivedAt":"2020-02-14T23:08:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series introduces wildcard matching (that is, urlmatch support) for\ncredential config options, just like for the http options. This is\nhelpful in corporate environments where custom credentials should be\nused across a wide variety of subdomains.\n\nIn addition, there's an additional test for urlmatch behavior with\nmultiple subdomains and a mailmap update for the email address used in\nthis series.\n\nbrian m. carlson (3):\n mailmap: add an additional email address for brian m. carlson\n t1300: add test for urlmatch with multiple wildcards\n credential: allow wildcard patterns when matching config\n\n .mailmap | 1 +\n Documentation/gitcredentials.txt | 4 +++-\n credential.c | 41 +++++++++++++++++---------------\n t/t0300-credentials.sh | 20 ++++++++++++++++\n t/t1300-config.sh | 6 +++++\n 5 files changed, 52 insertions(+), 20 deletions(-)\n\n"},{"id":"391806","messageId":"20200214225929.541306-3-sandals@crustytoothpaste.net","threadId":"52808","inReplyTo":"20200214225929.541306-1-sandals@crustytoothpaste.net","subject":"[PATCH 2/3] t1300: add test for urlmatch with multiple wildcards","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-14T22:59:28Z","receivedAt":"2020-02-14T23:08:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: brian m. carlson \n\nOur urlmatch code handles multiple wildcards, but we don't currently\nhave a test that checks this code path. Add a test that we handle this\ncase correctly to avoid any regressions.\n\nSigned-off-by: brian m. carlson \n---\n t/t1300-config.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 983a0a1583..8b0cac80a3 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1408,6 +1408,8 @@ test_expect_success 'urlmatch favors more specific URLs' '\n \t\tcookieFile = /tmp/wildcard.txt\n \t[http \"https://*.example.com/wildcardwithsubdomain\"]\n \t\tcookieFile = /tmp/wildcardwithsubdomain.txt\n+\t[http \"https://*.example.*\"]\n+\t\tcookieFile = /tmp/multiwildcard.txt\n \t[http \"https://trailing.example.com\"]\n \t\tcookieFile = /tmp/trailing.txt\n \t[http \"https://user@*.example.com/\"]\n@@ -1454,6 +1456,10 @@ test_expect_success 'urlmatch favors more specific URLs' '\n \n \techo http.cookiefile /tmp/sub.txt >expect &&\n \tgit config --get-urlmatch HTTP https://user@sub.example.com >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo http.cookiefile /tmp/multiwildcard.txt >expect &&\n+\tgit config --get-urlmatch HTTP https://wildcard.example.org >actual &&\n \ttest_cmp expect actual\n '\n \n"},{"id":"391807","messageId":"20200214225929.541306-4-sandals@crustytoothpaste.net","threadId":"52808","inReplyTo":"20200214225929.541306-1-sandals@crustytoothpaste.net","subject":"[PATCH 3/3] credential: allow wildcard patterns when matching config","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-14T22:59:29Z","receivedAt":"2020-02-14T23:08:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: brian m. carlson \n\nIn some cases, a user will want to use a specific credential helper for\na wildcard pattern, such as https://*.corp.example.com. Use the urlmatch\ncode to match a pattern for a URL to allow this behavior.\n\nSince we are handling URLs using urlmatch, remove the custom code to\nmatch URLs, since it is no longer needed.\n\nSigned-off-by: brian m. carlson \n---\n Documentation/gitcredentials.txt | 4 +++-\n credential.c | 41 +++++++++++++++++---------------\n t/t0300-credentials.sh | 20 ++++++++++++++++\n 3 files changed, 45 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex ea759fdee5..12cb032352 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -131,7 +131,9 @@ context would not match:\n because the hostnames differ. Nor would it match `foo.example.com`; Git\n compares hostnames exactly, without considering whether two hosts are part of\n the same domain. Likewise, a config entry for `http://example.com` would not\n-match: Git compares the protocols exactly.\n+match: Git compares the protocols exactly. However, you may use wildcards in\n+the domain name and other pattern matching techniques as with the `http..*`\n+options.\n \n If the \"pattern\" URL does include a path component, then this too must match\n exactly: the context `https://example.com/bar/baz.git` will match a config\ndiff --git a/credential.c b/credential.c\nindex 62be651b03..08904e247f 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -6,6 +6,7 @@\n #include \"url.h\"\n #include \"prompt.h\"\n #include \"sigchain.h\"\n+#include \"urlmatch.h\"\n \n void credential_init(struct credential *c)\n {\n@@ -40,7 +41,7 @@ static int credential_config_callback(const char *var, const char *value,\n \t\t\t\t void *data)\n {\n \tstruct credential *c = data;\n-\tconst char *key, *dot;\n+\tconst char *key;\n \n \tif (!skip_prefix(var, \"credential.\", &key))\n \t\treturn 0;\n@@ -48,23 +49,6 @@ static int credential_config_callback(const char *var, const char *value,\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n \n-\tdot = strrchr(key, '.');\n-\tif (dot) {\n-\t\tstruct credential want = CREDENTIAL_INIT;\n-\t\tchar *url = xmemdupz(key, dot - key);\n-\t\tint matched;\n-\n-\t\tcredential_from_url(&want, url);\n-\t\tmatched = credential_match(&want, c);\n-\n-\t\tcredential_clear(&want);\n-\t\tfree(url);\n-\n-\t\tif (!matched)\n-\t\t\treturn 0;\n-\t\tkey = dot + 1;\n-\t}\n-\n \tif (!strcmp(key, \"helper\")) {\n \t\tif (*value)\n \t\t\tstring_list_append(&c->helpers, value);\n@@ -87,11 +71,30 @@ static int proto_is_http(const char *s)\n \treturn !strcmp(s, \"https\") || !strcmp(s, \"http\");\n }\n \n+static void credential_describe(struct credential *c, struct strbuf *out);\n+\n static void credential_apply_config(struct credential *c)\n {\n+\tchar *normalized_url;\n+\tstruct urlmatch_config config = { STRING_LIST_INIT_DUP };\n+\tstruct strbuf url = STRBUF_INIT;\n+\n \tif (c->configured)\n \t\treturn;\n-\tgit_config(credential_config_callback, c);\n+\n+\tconfig.section = \"credential\";\n+\tconfig.key = NULL;\n+\tconfig.collect_fn = credential_config_callback;\n+\tconfig.cascade_fn = git_default_config;\n+\tconfig.cb = c;\n+\n+\tcredential_describe(c, &url);\n+\tnormalized_url = url_normalize(url.buf, &config.url);\n+\n+\tgit_config(urlmatch_config_entry, &config);\n+\tfree(normalized_url);\n+\tstrbuf_release(&url);\n+\n \tc->configured = 1;\n \n \tif (!c->use_http_path && proto_is_http(c->protocol)) {\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 82eaaea0f4..516b3268f9 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -289,6 +289,26 @@ test_expect_success 'http paths can be part of context' '\n \tEOF\n '\n \n+test_expect_success 'context uses urlmatch' '\n+\ttest_config \"credential.https://*.org.useHttpPath\" true &&\n+\tcheck fill \"verbatim foo bar\" <<-\\EOF\n+\tprotocol=https\n+\thost=example.org\n+\tpath=foo.git\n+\t--\n+\tprotocol=https\n+\thost=example.org\n+\tpath=foo.git\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.org\n+\tverbatim: path=foo.git\n+\tEOF\n+'\n+\n test_expect_success 'helpers can abort the process' '\n \ttest_must_fail git \\\n \t\t-c credential.helper=\"!f() { echo quit=1; }; f\" \\\n"},{"id":"391809","messageId":"20200214235822.GA11400@syl.local","threadId":"52808","inReplyTo":"20200214225929.541306-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/3] Wildcard matching for credentials","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-14T23:58:22Z","receivedAt":"2020-02-14T23:58:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi brian,\n\nOn Fri, Feb 14, 2020 at 10:59:26PM +0000, brian m. carlson wrote:\n> This series introduces wildcard matching (that is, urlmatch support) for\n> credential config options, just like for the http options. This is\n> helpful in corporate environments where custom credentials should be\n> used across a wide variety of subdomains.\n>\n> In addition, there's an additional test for urlmatch behavior with\n> multiple subdomains and a mailmap update for the email address used in\n> this series.\n\nI can imagine that this is perhaps for Git LFS, which I could see\nbenefiting from this change. My review has nothing to do with my\naffiliation (or lack thereof) to LFS.\n\nI gave your patches a review, and they all look quite good to me. Thanks\nespecially for 2/3, which I would have suggested were it not already\nthere ;-).\n\nThis looks good to me, so please have my:\n\n Reviewed-by: Taylor Blau \n\n> brian m. carlson (3):\n> mailmap: add an additional email address for brian m. carlson\n> t1300: add test for urlmatch with multiple wildcards\n> credential: allow wildcard patterns when matching config\n>\n> .mailmap | 1 +\n> Documentation/gitcredentials.txt | 4 +++-\n> credential.c | 41 +++++++++++++++++---------------\n> t/t0300-credentials.sh | 20 ++++++++++++++++\n> t/t1300-config.sh | 6 +++++\n> 5 files changed, 52 insertions(+), 20 deletions(-)\n\nThanks,\nTaylor\n"},{"id":"391813","messageId":"20200215001357.GB6134@camp.crustytoothpaste.net","threadId":"52808","inReplyTo":"20200214235822.GA11400@syl.local","subject":"Re: [PATCH 0/3] Wildcard matching for credentials","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-15T00:13:57Z","receivedAt":"2020-02-15T00:14:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-14 at 23:58:22, Taylor Blau wrote:\n> Hi brian,\n> \n> On Fri, Feb 14, 2020 at 10:59:26PM +0000, brian m. carlson wrote:\n> > This series introduces wildcard matching (that is, urlmatch support) for\n> > credential config options, just like for the http options. This is\n> > helpful in corporate environments where custom credentials should be\n> > used across a wide variety of subdomains.\n> >\n> > In addition, there's an additional test for urlmatch behavior with\n> > multiple subdomains and a mailmap update for the email address used in\n> > this series.\n> \n> I can imagine that this is perhaps for Git LFS, which I could see\n> benefiting from this change. My review has nothing to do with my\n> affiliation (or lack thereof) to LFS.\n\nIt was originally prompted by a discussion that someone started on the\nGit LFS issue tracker, yes. I pointed out that it's actually Git that\ncontrols credential helper matching and so I sent a patch.\n\nI've also worked somewhere with multiple internal Git servers, so I can\nimagine this would also be valuable in such an environment.\n\n> I gave your patches a review, and they all look quite good to me. Thanks\n> especially for 2/3, which I would have suggested were it not already\n> there ;-).\n\nI think there was some discussion on the list about whether allowing\nmultiple wildcards and wildcards in any part of the domain name was\nintentional or not, and it was decided that it was. I promised to\nfollow up with a patch to the testsuite, and here I am, some time later.\n\n> This looks good to me, so please have my:\n> \n> Reviewed-by: Taylor Blau \n\nThanks.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"391861","messageId":"20200216061323.GA2397568@coredump.intra.peff.net","threadId":"52808","inReplyTo":"20200214225929.541306-4-sandals@crustytoothpaste.net","subject":"Re: [PATCH 3/3] credential: allow wildcard patterns when matching config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-16T06:13:23Z","receivedAt":"2020-02-16T06:13:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 14, 2020 at 10:59:29PM +0000, brian m. carlson wrote:\n\n> From: brian m. carlson \n> \n> In some cases, a user will want to use a specific credential helper for\n> a wildcard pattern, such as https://*.corp.example.com. Use the urlmatch\n> code to match a pattern for a URL to allow this behavior.\n> \n> Since we are handling URLs using urlmatch, remove the custom code to\n> match URLs, since it is no longer needed.\n\nI think it's a good idea to unify the matching for these two subsystems\nif we can. Playing devil's advocate for a moment:\n\n - are there cases that would have matched before that won't now?\n Basic stuff like:\n\n echo url=https://example.com/foo |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com/foo.helper='!echo >&2 full helper' \\\n -c credential.https://example.com/fo.helper='!echo >&2 partial helper' \\\n credential fill\n\n seems to behave the same (we respect only the full match both before\n and after your patch). I wondered if we might run into problems\n matching usernames. But this:\n\n echo url=https://user@example.com/ |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com.helper='!echo >&2 bare helper' \\\n -c credential.https://user@example.com.helper='!echo >&2 user helper' \\\n credential fill\n\n seems to be behave the same both before and after (it runs both\n helpers). And this:\n\n echo url=https://example.com/ |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com.helper='!echo >&2 bare helper' \\\n -c credential.https://user@example.com.helper='!echo >&2 user helper' \\\n credential fill\n\n likewise behaves the same before and after, running only the bare\n helper (I think we _could_ match the user-helper by doing a second\n pass over the config after getting the username from config, from\n another helper, or from the user, but I doubt anybody is clamoring\n for that feature).\n\n So I think we're OK here, unless you can come up with any more\n obscure case.\n\n - are there cases that match now that didn't before? Those are a bit\n dangerous become they may mean unexpectedly sharing credentials with\n hosts the user didn't mean to.\n\n Obviously anything with a \"*\" in it will behave differently, but I\n think that's OK (i.e,. I doubt anybody wrote \"*\" in a hostname and\n didn't mean it for it to glob).\n\n One interesting one is that the credential code currently requires a\n full path match instead of a prefix match. So:\n\n echo url=https://example.com/foo/bar |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com/foo.helper='!echo >&2 run helper' \\\n credential fill\n\n doesn't currently trigger the helper, but does with your patch. I\n think this is _probably_ OK. I think the existing behavior was done\n to just always err on the side of strictness in matching, with the\n thought that nobody cared that much about path matching anyway (by\n default we ignore the paths completely).\n\n - The rules for ordering are a bit different, in that the credential\n code takes any matches in the order in which they appear in the\n config file. Whereas urlmatch won't pass on any less-specific\n matches. So doing:\n\n echo url=https://example.com/foo |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com/foo.helper='!echo >&2 path helper' \\\n -c credential.https://example.com.helper='!echo >&2 bare-domain helper' \\\n credential fill\n\n right now will trigger both helpers (path then bare-domain), but\n after your patch will just trigger the path helper. Curiously, if you\n flip the order of the two config keys, it will still run both. That's\n because urlmatch assumes that the receiver is handling the \"last one\n wins\" semantics, but the credential code is actually building a list\n of helpers.\n\n Weirder still, the current strategy for single-valued items like\n credential.*.username is \"first one wins\". So:\n\n echo url=https://example.com/foo |\n git \\\n -c credential.https://example.com.username=domain-user \\\n -c credential.https://example.com/foo.username=path-user \\\n credential fill\n\n will prompt for the password of domain-user, but flipping it will ask\n for path-user. Which seems...weird. That actually doesn't change with\n your patch, because we'll either send both to the credential callback\n (in the order above) or we'd just send the first one (if the order is\n flipped).\n\n I'd be in favor of changing that to the usual \"last one wins\",\n because it makes no sense and is unlike the rest of Git's config.\n However, I think the reason it's written that way is so that:\n\n echo url=https://foo@example.com |\n git \\\n -c credential.https://example.com.username=bar \\\n credential fill\n\n still takes the username=foo from the command-line. We'd want to take\n care not to break that case, which implies remembering whether\n c->username came from a previous config value, or was part of the\n initial credential lookup.\n\n That doesn't _technically_ have to be part of your patch, because\n your patch doesn't make it any worse than it is now. But it would be\n nice to cleanup (since urlmatch gives us nice \"most specific wins\"\n semantics).\n\n But I do think we need to deal with the helper-list behavior as part\n of your patch, because it _does_ regress. And while the example above\n is probably fairly contrived, I think urlmatch is responsible for\n matching credential.helper, too (as the least specific possible\n match), and it's not outrageous to have something like:\n\n [credential]\n helper = cache\n [credential \"https://example.com\"]\n helper = !some-host-specific-helper\n\n Some options are:\n\n - teach urlmatch to pass matching config keys to our callback even\n if they're \"worse\" than a previously-seen key, so that we can\n then record all helpers in the order they appear in the config\n (retaining the existing behavior)\n\n - use urlmatch's cmp_matches() to order the list of helper. This\n would be a change in behavior, but I wonder if it might be what\n people prefer. I suspect it would make some happy (if the\n host-specific helper can answer the query above, you'd just as\n soon not run the cache helper at all) and others not (if the\n host-specific one is expensive or requires user interaction, you\n may want to try the cache first). So I'm not sure if it would be\n a good idea or not.\n\nA few comments on the patch itself:\n\n> --- a/Documentation/gitcredentials.txt\n> +++ b/Documentation/gitcredentials.txt\n> @@ -131,7 +131,9 @@ context would not match:\n> because the hostnames differ. Nor would it match `foo.example.com`; Git\n> compares hostnames exactly, without considering whether two hosts are part of\n> the same domain. Likewise, a config entry for `http://example.com` would not\n> -match: Git compares the protocols exactly.\n> +match: Git compares the protocols exactly. However, you may use wildcards in\n> +the domain name and other pattern matching techniques as with the `http..*`\n> +options.\n\nYou'd probably want to review the documentation to accommodate any of\nthe behavior changes discussed above that we end up with.\n\n> static void credential_apply_config(struct credential *c)\n> {\n> +\tchar *normalized_url;\n> +\tstruct urlmatch_config config = { STRING_LIST_INIT_DUP };\n\nHere we initialize a string_list, but I don't think we ever free it. It\nlooks like the urlmatch_config_entry() callback will stash items here\nfor its \"more specific match\" checks.\n\nI think we need:\n\n string_list_clear(&config.vars, 1);\n\nwhich builtin/config.c:get_urlmatch() does. It also seems to free\nconfig.url.url, which is touched by the normalization process. I _think_\nthis is always the same as the return value normalized_url, so we'd be\nOK there.\n\n> +\tconfig.section = \"credential\";\n> +\tconfig.key = NULL;\n> +\tconfig.collect_fn = credential_config_callback;\n> +\tconfig.cascade_fn = git_default_config;\n> +\tconfig.cb = c;\n\nI don't think the old code would ever call git_default_config (we _just_\nwant to load values for this specific URL). So I think you'd want to\nleave the cascade_fn NULL here?\n\n> +\tcredential_describe(c, &url);\n> +\tnormalized_url = url_normalize(url.buf, &config.url);\n\nThe purpose of credential_describe() so far has been to show the URL to\nthe user. It won't do any %-encoding that would be required for more\nexotic URLs. But I assume we'd want that in whatever we feed to\nurl_normalize. So for example:\n\n echo url=https://example.com/%2566 |\n GIT_TERMINAL_PROMPT=0 git \\\n -c credential.https://example.com/%2566.helper='!echo >&2 run helper'\n credential fill\n\nmatches in the current code, but not after your patch (we decode %25\ninto just \"%\", and then feed \"%66\" to the url normalizer, which decodes\nit to \"f\".\n\n-Peff\n"},{"id":"391875","messageId":"20200216205329.GD6134@camp.crustytoothpaste.net","threadId":"52808","inReplyTo":"20200216061323.GA2397568@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] credential: allow wildcard patterns when matching config","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-16T20:53:29Z","receivedAt":"2020-02-16T20:53:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-16 at 06:13:23, Jeff King wrote:\n> So I think we're OK here, unless you can come up with any more\n> obscure case.\n\nYeah, I made this implicit in my patch, but I couldn't think of a\nsituation where we'd hit this case. I'll update the commit message to\nreflect that we don't intend for this behavior to change.\n\n> Some options are:\n> \n> - teach urlmatch to pass matching config keys to our callback even\n> if they're \"worse\" than a previously-seen key, so that we can\n> then record all helpers in the order they appear in the config\n> (retaining the existing behavior)\n> \n> - use urlmatch's cmp_matches() to order the list of helper. This\n> would be a change in behavior, but I wonder if it might be what\n> people prefer. I suspect it would make some happy (if the\n> host-specific helper can answer the query above, you'd just as\n> soon not run the cache helper at all) and others not (if the\n> host-specific one is expensive or requires user interaction, you\n> may want to try the cache first). So I'm not sure if it would be\n> a good idea or not.\n\nI think it should be reasonably simple to adjust the logic to do the\nformer. I'd like to avoid making non-backwards compatible changes in\nthis series. I'll add some tests for this case as well, since I think\nit's going to be important to get right. Thanks for the sanity check.\n\n> A few comments on the patch itself:\n> \n> > --- a/Documentation/gitcredentials.txt\n> > +++ b/Documentation/gitcredentials.txt\n> > @@ -131,7 +131,9 @@ context would not match:\n> > because the hostnames differ. Nor would it match `foo.example.com`; Git\n> > compares hostnames exactly, without considering whether two hosts are part of\n> > the same domain. Likewise, a config entry for `http://example.com` would not\n> > -match: Git compares the protocols exactly.\n> > +match: Git compares the protocols exactly. However, you may use wildcards in\n> > +the domain name and other pattern matching techniques as with the `http..*`\n> > +options.\n> \n> You'd probably want to review the documentation to accommodate any of\n> the behavior changes discussed above that we end up with.\n\nAs mentioned, my hope is to not need to do this.\n\n> > +\tconfig.section = \"credential\";\n> > +\tconfig.key = NULL;\n> > +\tconfig.collect_fn = credential_config_callback;\n> > +\tconfig.cascade_fn = git_default_config;\n> > +\tconfig.cb = c;\n> \n> I don't think the old code would ever call git_default_config (we _just_\n> want to load values for this specific URL). So I think you'd want to\n> leave the cascade_fn NULL here?\n\nOkay, can do.\n\n> > +\tcredential_describe(c, &url);\n> > +\tnormalized_url = url_normalize(url.buf, &config.url);\n> \n> The purpose of credential_describe() so far has been to show the URL to\n> the user. It won't do any %-encoding that would be required for more\n> exotic URLs. But I assume we'd want that in whatever we feed to\n> url_normalize. So for example:\n> \n> echo url=https://example.com/%2566 |\n> GIT_TERMINAL_PROMPT=0 git \\\n> -c credential.https://example.com/%2566.helper='!echo >&2 run helper'\n> credential fill\n> \n> matches in the current code, but not after your patch (we decode %25\n> into just \"%\", and then feed \"%66\" to the url normalizer, which decodes\n> it to \"f\".\n\nGood point. I'll fix this and add a test.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}