{"thread":{"id":"41272","subject":"[PATCH v2 5/9] branch: handle config errors when unsetting upstream","startedAt":"2016-01-28T09:00:28Z","lastAt":"2016-01-30T16:03:42Z","messageCount":13,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":9},"messages":[{"id":"293764","messageId":"1453971637-22273-1-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":null,"subject":"[PATCH v2 0/9] Handle errors when setting configs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:28Z","receivedAt":"2016-01-28T09:00:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"I've finally got around to producing version two of my previous\npatch to handle errors when setting configs. Back in September\nI've posted a single patch to handle errors when\n`install_branch_config` fails due to configuration failures [1].\n\nFailure to write the configuration file may be caused by multiple\nconditions, but the most common one will surely be the case where\nthe configuration 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\nVersion two of this patch is somewhat more involved in that I\ntried to track down all relevant places where we set configs\nwithout checking for error conditions. My current approach to\nmost of those cases is to just die with an error message, this\nremains up to discussion though for the individual cases.\n\n[1]: $gmane/278544\n\nPatrick Steinhardt (9):\n  config: introduce set_or_die wrappers\n  branch: return error code for install_branch_config\n  remote: handle config errors in set_url\n  clone: handle config errors in cmd_clone\n  branch: handle config errors when unsetting upstream\n  init-db: handle config errors\n  sequencer: handle config errors when saving opts\n  submodule--helper: handle config errors\n  compat: die when unable to set core.precomposeunicode\n\n branch.c                    | 31 +++++++++++++++++++++----------\n branch.h                    |  3 ++-\n builtin/branch.c            |  4 ++--\n builtin/clone.c             | 17 ++++++++++-------\n builtin/init-db.c           | 20 ++++++++++----------\n builtin/remote.c            | 11 ++++++-----\n builtin/submodule--helper.c |  4 ++--\n cache.h                     |  4 ++++\n compat/precompose_utf8.c    |  3 ++-\n config.c                    | 27 +++++++++++++++++++++++++++\n sequencer.c                 | 22 +++++++++++-----------\n t/t3200-branch.sh           | 16 +++++++++++++++-\n t/t5505-remote.sh           |  9 +++++++++\n transport.c                 | 11 ++++++-----\n 14 files changed, 127 insertions(+), 55 deletions(-)\n\n-- \n2.7.0\n"},{"id":"276976","messageId":"1453971637-22273-2-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 1/9] config: introduce set_or_die wrappers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:29Z","receivedAt":"2016-01-28T09:00:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"A lot of call-sites for the existing family of `git_config_set`\nfunctions do not check for errors that may occur, e.g. the\nconfiguration file being locked. In many cases we simply want to\ndie when such a situation arises.\n\nIntroduce wrappers that will cause the program to die in those\ncases.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n cache.h  |  4 ++++\n config.c | 27 +++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex dfc459c..c1f844d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1508,11 +1508,15 @@ 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 void git_config_set_in_file_or_die(const char *, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n+extern void git_config_set_or_die(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 void git_config_set_multivar_or_die(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_in_file_or_die(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..856f7d34 100644\n--- a/config.c\n+++ b/config.c\n@@ -1831,11 +1831,22 @@ int git_config_set_in_file(const char *config_filename,\n \treturn git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);\n }\n \n+void git_config_set_in_file_or_die(const char *config_filename,\n+\t\t\tconst char *key, const char *value)\n+{\n+\tgit_config_set_multivar_in_file_or_die(config_filename, key, value, NULL, 0);\n+}\n+\n int git_config_set(const char *key, const char *value)\n {\n \treturn git_config_set_multivar(key, value, NULL, 0);\n }\n \n+void git_config_set_or_die(const char *key, const char *value)\n+{\n+\tgit_config_set_multivar_or_die(key, value, NULL, 0);\n+}\n+\n /*\n  * Auxiliary function to sanity-check and split the key into the section\n  * identifier and variable name.\n@@ -2179,6 +2190,15 @@ write_err_out:\n \n }\n \n+void git_config_set_multivar_in_file_or_die(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(config_filename, key, value,\n+\t\t\t\t\t    value_regex, multi_replace) < 0)\n+\t\tdie(_(\"Could not set '%s' to '%s'\"), key, value);\n+}\n+\n int git_config_set_multivar(const char *key, const char *value,\n \t\t\tconst char *value_regex, int multi_replace)\n {\n@@ -2186,6 +2206,13 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t\t\t\t       multi_replace);\n }\n \n+void git_config_set_multivar_or_die(const char *key, const char *value,\n+\t\t\tconst char *value_regex, int multi_replace)\n+{\n+\tgit_config_set_multivar_in_file_or_die(NULL, key, value, value_regex,\n+\t\t\t\t\t       multi_replace);\n+}\n+\n static int section_name_match (const char *buf, const char *name)\n {\n \tint i = 0, j = 0, dot = 0;\n-- \n2.7.0\n"},{"id":"276978","messageId":"1453971637-22273-3-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 2/9] branch: return error code for install_branch_config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:30Z","receivedAt":"2016-01-28T09:00:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The install_branch_config function may fail to write the\nconfiguration file due to locking. To accomodate for this\nscenario, introduce a return value which may be used to check if\nthe function did finish correctly and adjust callers to use this\nerror code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n branch.c          | 31 +++++++++++++++++++++----------\n branch.h          |  3 ++-\n builtin/clone.c   |  9 ++++++---\n t/t3200-branch.sh |  9 ++++++++-\n transport.c       | 11 ++++++-----\n 5 files changed, 43 insertions(+), 20 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 7ff3f20..7f35bcf 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -49,33 +49,38 @@ static int should_setup_rebase(const char *origin)\n \treturn 0;\n }\n \n-void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n+int install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n \tconst char *shortname = NULL;\n \tstruct strbuf key = STRBUF_INIT;\n-\tint rebasing = should_setup_rebase(origin);\n+\tint ret = 0, rebasing = should_setup_rebase(origin);\n \n \tif (skip_prefix(remote, \"refs/heads/\", &shortname)\n \t    && !strcmp(local, shortname)\n \t    && !origin) {\n \t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n \t\t\tlocal);\n-\t\treturn;\n+\t\treturn ret;\n \t}\n \n \tstrbuf_addf(&key, \"branch.%s.remote\", local);\n-\tgit_config_set(key.buf, origin ? origin : \".\");\n+\tret = git_config_set(key.buf, origin ? origin : \".\");\n+\tif (ret)\n+\t\tgoto out;\n \n \tstrbuf_reset(&key);\n \tstrbuf_addf(&key, \"branch.%s.merge\", local);\n-\tgit_config_set(key.buf, remote);\n+\tret = git_config_set(key.buf, remote);\n+\tif (ret)\n+\t\tgoto out;\n \n \tif (rebasing) {\n \t\tstrbuf_reset(&key);\n \t\tstrbuf_addf(&key, \"branch.%s.rebase\", local);\n-\t\tgit_config_set(key.buf, \"true\");\n+\t\tret = git_config_set(key.buf, \"true\");\n+\t\tif (ret)\n+\t\t\tgoto out;\n \t}\n-\tstrbuf_release(&key);\n \n \tif (flag & BRANCH_CONFIG_VERBOSE) {\n \t\tif (shortname) {\n@@ -102,6 +107,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t\t\t\t\t  local, remote);\n \t\t}\n \t}\n+\n+out:\n+\tstrbuf_release(&key);\n+\treturn ret;\n }\n \n /*\n@@ -134,8 +143,9 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \t\treturn error(_(\"Not tracking: ambiguous information for ref %s\"),\n \t\t\t\torig_ref);\n \n-\tinstall_branch_config(config_flags, new_ref, tracking.remote,\n-\t\t\t      tracking.src ? tracking.src : orig_ref);\n+\tif (install_branch_config(config_flags, new_ref, tracking.remote,\n+\t\t\t\t  tracking.src ? tracking.src : orig_ref))\n+\t\treturn error(_(\"Could not install branch configuration\"));\n \n \tfree(tracking.src);\n \treturn 0;\n@@ -295,7 +305,8 @@ void create_branch(const char *head,\n \t}\n \n \tif (real_ref && track)\n-\t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n+\t\tif (setup_tracking(ref.buf + 11, real_ref, track, quiet) < 0)\n+\t\t\tdie(_(\"Could not setup tracking branch\"));\n \n \tstrbuf_release(&ref);\n \tfree(real_ref);\ndiff --git a/branch.h b/branch.h\nindex 58aa45f..78ad438 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -43,9 +43,10 @@ void remove_branch_state(void);\n /*\n  * Configure local branch \"local\" as downstream to branch \"remote\"\n  * from remote \"origin\".  Used by git branch --set-upstream.\n+ * Returns 0 on success.\n  */\n #define BRANCH_CONFIG_VERBOSE 01\n-extern void install_branch_config(int flag, const char *local, const char *origin, const char *remote);\n+extern int install_branch_config(int flag, const char *local, const char *origin, const char *remote);\n \n /*\n  * Read branch description\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a7c8def..8b11650 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -648,6 +648,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n \tconst char *head;\n+\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n \t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n@@ -655,7 +656,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_oid.hash, NULL, 0,\n \t\t\t\t   UPDATE_REFS_DIE_ON_ERR);\n-\t\t\tinstall_branch_config(0, head, option_origin, our->name);\n+\t\t\tif (install_branch_config(0, head, option_origin, our->name))\n+\t\t\t\tdie(_(\"Could not update '%s'\"), head);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(our->old_oid.hash);\n@@ -1054,8 +1056,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tremote_head = NULL;\n \t\toption_no_checkout = 1;\n \t\tif (!option_bare)\n-\t\t\tinstall_branch_config(0, \"master\", option_origin,\n-\t\t\t\t\t      \"refs/heads/master\");\n+\t\t\tif (install_branch_config(0, \"master\", option_origin,\n+\t\t\t\t\t\t  \"refs/heads/master\"))\n+\t\t\t\tdie(_(\"Could not set up master branch\"));\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cdaf6f6..dd776b3 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -446,6 +446,13 @@ test_expect_success '--set-upstream-to fails on a non-ref' '\n \ttest_must_fail git branch --set-upstream-to HEAD^{}\n '\n \n+test_expect_success '--set-upstream-to fails on locked config' '\n+\ttest_when_finished \"rm -f .git/config.lock\" &&\n+\t>.git/config.lock &&\n+\tgit branch locked &&\n+\ttest_must_fail git branch --set-upstream-to locked\n+'\n+\n test_expect_success 'use --set-upstream-to modify HEAD' '\n \ttest_config branch.master.remote foo &&\n \ttest_config branch.master.merge foo &&\n@@ -579,7 +586,7 @@ test_expect_success 'avoid ambiguous track' '\n \tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&\n \tgit config remote.ambi2.url lilili &&\n \tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/master &&\n-\tgit branch all1 master &&\n+\ttest_must_fail git branch all1 master &&\n \ttest -z \"$(git config branch.all1.merge)\"\n '\n \ndiff --git a/transport.c b/transport.c\nindex 67f3666..f7ad752 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -181,11 +181,12 @@ static void set_upstreams(struct transport *transport, struct ref *refs,\n \t\tif (!remotename || !starts_with(remotename, \"refs/heads/\"))\n \t\t\tcontinue;\n \n-\t\tif (!pretend)\n-\t\t\tinstall_branch_config(BRANCH_CONFIG_VERBOSE,\n-\t\t\t\tlocalname + 11, transport->remote->name,\n-\t\t\t\tremotename);\n-\t\telse\n+\t\tif (!pretend) {\n+\t\t\tif (install_branch_config(BRANCH_CONFIG_VERBOSE,\n+\t\t\t\t    localname + 11, transport->remote->name,\n+\t\t\t\t    remotename))\n+\t\t\t\tdie(_(\"Could not set upstream\"));\n+\t\t} else\n \t\t\tprintf(\"Would set upstream of '%s' to '%s' of '%s'\\n\",\n \t\t\t\tlocalname + 11, remotename + 11,\n \t\t\t\ttransport->remote->name);\n-- \n2.7.0\n"},{"id":"276971","messageId":"1453971637-22273-4-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 3/9] remote: handle config errors in set_url","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:31Z","receivedAt":"2016-01-28T09:00:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The return codes for function calls that write the new URL\nconfiguration are not being checked in `set_url`. As the function\nis exposed to the user directly through `git remote set-url` we\nwant to inform her that we were not able to complete the request\nand abort the program.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/remote.c  | 11 ++++++-----\n t/t5505-remote.sh |  9 +++++++++\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 2b2ff9b..8b78c3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1583,11 +1583,12 @@ static int set_url(int argc, const char **argv)\n \t/* Special cases that add new entry. */\n \tif ((!oldurl && !delete_mode) || add_mode) {\n \t\tif (add_mode)\n-\t\t\tgit_config_set_multivar(name_buf.buf, newurl,\n-\t\t\t\t\"^$\", 0);\n+\t\t\tgit_config_set_multivar_or_die(name_buf.buf, newurl,\n+\t\t\t\t\t\t       \"^$\", 0);\n \t\telse\n-\t\t\tgit_config_set(name_buf.buf, newurl);\n+\t\t\tgit_config_set_or_die(name_buf.buf, newurl);\n \t\tstrbuf_release(&name_buf);\n+\n \t\treturn 0;\n \t}\n \n@@ -1608,9 +1609,9 @@ static int set_url(int argc, const char **argv)\n \tregfree(&old_regex);\n \n \tif (!delete_mode)\n-\t\tgit_config_set_multivar(name_buf.buf, newurl, oldurl, 0);\n+\t\tgit_config_set_multivar_or_die(name_buf.buf, newurl, oldurl, 0);\n \telse\n-\t\tgit_config_set_multivar(name_buf.buf, NULL, oldurl, 1);\n+\t\tgit_config_set_multivar_or_die(name_buf.buf, NULL, oldurl, 1);\n \treturn 0;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1a8e3b8..e019f27 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -932,6 +932,15 @@ test_expect_success 'get-url on new remote' '\n \techo foo | get_url_test --push --all someremote\n '\n \n+test_expect_success 'remote set-url with locked config' '\n+\ttest_when_finished \"rm -f .git/config.lock\" &&\n+\tgit config --get-all remote.someremote.url >expect &&\n+\t>.git/config.lock &&\n+\ttest_must_fail git remote set-url someremote baz &&\n+\tgit config --get-all remote.someremote.url >actual &&\n+\tcmp expect actual\n+'\n+\n test_expect_success 'remote set-url bar' '\n \tgit remote set-url someremote bar &&\n \techo bar >expect &&\n-- \n2.7.0\n"},{"id":"276977","messageId":"1453971637-22273-5-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 4/9] clone: handle config errors in cmd_clone","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:32Z","receivedAt":"2016-01-28T09:00:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The clone command does not check for error codes returned by\n`git_config_set` functions. This may cause the user to end up\nwith an inconsistent repository without any indication with what\nwent wrong.\n\nFix this problem by dying with an error message when we are\nunable to write the configuration files to disk.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/clone.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 8b11650..5f6f995 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -788,12 +788,12 @@ static void write_refspec_config(const char *src_ref_prefix,\n \t\t/* Configure the remote */\n \t\tif (value.len) {\n \t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n-\t\t\tgit_config_set_multivar(key.buf, value.buf, \"^$\", 0);\n+\t\t\tgit_config_set_multivar_or_die(key.buf, value.buf, \"^$\", 0);\n \t\t\tstrbuf_reset(&key);\n \n \t\t\tif (option_mirror) {\n \t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", option_origin);\n-\t\t\t\tgit_config_set(key.buf, \"true\");\n+\t\t\t\tgit_config_set_or_die(key.buf, \"true\");\n \t\t\t\tstrbuf_reset(&key);\n \t\t\t}\n \t\t}\n@@ -951,14 +951,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tsrc_ref_prefix = \"refs/\";\n \t\tstrbuf_addstr(&branch_top, src_ref_prefix);\n \n-\t\tgit_config_set(\"core.bare\", \"true\");\n+\t\tgit_config_set_or_die(\"core.bare\", \"true\");\n \t} else {\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n \tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n-\tgit_config_set(key.buf, repo);\n+\tgit_config_set_or_die(key.buf, repo);\n \tstrbuf_reset(&key);\n \n \tif (option_reference.nr)\n-- \n2.7.0\n"},{"id":"276970","messageId":"1453971637-22273-6-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 5/9] branch: handle config errors when unsetting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:33Z","receivedAt":"2016-01-28T09:00:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we try to unset upstream configurations we do not check\nreturn codes for the `git_config_set` functions. As those may\nindicate that we were unable to unset the respective\nconfiguration we may exit successfully without any error message\nwhile in fact the upstream configuration was not unset.\n\nFix this by dying with an error message when we cannot unset the\nconfiguration.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/branch.c  | 4 ++--\n t/t3200-branch.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3f6c825..0978287 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,10 +791,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n-\t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n+\t\tgit_config_set_multivar_or_die(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.merge\", branch->name);\n-\t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n+\t\tgit_config_set_multivar_or_die(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n \t\tstruct branch *branch = branch_get(argv[0]);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd776b3..a897248 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -473,6 +473,13 @@ test_expect_success '--unset-upstream should fail if given a non-existent branch\n \ttest_must_fail git branch --unset-upstream i-dont-exist\n '\n \n+test_expect_success '--unset-upstream should fail if config is locked' '\n+\ttest_when_finished \"rm -f .git/config.lock\" &&\n+\tgit branch --set-upstream-to locked &&\n+\t>.git/config.lock &&\n+\ttest_must_fail git branch --unset-upstream\n+'\n+\n test_expect_success 'test --unset-upstream on HEAD' '\n \tgit branch my14 &&\n \ttest_config branch.master.remote foo &&\n-- \n2.7.0\n"},{"id":"276973","messageId":"1453971637-22273-7-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 6/9] init-db: handle config errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:34Z","receivedAt":"2016-01-28T09:00:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating default files we do not check for error codes\nreturned by `git_config_set` functions. This may cause the user\nto end up with an inconsistent repository without any indication\nfor the user.\n\nFix this problem by dying with an error message when we are\nunable to write the configuration files to disk.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/init-db.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 07229d6..ef19048 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -227,7 +227,7 @@ static int create_default_files(const char *template_path)\n \t/* This forces creation of new config file */\n \txsnprintf(repo_version_string, sizeof(repo_version_string),\n \t\t  \"%d\", GIT_REPO_VERSION);\n-\tgit_config_set(\"core.repositoryformatversion\", repo_version_string);\n+\tgit_config_set_or_die(\"core.repositoryformatversion\", repo_version_string);\n \n \t/* Check filemode trustability */\n \tpath = git_path_buf(&buf, \"config\");\n@@ -241,18 +241,18 @@ static int create_default_files(const char *template_path)\n \t\tif (filemode && !reinit && (st1.st_mode & S_IXUSR))\n \t\t\tfilemode = 0;\n \t}\n-\tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n+\tgit_config_set_or_die(\"core.filemode\", filemode ? \"true\" : \"false\");\n \n \tif (is_bare_repository())\n-\t\tgit_config_set(\"core.bare\", \"true\");\n+\t\tgit_config_set_or_die(\"core.bare\", \"true\");\n \telse {\n \t\tconst char *work_tree = get_git_work_tree();\n-\t\tgit_config_set(\"core.bare\", \"false\");\n+\t\tgit_config_set_or_die(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n \t\tif (log_all_ref_updates == -1)\n-\t\t    git_config_set(\"core.logallrefupdates\", \"true\");\n+\t\t\tgit_config_set_or_die(\"core.logallrefupdates\", \"true\");\n \t\tif (needs_work_tree_config(get_git_dir(), work_tree))\n-\t\t\tgit_config_set(\"core.worktree\", work_tree);\n+\t\t\tgit_config_set_or_die(\"core.worktree\", work_tree);\n \t}\n \n \tif (!reinit) {\n@@ -265,12 +265,12 @@ static int create_default_files(const char *template_path)\n \t\t    S_ISLNK(st1.st_mode))\n \t\t\tunlink(path); /* good */\n \t\telse\n-\t\t\tgit_config_set(\"core.symlinks\", \"false\");\n+\t\t\tgit_config_set_or_die(\"core.symlinks\", \"false\");\n \n \t\t/* Check if the filesystem is case-insensitive */\n \t\tpath = git_path_buf(&buf, \"CoNfIg\");\n \t\tif (!access(path, F_OK))\n-\t\t\tgit_config_set(\"core.ignorecase\", \"true\");\n+\t\t\tgit_config_set_or_die(\"core.ignorecase\", \"true\");\n \t\tprobe_utf8_pathname_composition();\n \t}\n \n@@ -386,8 +386,8 @@ int init_db(const char *template_dir, unsigned int flags)\n \t\t\txsnprintf(buf, sizeof(buf), \"%d\", OLD_PERM_EVERYBODY);\n \t\telse\n \t\t\tdie(\"BUG: invalid value for shared_repository\");\n-\t\tgit_config_set(\"core.sharedrepository\", buf);\n-\t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n+\t\tgit_config_set_or_die(\"core.sharedrepository\", buf);\n+\t\tgit_config_set_or_die(\"receive.denyNonFastforwards\", \"true\");\n \t}\n \n \tif (!(flags & INIT_DB_QUIET)) {\n-- \n2.7.0\n"},{"id":"276972","messageId":"1453971637-22273-8-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 7/9] sequencer: handle config errors when saving opts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:35Z","receivedAt":"2016-01-28T09:00:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When we start picking a range of revisions we save the replay\noptions that are required to restore state when interrupting and\nlater continuing picking the revisions. However, we do not check\nthe return values of the `git_config_set` functions, which may\nlead us to store incomplete information. As this may lead us to\nfail when trying to continue the sequence this error can be\nfatal.\n\nFix this by dying immediately when we are unable to write back\nany replay option.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n sequencer.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8c58fa2..3c109b6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -933,31 +933,31 @@ static void save_opts(struct replay_opts *opts)\n \tconst char *opts_file = git_path_opts_file();\n \n \tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.no-commit\", \"true\");\n \tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.edit\", \"true\");\n \tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.signoff\", \"true\");\n \tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.record-origin\", \"true\");\n \tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.allow-ff\", \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.mainline\", buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.strategy\", opts->strategy);\n \tif (opts->gpg_sign)\n-\t\tgit_config_set_in_file(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n+\t\tgit_config_set_in_file_or_die(opts_file, \"options.gpg-sign\", opts->gpg_sign);\n \tif (opts->xopts) {\n \t\tint i;\n \t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n-\t\t\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n+\t\t\tgit_config_set_multivar_in_file_or_die(opts_file,\n+\t\t\t\t\t\t\t       \"options.strategy-option\",\n+\t\t\t\t\t\t\t       opts->xopts[i], \"^$\", 0);\n \t}\n }\n \n-- \n2.7.0\n"},{"id":"276974","messageId":"1453971637-22273-9-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 8/9] submodule--helper: handle config errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:36Z","receivedAt":"2016-01-28T09:00:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When setting the 'core.worktree' option for a newly cloned\nsubmodule we ignore the return value of `git_config_set_in_file`.\nInstead, we want to inform the user that something has gone wrong\nby printing an error message and aborting the program.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..c7e1ea2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -245,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tp = git_pathdup_submodule(path, \"config\");\n \tif (!p)\n \t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n-\tgit_config_set_in_file(p, \"core.worktree\",\n-\t\t\t       relative_path(sb.buf, sm_gitdir, &rel_path));\n+\tgit_config_set_in_file_or_die(p, \"core.worktree\",\n+\t\t\t\t      relative_path(sb.buf, sm_gitdir, &rel_path));\n \tstrbuf_release(&sb);\n \tstrbuf_release(&rel_path);\n \tfree(sm_gitdir);\n-- \n2.7.0\n"},{"id":"276975","messageId":"1453971637-22273-10-git-send-email-ps@pks.im","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"[PATCH v2 9/9] compat: die when unable to set core.precomposeunicode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-28T09:00:37Z","receivedAt":"2016-01-28T09:00:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When calling `git_config_set` to set 'core.precomposeunicode' we\nignore the return value of the function, which may indicate that\nwe were unable to write the value back to disk.\n\nAs surrounding code is already dying when an error occurs we\nsimply die and print an error message when we are unable to write\nthe config value.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n compat/precompose_utf8.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c\nindex 079070f..9ff1ebe 100644\n--- a/compat/precompose_utf8.c\n+++ b/compat/precompose_utf8.c\n@@ -50,7 +50,8 @@ void probe_utf8_pathname_composition(void)\n \t\tclose(output_fd);\n \t\tgit_path_buf(&path, \"%s\", auml_nfd);\n \t\tprecomposed_unicode = access(path.buf, R_OK) ? 0 : 1;\n-\t\tgit_config_set(\"core.precomposeunicode\", precomposed_unicode ? \"true\" : \"false\");\n+\t\tgit_config_set_or_die(\"core.precomposeunicode\",\n+\t\t\t\t      precomposed_unicode ? \"true\" : \"false\");\n \t\tgit_path_buf(&path, \"%s\", auml_nfc);\n \t\tif (unlink(path.buf))\n \t\t\tdie_errno(_(\"failed to unlink '%s'\"), path.buf);\n-- \n2.7.0\n"},{"id":"277044","messageId":"20160129082036.GA8591@sigill.intra.peff.net","threadId":"41272","inReplyTo":"1453971637-22273-1-git-send-email-ps@pks.im","subject":"Re: [PATCH v2 0/9] Handle errors when setting configs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-29T08:20:36Z","receivedAt":"2016-01-29T08:20:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2016 at 10:00:28AM +0100, Patrick Steinhardt wrote:\n\n> I've finally got around to producing version two of my previous\n> patch to handle errors when setting configs. Back in September\n> I've posted a single patch to handle errors when\n> `install_branch_config` fails due to configuration failures [1].\n\nThanks for following up.\n\nHaving read through the series, I don't see anything _too_ wrong with\nit. And certainly catching these errors and dying is better than letting\nthem go unnoticed.\n\nRegarding this:\n\n> Version two of this patch is somewhat more involved in that I\n> tried to track down all relevant places where we set configs\n> without checking for error conditions. My current approach to\n> most of those cases is to just die with an error message, this\n> remains up to discussion though for the individual cases.\n\nI was a little surprised to see all of the effort in patch 2 to carry\nthe return value up the call stack, just to die a little later. Having\nre-read our original thread, I did say that possibly \"checkout -b\"\nshould not directly die when failing to set the upstream. But looking at\nthe code again and thinking on it more, I don't really think that buys\nus anything interesting (unless it is to roll back the branch creation,\nbut as before, I don't think it's really worth the effort).\n\nSo I wondered if patch 2 could simply use the \"or_die\" variant.\n\nWhich then made me wonder: doesn't basically everybody want to die if\nsetting config fails? The exception might be builtin/config.c, just\nbecause it wants to return a custom exit code for some errors.\n\nSo would this series be a lot more pleasant if we went the other way?\nThat is, make git_config_set() die by default, and add a \"_gently\" form.\n\nThe end result is roughly the same, but it's a lot less churn, and it's\nmore likely for new callers to get it right, because they have to go the\nextra mile to ignore the error. I say \"roughly\" because it treats cases\nwe missed as \"die\", whereas yours leaves them as \"ignore\". I find it\nhighly unlikely that any of them actually _want_ the ignore behavior,\nthough.\n\nI'm just pondering, though. I don't find the \"or_die\" variant bad at\nall, so if you really prefer it, I don't mind.\n\nJust to get a sense of what the reverse would look like, I worked up the\npatch below (which compiles but does not link, as I did not actually\nimplement the \"gently\" form). Some error-checking call-sites are\nconverted to the \"die\" form, because that's essentially what happens\nanyway (and I'd venture to say that the config code can provide a much\nbetter error message).\n\n---\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3f6c825..4ab8b35 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -570,7 +570,6 @@ static const char edit_description[] = \"BRANCH_DESCRIPTION\";\n \n static int edit_branch_description(const char *branch_name)\n {\n-\tint status;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf name = STRBUF_INIT;\n \n@@ -595,11 +594,10 @@ 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-\n-\treturn status;\n+\treturn 0;\n }\n \n int cmd_branch(int argc, const char **argv, const char *prefix)\ndiff --git a/builtin/config.c b/builtin/config.c\nindex adc7727..2e6fd3c 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@@ -637,7 +637,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\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\telse\n-\t\t\treturn git_config_set_in_file(given_config_source.file,\n+\t\t\treturn git_config_set_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t      argv[0], NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 2b2ff9b..0f69c30 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\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@@ -215,16 +214,14 @@ static int add(int argc, const char **argv)\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       fetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\");\n \t}\n \n \tif (fetch && fetch_remote(name))\n@@ -689,9 +686,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 +784,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}\ndiff --git a/cache.h b/cache.h\nindex dfc459c..a1c7782 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1507,8 +1507,9 @@ 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_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);\ndiff --git a/config.c b/config.c\nindex 86a5eb2..54c3f30 100644\n--- a/config.c\n+++ b/config.c\n@@ -1825,15 +1825,15 @@ 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+void git_config_set(const char *key, const char *value)\n {\n-\treturn git_config_set_multivar(key, value, NULL, 0);\n+\tgit_config_set_multivar(key, value, NULL, 0);\n }\n \n /*\ndiff --git a/submodule.c b/submodule.c\nindex b83939c..b3fc6ac 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"},{"id":"277060","messageId":"xmqqsi1gi5af.fsf@gitster.mtv.corp.google.com","threadId":"41272","inReplyTo":"20160129082036.GA8591@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/9] Handle errors when setting configs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-29T18:55:52Z","receivedAt":"2016-01-29T18:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The end result is roughly the same, but it's a lot less churn, and it's\n> more likely for new callers to get it right, because they have to go the\n> extra mile to ignore the error. I say \"roughly\" because it treats cases\n> we missed as \"die\", whereas yours leaves them as \"ignore\". I find it\n> highly unlikely that any of them actually _want_ the ignore behavior,\n> though.\n\nYes, I like this approach better.  It admittedly is more risky in\nthat it would die if the conversion missed a case that should\nignore, but I suspect that such a breakage would be found rather\nquickly (and the one that goes latent are the ones that do not\nmatter in practice because people would not encounter them).\n\n> I'm just pondering, though. I don't find the \"or_die\" variant bad at\n> all, so if you really prefer it, I don't mind.\n>\n> Just to get a sense of what the reverse would look like, I worked up the\n> patch below (which compiles but does not link, as I did not actually\n> implement the \"gently\" form). Some error-checking call-sites are\n> converted to the \"die\" form, because that's essentially what happens\n> anyway (and I'd venture to say that the config code can provide a much\n> better error message).\n\nThis variant certainly looks nicer to me, for the reasons give above.\n"},{"id":"277088","messageId":"20160130160342.GA790@pks-xps","threadId":"41272","inReplyTo":"xmqqsi1gi5af.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/9] Handle errors when setting configs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2016-01-30T16:03:42Z","receivedAt":"2016-01-30T16:03:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 29, 2016 at 10:55:52AM -0800, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > The end result is roughly the same, but it's a lot less churn, and it's\n> > more likely for new callers to get it right, because they have to go the\n> > extra mile to ignore the error. I say \"roughly\" because it treats cases\n> > we missed as \"die\", whereas yours leaves them as \"ignore\". I find it\n> > highly unlikely that any of them actually _want_ the ignore behavior,\n> > though.\n> \n> Yes, I like this approach better.  It admittedly is more risky in\n> that it would die if the conversion missed a case that should\n> ignore, but I suspect that such a breakage would be found rather\n> quickly (and the one that goes latent are the ones that do not\n> matter in practice because people would not encounter them).\n> \n> > I'm just pondering, though. I don't find the \"or_die\" variant bad at\n> > all, so if you really prefer it, I don't mind.\n> >\n> > Just to get a sense of what the reverse would look like, I worked up the\n> > patch below (which compiles but does not link, as I did not actually\n> > implement the \"gently\" form). Some error-checking call-sites are\n> > converted to the \"die\" form, because that's essentially what happens\n> > anyway (and I'd venture to say that the config code can provide a much\n> > better error message).\n> \n> This variant certainly looks nicer to me, for the reasons give above.\n\nOkay, thanks for your feedback. I'll create a new version next\nweek, then.\n\nPatrick\n"}]}