{"thread":{"id":"27380","subject":"[PATCH/RFC] config: Give error message when not changing a multivar","startedAt":"2011-05-17T11:34:58Z","lastAt":"2011-05-18T18:49:10Z","messageCount":11,"participants":["Michael J Gruber","Jeff King","Thiago Farina","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168052","messageId":"42ab57ae5a2cb7d6860e43e7c3061f9c38cf1b99.1305632091.git.git@drmicha.warpmail.net","threadId":"27380","inReplyTo":null,"subject":"[PATCH/RFC] config: Give error message when not changing a multivar","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-17T11:34:58Z","receivedAt":"2011-05-17T11:34:58Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When trying to set a multivar with \"git config var value\", \"git config\"\nissues\n\nwarning: remote.repoor.push has multiple values\n\nleaving the user under the impression that the operation succeeded,\nunless one checks the return value.\n\nInstead, make it\n\nwarning: remote.repoor.push has multiple values\nerror: Use a regexp, --add or --set-all to change remote.repoor.push.\n\nto be clear and helpful.\n\nNote: The \"warning\" is raised through other code paths also so that it\nneeds to remain a warning for these (which do not raise the error). Only\nthe caller can determine how to go on from that.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n builtin/config.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 3e3c528..c438ef4 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -436,9 +436,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t      NULL, NULL);\n \t}\n \telse if (actions == ACTION_SET) {\n+\t\tint ret;\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set(argv[0], value);\n+\t\tret = git_config_set(argv[0], value);\n+\t\tif (ret == 5)\n+\t\t\terror(\"Use a regexp, --add or --set-all to change %s.\", argv[0]);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n \t\tcheck_argc(argc, 2, 3);\n-- \n1.7.5.1.514.gd181fb\n"},{"id":"168055","messageId":"20110517123840.GB13176@sigill.intra.peff.net","threadId":"27380","inReplyTo":"42ab57ae5a2cb7d6860e43e7c3061f9c38cf1b99.1305632091.git.git@drmicha.warpmail.net","subject":"Re: [PATCH/RFC] config: Give error message when not changing a multivar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T12:38:40Z","receivedAt":"2011-05-17T12:38:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 01:34:58PM +0200, Michael J Gruber wrote:\n\n> Instead, make it\n> \n> warning: remote.repoor.push has multiple values\n> error: Use a regexp, --add or --set-all to change remote.repoor.push.\n> \n> to be clear and helpful.\n> \n> Note: The \"warning\" is raised through other code paths also so that it\n> needs to remain a warning for these (which do not raise the error). Only\n> the caller can determine how to go on from that.\n\nMakes sense, and I think trying to change the \"warning\" text is not\nworth the effort.\n\n>  \telse if (actions == ACTION_SET) {\n> +\t\tint ret;\n>  \t\tcheck_argc(argc, 2, 2);\n>  \t\tvalue = normalize_value(argv[0], argv[1]);\n> -\t\treturn git_config_set(argv[0], value);\n> +\t\tret = git_config_set(argv[0], value);\n> +\t\tif (ret == 5)\n> +\t\t\terror(\"Use a regexp, --add or --set-all to change %s.\", argv[0]);\n> +\t\treturn ret;\n>  \t}\n\nWhat the heck is this 5? In fact, what in the world is going on with the\nreturn values from git_config_set_multivar? It looks like we can return\n3, 4, 5, or 6 to return various errors, but they're not documented\nanywhere. Or we can return 2, propagated from parse_key. Or we can\nreturn -1 (negative!) if the lock doesn't work.\n\nI think the last one is a straight-out error. For the other ones, we\nshould probably document them in git-config(1). And it would be nice to\nat least have some symbolic constants in the code.\n\nNone of these problems is introduced by your patch, of course. But it\nmight be nice to at least do the symbolic constants while we're looking\nat this so that your patch can use them.\n\n-Peff\n"},{"id":"168056","messageId":"4DD28014.4090505@drmicha.warpmail.net","threadId":"27380","inReplyTo":"20110517123840.GB13176@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] config: Give error message when not changing a multivar","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-17T14:03:00Z","receivedAt":"2011-05-17T14:03:00Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 17.05.2011 14:38:\n> On Tue, May 17, 2011 at 01:34:58PM +0200, Michael J Gruber wrote:\n> \n>> Instead, make it\n>>\n>> warning: remote.repoor.push has multiple values\n>> error: Use a regexp, --add or --set-all to change remote.repoor.push.\n>>\n>> to be clear and helpful.\n>>\n>> Note: The \"warning\" is raised through other code paths also so that it\n>> needs to remain a warning for these (which do not raise the error). Only\n>> the caller can determine how to go on from that.\n> \n> Makes sense, and I think trying to change the \"warning\" text is not\n> worth the effort.\n> \n>>  \telse if (actions == ACTION_SET) {\n>> +\t\tint ret;\n>>  \t\tcheck_argc(argc, 2, 2);\n>>  \t\tvalue = normalize_value(argv[0], argv[1]);\n>> -\t\treturn git_config_set(argv[0], value);\n>> +\t\tret = git_config_set(argv[0], value);\n>> +\t\tif (ret == 5)\n>> +\t\t\terror(\"Use a regexp, --add or --set-all to change %s.\", argv[0]);\n>> +\t\treturn ret;\n>>  \t}\n> \n> What the heck is this 5? In fact, what in the world is going on with the\n> return values from git_config_set_multivar? It looks like we can return\n> 3, 4, 5, or 6 to return various errors, but they're not documented\n> anywhere. Or we can return 2, propagated from parse_key. Or we can\n> return -1 (negative!) if the lock doesn't work.\n> \n> I think the last one is a straight-out error. For the other ones, we\n> should probably document them in git-config(1). And it would be nice to\n> at least have some symbolic constants in the code.\n> \n> None of these problems is introduced by your patch, of course. But it\n> might be nice to at least do the symbolic constants while we're looking\n> at this so that your patch can use them.\n\nOK, makes sense, I just wanted to request for comments early enough -\nsomeone might think we should remove the warning from the present site\nand make all callers deal with the return codes. But I don't think it's\nworth it, and we have other places where we warn first, then error out.\n\nMichael\n"},{"id":"168058","messageId":"20110517140725.GA17193@sigill.intra.peff.net","threadId":"27380","inReplyTo":"4DD28014.4090505@drmicha.warpmail.net","subject":"Re: [PATCH/RFC] config: Give error message when not changing a multivar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-17T14:07:25Z","receivedAt":"2011-05-17T14:07:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 04:03:00PM +0200, Michael J Gruber wrote:\n\n> >> warning: remote.repoor.push has multiple values\n> >> error: Use a regexp, --add or --set-all to change remote.repoor.push.\n> [...]\n> OK, makes sense, I just wanted to request for comments early enough -\n> someone might think we should remove the warning from the present site\n> and make all callers deal with the return codes. But I don't think it's\n> worth it, and we have other places where we warn first, then error out.\n\nYou could maybe make it a little more clear what is going on with:\n\n  warning: remote.repoor.push has multiple values\n  error: cannot overwrite multiple values with a single value\n         Use a regexp, --add, or --set-all to change remote.repoor.push\n\nThen the error stands on its own, but the warning is not a bad thing to\nhave, also.\n\n-Peff\n"},{"id":"168064","messageId":"6301052eb455e9088e50f78760e6ca1b9499564a.1305646709.git.git@drmicha.warpmail.net","threadId":"27380","inReplyTo":"20110517140725.GA17193@sigill.intra.peff.net","subject":"[PATCH 1/2] config: define and document exit codes","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-17T15:38:52Z","receivedAt":"2011-05-17T15:38:52Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"The return codes of git_config_set() and friends are magic numbers right\nin the source. #define them in cache.h where the functions are declared,\nand use the constants in the source.\n\nAlso, mention the resulting exit codes of \"git config\" in its man page\n(and complete the list).\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n Documentation/git-config.txt |   18 ++++++++++--------\n cache.h                      |   10 ++++++++++\n config.c                     |   20 ++++++++++----------\n 3 files changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 8804de3..e7ecf5d 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -50,16 +50,18 @@ The default is to assume the config file of the current repository,\n .git/config unless defined otherwise with GIT_DIR and GIT_CONFIG\n (see <<FILES>>).\n \n-This command will fail if:\n-\n-. The config file is invalid,\n-. Can not write to the config file,\n-. no section was provided,\n-. the section or key is invalid,\n-. you try to unset an option which does not exist,\n-. you try to unset/set an option for which multiple lines match, or\n-. you use '--global' option without $HOME being properly set.\n-\n+This command will fail (with exit code ret) if:\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),\n+. you try to use an invalid regexp (ret=6), or\n+. you use '--global' option without $HOME being properly set (ret=128).\n+\n+On success, the command returns the exit code 0.\n \n OPTIONS\n -------\ndiff --git a/cache.h b/cache.h\nindex 2225c3f..57a98d2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1018,6 +1018,16 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne\n /* Dumb servers support */\n extern int update_server_info(int);\n \n+/* git_config_parse_key() returns these negated: */\n+#define CONFIG_INVALID_KEY 1\n+#define CONFIG_NO_SECTION_OR_NAME 2\n+/* git_config_set(), git_config_set_multivar() return the above or these: */\n+#define CONFIG_NO_LOCK -1\n+#define CONFIG_INVALID_FILE 3\n+#define CONFIG_NO_WRITE 4\n+#define CONFIG_NOTHING_SET 5\n+#define CONFIG_INVALID_PATTERN 6\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\ndiff --git a/config.c b/config.c\nindex 671c8df..9d36848 100644\n--- a/config.c\n+++ b/config.c\n@@ -1123,12 +1123,12 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \n \tif (last_dot == NULL || last_dot == key) {\n \t\terror(\"key does not contain a section: %s\", key);\n-\t\treturn -2;\n+\t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n \n \tif (!last_dot[1]) {\n \t\terror(\"key does not contain variable name: %s\", key);\n-\t\treturn -2;\n+\t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n \n \tbaselen = last_dot - key;\n@@ -1165,7 +1165,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \n out_free_ret_1:\n \tfree(*store_key);\n-\treturn -1;\n+\treturn -CONFIG_INVALID_KEY;\n }\n \n /*\n@@ -1221,7 +1221,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \tif (fd < 0) {\n \t\terror(\"could not lock config file %s: %s\", config_filename, strerror(errno));\n \t\tfree(store.key);\n-\t\tret = -1;\n+\t\tret = CONFIG_NO_LOCK;\n \t\tgoto out_free;\n \t}\n \n@@ -1235,12 +1235,12 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\tif ( ENOENT != errno ) {\n \t\t\terror(\"opening %s: %s\", config_filename,\n \t\t\t      strerror(errno));\n-\t\t\tret = 3; /* same as \"invalid config file\" */\n+\t\t\tret = CONFIG_INVALID_FILE; /* same as \"invalid config file\" */\n \t\t\tgoto out_free;\n \t\t}\n \t\t/* if nothing to unset, error out */\n \t\tif (value == NULL) {\n-\t\t\tret = 5;\n+\t\t\tret = CONFIG_NOTHING_SET;\n \t\t\tgoto out_free;\n \t\t}\n \n@@ -1268,7 +1268,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t\t\t\tREG_EXTENDED)) {\n \t\t\t\terror(\"invalid pattern: %s\", value_regex);\n \t\t\t\tfree(store.value_regex);\n-\t\t\t\tret = 6;\n+\t\t\t\tret = CONFIG_INVALID_PATTERN;\n \t\t\t\tgoto out_free;\n \t\t\t}\n \t\t}\n@@ -1290,7 +1290,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t\t\tregfree(store.value_regex);\n \t\t\t\tfree(store.value_regex);\n \t\t\t}\n-\t\t\tret = 3;\n+\t\t\tret = CONFIG_INVALID_FILE;\n \t\t\tgoto out_free;\n \t\t}\n \n@@ -1303,7 +1303,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t/* if nothing to unset, or too many matches, error out */\n \t\tif ((store.seen == 0 && value == NULL) ||\n \t\t\t\t(store.seen > 1 && multi_replace == 0)) {\n-\t\t\tret = 5;\n+\t\t\tret = CONFIG_NOTHING_SET;\n \t\t\tgoto out_free;\n \t\t}\n \n@@ -1364,7 +1364,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \n \tif (commit_lock_file(lock) < 0) {\n \t\terror(\"could not commit config file %s\", config_filename);\n-\t\tret = 4;\n+\t\tret = CONFIG_NO_WRITE;\n \t\tgoto out_free;\n \t}\n \n-- \n1.7.5.1.514.gd181fb\n"},{"id":"168065","messageId":"51b0df452d9cb7f3dbe0a47ef639b6a45c7de1b3.1305646709.git.git@drmicha.warpmail.net","threadId":"27380","inReplyTo":"20110517140725.GA17193@sigill.intra.peff.net","subject":"[PATCH 2/2] config: Give error message when not changing a multivar","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-17T15:38:53Z","receivedAt":"2011-05-17T15:38:53Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When trying to set a multivar with \"git config var value\", \"git config\"\nissues\n\nwarning: remote.repoor.push has multiple values\n\nleaving the user under the impression that the operation succeeded,\nunless one checks the return value.\n\nInstead, make it\n\nwarning: remote.repoor.push has multiple values\nerror: cannot overwrite multiple values with a single value\n       Use a regexp, --add or --set-all to change remote.repoor.push.\n\nto be clear and helpful.\n\nNote: The \"warning\" is raised through other code paths also so that it\nneeds to remain a warning for these (which do not raise the error). Only\nthe caller can determine how to go on from that.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n builtin/config.c |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 3e3c528..211e118 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -436,9 +436,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t      NULL, NULL);\n \t}\n \telse if (actions == ACTION_SET) {\n+\t\tint ret;\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set(argv[0], value);\n+\t\tret = git_config_set(argv[0], value);\n+\t\tif (ret == CONFIG_NOTHING_SET)\n+\t\t\terror(\"cannot overwrite multiple values with a single value\\n\"\n+\t\t\t\"       Use a regexp, --add or --set-all to change %s.\", argv[0]);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n \t\tcheck_argc(argc, 2, 3);\n-- \n1.7.5.1.514.gd181fb\n"},{"id":"168066","messageId":"BANLkTikjY956f7LxXGY5fcLkJJDuMnt6OQ@mail.gmail.com","threadId":"27380","inReplyTo":"6301052eb455e9088e50f78760e6ca1b9499564a.1305646709.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] config: define and document exit codes","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2011-05-17T15:46:36Z","receivedAt":"2011-05-17T15:46:36Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber\n<git@drmicha.warpmail.net> wrote:\n> The return codes of git_config_set() and friends are magic numbers right\n> in the source. #define them in cache.h where the functions are declared,\n\nWhy defining them on cache.h? Just because the functions are declared\nthere? Is this a good reason? I think this pollute even more the\ncache.h and these constants are not used outside of config.c. So I'd\nmove them back onto config.c. Maybe it's fine as is though.\n\n> and use the constants in the source.\n>\n> Also, mention the resulting exit codes of \"git config\" in its man page\n> (and complete the list).\n>\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n>  Documentation/git-config.txt |   18 ++++++++++--------\n>  cache.h                      |   10 ++++++++++\n>  config.c                     |   20 ++++++++++----------\n>  3 files changed, 30 insertions(+), 18 deletions(-)\n>\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 8804de3..e7ecf5d 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -50,16 +50,18 @@ The default is to assume the config file of the current repository,\n>  .git/config unless defined otherwise with GIT_DIR and GIT_CONFIG\n>  (see <<FILES>>).\n>\n> -This command will fail if:\n> -\n> -. The config file is invalid,\n> -. Can not write to the config file,\n> -. no section was provided,\n> -. the section or key is invalid,\n> -. you try to unset an option which does not exist,\n> -. you try to unset/set an option for which multiple lines match, or\n> -. you use '--global' option without $HOME being properly set.\n> -\n> +This command will fail (with exit code ret) if:\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),\n> +. you try to use an invalid regexp (ret=6), or\n> +. you use '--global' option without $HOME being properly set (ret=128).\n> +\n> +On success, the command returns the exit code 0.\n>\n>  OPTIONS\n>  -------\n> diff --git a/cache.h b/cache.h\n> index 2225c3f..57a98d2 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1018,6 +1018,16 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne\n>  /* Dumb servers support */\n>  extern int update_server_info(int);\n>\n> +/* git_config_parse_key() returns these negated: */\n> +#define CONFIG_INVALID_KEY 1\n> +#define CONFIG_NO_SECTION_OR_NAME 2\n> +/* git_config_set(), git_config_set_multivar() return the above or these: */\n> +#define CONFIG_NO_LOCK -1\n> +#define CONFIG_INVALID_FILE 3\n> +#define CONFIG_NO_WRITE 4\n> +#define CONFIG_NOTHING_SET 5\n> +#define CONFIG_INVALID_PATTERN 6\n> +\n>  typedef int (*config_fn_t)(const char *, const char *, void *);\n>  extern int git_default_config(const char *, const char *, void *);\n>  extern int git_config_from_file(config_fn_t fn, const char *, void *);\n> diff --git a/config.c b/config.c\n> index 671c8df..9d36848 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1123,12 +1123,12 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n>\n>        if (last_dot == NULL || last_dot == key) {\n>                error(\"key does not contain a section: %s\", key);\n> -               return -2;\n> +               return -CONFIG_NO_SECTION_OR_NAME;\n>        }\n>\n>        if (!last_dot[1]) {\n>                error(\"key does not contain variable name: %s\", key);\n> -               return -2;\n> +               return -CONFIG_NO_SECTION_OR_NAME;\n>        }\n>\n>        baselen = last_dot - key;\n> @@ -1165,7 +1165,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n>\n>  out_free_ret_1:\n>        free(*store_key);\n> -       return -1;\n> +       return -CONFIG_INVALID_KEY;\n>  }\n>\n>  /*\n> @@ -1221,7 +1221,7 @@ int git_config_set_multivar(const char *key, const char *value,\n>        if (fd < 0) {\n>                error(\"could not lock config file %s: %s\", config_filename, strerror(errno));\n>                free(store.key);\n> -               ret = -1;\n> +               ret = CONFIG_NO_LOCK;\n>                goto out_free;\n>        }\n>\n> @@ -1235,12 +1235,12 @@ int git_config_set_multivar(const char *key, const char *value,\n>                if ( ENOENT != errno ) {\n>                        error(\"opening %s: %s\", config_filename,\n>                              strerror(errno));\n> -                       ret = 3; /* same as \"invalid config file\" */\n> +                       ret = CONFIG_INVALID_FILE; /* same as \"invalid config file\" */\n>                        goto out_free;\n>                }\n>                /* if nothing to unset, error out */\n>                if (value == NULL) {\n> -                       ret = 5;\n> +                       ret = CONFIG_NOTHING_SET;\n>                        goto out_free;\n>                }\n>\n> @@ -1268,7 +1268,7 @@ int git_config_set_multivar(const char *key, const char *value,\n>                                        REG_EXTENDED)) {\n>                                error(\"invalid pattern: %s\", value_regex);\n>                                free(store.value_regex);\n> -                               ret = 6;\n> +                               ret = CONFIG_INVALID_PATTERN;\n>                                goto out_free;\n>                        }\n>                }\n> @@ -1290,7 +1290,7 @@ int git_config_set_multivar(const char *key, const char *value,\n>                                regfree(store.value_regex);\n>                                free(store.value_regex);\n>                        }\n> -                       ret = 3;\n> +                       ret = CONFIG_INVALID_FILE;\n>                        goto out_free;\n>                }\n>\n> @@ -1303,7 +1303,7 @@ int git_config_set_multivar(const char *key, const char *value,\n>                /* if nothing to unset, or too many matches, error out */\n>                if ((store.seen == 0 && value == NULL) ||\n>                                (store.seen > 1 && multi_replace == 0)) {\n> -                       ret = 5;\n> +                       ret = CONFIG_NOTHING_SET;\n>                        goto out_free;\n>                }\n>\n> @@ -1364,7 +1364,7 @@ int git_config_set_multivar(const char *key, const char *value,\n>\n>        if (commit_lock_file(lock) < 0) {\n>                error(\"could not commit config file %s\", config_filename);\n> -               ret = 4;\n> +               ret = CONFIG_NO_WRITE;\n>                goto out_free;\n>        }\n>\n> --\n> 1.7.5.1.514.gd181fb\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"168067","messageId":"4DD2990A.8070400@drmicha.warpmail.net","threadId":"27380","inReplyTo":"BANLkTikjY956f7LxXGY5fcLkJJDuMnt6OQ@mail.gmail.com","subject":"Re: [PATCH 1/2] config: define and document exit codes","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-17T15:49:30Z","receivedAt":"2011-05-17T15:49:30Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Thiago Farina venit, vidit, dixit 17.05.2011 17:46:\n> On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber\n> <git@drmicha.warpmail.net> wrote:\n>> The return codes of git_config_set() and friends are magic numbers right\n>> in the source. #define them in cache.h where the functions are declared,\n> \n> Why defining them on cache.h? Just because the functions are declared\n> there? Is this a good reason? I think this pollute even more the\n> cache.h and these constants are not used outside of config.c. So I'd\n> move them back onto config.c. Maybe it's fine as is though.\n\nBecause, potentially, they are used by callers of these external\nfunctions. It's the interface declaration. And in fact, 2/2 creates such\na use.\n\nMichael\n"},{"id":"168115","messageId":"20110518082130.GG27482@sigill.intra.peff.net","threadId":"27380","inReplyTo":"6301052eb455e9088e50f78760e6ca1b9499564a.1305646709.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] config: define and document exit codes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-18T08:21:30Z","receivedAt":"2011-05-18T08:21:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 05:38:52PM +0200, Michael J Gruber wrote:\n\n> The return codes of git_config_set() and friends are magic numbers right\n> in the source. #define them in cache.h where the functions are declared,\n> and use the constants in the source.\n> \n> Also, mention the resulting exit codes of \"git config\" in its man page\n> (and complete the list).\n\nThis version (and the accompanying 2/2) both look sane to me. Thanks for\ntaking time to clean up.\n\n-Peff\n"},{"id":"168143","messageId":"7vfwocc8z5.fsf@alter.siamese.dyndns.org","threadId":"27380","inReplyTo":"6301052eb455e9088e50f78760e6ca1b9499564a.1305646709.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] config: define and document exit codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-18T15:41:02Z","receivedAt":"2011-05-18T15:41:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> +This command will fail (with exit code ret) if:\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),\n> +. you try to use an invalid regexp (ret=6), or\n> +. you use '--global' option without $HOME being properly set (ret=128).\n\nI wonder if you want to sort this in the order of return codes.\n\n> +/* git_config_parse_key() returns these negated: */\n> +#define CONFIG_INVALID_KEY 1\n> +#define CONFIG_NO_SECTION_OR_NAME 2\n> +/* git_config_set(), git_config_set_multivar() return the above or these: */\n> +#define CONFIG_NO_LOCK -1\n> +#define CONFIG_INVALID_FILE 3\n> +#define CONFIG_NO_WRITE 4\n> +#define CONFIG_NOTHING_SET 5\n> +#define CONFIG_INVALID_PATTERN 6\n\nSymbols \"CONFIG_FOO\" looks too much like they are about the feature set\nchosen when compiling git, at least to me. But I do not think of a better\nnaming scheme (\"CONFIG_ERR_FOO\" may be a slight improvement but not good\nenough improvement for the price we pay by having such long symbol names).\n\nBy the way, Are you still interested in \"diff --stat-count\" topic, or have\nyou abandoned it?\n"},{"id":"168151","messageId":"4DD414A6.2060807@drmicha.warpmail.net","threadId":"27380","inReplyTo":"7vfwocc8z5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] config: define and document exit codes","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2011-05-18T18:49:10Z","receivedAt":"2011-05-18T18:49:10Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 18.05.2011 17:41:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>> +This command will fail (with exit code ret) if:\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),\n>> +. you try to use an invalid regexp (ret=6), or\n>> +. you use '--global' option without $HOME being properly set (ret=128).\n> \n> I wonder if you want to sort this in the order of return codes.\n\nWell, if I wanted to... ;)\n\nI don't mind resorting.\n\n>> +/* git_config_parse_key() returns these negated: */\n>> +#define CONFIG_INVALID_KEY 1\n>> +#define CONFIG_NO_SECTION_OR_NAME 2\n>> +/* git_config_set(), git_config_set_multivar() return the above or these: */\n>> +#define CONFIG_NO_LOCK -1\n>> +#define CONFIG_INVALID_FILE 3\n>> +#define CONFIG_NO_WRITE 4\n>> +#define CONFIG_NOTHING_SET 5\n>> +#define CONFIG_INVALID_PATTERN 6\n> \n> Symbols \"CONFIG_FOO\" looks too much like they are about the feature set\n> chosen when compiling git, at least to me. But I do not think of a better\n> naming scheme (\"CONFIG_ERR_FOO\" may be a slight improvement but not good\n> enough improvement for the price we pay by having such long symbol names).\n\nWell, we could do CONFIG_ENOLOCK and such. I didn't find a good sample\nto follow. Thinking more about it, using CONFIG_ENOPARSE would be nice,\nthough I can't decide between 1, 2 or 3 for that...\n\n> By the way, Are you still interested in \"diff --stat-count\" topic, or have\n> you abandoned it?\n\nYes. I've reworked it a bit but there are three goals:\n\n- count right\n- determine the correct maximal file name length\n- don't sacrifice efficiency\n\nThe middle one is problematic even before my patch (that first loop does\nnot drop all items that the latter one drops).\n\nMaybe the third one is no real problem so that I could simply go through\nthe list twice.\n\nOh, and I've checked that fmt-merge-msg does this:\n\n- output at most <count> items\n- if there are more, output \"...\"\n\nSo, there's progress, though not visible yet.\n\nMichael\n"}]}