{"thread":{"id":"41533","subject":"[PATCH] credential: let empty credential specs reset helper list","startedAt":"2016-02-26T10:51:35Z","lastAt":"2016-02-26T23:26:06Z","messageCount":6,"participants":["Jeff King","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279511","messageId":"20160226105135.GA30215@sigill.intra.peff.net","threadId":"41533","inReplyTo":null,"subject":"[PATCH] credential: let empty credential specs reset helper list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T10:51:35Z","receivedAt":"2016-02-26T10:51:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Sine the credential.helper key is a multi-valued config\nlist, there's no way to \"unset\" a helper once it's been set.\nSo if your system /etc/gitconfig sets one, you can never\navoid running it, but only add your own helpers on top.\n\nSince an empty value for credential.helper is nonsensical\n(it would just try to run \"git-credential-\"), we can assume\nnobody is using it. Let's define it to reset the helper\nlist, letting you override lower-priority instances which\nhave come before.\n\nSigned-off-by: Jeff King \n---\nThis doesn't solve the multi-value config problem in the general sense,\nbut it's simple and easy. And even if we do solve it later, it's not a\nbig deal to keep supporting this scheme for compatibility.\n\n Documentation/config.txt | 5 +++--\n Documentation/gitcredentials.txt | 5 +++++\n credential.c | 9 ++++++---\n t/t0300-credentials.sh | 11 +++++++++++\n 4 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2cd6bdd..4b85095 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1113,8 +1113,9 @@ commit.template::\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\n-\tstorage to avoid prompting the user for the credentials. See\n-\tlinkgit:gitcredentials[7] for details.\n+\tstorage to avoid prompting the user for the credentials. Note\n+\tthat multiple helpers may be defined. See linkgit:gitcredentials[7]\n+\tfor details.\n \n credential.useHttpPath::\n \tWhen acquiring credentials, consider the \"path\" component of an http\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 1c75be0..f3a75d1 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -106,6 +106,11 @@ variable, each helper will be tried in turn, and may provide a username,\n password, or nothing. Once Git has acquired both a username and a\n password, no more helpers will be tried.\n \n+If `credential.helper` is configured to the empty string, this resets\n+the helper list to empty (so you may override a helper set by a\n+lower-priority config file by configuring the empty-string helper,\n+followed by whatever set of helpers you would like).\n+\n \n CREDENTIAL CONTEXTS\n -------------------\ndiff --git a/credential.c b/credential.c\nindex 7d6501d..aa99666 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,\n \t\tkey = dot + 1;\n \t}\n \n-\tif (!strcmp(key, \"helper\"))\n-\t\tstring_list_append(&c->helpers, value);\n-\telse if (!strcmp(key, \"username\")) {\n+\tif (!strcmp(key, \"helper\")) {\n+\t\tif (*value)\n+\t\t\tstring_list_append(&c->helpers, value);\n+\t\telse\n+\t\t\tstring_list_clear(&c->helpers, 0);\n+\t} else if (!strcmp(key, \"username\")) {\n \t\tif (!c->username)\n \t\t\tc->username = xstrdup(value);\n \t}\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex d7ef44b..03bd31e 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -298,4 +298,15 @@ test_expect_success 'helpers can abort the process' '\n \ttest_cmp expect stdout\n '\n \n+test_expect_success 'empty helper spec resets helper list' '\n+\ttest_config credential.helper \"verbatim file file\" &&\n+\tcheck fill \"\" \"verbatim cmdline cmdline\" <<-\\EOF\n+\t--\n+\tusername=cmdline\n+\tpassword=cmdline\n+\t--\n+\tverbatim: get\n+\tEOF\n+'\n+\n test_done\n-- \n2.7.2.767.g705917e\n"},{"id":"279541","messageId":"xmqqa8mnl71v.fsf@gitster.mtv.corp.google.com","threadId":"41533","inReplyTo":"20160226105135.GA30215@sigill.intra.peff.net","subject":"Re: [PATCH] credential: let empty credential specs reset helper list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T17:23:24Z","receivedAt":"2016-02-26T17:23:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Sine the credential.helper key is a multi-valued config\n\ns/Sine/Since/;\n\n> diff --git a/credential.c b/credential.c\n> index 7d6501d..aa99666 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,\n> \t\tkey = dot + 1;\n> \t}\n> \n> -\tif (!strcmp(key, \"helper\"))\n> -\t\tstring_list_append(&c->helpers, value);\n> -\telse if (!strcmp(key, \"username\")) {\n> +\tif (!strcmp(key, \"helper\")) {\n> +\t\tif (*value)\n> +\t\t\tstring_list_append(&c->helpers, value);\n> +\t\telse\n> +\t\t\tstring_list_clear(&c->helpers, 0);\n> +\t} else if (!strcmp(key, \"username\")) {\n\nI wondered why neither the existing code nor the updated one has a\ncheck for !value, but this callback assumes no credential\nconfiguration variable will ever be a boolean and rejects it\nupfront, so this code before or after the change is safe.\n\nNot pointing out anything that needs to be changed; demonstrating\nthat I did read this sufficiently well to say that I have reviewed\nit ;-)\n"},{"id":"279557","messageId":"xmqqk2lrjmff.fsf@gitster.mtv.corp.google.com","threadId":"41533","inReplyTo":"xmqqa8mnl71v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] credential: let empty credential specs reset helper list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T19:34:12Z","receivedAt":"2016-02-26T19:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Sine the credential.helper key is a multi-valued config\n>\n> s/Sine/Since/;\n>\n>> diff --git a/credential.c b/credential.c\n>> index 7d6501d..aa99666 100644\n>> --- a/credential.c\n>> +++ b/credential.c\n>> @@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,\n>> \t\tkey = dot + 1;\n>> \t}\n>> \n>> -\tif (!strcmp(key, \"helper\"))\n>> -\t\tstring_list_append(&c->helpers, value);\n>> -\telse if (!strcmp(key, \"username\")) {\n>> +\tif (!strcmp(key, \"helper\")) {\n>> +\t\tif (*value)\n>> +\t\t\tstring_list_append(&c->helpers, value);\n>> +\t\telse\n>> +\t\t\tstring_list_clear(&c->helpers, 0);\n>> +\t} else if (!strcmp(key, \"username\")) {\n>\n> I wondered why neither the existing code nor the updated one has a\n> check for !value, but this callback assumes no credential\n> configuration variable will ever be a boolean and rejects it\n> upfront, so this code before or after the change is safe.\n>\n> Not pointing out anything that needs to be changed; demonstrating\n> that I did read this sufficiently well to say that I have reviewed\n> it ;-)\n\nThis reminds me of one thing. The only reason why we are hesitant\nto introduce a new syntax like\n\n\t[credential]\n \t!helper ;# clear\n helper = ...\n\nto allow explicit clearing of accumulated values so far IIRC is\nbecause such a _file_ will not be readable by existing versions of\nGit. Am I correct?\n\nIf that is the case, then that reasoning will still not prevent us\nfrom adding corresponding support for a command-line overide, i.e.\neither one or both of these:\n\n\t$ git -c credential.!helper cmd\n\t$ git -c !credential.helper cmd\n\nno?\n\nOf course, the code in the configuration subsystem for updated\nversion of Git needs to become aware of the new syntax, and those\nthat deal with the multi-value variables need custom code, which is\nsimilar to the way you special cased an empty value in the above\npatch, so I am not sure how much this would help.\n"},{"id":"279572","messageId":"CA+P7+xoSPmvZp-3rgDc+JjuDKEdNvayPMM4azb2UgDaSk_gJ7g@mail.gmail.com","threadId":"41533","inReplyTo":"xmqqk2lrjmff.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] credential: let empty credential specs reset helper list","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-02-26T22:13:55Z","receivedAt":"2016-02-26T22:13:55Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Feb 26, 2016 at 11:34 AM, Junio C Hamano wrote:\n>\n> Of course, the code in the configuration subsystem for updated\n> version of Git needs to become aware of the new syntax, and those\n> that deal with the multi-value variables need custom code, which is\n> similar to the way you special cased an empty value in the above\n> patch, so I am not sure how much this would help.\n>\n\nFor most cases, I think the previous approach of setting to empty\nstring makes sense and is more easilly intuitive, but would only work\nfor those values which the empty string has no valid meaning.\n\nI would think either approach here is fine, and i think *most* cases\nof multivariable configuration don't need to be cleared from within a\nconfiguration file but only on the command line would be acceptable.\n\nIf we go this route I would prefer \"-c !\" vs \"-c .!\"\n\nThanks,\nJake\n"},{"id":"279576","messageId":"20160226223719.GA2429@sigill.intra.peff.net","threadId":"41533","inReplyTo":"xmqqk2lrjmff.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] credential: let empty credential specs reset helper list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T22:37:20Z","receivedAt":"2016-02-26T22:37:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2016 at 11:34:12AM -0800, Junio C Hamano wrote:\n\n> >> -\tif (!strcmp(key, \"helper\"))\n> >> -\t\tstring_list_append(&c->helpers, value);\n> >> -\telse if (!strcmp(key, \"username\")) {\n> >> +\tif (!strcmp(key, \"helper\")) {\n> >> +\t\tif (*value)\n> >> +\t\t\tstring_list_append(&c->helpers, value);\n> >> +\t\telse\n> >> +\t\t\tstring_list_clear(&c->helpers, 0);\n> >> +\t} else if (!strcmp(key, \"username\")) {\n> >\n> > I wondered why neither the existing code nor the updated one has a\n> > check for !value, but this callback assumes no credential\n> > configuration variable will ever be a boolean and rejects it\n> > upfront, so this code before or after the change is safe.\n> >\n> > Not pointing out anything that needs to be changed; demonstrating\n> > that I did read this sufficiently well to say that I have reviewed\n> > it ;-)\n> \n> This reminds me of one thing. The only reason why we are hesitant\n> to introduce a new syntax like\n> \n> \t[credential]\n> \t!helper ;# clear\n> helper = ...\n> \n> to allow explicit clearing of accumulated values so far IIRC is\n> because such a _file_ will not be readable by existing versions of\n> Git. Am I correct?\n\nI think there is another reason, which is that the interface we expose\nto config callbacks (and via \"config --get-all\") is to sequentially pass\nin all values. How does that interact with this \"reset\"? For example,\nwhat is the output of:\n\n git config foo.bar one\n git -c '!foo.bar' config --get-all foo.bar\n\n?\n\nDo we continue to output the \"reset\" values, or do we quietly munge the\nlist on behalf of the caller? If the former, how do we represent that in\nthe output? I can see arguments both ways.\n\nImplementation-wise (both for git-config and for internal callbacks), it\nmeans we cannot parse the config as a single pass anymore. That's\nprobably OK; we've already moved partially toward that with the\nconfigset stuff. If we _just_ support this via command-line options, we\ncould do an initial pass over those, looking for negatives, and then\nsimply skip all negatives while parsing the config files.\n\n> If that is the case, then that reasoning will still not prevent us\n> from adding corresponding support for a command-line overide, i.e.\n> either one or both of these:\n> \n> \t$ git -c credential.!helper cmd\n> \t$ git -c !credential.helper cmd\n> \n> no?\n\nYes, that would work, though to me it really feels like a\nhalf-implemented feature. You cannot override a bad /etc/gitconfig line\nvia your ~/.gitconfig or repo-specific .git/config. Those things are\nuseful.\n\nOne other thing that occurred to me is that Apple Git hard-codes the\nosxkeychain helper (rather than putting it into the system-wide\ngitconfig ). No config-based system can \"undo\" that, but my patch\ndoes. I admit that's probably not the best argument; hitting Apple with\na clue-stick is a cleaner approach.\n\n> Of course, the code in the configuration subsystem for updated\n> version of Git needs to become aware of the new syntax, and those\n> that deal with the multi-value variables need custom code, which is\n> similar to the way you special cased an empty value in the above\n> patch, so I am not sure how much this would help.\n\nI think you could get away without changing the users of the multi-value\nvariables, using the \"negative\" approach I mentioned above. Basically:\n\n 1. parse GIT_CONFIG_PARAMETERS looking for negatives; stick them in a\n string-list or whatever.\n\n 2. parse the files; look up each key in the string-list, and if it\n matches, don't even send it to the callback\n\n 3. clear the string-list\n\n 4. parse GIT_CONFIG_PARAMETERS again, ignoring any negatives\n\nBut like I said, that does feel somewhat half-implemented to me, since\nit treats the command-line specially.\n\n-Peff\n"},{"id":"279579","messageId":"xmqqy4a7hx4h.fsf@gitster.mtv.corp.google.com","threadId":"41533","inReplyTo":"20160226223719.GA2429@sigill.intra.peff.net","subject":"Re: [PATCH] credential: let empty credential specs reset helper list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T23:26:06Z","receivedAt":"2016-02-26T23:26:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think there is another reason, which is that the interface we expose\n> to config callbacks (and via \"config --get-all\") is to sequentially pass\n> in all values. How does that interact with this \"reset\"? For example,\n> what is the output of:\n>\n> git config foo.bar one\n> git -c '!foo.bar' config --get-all foo.bar\n\nThe callback API needs to be extended to include this \"reset\" bit,\nnot just \"var\" and \"value\", obviously. And then of course the users\nof the API need to be updated to act on that \"reset\" thing.\n\nThat is what I meant by \"multi-value things need custom code\". The\nsingle-valued ones can do \"last one wins\", and \"reset\" can be turned\ninto \"the initial state\" assignment for the variable; multi-value\ncumulative ones already have custom code to accumulate things into\nlist, and they need to learn to empty the list upon seeing the\n\"reset\" bit.\n\nBut that is not as fundamental as \"new syntax breaks old clients\".\nIf we want our new code to do useful things, obviously we would need\nto work on making it do so ;-)\n\n> I think you could get away without changing the users of the multi-value\n> variables, using the \"negative\" approach I mentioned above. Basically:\n> ...\n> But like I said, that does feel somewhat half-implemented to me, since\n> it treats the command-line specially.\n\nI agree with you that that is a kludge and not the right way\nforward.\n"}]}