Merge branch 'jt/config-lock-timeout' into seenseen

Configuration file locking now retries for a short period, avoiding
failures when multiple processes attempt to update the configuration
simultaneously.

Comments?
cf. <xmqqzf1xbl4i.fsf@gitster.g>

* jt/config-lock-timeout:
  config: retry acquiring config.lock, configurable via core.configLockTimeout

5 files changed, 53 insertions, 5 deletions

diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index a0ebf03e2e..340329edc3 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -589,6 +589,14 @@ core.packedRefsTimeout::
 	all; -1 means to try indefinitely. Default is 1000 (i.e.,
 	retry for 1 second).
 
+core.configLockTimeout::
+	The length of time, in milliseconds, to retry when trying to
+	lock a configuration file for writing. Value 0 means not to
+	retry at all; -1 means to try indefinitely. Default is 1000
+	(i.e., retry for 1 second). This is read from the configuration
+	that is already on disk before the lock is taken, so it can be
+	set persistently like any other option.
+
 core.pager::
 	Text viewer for use by Git commands (e.g., 'less').  The value
 	is meant to be interpreted by the shell.  The order of preference
diff --git a/config.c b/config.c
index c250e56214..8e47d11189 100644
--- a/config.c
+++ b/config.c
@@ -2934,6 +2934,24 @@ char *git_config_prepare_comment_string(const char *comment)
 	return prepared;
 }
 
+/*
+ * How long to retry acquiring config.lock when another process holds
+ * it. Default matches core.packedRefsTimeout; override via
+ * core.configLockTimeout.
+ */
+static long config_lock_timeout_ms(struct repository *r)
+{
+	static int configured;
+	static int timeout_ms = 1000;
+
+	if (!configured) {
+		repo_config_get_int(r, "core.configlocktimeout", &timeout_ms);
+		configured = 1;
+	}
+
+	return timeout_ms;
+}
+
 static void validate_comment_string(const char *comment)
 {
 	size_t leading_blanks;
@@ -3017,7 +3035,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	fd = hold_lock_file_for_update(&lock, config_filename, 0);
+	fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
+					       config_lock_timeout_ms(r));
 	if (fd < 0) {
 		error_errno(_("could not lock config file %s"), config_filename);
 		ret = CONFIG_NO_LOCK;
@@ -3362,7 +3381,8 @@ static int repo_config_copy_or_rename_section_in_file(
 	if (!config_filename)
 		config_filename = filename_buf = repo_git_path(r, "config");
 
-	out_fd = hold_lock_file_for_update(&lock, config_filename, 0);
+	out_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,
+						   config_lock_timeout_ms(r));
 	if (out_fd < 0) {
 		ret = error(_("could not lock config file %s"), config_filename);
 		goto out;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index a00314d077..a68ec92bdb 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2993,4 +2993,21 @@ test_expect_success 'writing value with trailing CR not stripped on read' '
 	test_cmp expect actual
 '
 
+test_expect_success 'writing config fails immediately with core.configLockTimeout=0' '
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	test_must_fail git -c core.configLockTimeout=0 config foo.bar baz 2>err &&
+	test_grep "could not lock config file" err
+'
+
+test_expect_success 'writing config retries until lock is released' '
+	test_when_finished "rm -f .git/config.lock" &&
+	>.git/config.lock &&
+	{
+		( sleep 1 && rm -f .git/config.lock ) &
+	} &&
+	git -c core.configLockTimeout=5000 config retried.key value &&
+	test "$(git config retried.key)" = value
+'
+
 test_done
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a36e5ee80a..58c23a7702 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1037,7 +1037,8 @@ 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 2>err &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		branch --set-upstream-to locked 2>err &&
 	test_grep "could not lock config file .git/config" err
 '
 
@@ -1068,7 +1069,8 @@ 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 2>err &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		branch --unset-upstream 2>err &&
 	test_grep "could not lock config file .git/config" err
 '
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e592c0bcde..aea9222649 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1327,7 +1327,8 @@ 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 &&
+	test_must_fail git -c core.configLockTimeout=0 \
+		remote set-url someremote baz &&
 	git config --get-all remote.someremote.url >actual &&
 	cmp expect actual
 '