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

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

[Patrick Steinhardt]

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].

Failure to write the configuration file may be caused by multiple
conditions, but the most common one will surely be the case where
the configuration is locked because of a leftover lock file or
because another process is currently writing to it. We used to
ignore those errors in many cases, possibly leading to
inconsistently configured repositories. More often than not git
even pretended that everything was fine and that the settings
have been applied when indeed they were not.

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.

[1]: $gmane/278544

Patrick Steinhardt (9):
  config: introduce set_or_die wrappers
  branch: return error code for install_branch_config
  remote: handle config errors in set_url
  clone: handle config errors in cmd_clone
  branch: handle config errors when unsetting upstream
  init-db: handle config errors
  sequencer: handle config errors when saving opts
  submodule--helper: handle config errors
  compat: die when unable to set core.precomposeunicode

 branch.c                    | 31 +++++++++++++++++++++----------
 branch.h                    |  3 ++-
 builtin/branch.c            |  4 ++--
 builtin/clone.c             | 17 ++++++++++-------
 builtin/init-db.c           | 20 ++++++++++----------
 builtin/remote.c            | 11 ++++++-----
 builtin/submodule--helper.c |  4 ++--
 cache.h                     |  4 ++++
 compat/precompose_utf8.c    |  3 ++-
 config.c                    | 27 +++++++++++++++++++++++++++
 sequencer.c                 | 22 +++++++++++-----------
 t/t3200-branch.sh           | 16 +++++++++++++++-
 t/t5505-remote.sh           |  9 +++++++++
 transport.c                 | 11 ++++++-----
 14 files changed, 127 insertions(+), 55 deletions(-)

-- 
2.7.0

[PATCH v2 1/9] config: introduce set_or_die wrappers

[Patrick Steinhardt]

A lot of call-sites for the existing family of `git_config_set`
functions do not check for errors that may occur, e.g. the
configuration file being locked. In many cases we simply want to
die when such a situation arises.

Introduce wrappers that will cause the program to die in those
cases.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 cache.h  |  4 ++++
 config.c | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/cache.h b/cache.h
index dfc459c..c1f844d 100644
--- a/cache.h
+++ b/cache.h
@@ -1508,11 +1508,15 @@ 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 void git_config_set_in_file_or_die(const char *, const char *, const char *);
 extern int git_config_set(const char *, const char *);
+extern void git_config_set_or_die(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);
+extern void git_config_set_multivar_or_die(const char *, const char *, const char *, int);
 extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+extern void git_config_set_multivar_in_file_or_die(const char *, const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
 extern int git_config_rename_section_in_file(const char *, const char *, const char *);
 extern const char *git_etc_gitconfig(void);
diff --git a/config.c b/config.c
index 86a5eb2..856f7d34 100644
--- a/config.c
+++ b/config.c
@@ -1831,11 +1831,22 @@ int git_config_set_in_file(const char *config_filename,
 	return git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);
 }
 
+void git_config_set_in_file_or_die(const char *config_filename,
+			const char *key, const char *value)
+{
+	git_config_set_multivar_in_file_or_die(config_filename, key, value, NULL, 0);
+}
+
 int git_config_set(const char *key, const char *value)
 {
 	return git_config_set_multivar(key, value, NULL, 0);
 }
 
+void git_config_set_or_die(const char *key, const char *value)
+{
+	git_config_set_multivar_or_die(key, value, NULL, 0);
+}
+
 /*
  * Auxiliary function to sanity-check and split the key into the section
  * identifier and variable name.
@@ -2179,6 +2190,15 @@ write_err_out:
 
 }
 
+void git_config_set_multivar_in_file_or_die(const char *config_filename,
+				const char *key, const char *value,
+				const char *value_regex, int multi_replace)
+{
+	if (git_config_set_multivar_in_file(config_filename, key, value,
+					    value_regex, multi_replace) < 0)
+		die(_("Could not set '%s' to '%s'"), key, value);
+}
+
 int git_config_set_multivar(const char *key, const char *value,
 			const char *value_regex, int multi_replace)
 {
@@ -2186,6 +2206,13 @@ int git_config_set_multivar(const char *key, const char *value,
 					       multi_replace);
 }
 
+void git_config_set_multivar_or_die(const char *key, const char *value,
+			const char *value_regex, int multi_replace)
+{
+	git_config_set_multivar_in_file_or_die(NULL, key, value, value_regex,
+					       multi_replace);
+}
+
 static int section_name_match (const char *buf, const char *name)
 {
 	int i = 0, j = 0, dot = 0;
-- 
2.7.0

[PATCH v2 2/9] branch: return error code for install_branch_config

[Patrick Steinhardt]

The install_branch_config function may fail to write the
configuration file due to locking. To accomodate for this
scenario, introduce a return value which may be used to check if
the function did finish correctly and adjust callers to use this
error code.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 branch.c          | 31 +++++++++++++++++++++----------
 branch.h          |  3 ++-
 builtin/clone.c   |  9 ++++++---
 t/t3200-branch.sh |  9 ++++++++-
 transport.c       | 11 ++++++-----
 5 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/branch.c b/branch.c
index 7ff3f20..7f35bcf 100644
--- a/branch.c
+++ b/branch.c
@@ -49,33 +49,38 @@ static int should_setup_rebase(const char *origin)
 	return 0;
 }
 
-void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
+int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
 	const char *shortname = NULL;
 	struct strbuf key = STRBUF_INIT;
-	int rebasing = should_setup_rebase(origin);
+	int ret = 0, rebasing = should_setup_rebase(origin);
 
 	if (skip_prefix(remote, "refs/heads/", &shortname)
 	    && !strcmp(local, shortname)
 	    && !origin) {
 		warning(_("Not setting branch %s as its own upstream."),
 			local);
-		return;
+		return ret;
 	}
 
 	strbuf_addf(&key, "branch.%s.remote", local);
-	git_config_set(key.buf, origin ? origin : ".");
+	ret = git_config_set(key.buf, origin ? origin : ".");
+	if (ret)
+		goto out;
 
 	strbuf_reset(&key);
 	strbuf_addf(&key, "branch.%s.merge", local);
-	git_config_set(key.buf, remote);
+	ret = git_config_set(key.buf, remote);
+	if (ret)
+		goto out;
 
 	if (rebasing) {
 		strbuf_reset(&key);
 		strbuf_addf(&key, "branch.%s.rebase", local);
-		git_config_set(key.buf, "true");
+		ret = git_config_set(key.buf, "true");
+		if (ret)
+			goto out;
 	}
-	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
 		if (shortname) {
@@ -102,6 +107,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 					  local, remote);
 		}
 	}
+
+out:
+	strbuf_release(&key);
+	return ret;
 }
 
 /*
@@ -134,8 +143,9 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 		return error(_("Not tracking: ambiguous information for ref %s"),
 				orig_ref);
 
-	install_branch_config(config_flags, new_ref, tracking.remote,
-			      tracking.src ? tracking.src : orig_ref);
+	if (install_branch_config(config_flags, new_ref, tracking.remote,
+				  tracking.src ? tracking.src : orig_ref))
+		return error(_("Could not install branch configuration"));
 
 	free(tracking.src);
 	return 0;
@@ -295,7 +305,8 @@ void create_branch(const char *head,
 	}
 
 	if (real_ref && track)
-		setup_tracking(ref.buf + 11, real_ref, track, quiet);
+		if (setup_tracking(ref.buf + 11, real_ref, track, quiet) < 0)
+			die(_("Could not setup tracking branch"));
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/branch.h b/branch.h
index 58aa45f..78ad438 100644
--- a/branch.h
+++ b/branch.h
@@ -43,9 +43,10 @@ void remove_branch_state(void);
 /*
  * Configure local branch "local" as downstream to branch "remote"
  * from remote "origin".  Used by git branch --set-upstream.
+ * Returns 0 on success.
  */
 #define BRANCH_CONFIG_VERBOSE 01
-extern void install_branch_config(int flag, const char *local, const char *origin, const char *remote);
+extern int install_branch_config(int flag, const char *local, const char *origin, const char *remote);
 
 /*
  * Read branch description
diff --git a/builtin/clone.c b/builtin/clone.c
index a7c8def..8b11650 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -648,6 +648,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
 	const char *head;
+
 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
 		/* Local default branch link */
 		if (create_symref("HEAD", our->name, NULL) < 0)
@@ -655,7 +656,8 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		if (!option_bare) {
 			update_ref(msg, "HEAD", our->old_oid.hash, NULL, 0,
 				   UPDATE_REFS_DIE_ON_ERR);
-			install_branch_config(0, head, option_origin, our->name);
+			if (install_branch_config(0, head, option_origin, our->name))
+				die(_("Could not update '%s'"), head);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(our->old_oid.hash);
@@ -1054,8 +1056,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		remote_head = NULL;
 		option_no_checkout = 1;
 		if (!option_bare)
-			install_branch_config(0, "master", option_origin,
-					      "refs/heads/master");
+			if (install_branch_config(0, "master", option_origin,
+						  "refs/heads/master"))
+				die(_("Could not set up master branch"));
 	}
 
 	write_refspec_config(src_ref_prefix, our_head_points_at,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cdaf6f6..dd776b3 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -446,6 +446,13 @@ test_expect_success '--set-upstream-to fails on a non-ref' '
 	test_must_fail git branch --set-upstream-to HEAD^{}
 '
 
+test_expect_success '--set-upstream-to fails on locked config' '
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	git branch locked &&
+	test_must_fail git branch --set-upstream-to locked
+'
+
 test_expect_success 'use --set-upstream-to modify HEAD' '
 	test_config branch.master.remote foo &&
 	test_config branch.master.merge foo &&
@@ -579,7 +586,7 @@ test_expect_success 'avoid ambiguous track' '
 	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&
 	git config remote.ambi2.url lilili &&
 	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/master &&
-	git branch all1 master &&
+	test_must_fail git branch all1 master &&
 	test -z "$(git config branch.all1.merge)"
 '
 
diff --git a/transport.c b/transport.c
index 67f3666..f7ad752 100644
--- a/transport.c
+++ b/transport.c
@@ -181,11 +181,12 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 		if (!remotename || !starts_with(remotename, "refs/heads/"))
 			continue;
 
-		if (!pretend)
-			install_branch_config(BRANCH_CONFIG_VERBOSE,
-				localname + 11, transport->remote->name,
-				remotename);
-		else
+		if (!pretend) {
+			if (install_branch_config(BRANCH_CONFIG_VERBOSE,
+				    localname + 11, transport->remote->name,
+				    remotename))
+				die(_("Could not set upstream"));
+		} else
 			printf("Would set upstream of '%s' to '%s' of '%s'\n",
 				localname + 11, remotename + 11,
 				transport->remote->name);
-- 
2.7.0

[PATCH v2 3/9] remote: handle config errors in set_url

[Patrick Steinhardt]

The return codes for function calls that write the new URL
configuration are not being checked in `set_url`. As the function
is exposed to the user directly through `git remote set-url` we
want to inform her that we were not able to complete the request
and abort the program.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/remote.c  | 11 ++++++-----
 t/t5505-remote.sh |  9 +++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..8b78c3d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1583,11 +1583,12 @@ static int set_url(int argc, const char **argv)
 	/* Special cases that add new entry. */
 	if ((!oldurl && !delete_mode) || add_mode) {
 		if (add_mode)
-			git_config_set_multivar(name_buf.buf, newurl,
-				"^$", 0);
+			git_config_set_multivar_or_die(name_buf.buf, newurl,
+						       "^$", 0);
 		else
-			git_config_set(name_buf.buf, newurl);
+			git_config_set_or_die(name_buf.buf, newurl);
 		strbuf_release(&name_buf);
+
 		return 0;
 	}
 
@@ -1608,9 +1609,9 @@ static int set_url(int argc, const char **argv)
 	regfree(&old_regex);
 
 	if (!delete_mode)
-		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
+		git_config_set_multivar_or_die(name_buf.buf, newurl, oldurl, 0);
 	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+		git_config_set_multivar_or_die(name_buf.buf, NULL, oldurl, 1);
 	return 0;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1a8e3b8..e019f27 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -932,6 +932,15 @@ test_expect_success 'get-url on new remote' '
 	echo foo | get_url_test --push --all someremote
 '
 
+test_expect_success 'remote set-url with locked config' '
+	test_when_finished "rm -f .git/config.lock" &&
+	git config --get-all remote.someremote.url >expect &&
+	>.git/config.lock &&
+	test_must_fail git remote set-url someremote baz &&
+	git config --get-all remote.someremote.url >actual &&
+	cmp expect actual
+'
+
 test_expect_success 'remote set-url bar' '
 	git remote set-url someremote bar &&
 	echo bar >expect &&
-- 
2.7.0

[PATCH v2 4/9] clone: handle config errors in cmd_clone

[Patrick Steinhardt]

The clone command does not check for error codes returned by
`git_config_set` functions. This may cause the user to end up
with an inconsistent repository without any indication with what
went wrong.

Fix this problem by dying with an error message when we are
unable to write the configuration files to disk.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 8b11650..5f6f995 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -788,12 +788,12 @@ static void write_refspec_config(const char *src_ref_prefix,
 		/* Configure the remote */
 		if (value.len) {
 			strbuf_addf(&key, "remote.%s.fetch", option_origin);
-			git_config_set_multivar(key.buf, value.buf, "^$", 0);
+			git_config_set_multivar_or_die(key.buf, value.buf, "^$", 0);
 			strbuf_reset(&key);
 
 			if (option_mirror) {
 				strbuf_addf(&key, "remote.%s.mirror", option_origin);
-				git_config_set(key.buf, "true");
+				git_config_set_or_die(key.buf, "true");
 				strbuf_reset(&key);
 			}
 		}
@@ -951,14 +951,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			src_ref_prefix = "refs/";
 		strbuf_addstr(&branch_top, src_ref_prefix);
 
-		git_config_set("core.bare", "true");
+		git_config_set_or_die("core.bare", "true");
 	} else {
 		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
 
 	strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
 	strbuf_addf(&key, "remote.%s.url", option_origin);
-	git_config_set(key.buf, repo);
+	git_config_set_or_die(key.buf, repo);
 	strbuf_reset(&key);
 
 	if (option_reference.nr)
-- 
2.7.0

[PATCH v2 5/9] branch: handle config errors when unsetting upstream

[Patrick Steinhardt]

When we try to unset upstream configurations we do not check
return codes for the `git_config_set` functions. As those may
indicate that we were unable to unset the respective
configuration we may exit successfully without any error message
while in fact the upstream configuration was not unset.

Fix this by dying with an error message when we cannot unset the
configuration.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/branch.c  | 4 ++--
 t/t3200-branch.sh | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3f6c825..0978287 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -791,10 +791,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar_or_die(buf.buf, NULL, NULL, 1);
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.merge", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar_or_die(buf.buf, NULL, NULL, 1);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		struct branch *branch = branch_get(argv[0]);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd776b3..a897248 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -473,6 +473,13 @@ test_expect_success '--unset-upstream should fail if given a non-existent branch
 	test_must_fail git branch --unset-upstream i-dont-exist
 '
 
+test_expect_success '--unset-upstream should fail if config is locked' '
+	test_when_finished "rm -f .git/config.lock" &&
+	git branch --set-upstream-to locked &&
+	>.git/config.lock &&
+	test_must_fail git branch --unset-upstream
+'
+
 test_expect_success 'test --unset-upstream on HEAD' '
 	git branch my14 &&
 	test_config branch.master.remote foo &&
-- 
2.7.0

[PATCH v2 6/9] init-db: handle config errors

[Patrick Steinhardt]

When creating default files we do not check for error codes
returned by `git_config_set` functions. This may cause the user
to end up with an inconsistent repository without any indication
for the user.

Fix this problem by dying with an error message when we are
unable to write the configuration files to disk.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/init-db.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 07229d6..ef19048 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -227,7 +227,7 @@ static int create_default_files(const char *template_path)
 	/* This forces creation of new config file */
 	xsnprintf(repo_version_string, sizeof(repo_version_string),
 		  "%d", GIT_REPO_VERSION);
-	git_config_set("core.repositoryformatversion", repo_version_string);
+	git_config_set_or_die("core.repositoryformatversion", repo_version_string);
 
 	/* Check filemode trustability */
 	path = git_path_buf(&buf, "config");
@@ -241,18 +241,18 @@ static int create_default_files(const char *template_path)
 		if (filemode && !reinit && (st1.st_mode & S_IXUSR))
 			filemode = 0;
 	}
-	git_config_set("core.filemode", filemode ? "true" : "false");
+	git_config_set_or_die("core.filemode", filemode ? "true" : "false");
 
 	if (is_bare_repository())
-		git_config_set("core.bare", "true");
+		git_config_set_or_die("core.bare", "true");
 	else {
 		const char *work_tree = get_git_work_tree();
-		git_config_set("core.bare", "false");
+		git_config_set_or_die("core.bare", "false");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
-		    git_config_set("core.logallrefupdates", "true");
+			git_config_set_or_die("core.logallrefupdates", "true");
 		if (needs_work_tree_config(get_git_dir(), work_tree))
-			git_config_set("core.worktree", work_tree);
+			git_config_set_or_die("core.worktree", work_tree);
 	}
 
 	if (!reinit) {
@@ -265,12 +265,12 @@ static int create_default_files(const char *template_path)
 		    S_ISLNK(st1.st_mode))
 			unlink(path); /* good */
 		else
-			git_config_set("core.symlinks", "false");
+			git_config_set_or_die("core.symlinks", "false");
 
 		/* Check if the filesystem is case-insensitive */
 		path = git_path_buf(&buf, "CoNfIg");
 		if (!access(path, F_OK))
-			git_config_set("core.ignorecase", "true");
+			git_config_set_or_die("core.ignorecase", "true");
 		probe_utf8_pathname_composition();
 	}
 
@@ -386,8 +386,8 @@ int init_db(const char *template_dir, unsigned int flags)
 			xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
 		else
 			die("BUG: invalid value for shared_repository");
-		git_config_set("core.sharedrepository", buf);
-		git_config_set("receive.denyNonFastforwards", "true");
+		git_config_set_or_die("core.sharedrepository", buf);
+		git_config_set_or_die("receive.denyNonFastforwards", "true");
 	}
 
 	if (!(flags & INIT_DB_QUIET)) {
-- 
2.7.0

[PATCH v2 7/9] sequencer: handle config errors when saving opts

[Patrick Steinhardt]

When we start picking a range of revisions we save the replay
options that are required to restore state when interrupting and
later continuing picking the revisions. However, we do not check
the return values of the `git_config_set` functions, which may
lead us to store incomplete information. As this may lead us to
fail when trying to continue the sequence this error can be
fatal.

Fix this by dying immediately when we are unable to write back
any replay option.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sequencer.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8c58fa2..3c109b6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -933,31 +933,31 @@ static void save_opts(struct replay_opts *opts)
 	const char *opts_file = git_path_opts_file();
 
 	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
+		git_config_set_in_file_or_die(opts_file, "options.no-commit", "true");
 	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
+		git_config_set_in_file_or_die(opts_file, "options.edit", "true");
 	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
+		git_config_set_in_file_or_die(opts_file, "options.signoff", "true");
 	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
+		git_config_set_in_file_or_die(opts_file, "options.record-origin", "true");
 	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+		git_config_set_in_file_or_die(opts_file, "options.allow-ff", "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		git_config_set_in_file_or_die(opts_file, "options.mainline", buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+		git_config_set_in_file_or_die(opts_file, "options.strategy", opts->strategy);
 	if (opts->gpg_sign)
-		git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+		git_config_set_in_file_or_die(opts_file, "options.gpg-sign", opts->gpg_sign);
 	if (opts->xopts) {
 		int i;
 		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
-							"options.strategy-option",
-							opts->xopts[i], "^$", 0);
+			git_config_set_multivar_in_file_or_die(opts_file,
+							       "options.strategy-option",
+							       opts->xopts[i], "^$", 0);
 	}
 }
 
-- 
2.7.0

[PATCH v2 8/9] submodule--helper: handle config errors

[Patrick Steinhardt]

When setting the 'core.worktree' option for a newly cloned
submodule we ignore the return value of `git_config_set_in_file`.
Instead, we want to inform the user that something has gone wrong
by printing an error message and aborting the program.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..c7e1ea2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -245,8 +245,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	p = git_pathdup_submodule(path, "config");
 	if (!p)
 		die(_("could not get submodule directory for '%s'"), path);
-	git_config_set_in_file(p, "core.worktree",
-			       relative_path(sb.buf, sm_gitdir, &rel_path));
+	git_config_set_in_file_or_die(p, "core.worktree",
+				      relative_path(sb.buf, sm_gitdir, &rel_path));
 	strbuf_release(&sb);
 	strbuf_release(&rel_path);
 	free(sm_gitdir);
-- 
2.7.0

[PATCH v2 9/9] compat: die when unable to set core.precomposeunicode

[Patrick Steinhardt]

When calling `git_config_set` to set 'core.precomposeunicode' we
ignore the return value of the function, which may indicate that
we were unable to write the value back to disk.

As surrounding code is already dying when an error occurs we
simply die and print an error message when we are unable to write
the config value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/precompose_utf8.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 079070f..9ff1ebe 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -50,7 +50,8 @@ void probe_utf8_pathname_composition(void)
 		close(output_fd);
 		git_path_buf(&path, "%s", auml_nfd);
 		precomposed_unicode = access(path.buf, R_OK) ? 0 : 1;
-		git_config_set("core.precomposeunicode", precomposed_unicode ? "true" : "false");
+		git_config_set_or_die("core.precomposeunicode",
+				      precomposed_unicode ? "true" : "false");
 		git_path_buf(&path, "%s", auml_nfc);
 		if (unlink(path.buf))
 			die_errno(_("failed to unlink '%s'"), path.buf);
-- 
2.7.0

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Yes, I like this approach better.  It admittedly is more risky in
that it would die if the conversion missed a case that should
ignore, but I suspect that such a breakage would be found rather
quickly (and the one that goes latent are the ones that do not
matter in practice because people would not encounter them).

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

This variant certainly looks nicer to me, for the reasons give above.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On Fri, Jan 29, 2016 at 10:55:52AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Yes, I like this approach better.  It admittedly is more risky in
> that it would die if the conversion missed a case that should
> ignore, but I suspect that such a breakage would be found rather
> quickly (and the one that goes latent are the ones that do not
> matter in practice because people would not encounter them).
> 
> > 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).
> 
> This variant certainly looks nicer to me, for the reasons give above.

Okay, thanks for your feedback. I'll create a new version next
week, then.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]