{"thread":{"id":"31121","subject":"[PATCH] buitin_config: return postitive status in get_value","startedAt":"2012-07-28T11:42:10Z","lastAt":"2012-07-30T15:51:51Z","messageCount":10,"participants":["Nikolai Vladimirov","Nguyen Thai Ngoc Duy","Nikolay Vladimirov","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196017","messageId":"1343475730-18743-1-git-send-email-nikolay@vladimiroff.com","threadId":"31121","inReplyTo":null,"subject":"[PATCH] buitin_config: return postitive status in get_value","fromName":"Nikolai Vladimirov","fromEmail":"nikolay@vladimiroff.com","sentAt":"2012-07-28T11:42:10Z","receivedAt":"2012-07-28T11:42:10Z","isPatch":true,"sender":{"key":"nikolay@vladimiroff.com","avatar":null},"body":"Returning -1 instead of 1 results in wrong exit status(255) since\nthe output of get_value is passed to exit().\n\n'git config missing_section' should now return proper exit status = 1,\nas specified by the git config documentation.\n\nSigned-off-by: Nikolai Vladimirov <nikolay@vladimiroff.com>\n---\n builtin/config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8cd08da..c262cbb 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \n static int get_value(const char *key_, const char *regex_)\n {\n-\tint ret = -1;\n+\tint ret = 1;\n \tchar *global = NULL, *xdg = NULL, *repo_config = NULL;\n \tconst char *system_wide = NULL, *local;\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n-- \n1.7.11.2\n"},{"id":"196019","messageId":"CACsJy8BUU13H1tRfTtUme_7u0Jf0upca31U-VNhXva_0R2MLoQ@mail.gmail.com","threadId":"31121","inReplyTo":"1343475730-18743-1-git-send-email-nikolay@vladimiroff.com","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-07-28T12:59:36Z","receivedAt":"2012-07-28T12:59:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 28, 2012 at 6:42 PM, Nikolai Vladimirov\n<nikolay@vladimiroff.com> wrote:\n> Returning -1 instead of 1 results in wrong exit status(255) since\n> the output of get_value is passed to exit().\n>\n> 'git config missing_section' should now return proper exit status = 1,\n> as specified by the git config documentation.\n\nI'm curious. Why is -1 (or 255) wrong? It was introduced in the first\nversion of get_value in 4ddba79 (git-config-set: add more options -\n2005-11-20). Back then it returned -1 when there's regex compile error\nto distinguish with 0 and 1 (but git-config-set.txt in the same commit\ndid not get update about exit code). Maybe we should update document\ninstead of the code.\n-- \nDuy\n"},{"id":"196020","messageId":"CAJg5NvdAq_zEx2phVGS4EiUfqMtFJA2Bo=y1d3FJyWXtzMwCTw@mail.gmail.com","threadId":"31121","inReplyTo":"CACsJy8BUU13H1tRfTtUme_7u0Jf0upca31U-VNhXva_0R2MLoQ@mail.gmail.com","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Nikolay Vladimirov","fromEmail":"nikolay@vladimiroff.com","sentAt":"2012-07-28T13:18:49Z","receivedAt":"2012-07-28T13:18:49Z","isPatch":true,"sender":{"key":"nikolay@vladimiroff.com","avatar":null},"body":"On 28 July 2012 15:59, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:\n> I'm curious. Why is -1 (or 255) wrong? It was introduced in the first\n> version of get_value in 4ddba79 (git-config-set: add more options -\n> 2005-11-20). Back then it returned -1 when there's regex compile error\n> to distinguish with 0 and 1 (but git-config-set.txt in the same commit\n> did not get update about exit code). Maybe we should update document\n> instead of the code.\n> --\n> Duy\n\nThat sounds great.\nBut the behavior now seems kind of strange, or maybe I'm missing something:\n# git config foobar; echo $?\nerror: key does not contain a section: foobar\n255\n\n# git config foobar.info; echo $?\n1\n\ngit version 1.7.11.2\n\nI would generally expect the both to behave the same way.\n"},{"id":"196021","messageId":"20120728132923.GA31388@do","threadId":"31121","inReplyTo":"CAJg5NvdAq_zEx2phVGS4EiUfqMtFJA2Bo=y1d3FJyWXtzMwCTw@mail.gmail.com","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-07-28T13:29:23Z","receivedAt":"2012-07-28T13:29:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:\n> But the behavior now seems kind of strange, or maybe I'm missing something:\n> # git config foobar; echo $?\n> error: key does not contain a section: foobar\n> 255\n> \n> # git config foobar.info; echo $?\n> 1\n> \n> git version 1.7.11.2\n> \n> I would generally expect the both to behave the same way.\n\nThen the following patch may be better because it leaves other cases\nuntouched (I'm not saying that we should or should not do it though)\n\n-- 8< --\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8cd08da..d048ebf 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -199,8 +199,10 @@ static int get_value(const char *key_, const char *regex_)\n \t\t\tgoto free_strings;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL))\n+\t\tif (git_config_parse_key(key_, &key, NULL)) {\n+\t\t\tret = 1;\n \t\t\tgoto free_strings;\n+\t\t}\n \t}\n \n \tif (regex_) {\n-- 8< --\n\n-- \nDuy\n"},{"id":"196055","messageId":"7vipd7vzql.fsf@alter.siamese.dyndns.org","threadId":"31121","inReplyTo":"20120728132923.GA31388@do","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-29T06:38:10Z","receivedAt":"2012-07-29T06:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:\n>> But the behavior now seems kind of strange, or maybe I'm missing something:\n>> # git config foobar; echo $?\n>> error: key does not contain a section: foobar\n>> 255\n>> \n>> # git config foobar.info; echo $?\n>> 1\n>> \n>> git version 1.7.11.2\n>> \n>> I would generally expect the both to behave the same way.\n>\n> Then the following patch may be better because it leaves other cases\n> untouched (I'm not saying that we should or should not do it though)\n\nI personally think that the documentation unnecessarily exposes the\nuseless value assignment of the exit codes (the use of different\nexit codes was done merely to aid debugging the git-config command\nitself) by describing the then-current set of conditions, and should\nbe reduced to say \"0 for success, non-zero for any error\".\n\nBut if we really want to follow that \"documented\" subset of possible\nconditions, I agree that your version to return \"1\" in this case,\ntogether with a change to initialize \"ret\" to \"7\" and document it as\n\"all other errors (ret=7), would make more sense.  Other conditions\nthat have been added since that partial enumeration of the exit code\nwas done are regexp errors, which I think will get -1 from the same\nfunction.\n\n>\n> -- 8< --\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 8cd08da..d048ebf 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -199,8 +199,10 @@ static int get_value(const char *key_, const char *regex_)\n>  \t\t\tgoto free_strings;\n>  \t\t}\n>  \t} else {\n> -\t\tif (git_config_parse_key(key_, &key, NULL))\n> +\t\tif (git_config_parse_key(key_, &key, NULL)) {\n> +\t\t\tret = 1;\n>  \t\t\tgoto free_strings;\n> +\t\t}\n>  \t}\n>  \n>  \tif (regex_) {\n> -- 8< --\n"},{"id":"196076","messageId":"7v4noqwb6e.fsf@alter.siamese.dyndns.org","threadId":"31121","inReplyTo":"7vipd7vzql.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-29T20:43:21Z","receivedAt":"2012-07-29T20:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I personally think that the documentation unnecessarily exposes the\n> useless value assignment of the exit codes (the use of different\n> exit codes was done merely to aid debugging the git-config command\n> itself) by describing the then-current set of conditions, and should\n> be reduced to say \"0 for success, non-zero for any error\".\n>\n> But if we really want to follow that \"documented\" subset of possible\n> conditions, I agree that your version to return \"1\" in this case,\n> together with a change to initialize \"ret\" to \"7\" and document it as\n> \"all other errors (ret=7), would make more sense.  Other conditions\n> that have been added since that partial enumeration of the exit code\n> was done are regexp errors, which I think will get -1 from the same\n> function.\n\nIOW, something like this.\n\n Documentation/git-config.txt | 18 ++++++++++--------\n builtin/config.c             |  8 ++++++--\n 2 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 2d6ef32..b071d65 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -54,19 +54,21 @@ configuration file by default, and options '--system', '--global',\n '--file <filename>' can be used to tell the command to write to\n that location (you can say '--local' but that is the default).\n \n-This command will fail (with exit code ret) if:\n+This command will fail with non-zero status if:\n \n+. The section name or key name is invalid (ret=1),\n+. No section name or key name was provided (ret=2),\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),\n-. you try to use an invalid regexp (ret=6), or\n-. you use '--global' option without $HOME being properly set (ret=128).\n+. The config file cannot be written (ret=4),\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),\n+. You try to use an invalid regexp (ret=6),\n+. You use '--global' option without $HOME being properly set (ret=128),\n+. Any other errors (ret=7).\n \n On success, the command returns the exit code 0.\n \n+\n OPTIONS\n -------\n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8cd08da..2318308 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char *value_, void *cb)\n \n static int get_value(const char *key_, const char *regex_)\n {\n-\tint ret = -1;\n+\tint ret = 7;\n \tchar *global = NULL, *xdg = NULL, *repo_config = NULL;\n \tconst char *system_wide = NULL, *local;\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n@@ -196,11 +196,14 @@ static int get_value(const char *key_, const char *regex_)\n \t\tif (regcomp(key_regexp, key, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid key pattern: %s\\n\", key_);\n \t\t\tfree(key);\n+\t\t\tret = 6;\n \t\t\tgoto free_strings;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL))\n+\t\tif (git_config_parse_key(key_, &key, NULL)) {\n+\t\t\tret = 1;\n \t\t\tgoto free_strings;\n+\t\t}\n \t}\n \n \tif (regex_) {\n@@ -212,6 +215,7 @@ static int get_value(const char *key_, const char *regex_)\n \t\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n \t\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n \t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\", regex_);\n+\t\t\tret = 6;\n \t\t\tgoto free_strings;\n \t\t}\n \t}\n"},{"id":"196082","messageId":"20120729213928.GA3071@sigill.intra.peff.net","threadId":"31121","inReplyTo":"7vipd7vzql.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-29T21:39:28Z","receivedAt":"2012-07-29T21:39:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2012 at 11:38:10PM -0700, Junio C Hamano wrote:\n\n> > Then the following patch may be better because it leaves other cases\n> > untouched (I'm not saying that we should or should not do it though)\n> \n> I personally think that the documentation unnecessarily exposes the\n> useless value assignment of the exit codes (the use of different\n> exit codes was done merely to aid debugging the git-config command\n> itself) by describing the then-current set of conditions, and should\n> be reduced to say \"0 for success, non-zero for any error\".\n\nWe use ret=5 in the test suite to say \"unset this variable, but it's OK if\nit wasn't set in the first place\" but still fail on error. The only\nother one I can imagine that would be useful is \"you tried to get a\nvariable but it did not exist, and there was no other error\". Which is\nprobably what ret=1 is attempting to do, though it also encompasses\nsyntactically bogus keys.\n\n-Peff\n"},{"id":"196083","messageId":"20120729214117.GB3071@sigill.intra.peff.net","threadId":"31121","inReplyTo":"7v4noqwb6e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-29T21:41:17Z","receivedAt":"2012-07-29T21:41:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2012 at 01:43:21PM -0700, Junio C Hamano wrote:\n\n> > But if we really want to follow that \"documented\" subset of possible\n> > conditions, I agree that your version to return \"1\" in this case,\n> > together with a change to initialize \"ret\" to \"7\" and document it as\n> > \"all other errors (ret=7), would make more sense.  Other conditions\n> > that have been added since that partial enumeration of the exit code\n> > was done are regexp errors, which I think will get -1 from the same\n> > function.\n> \n> IOW, something like this.\n> \n>  Documentation/git-config.txt | 18 ++++++++++--------\n>  builtin/config.c             |  8 ++++++--\n>  2 files changed, 16 insertions(+), 10 deletions(-)\n\nThis looks right to me. Even if we are missing an error case, it is\ncertainly going in the right direction.\n\n-Peff\n"},{"id":"196118","messageId":"CACsJy8CSXo-BsVsqNb4+f97ns30SuSn_iRJgEk=WjgGyEQaywQ@mail.gmail.com","threadId":"31121","inReplyTo":"7v4noqwb6e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-07-30T12:26:51Z","receivedAt":"2012-07-30T12:26:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> +. You use '--global' option without $HOME being properly set (ret=128),\n> +. Any other errors (ret=7).\n\nTo be pedantic, ret=128 is a result of die() and not setting $HOME is\njust one of them. There's also ret=129 for usage(), which is not\ndocumented. So maybe we can make it clear that we document some exit\ncodes, but not all of them.\n\n> -This command will fail (with exit code ret) if:\n> +This command will fail with non-zero status if:\n\n\"This command will fail with non-zero status. Some important exit codes are:\"\n-- \nDuy\n"},{"id":"196148","messageId":"7vehnts0vc.fsf@alter.siamese.dyndns.org","threadId":"31121","inReplyTo":"CACsJy8CSXo-BsVsqNb4+f97ns30SuSn_iRJgEk=WjgGyEQaywQ@mail.gmail.com","subject":"Re: [PATCH] buitin_config: return postitive status in get_value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-30T15:51:51Z","receivedAt":"2012-07-30T15:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +. You use '--global' option without $HOME being properly set (ret=128),\n>> +. Any other errors (ret=7).\n>\n> To be pedantic, ret=128 is a result of die() and not setting $HOME is\n> just one of them. There's also ret=129 for usage(), which is not\n> documented. So maybe we can make it clear that we document some exit\n> codes, but not all of them.\n>\n>> -This command will fail (with exit code ret) if:\n>> +This command will fail with non-zero status if:\n>\n> \"This command will fail with non-zero status. Some important exit codes are:\"\n\nSounds good; will squash in.\n"}]}