{"thread":{"id":"41557","subject":"git config --get-urlmatch does not set exit code 1 when no match is found","startedAt":"2016-02-28T04:39:12Z","lastAt":"2016-03-01T15:03:21Z","messageCount":11,"participants":["Guilherme","John Keeping","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"279681","messageId":"CAMDzUtzNKAYSKYkt3WagkUrA2mKaoDu1rT6Nhf89pXSMg0wZwA@mail.gmail.com","threadId":"41557","inReplyTo":null,"subject":"git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"Guilherme","fromEmail":"guibufolo@gmail.com","sentAt":"2016-02-28T04:39:12Z","receivedAt":"2016-02-28T04:39:12Z","isPatch":false,"sender":{"key":"guibufolo@gmail.com","avatar":null},"body":"Hello,\n\nMy current woes are with multi-valued configuration values. More\nspecifically credential.helper\n\nThe documentation of git config says that when a value is not matched\nit should return 1.\n\nTo reproduce make sure that credential.helper is not set.\n\ngit config --get-urlmatch credential.helper http://somedomain:1234/\necho %ERRORLEVEL%\n0\n\ngit config --get credential.helper\necho %ERRORLEVEL%\n1\n\ngit config --get credential.http://somedomain:1234/.helper\necho %ERRORLEVEL%\n1\n\nThe documentation says that for credential.helper is not found for a\ndomain it should fall back to credential.helper if it is set. So I\nthink that all those tests above should have returned 0. Am i right?\n\nCheers.\n"},{"id":"279707","messageId":"20160228104557.GT1766@serenity.lan","threadId":"41557","inReplyTo":"CAMDzUtzNKAYSKYkt3WagkUrA2mKaoDu1rT6Nhf89pXSMg0wZwA@mail.gmail.com","subject":"Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T10:45:57Z","receivedAt":"2016-02-28T10:45:57Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:\n> My current woes are with multi-valued configuration values. More\n> specifically credential.helper\n> \n> The documentation of git config says that when a value is not matched\n> it should return 1.\n> \n> To reproduce make sure that credential.helper is not set.\n> \n> git config --get-urlmatch credential.helper http://somedomain:1234/\n> echo %ERRORLEVEL%\n> 0\n> \n> git config --get credential.helper\n> echo %ERRORLEVEL%\n> 1\n> \n> git config --get credential.http://somedomain:1234/.helper\n> echo %ERRORLEVEL%\n> 1\n> \n> The documentation says that for credential.helper is not found for a\n> domain it should fall back to credential.helper if it is set. So I\n> think that all those tests above should have returned 0. Am i right?\n\nIt looks to me like a simple bug that --get-urlmatch doesn't return 1 if\nthe key isn't found, but git-config(1) isn't entirely clear.  The\noverall documentation on exit codes at the end of DESCRIPTION says that\nexit code 1 means:\n\n\tthe section or key is invalid (ret=1)\n\nThen the documentation for the --get option says:\n\n\tReturns error code 1 if the key was not found.\n\nand --get-all says:\n\n\tLike get, but does not fail if the number of values for the key\n\tis not exactly one.\n\nalthough it does return 1 if there are zero values.  --get-regexp\nbehaves in the same way.\n\nOverall I think that the fact that --get-urlmatch is the outlier here\nmeans that it should change to match the other --get* options (ignoring\n--get-color and --get-colorbool which are very different).  Although I\nwonder if anyone is relying on the current behaviour and will find their\nworkflow broken if we change this.\n\nThe documentation could also use some clarification since most of the\nreturn codes only apply for the \"set\" options and in some cases this\nisn't clear from the existing descriptions.\n"},{"id":"279709","messageId":"20160228115227.GU1766@serenity.lan","threadId":"41557","inReplyTo":"20160228104557.GT1766@serenity.lan","subject":"Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T11:52:27Z","receivedAt":"2016-02-28T11:52:27Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Feb 28, 2016 at 10:45:57AM +0000, John Keeping wrote:\n> On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:\n> > My current woes are with multi-valued configuration values. More\n> > specifically credential.helper\n> > \n> > The documentation of git config says that when a value is not matched\n> > it should return 1.\n> > \n> > To reproduce make sure that credential.helper is not set.\n> > \n> > git config --get-urlmatch credential.helper http://somedomain:1234/\n> > echo %ERRORLEVEL%\n> > 0\n> > \n> > git config --get credential.helper\n> > echo %ERRORLEVEL%\n> > 1\n> > \n> > git config --get credential.http://somedomain:1234/.helper\n> > echo %ERRORLEVEL%\n> > 1\n> > \n> > The documentation says that for credential.helper is not found for a\n> > domain it should fall back to credential.helper if it is set. So I\n> > think that all those tests above should have returned 0. Am i right?\n\nI misread this as \"should have returned 1\", which is what the text below\nagrees with.\n\nThe \"git config\" command does not know anything about the semantics of\nparticular config keys.  It is purely an interface to parse and query\nthe config file format and it is up to the consumer to know what to do\nif a key doesn't exist.\n\nBoth of the \"git config --get\" examples you give are behaving as\ndocumented in git-config(1).\n\n> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if\n> the key isn't found, but git-config(1) isn't entirely clear.  The\n> overall documentation on exit codes at the end of DESCRIPTION says that\n> exit code 1 means:\n> \n> \tthe section or key is invalid (ret=1)\n> \n> Then the documentation for the --get option says:\n> \n> \tReturns error code 1 if the key was not found.\n> \n> and --get-all says:\n> \n> \tLike get, but does not fail if the number of values for the key\n> \tis not exactly one.\n> \n> although it does return 1 if there are zero values.  --get-regexp\n> behaves in the same way.\n> \n> Overall I think that the fact that --get-urlmatch is the outlier here\n> means that it should change to match the other --get* options (ignoring\n> --get-color and --get-colorbool which are very different).  Although I\n> wonder if anyone is relying on the current behaviour and will find their\n> workflow broken if we change this.\n> \n> The documentation could also use some clarification since most of the\n> return codes only apply for the \"set\" options and in some cases this\n> isn't clear from the existing descriptions.\n"},{"id":"279710","messageId":"cover.1456660027.git.john@keeping.me.uk","threadId":"41557","inReplyTo":"20160228104557.GT1766@serenity.lan","subject":"[PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T11:54:34Z","receivedAt":"2016-02-28T11:54:34Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Feb 28, 2016 at 10:45:57AM +0000, John Keeping wrote:\n> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if\n> the key isn't found, but git-config(1) isn't entirely clear.  The\n> overall documentation on exit codes at the end of DESCRIPTION says that\n> exit code 1 means:\n> \n>       the section or key is invalid (ret=1)\n> \n> Then the documentation for the --get option says:\n> \n>       Returns error code 1 if the key was not found.\n> \n> and --get-all says:\n> \n>       Like get, but does not fail if the number of values for the key\n>       is not exactly one.\n> \n> although it does return 1 if there are zero values.  --get-regexp\n> behaves in the same way.\n> \n> Overall I think that the fact that --get-urlmatch is the outlier here\n> means that it should change to match the other --get* options (ignoring\n> --get-color and --get-colorbool which are very different).  Although I\n> wonder if anyone is relying on the current behaviour and will find their\n> workflow broken if we change this.\n> \n> The documentation could also use some clarification since most of the\n> return codes only apply for the \"set\" options and in some cases this\n> isn't clear from the existing descriptions.\n\nHere's a series that changes the behaviour of \"git config --get-urlmatch\"\nwhen no appropriate key is found as well as a couple of improvements to\nthe documentation while we're here.\n\nThe second two patches are independent of the first and I think they\nshould be picked up even if we decide the change to --get-urlmatch's\nexit code is not desirable.\n\nJohn Keeping (3):\n  config: fail if --get-urlmatch finds no value\n  Documentation/git-config: use bulleted list for exit codes\n  Documentation/git-config: fix --get-all description\n\n Documentation/git-config.txt | 19 +++++++++----------\n builtin/config.c             |  5 ++++-\n t/t1300-repo-config.sh       |  3 +++\n 3 files changed, 16 insertions(+), 11 deletions(-)\n\n-- \n2.7.1.503.g3cfa3ac\n"},{"id":"279712","messageId":"9ed75bdb80839deaebac642a8abd1da5b6a969d1.1456660027.git.john@keeping.me.uk","threadId":"41557","inReplyTo":"cover.1456660027.git.john@keeping.me.uk","subject":"[PATCH 1/3] config: fail if --get-urlmatch finds no value","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T11:54:35Z","receivedAt":"2016-02-28T11:54:35Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The --get, --get-all and --get-regexp options to git-config exit with\nstatus 1 if the key is not found but --get-urlmatch succeeds in this\ncase.\n\nChange --get-urlmatch to behave in the same way as the other --get*\noptions so that all four are consistent.  --get-color is a special case\nbecause it accepts a default value to return and so should not return an\nerror if the key is not found.\n\nAlso clarify this behaviour in the documentation.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n Documentation/git-config.txt | 2 +-\n builtin/config.c             | 5 ++++-\n t/t1300-repo-config.sh       | 3 +++\n 3 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 153b2d8..2a04e87 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -102,7 +102,7 @@ OPTIONS\n \tgiven URL is returned (if no such key exists, the value for\n \tsection.key is used as a fallback).  When given just the\n \tsection as name, do so for all the keys in the section and\n-\tlist them.\n+\tlist them.  Returns error code 1 if no value is found.\n \n --global::\n \tFor writing options: write to global `~/.gitconfig` file\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ca9f834..1d7c6ef 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -417,6 +417,7 @@ static int urlmatch_collect_fn(const char *var, const char *value, void *cb)\n \n static int get_urlmatch(const char *var, const char *url)\n {\n+\tint ret;\n \tchar *section_tail;\n \tstruct string_list_item *item;\n \tstruct urlmatch_config config = { STRING_LIST_INIT_DUP };\n@@ -443,6 +444,8 @@ static int get_urlmatch(const char *var, const char *url)\n \tgit_config_with_options(urlmatch_config_entry, &config,\n \t\t\t\t&given_config_source, respect_includes);\n \n+\tret = !values.nr;\n+\n \tfor_each_string_list_item(item, &values) {\n \t\tstruct urlmatch_current_candidate_value *matched = item->util;\n \t\tstruct strbuf buf = STRBUF_INIT;\n@@ -459,7 +462,7 @@ static int get_urlmatch(const char *var, const char *url)\n \tfree(config.url.url);\n \n \tfree((void *)config.section);\n-\treturn 0;\n+\treturn ret;\n }\n \n static char *default_user_config(void)\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 8867ce1..89d8c47 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1148,6 +1148,9 @@ test_expect_success 'urlmatch' '\n \t\tcookieFile = /tmp/cookie.txt\n \tEOF\n \n+\ttest_expect_code 1 git config --bool --get-urlmatch doesnt.exist https://good.example.com >actual &&\n+\ttest_must_be_empty actual &&\n+\n \techo true >expect &&\n \tgit config --bool --get-urlmatch http.SSLverify https://good.example.com >actual &&\n \ttest_cmp expect actual &&\n-- \n2.7.1.503.g3cfa3ac\n"},{"id":"279711","messageId":"0b2892dfb713596f3b509c8d4d3f0f31bdd56cdf.1456660027.git.john@keeping.me.uk","threadId":"41557","inReplyTo":"cover.1456660027.git.john@keeping.me.uk","subject":"[PATCH 2/3] Documentation/git-config: use bulleted list for exit codes","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T11:54:36Z","receivedAt":"2016-02-28T11:54:36Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Using a numbered list is confusing because the exit codes are not listed\nin order so the numbers at the start of each line do not match the exit\ncodes described by the following text.  Switch to a bulleted list so\nthat the only number appearing on each line is the exit code described.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n Documentation/git-config.txt | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 2a04e87..e9c755f 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -58,13 +58,13 @@ that location (you can say '--local' but that is the default).\n This command will fail with non-zero status upon error.  Some exit\n codes are:\n \n-. The config file is invalid (ret=3),\n-. can not write to the config file (ret=4),\n-. no section or name was provided (ret=2),\n-. the section or key is invalid (ret=1),\n-. you try to unset an option which does not exist (ret=5),\n-. you try to unset/set an option for which multiple lines match (ret=5), or\n-. you try to use an invalid regexp (ret=6).\n+- The config file is invalid (ret=3),\n+- can not write to the config file (ret=4),\n+- no section or name was provided (ret=2),\n+- the section or key is invalid (ret=1),\n+- you try to unset an option which does not exist (ret=5),\n+- you try to unset/set an option for which multiple lines match (ret=5), or\n+- you try to use an invalid regexp (ret=6).\n \n On success, the command returns the exit code 0.\n \n-- \n2.7.1.503.g3cfa3ac\n"},{"id":"279713","messageId":"77578beabdb50d696ad9155ef2e0e1847c22a9ad.1456660027.git.john@keeping.me.uk","threadId":"41557","inReplyTo":"cover.1456660027.git.john@keeping.me.uk","subject":"[PATCH 3/3] Documentation/git-config: fix --get-all description","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-28T11:54:37Z","receivedAt":"2016-02-28T11:54:37Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"--get does not fail if a key is multi-valued, it returns the last value\nas described in its documentation.  Clarify the description of --get-all\nto avoid implying that --get does fail in this case.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n Documentation/git-config.txt | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex e9c755f..6fc08e3 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -86,8 +86,7 @@ OPTIONS\n \tfound and the last value if multiple key values were found.\n \n --get-all::\n-\tLike get, but does not fail if the number of values for the key\n-\tis not exactly one.\n+\tLike get, but returns all values for a multi-valued key.\n \n --get-regexp::\n \tLike --get-all, but interprets the name as a regular expression and\n-- \n2.7.1.503.g3cfa3ac\n"},{"id":"279729","messageId":"xmqq8u24eh9s.fsf@gitster.mtv.corp.google.com","threadId":"41557","inReplyTo":"cover.1456660027.git.john@keeping.me.uk","subject":"Re: [PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-28T20:01:19Z","receivedAt":"2016-02-28T20:01:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Here's a series that changes the behaviour of \"git config --get-urlmatch\"\n> when no appropriate key is found as well as a couple of improvements to\n> the documentation while we're here.\n\nSounds sensible.  It does change the behaviour, but it is inevitable\nthat a bugfix has to change existing behaviour, so...\n\n>\n> John Keeping (3):\n>   config: fail if --get-urlmatch finds no value\n>   Documentation/git-config: use bulleted list for exit codes\n>   Documentation/git-config: fix --get-all description\n>\n>  Documentation/git-config.txt | 19 +++++++++----------\n>  builtin/config.c             |  5 ++++-\n>  t/t1300-repo-config.sh       |  3 +++\n>  3 files changed, 16 insertions(+), 11 deletions(-)\n"},{"id":"279770","messageId":"20160229115355.GA31273@sigill.intra.peff.net","threadId":"41557","inReplyTo":"CAMDzUtzNKAYSKYkt3WagkUrA2mKaoDu1rT6Nhf89pXSMg0wZwA@mail.gmail.com","subject":"Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T11:53:55Z","receivedAt":"2016-02-29T11:53:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:\n\n> My current woes are with multi-valued configuration values. More\n> specifically credential.helper\n> \n> The documentation of git config says that when a value is not matched\n> it should return 1.\n> \n> To reproduce make sure that credential.helper is not set.\n> \n> git config --get-urlmatch credential.helper http://somedomain:1234/\n> echo %ERRORLEVEL%\n> 0\n\nThis isn't really addressing your question, but I should warn you that\ninternally, the credential code _doesn't_ use the urlmatch\ninfrastructure. It predates the urlmatch code, and was never converted\n(so basically only http.* uses urlmatch).  I think there are some corner\ncases where the two behave differently.\n\nI'm not sure what you're using this for, but you may get surprising\nresults.\n\n-Peff\n"},{"id":"279776","messageId":"CAMDzUtwJVyaQbjgdQLi17_4ejGofpRFBDxXxjseaVcHLXCAwRA@mail.gmail.com","threadId":"41557","inReplyTo":"20160229115355.GA31273@sigill.intra.peff.net","subject":"Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"Guilherme","fromEmail":"guibufolo@gmail.com","sentAt":"2016-02-29T13:08:28Z","receivedAt":"2016-02-29T13:08:28Z","isPatch":false,"sender":{"key":"guibufolo@gmail.com","avatar":null},"body":"@Peff Thank you for the heads up.\n\nI'm trying to find out if there are any credential helpers configured\nin the system that will be running tests. On the dedicated test\nmachines that is not a problem but the developer machines are.\n\nShould I already post a pre-emptive email asking about the corner cases?\n\nMore importantly for me is if there is a case where get-url would not\nshow a match where git clone would. If git clone skips a configuration\nthat config url-match doesn't then it's not so bad.\n"},{"id":"279971","messageId":"20160301150321.GM12887@sigill.intra.peff.net","threadId":"41557","inReplyTo":"CAMDzUtwJVyaQbjgdQLi17_4ejGofpRFBDxXxjseaVcHLXCAwRA@mail.gmail.com","subject":"Re: git config --get-urlmatch does not set exit code 1 when no match is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-01T15:03:21Z","receivedAt":"2016-03-01T15:03:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 29, 2016 at 06:38:28PM +0530, Guilherme wrote:\n\n> @Peff Thank you for the heads up.\n> \n> I'm trying to find out if there are any credential helpers configured\n> in the system that will be running tests. On the dedicated test\n> machines that is not a problem but the developer machines are.\n\nDo you need to do it in an automated way, or just once?\n\nIf manual is OK, I suspect running:\n\n  GIT_TRACE=1 git credential </dev/null\n\nwill give you a list of what gets run. That's awfully hacky, though.\nProbably a \"git credential list\" command would be useful. Or just\nadapting it to use the urlmatch stuff.\n\n> Should I already post a pre-emptive email asking about the corner cases?\n> \n> More importantly for me is if there is a case where get-url would not\n> show a match where git clone would. If git clone skips a configuration\n> that config url-match doesn't then it's not so bad.\n\nSorry, I don't recall the details. I feel like we discussed it a little\non the list, but I can't find it now. The closest I could find is:\n\n  http://article.gmane.org/gmane.comp.version-control.git/267895\n\nI think the main differences would be in ordering, not in what is\nselected.\n\n-Peff\n"}]}