config: retry acquiring config.lock, configurable via core.configLockTimeout

Concurrent config writers race for the ".lock" file, which is taken
with open(O_EXCL) and no retry, so the losers fail right away with
"could not lock config file".

This shows up with parallel "git worktree add -b" against the same
repository: each one writes a couple of branch.* keys and the losers
fail at random. Worse, "git worktree add" doesn't propagate that
failure to its exit code, so the tracking config is silently dropped.
(The swallowed error is a separate bug.)

Retry instead of giving up on the first EEXIST. The lock is only held
while rewriting a small file, so the loser only has to wait out the
other writers. Same approach as 4ff0f01 (refs: retry acquiring
reference locks for 100ms, 2017-08-21).

On the semantics: the on-disk config is read only after the lock is
taken, so writers touching different keys can't lose each other's
change. Writers touching the same key still get last-writer-wins, but
that is already the case today and would need a compare-and-swap config
API to fix. The retry only turns hard failures into successes.

Default to 1000ms, like core.packedRefsTimeout: same shape of problem,
one shared file everyone serializes through. A larger timeout only
costs anything when a stale lock is left behind by a crash, which is
rare; a smaller one fails spuriously on slow filesystems (NTFS has
been seen needing more than 100ms). Make it configurable as
core.configLockTimeout. There is no chicken-and-egg problem: we read
the config before we lock it.

microsoft/git carries a similar patch (core.configWriteLockTimeoutMS,
default off) for Scalar's tests. Defaulting to non-zero here because
the worktree case fails silently.

Helped-by: Patrick Steinhardt <[email protected]>
Helped-by: Johannes Schindelin <[email protected]>
Signed-off-by: Jörg Thalheim <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: Mic92

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

config.c

@@ -2903,6 +2903,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;
@@ -2986,7 +3004,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;
@@ -3331,7 +3350,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;

t/t1300-config.sh

@@ -2939,4 +2939,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

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
 '
 

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
 '