lock_packed_refs(): allow retries when acquiring the packed-refs lock

Currently, there is only one attempt to acquire any lockfile, and if
the lock is held by another process, the locking attempt fails
immediately.

This is not such a limitation for loose reference files. First, they
don't take long to rewrite. Second, most reference updates have a
known "old" value, so if another process is updating a reference at
the same moment that we are trying to lock it, then probably the
expected "old" value will not longer be valid, and the update will
fail anyway.

But these arguments do not hold for packed-refs:

* The packed-refs file can be large and take significant time to
  rewrite.

* Many references are stored in a single packed-refs file, so it could
  be that the other process was changing a different reference than
  the one that we are interested in.

Therefore, it is much more likely for there to be spurious lock
conflicts in connection to the packed-refs file, resulting in
unnecessary command failures.

So, if the first attempt to lock the packed-refs file fails, continue
retrying for a configurable length of time before giving up. The
default timeout is 1 second.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 948b8b0e5c8e8639e989d2f14dbd310fe146278b..863057b25f198de17548ca4b2ea1a7b6eb48f3e8 100644 (file)
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -624,6 +624,12 @@ core.commentChar::
 If set to "auto", `git-commit` would select a character that is not
 the beginning character of any line in existing commit messages.
 
+core.packedRefsTimeout::
+       The length of time, in milliseconds, to retry when trying to
+       lock the `packed-refs` file. Value 0 means not to retry at
+       all; -1 means to try indefinitely. Default is 1000 (i.e.,
+       retry for 1 second).
+
 sequence.editor::
        Text editor used by `git rebase -i` for editing the rebase instruction file.
        The value is meant to be interpreted by the shell when it is used.

diff --git a/refs.c b/refs.c
index 81a455b807e4ef7f79e3cdb252eedfccfa3c7d8a..8a4a9dd99c558355f50017021c9f514b9099a436 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2431,9 +2431,19 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 /* This should return a meaningful errno on failure */
 int lock_packed_refs(int flags)
 {
+       static int timeout_configured = 0;
+       static int timeout_value = 1000;
+
        struct packed_ref_cache *packed_ref_cache;
 
-       if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
+       if (!timeout_configured) {
+               git_config_get_int("core.packedrefstimeout", &timeout_value);
+               timeout_configured = 1;
+       }
+
+       if (hold_lock_file_for_update_timeout(
+                           &packlock, git_path("packed-refs"),
+                           flags, timeout_value) < 0)
                return -1;
        /*
         * Get the current packed-refs while holding the lock.  If the

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index aa9eb3a3e5ef0637615c6d958a29dbd9bbbf0895..8aae98d482aa572b3a9a55f9b38280ad716c7baf 100755 (executable)
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -187,4 +187,21 @@ test_expect_success 'notice d/f conflict with existing ref' '
        test_must_fail git branch foo/bar/baz/lots/of/extra/components
 '
 
+test_expect_success 'timeout if packed-refs.lock exists' '
+       LOCK=.git/packed-refs.lock &&
+       >"$LOCK" &&
+       test_when_finished "rm -f $LOCK" &&
+       test_must_fail git pack-refs --all --prune
+'
+
+test_expect_success 'retry acquiring packed-refs.lock' '
+       LOCK=.git/packed-refs.lock &&
+       >"$LOCK" &&
+       test_when_finished "wait; rm -f $LOCK" &&
+       {
+               ( sleep 1 ; rm -f $LOCK ) &
+       } &&
+       git -c core.packedrefstimeout=3000 pack-refs --all --prune
+'
+
 test_done