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 4ff0f01cb7 (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 <ps@pks.im>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index a0ebf03e2eb050c8c2a642953a3ca383c0d9f2e6..340329edc3814376d6ea99c7e905b862c4980ca4 100644 (file)
--- 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 156f2a24fa00271c6d6adafac16527e674f5ccd4..ac9563781bfb219bb5b81c1c39b8ea0425642308 100644 (file)
--- a/config.c
+++ b/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;

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 128971ee12fa6c510c4110d9b958037fbf578f42..12a1cb85809b8b8f4b528ac3b2430acc3d24b58e 100755 (executable)
--- a/t/t1300-config.sh
+++ b/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

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e7829c2c4bfdc3d26724a56ba109034a12d7103a..5f8a31c21d0eaf5582047d225a224db36aa4629a 100755 (executable)
--- 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 e592c0bcde91e9b41626d1ff7a355b4cc4a6580a..aea9222649f7bbaf7a02f3f0be711d3cb8b6cfe4 100755 (executable)
--- 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
 '