Re: [PATCH v2 0/9] Handle errors when setting configs

[Jeff King <peff@peff.net>]

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);