Re: [PATCH v2 0/9] Handle errors when setting configs
From: Jeff King <hidden>
Date: 2016-06-15 23:08:01
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Jan 28, 2016 at 10:00:28AM +0100, Patrick Steinhardt wrote:
I've finally got around to producing version two of my previous patch to handle errors when setting configs. Back in September I've posted a single patch to handle errors when `install_branch_config` fails due to configuration failures [1].
Thanks for following up. Having read through the series, I don't see anything _too_ wrong with it. And certainly catching these errors and dying is better than letting them go unnoticed. Regarding this:
Version two of this patch is somewhat more involved in that I tried to track down all relevant places where we set configs without checking for error conditions. My current approach to most of those cases is to just die with an error message, this remains up to discussion though for the individual cases.
I was a little surprised to see all of the effort in patch 2 to carry the return value up the call stack, just to die a little later. Having re-read our original thread, I did say that possibly "checkout -b" should not directly die when failing to set the upstream. But looking at the code again and thinking on it more, I don't really think that buys us anything interesting (unless it is to roll back the branch creation, but as before, I don't think it's really worth the effort). So I wondered if patch 2 could simply use the "or_die" variant. Which then made me wonder: doesn't basically everybody want to die if setting config fails? The exception might be builtin/config.c, just because it wants to return a custom exit code for some errors. So would this series be a lot more pleasant if we went the other way? That is, make git_config_set() die by default, and add a "_gently" form. The end result is roughly the same, but it's a lot less churn, and it's more likely for new callers to get it right, because they have to go the extra mile to ignore the error. I say "roughly" because it treats cases we missed as "die", whereas yours leaves them as "ignore". I find it highly unlikely that any of them actually _want_ the ignore behavior, though. I'm just pondering, though. I don't find the "or_die" variant bad at all, so if you really prefer it, I don't mind. Just to get a sense of what the reverse would look like, I worked up the patch below (which compiles but does not link, as I did not actually implement the "gently" form). Some error-checking call-sites are converted to the "die" form, because that's essentially what happens anyway (and I'd venture to say that the config code can provide a much better error message). ---
diff --git a/builtin/branch.c b/builtin/branch.c
index 3f6c825..4ab8b35 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c@@ -570,7 +570,6 @@ static const char edit_description[] = "BRANCH_DESCRIPTION"; static int edit_branch_description(const char *branch_name) { - int status; struct strbuf buf = STRBUF_INIT; struct strbuf name = STRBUF_INIT;
@@ -595,11 +594,10 @@ static int edit_branch_description(const char *branch_name) strbuf_stripspace(&buf, 1); strbuf_addf(&name, "branch.%s.description", branch_name); - status = git_config_set(name.buf, buf.len ? buf.buf : NULL); + git_config_set(name.buf, buf.len ? buf.buf : NULL); strbuf_release(&name); strbuf_release(&buf); - - return status; + return 0; } int cmd_branch(int argc, const char **argv, const char *prefix)
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..2e6fd3c 100644
--- a/builtin/config.c
+++ b/builtin/config.c@@ -582,7 +582,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - ret = git_config_set_in_file(given_config_source.file, argv[0], value); + ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); if (ret == CONFIG_NOTHING_SET) error("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s.", argv[0]);
@@ -637,7 +637,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) return git_config_set_multivar_in_file(given_config_source.file, argv[0], NULL, argv[1], 0); else - return git_config_set_in_file(given_config_source.file, + return git_config_set_in_file_gently(given_config_source.file, argv[0], NULL); } else if (actions == ACTION_UNSET_ALL) {
diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..0f69c30 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c@@ -197,8 +197,7 @@ static int add(int argc, const char **argv) die(_("'%s' is not a valid remote name"), name); strbuf_addf(&buf, "remote.%s.url", name); - if (git_config_set(buf.buf, url)) - return 1; + git_config_set(buf.buf, url); if (!mirror || mirror & MIRROR_FETCH) { strbuf_reset(&buf);
@@ -215,16 +214,14 @@ static int add(int argc, const char **argv) if (mirror & MIRROR_PUSH) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.mirror", name); - if (git_config_set(buf.buf, "true")) - return 1; + git_config_set(buf.buf, "true"); } if (fetch_tags != TAGS_DEFAULT) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.tagopt", name); - if (git_config_set(buf.buf, - fetch_tags == TAGS_SET ? "--tags" : "--no-tags")) - return 1; + git_config_set(buf.buf, + fetch_tags == TAGS_SET ? "--tags" : "--no-tags"); } if (fetch && fetch_remote(name))
@@ -689,9 +686,7 @@ static int mv(int argc, const char **argv) if (info->remote_name && !strcmp(info->remote_name, rename.old)) { strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.remote", item->string); - if (git_config_set(buf.buf, rename.new)) { - return error(_("Could not set '%s'"), buf.buf); - } + git_config_set(buf.buf, rename.new); } }
@@ -789,10 +784,7 @@ static int rm(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - if (git_config_set(buf.buf, NULL)) { - strbuf_release(&buf); - return -1; - } + git_config_set(buf.buf, NULL); } } }
diff --git a/cache.h b/cache.h
index dfc459c..a1c7782 100644
--- a/cache.h
+++ b/cache.h@@ -1507,8 +1507,9 @@ extern int git_config_bool(const char *, const char *); extern int git_config_maybe_bool(const char *, const char *); extern int git_config_string(const char **, const char *, const char *); extern int git_config_pathname(const char **, const char *, const char *); -extern int git_config_set_in_file(const char *, const char *, const char *); -extern int git_config_set(const char *, const char *); +extern void git_config_set_in_file(const char *, const char *, const char *); +extern int git_config_set_in_file_gently(const char *, const char *, const char *); +extern void git_config_set(const char *, const char *); extern int git_config_parse_key(const char *, char **, int *); extern int git_config_key_is_valid(const char *key); extern int git_config_set_multivar(const char *, const char *, const char *, int);
diff --git a/config.c b/config.c
index 86a5eb2..54c3f30 100644
--- a/config.c
+++ b/config.c@@ -1825,15 +1825,15 @@ contline: return offset; } -int git_config_set_in_file(const char *config_filename, +void git_config_set_in_file(const char *config_filename, const char *key, const char *value) { - return git_config_set_multivar_in_file(config_filename, key, value, NULL, 0); + git_config_set_multivar_in_file(config_filename, key, value, NULL, 0); } -int git_config_set(const char *key, const char *value) +void git_config_set(const char *key, const char *value) { - return git_config_set_multivar(key, value, NULL, 0); + git_config_set_multivar(key, value, NULL, 0); } /*
diff --git a/submodule.c b/submodule.c
index b83939c..b3fc6ac 100644
--- a/submodule.c
+++ b/submodule.c@@ -69,7 +69,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath) strbuf_addstr(&entry, "submodule."); strbuf_addstr(&entry, submodule->name); strbuf_addstr(&entry, ".path"); - if (git_config_set_in_file(".gitmodules", entry.buf, newpath) < 0) { + if (git_config_set_in_file_gently(".gitmodules", entry.buf, newpath) < 0) { /* Maybe the user already did that, don't error out here */ warning(_("Could not update .gitmodules entry %s"), entry.buf); strbuf_release(&entry);
@@ -1087,11 +1087,9 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) /* Update core.worktree setting */ strbuf_reset(&file_name); strbuf_addf(&file_name, "%s/config", git_dir); - if (git_config_set_in_file(file_name.buf, "core.worktree", - relative_path(real_work_tree, git_dir, - &rel_path))) - die(_("Could not set core.worktree in %s"), - file_name.buf); + git_config_set_in_file(file_name.buf, "core.worktree", + relative_path(real_work_tree, git_dir, + &rel_path)); strbuf_release(&file_name); strbuf_release(&rel_path);