{"thread":{"id":"41299","subject":"[PATCH v3] config: make git_config_set die on failure","startedAt":"2016-02-01T10:59:54Z","lastAt":"2016-02-01T19:10:33Z","messageCount":2,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"277168","messageId":"1454324394-854-1-git-send-email-ps@pks.im","threadId":"41299","inReplyTo":null,"subject":"[PATCH v3] config: make git_config_set die on failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-02-01T10:59:54Z","receivedAt":"2016-02-01T10:59:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Failure to write the configuration file may be caused by multiple\nconditions, the most common one being the case where the\nconfiguration is locked because of a leftover lock file or\nbecause another process is currently writing to it. We used to\nignore those errors in many cases, possibly leading to\ninconsistently configured repositories. More often than not git\neven pretended that everything was fine and that the settings\nhave been applied when indeed they were not.\n\nHandle these failures by dying by default whenever an error is\noccured by the `git_config_set` family of functions. Introduce a\nnew set of functions `git_config_set_gently` that instead returns\nan error code for the cases where we are required to explicitly\nhandle configuration errors.\n---\n\nIn contrast to the old version ([1]) this version of the patch\nintroduces a set of functions `git_config_set_gently` instead of\n`git_config_set_or_die`, thus going the other route of dying by\ndefault.\n\nIt first seemed that going the default-die route instead of\nor_die-wrappers might prove to be more contained and easier to\ngrasp. Seeing the result, though, I'm not convinced of this\nanymore. This is a minimal compiling example without even caring\nabout test failures, which do occur now.\n\nPreviously in v2 we had a diffstat of +127/-55 lines where we now\nhave +88/-82, which amounts to roughly the same amount of lines\nchanged. The previous approach, though, was much easier to grasp\nin my opinion as nearly all changes could be split up instead of\nrequiring one big change to make it compile. As said before, this\ndoes not yet include the changes required for handling test\nfailures, thus the patch is expected to become bigger rather than\nsmaller.\n\nFurthermore, I do think it's more explicit what the functions are\ndoing when there is a 'or_die' suffix. Without this suffix it may\nbe unexpected that the functions simply abort the program\nwhenever an error occurs.\n\nI'd thus prefer to use the old style used in version 2. I could\ntry to make the second patch ([2]) a little bit smaller by\navoiding all the fuss about passing up the error code only to die\na little later.\n\nOpinions?\n\nPatrick\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/285000\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/285002\n\n builtin/branch.c |  2 +-\n builtin/clone.c  |  2 +-\n builtin/config.c | 28 +++++++++++------------\n builtin/remote.c | 70 ++++++++++++++++++--------------------------------------\n cache.h          | 12 ++++++----\n config.c         | 46 ++++++++++++++++++++++++++++++-------\n submodule.c      | 10 ++++----\n 7 files changed, 88 insertions(+), 82 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3f6c825..461eebb 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -595,7 +595,7 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_stripspace(&buf, 1);\n \n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tstatus = git_config_set(name.buf, buf.len ? buf.buf : NULL);\n+\tgit_config_set(name.buf, buf.len ? buf.buf : NULL);\n \tstrbuf_release(&name);\n \tstrbuf_release(&buf);\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a7c8def..8c01975 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -735,7 +735,7 @@ static int checkout(void)\n \n static int write_one_config(const char *key, const char *value, void *data)\n {\n-\treturn git_config_set_multivar(key, value ? value : \"true\", \"^$\", 0);\n+\treturn git_config_set_multivar_gently(key, value ? value : \"true\", \"^$\", 0);\n }\n \n static void write_config(struct string_list *config)\ndiff --git a/builtin/config.c b/builtin/config.c\nindex adc7727..c26d6e7 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -582,7 +582,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\tret = git_config_set_in_file(given_config_source.file, argv[0], value);\n+\t\tret = git_config_set_in_file_gently(given_config_source.file, 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 --replace-all to change %s.\", argv[0]);\n@@ -592,23 +592,23 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, argv[2], 0);\n+\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t      argv[0], value, argv[2], 0);\n \t}\n \telse if (actions == ACTION_ADD) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value,\n-\t\t\t\t\t\t       CONFIG_REGEX_NONE, 0);\n+\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t      argv[0], value,\n+\t\t\t\t\t\t\t      CONFIG_REGEX_NONE, 0);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], value, argv[2], 1);\n+\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t      argv[0], value, argv[2], 1);\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -634,17 +634,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\tif (argc == 2)\n-\t\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t\t       argv[0], NULL, argv[1], 0);\n+\t\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t\t      argv[0], NULL, argv[1], 0);\n \t\telse\n-\t\t\treturn git_config_set_in_file(given_config_source.file,\n-\t\t\t\t\t\t      argv[0], NULL);\n+\t\t\treturn git_config_set_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t     argv[0], NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t       argv[0], NULL, argv[1], 1);\n+\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n+\t\t\t\t\t\t\t      argv[0], NULL, argv[1], 1);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 2b2ff9b..05d665f 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -108,7 +108,7 @@ enum {\n #define MIRROR_PUSH 2\n #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)\n \n-static int add_branch(const char *key, const char *branchname,\n+static void add_branch(const char *key, const char *branchname,\n \t\tconst char *remotename, int mirror, struct strbuf *tmp)\n {\n \tstrbuf_reset(tmp);\n@@ -119,7 +119,7 @@ static int add_branch(const char *key, const char *branchname,\n \telse\n \t\tstrbuf_addf(tmp, \"refs/heads/%s:refs/remotes/%s/%s\",\n \t\t\t\tbranchname, remotename, branchname);\n-\treturn git_config_set_multivar(key, tmp->buf, \"^$\", 0);\n+\tgit_config_set_multivar(key, tmp->buf, \"^$\", 0);\n }\n \n static const char mirror_advice[] =\n@@ -197,8 +197,7 @@ static int add(int argc, const char **argv)\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n-\tif (git_config_set(buf.buf, url))\n-\t\treturn 1;\n+\tgit_config_set(buf.buf, url);\n \n \tif (!mirror || mirror & MIRROR_FETCH) {\n \t\tstrbuf_reset(&buf);\n@@ -206,25 +205,22 @@ static int add(int argc, const char **argv)\n \t\tif (track.nr == 0)\n \t\t\tstring_list_append(&track, \"*\");\n \t\tfor (i = 0; i < track.nr; i++) {\n-\t\t\tif (add_branch(buf.buf, track.items[i].string,\n-\t\t\t\t       name, mirror, &buf2))\n-\t\t\t\treturn 1;\n+\t\t\tadd_branch(buf.buf, track.items[i].string,\n+\t\t\t\t   name, mirror, &buf2);\n \t\t}\n \t}\n \n \tif (mirror & MIRROR_PUSH) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.mirror\", name);\n-\t\tif (git_config_set(buf.buf, \"true\"))\n-\t\t\treturn 1;\n+\t\tgit_config_set(buf.buf, \"true\");\n \t}\n \n \tif (fetch_tags != TAGS_DEFAULT) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.tagopt\", name);\n-\t\tif (git_config_set(buf.buf,\n-\t\t\tfetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\"))\n-\t\t\treturn 1;\n+\t\tgit_config_set(buf.buf,\n+\t\t\t       fetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\");\n \t}\n \n \tif (fetch && fetch_remote(name))\n@@ -592,21 +588,15 @@ static int migrate_file(struct remote *remote)\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n \tfor (i = 0; i < remote->url_nr; i++)\n-\t\tif (git_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0))\n-\t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n-\t\t\t\t\tremote->url[i], buf.buf);\n+\t\tgit_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n \tfor (i = 0; i < remote->push_refspec_nr; i++)\n-\t\tif (git_config_set_multivar(buf.buf, remote->push_refspec[i], \"^$\", 0))\n-\t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n-\t\t\t\t\tremote->push_refspec[i], buf.buf);\n+\t\tgit_config_set_multivar(buf.buf, remote->push_refspec[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n \tfor (i = 0; i < remote->fetch_refspec_nr; i++)\n-\t\tif (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], \"^$\", 0))\n-\t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n-\t\t\t\t\tremote->fetch_refspec[i], buf.buf);\n+\t\tgit_config_set_multivar(buf.buf, remote->fetch_refspec[i], \"^$\", 0);\n \tif (remote->origin == REMOTE_REMOTES)\n \t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n \telse if (remote->origin == REMOTE_BRANCHES)\n@@ -657,8 +647,7 @@ static int mv(int argc, const char **argv)\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new);\n-\tif (git_config_set_multivar(buf.buf, NULL, NULL, 1))\n-\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n+\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n \tfor (i = 0; i < oldremote->fetch_refspec_nr; i++) {\n \t\tchar *ptr;\n@@ -678,8 +667,7 @@ static int mv(int argc, const char **argv)\n \t\t\t\t  \"\\tPlease update the configuration manually if necessary.\"),\n \t\t\t\tbuf2.buf);\n \n-\t\tif (git_config_set_multivar(buf.buf, buf2.buf, \"^$\", 0))\n-\t\t\treturn error(_(\"Could not append '%s'\"), buf.buf);\n+\t\tgit_config_set_multivar(buf.buf, buf2.buf, \"^$\", 0);\n \t}\n \n \tread_branches();\n@@ -689,9 +677,7 @@ static int mv(int argc, const char **argv)\n \t\tif (info->remote_name && !strcmp(info->remote_name, rename.old)) {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addf(&buf, \"branch.%s.remote\", item->string);\n-\t\t\tif (git_config_set(buf.buf, rename.new)) {\n-\t\t\t\treturn error(_(\"Could not set '%s'\"), buf.buf);\n-\t\t\t}\n+\t\t\tgit_config_set(buf.buf, rename.new);\n \t\t}\n \t}\n \n@@ -789,10 +775,7 @@ static int rm(int argc, const char **argv)\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n-\t\t\t\tif (git_config_set(buf.buf, NULL)) {\n-\t\t\t\t\tstrbuf_release(&buf);\n-\t\t\t\t\treturn -1;\n-\t\t\t\t}\n+\t\t\t\tgit_config_set(buf.buf, NULL);\n \t\t\t}\n \t\t}\n \t}\n@@ -1411,12 +1394,12 @@ static int update(int argc, const char **argv)\n \treturn retval;\n }\n \n-static int remove_all_fetch_refspecs(const char *remote, const char *key)\n+static void remove_all_fetch_refspecs(const char *remote, const char *key)\n {\n-\treturn git_config_set_multivar(key, NULL, NULL, 1);\n+\tgit_config_set_multivar(key, NULL, NULL, 1);\n }\n \n-static int add_branches(struct remote *remote, const char **branches,\n+static void add_branches(struct remote *remote, const char **branches,\n \t\t\tconst char *key)\n {\n \tconst char *remotename = remote->name;\n@@ -1424,13 +1407,9 @@ static int add_branches(struct remote *remote, const char **branches,\n \tstruct strbuf refspec = STRBUF_INIT;\n \n \tfor (; *branches; branches++)\n-\t\tif (add_branch(key, *branches, remotename, mirror, &refspec)) {\n-\t\t\tstrbuf_release(&refspec);\n-\t\t\treturn 1;\n-\t\t}\n+\t\tadd_branch(key, *branches, remotename, mirror, &refspec);\n \n \tstrbuf_release(&refspec);\n-\treturn 0;\n }\n \n static int set_remote_branches(const char *remotename, const char **branches,\n@@ -1445,14 +1424,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n \n-\tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n-\t\tstrbuf_release(&key);\n-\t\treturn 1;\n-\t}\n-\tif (add_branches(remote, branches, key.buf)) {\n-\t\tstrbuf_release(&key);\n-\t\treturn 1;\n-\t}\n+\tif (!add_mode)\n+\t\tremove_all_fetch_refspecs(remotename, key.buf);\n+\tadd_branches(remote, branches, key.buf);\n \n \tstrbuf_release(&key);\n \treturn 0;\ndiff --git a/cache.h b/cache.h\nindex dfc459c..4841f2c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1507,12 +1507,16 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_maybe_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n-extern int git_config_set_in_file(const char *, const char *, const char *);\n-extern int git_config_set(const char *, const char *);\n+extern void git_config_set_in_file(const char *, const char *, const char *);\n+extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n+extern void git_config_set(const char *, const char *);\n+extern int git_config_set_gently(const char *, const char *);\n extern int git_config_parse_key(const char *, char **, int *);\n extern int git_config_key_is_valid(const char *key);\n-extern int git_config_set_multivar(const char *, const char *, const char *, int);\n-extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n+extern void git_config_set_multivar(const char *, const char *, const char *, int);\n+extern int git_config_set_multivar_gently(const char *, const char *, const char *, int);\n+extern void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n+extern int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\n extern int git_config_rename_section_in_file(const char *, const char *, const char *);\n extern const char *git_etc_gitconfig(void);\ndiff --git a/config.c b/config.c\nindex 86a5eb2..b7dbd2b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1825,15 +1825,27 @@ contline:\n \treturn offset;\n }\n \n-int git_config_set_in_file(const char *config_filename,\n+void git_config_set_in_file(const char *config_filename,\n \t\t\tconst char *key, const char *value)\n {\n-\treturn git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);\n+\tgit_config_set_multivar_in_file(config_filename, key, value, NULL, 0);\n }\n \n-int git_config_set(const char *key, const char *value)\n+int git_config_set_in_file_gently(const char *config_filename,\n+\t\t\tconst char *key, const char *value)\n+{\n+\treturn git_config_set_multivar_in_file_gently(config_filename,\n+\t\t\t\t\t\t      key, value, NULL, 0);\n+}\n+\n+void git_config_set(const char *key, const char *value)\n+{\n+\tgit_config_set_multivar(key, value, NULL, 0);\n+}\n+\n+int git_config_set_gently(const char *key, const char *value)\n {\n-\treturn git_config_set_multivar(key, value, NULL, 0);\n+\treturn git_config_set_multivar_gently(key, value, NULL, 0);\n }\n \n /*\n@@ -1925,6 +1937,17 @@ int git_config_key_is_valid(const char *key)\n \treturn !git_config_parse_key_1(key, NULL, NULL, 1);\n }\n \n+void git_config_set_multivar_in_file(const char *config_filename,\n+\t\t\t\tconst char *key, const char *value,\n+\t\t\t\tconst char *value_regex, int multi_replace)\n+{\n+\tif (git_config_set_multivar_in_file_gently(config_filename,\n+\t\t\t\t\t\t   key, value,\n+\t\t\t\t\t\t   value_regex, multi_replace))\n+\t\tdie(_(\"Could not set configuration key '%s' to '%s'\"),\n+\t\t    key, value);\n+}\n+\n /*\n  * If value==NULL, unset in (remove from) config,\n  * if value_regex!=NULL, disregard key/value pairs where value does not match.\n@@ -1950,7 +1973,7 @@ int git_config_key_is_valid(const char *key)\n  * - the config file is removed and the lock file rename()d to it.\n  *\n  */\n-int git_config_set_multivar_in_file(const char *config_filename,\n+int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\tconst char *key, const char *value,\n \t\t\t\tconst char *value_regex, int multi_replace)\n {\n@@ -2179,11 +2202,18 @@ write_err_out:\n \n }\n \n-int git_config_set_multivar(const char *key, const char *value,\n+void git_config_set_multivar(const char *key, const char *value,\n \t\t\tconst char *value_regex, int multi_replace)\n {\n-\treturn git_config_set_multivar_in_file(NULL, key, value, value_regex,\n-\t\t\t\t\t       multi_replace);\n+\tgit_config_set_multivar_in_file(NULL, key, value, value_regex,\n+\t\t\t\t\tmulti_replace);\n+}\n+\n+int git_config_set_multivar_gently(const char *key, const char *value,\n+\t\t\tconst char *value_regex, int multi_replace)\n+{\n+\treturn git_config_set_multivar_in_file_gently(NULL, key, value,\n+\t\t\t\t\t\t      value_regex, multi_replace);\n }\n \n static int section_name_match (const char *buf, const char *name)\ndiff --git a/submodule.c b/submodule.c\nindex b83939c..bc977d1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -69,7 +69,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \tstrbuf_addstr(&entry, \"submodule.\");\n \tstrbuf_addstr(&entry, submodule->name);\n \tstrbuf_addstr(&entry, \".path\");\n-\tif (git_config_set_in_file(\".gitmodules\", entry.buf, newpath) < 0) {\n+\tif (git_config_set_in_file_gently(\".gitmodules\", entry.buf, newpath) < 0) {\n \t\t/* Maybe the user already did that, don't error out here */\n \t\twarning(_(\"Could not update .gitmodules entry %s\"), entry.buf);\n \t\tstrbuf_release(&entry);\n@@ -1087,11 +1087,9 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \t/* Update core.worktree setting */\n \tstrbuf_reset(&file_name);\n \tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n-\tif (git_config_set_in_file(file_name.buf, \"core.worktree\",\n-\t\t\t\t   relative_path(real_work_tree, git_dir,\n-\t\t\t\t\t\t &rel_path)))\n-\t\tdie(_(\"Could not set core.worktree in %s\"),\n-\t\t    file_name.buf);\n+\tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n+\t\t\t        relative_path(real_work_tree, git_dir,\n+\t\t\t\t\t      &rel_path));\n \n \tstrbuf_release(&file_name);\n \tstrbuf_release(&rel_path);\n-- \n2.7.0\n"},{"id":"277179","messageId":"xmqqzivkutzq.fsf@gitster.mtv.corp.google.com","threadId":"41299","inReplyTo":"1454324394-854-1-git-send-email-ps@pks.im","subject":"Re: [PATCH v3] config: make git_config_set die on failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T19:10:33Z","receivedAt":"2016-02-01T19:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Furthermore, I do think it's more explicit what the functions are\n> doing when there is a 'or_die' suffix. Without this suffix it may\n> be unexpected that the functions simply abort the program\n> whenever an error occurs.\n\nThat largely depends on what you are used to see.  Many internal API\nfunctions do \"if we cannot do this, die\" without or_die suffix.\n\nAs the endgame state, I'd find it far more preferrable to see a\nfunction that dies without _or_die(), while allowing outlier callers\nto call the _gently() variant to do their own clean-up [*1*].\n\nHow we get to that endgame is a different matter.  It is a viable\napproach like you did in the original series to first temporarily\nintroduce _or_die() and convert the current callers in small chunks.\nThe second step would be to rename git_config_set() and friends that\ndo not die to hae _gently() suffix, and update the remaining callers\nto call them.  At that point, nobody calls git_config_set(), so we\ncan drop the _or_die() suffix, which would lead us to the endgame\nstate.\n\nBut because we are talking about only a very small number of\ncallers, I think either is OK.\n\nThanks.\n\n\n[Footnote]\n\n*1* Another consideration while talking about a transition like this\n    is what would happen in topics in flight (either in my tree\n    above 'next' or people privately are working on).  New callsites\n    they added expecting git_config_set() to return error would have\n    to be adjusted to call _gently() variants, and we need to catch\n    such sites somehow.  In this partcular transision, it can easily\n    be done and you've done so already by making the functions\n    return no value, so anybody who checked their return values\n    would be flagged by the compiler.\n"}]}