On Fri, May 1, 2015 at 7:52 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxx>
> ---
>
> Notes (discussion):
> At first I wasn't going to make this setting configurable. After all,
> who could object to one second?
>
> But then I realized that making the length of the timeout configurable
> would make it easier to test. Since sleep(1) is only guaranteed to
> have a 1-second resolution, it is helpful to set the timeout longer
> than 1 second in the test. So I made it configurable, and documented
> the setting.
>
> I don't have a strong opinion either way.
>
> By the way, if anybody has a better idea for how to test this, please
> chime up. As written, the two new tests each take about one second to
> run (though they are mostly idle during that time, so they parallelize
> well with other tests).
>
> Documentation/config.txt | 6 ++++++
> refs.c | 12 +++++++++++-
> t/t3210-pack-refs.sh | 17 +++++++++++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..3c24e10 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -622,6 +622,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 47e4e53..3f8ac63 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2413,9 +2413,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;
I'd personally be more happier with a default value of 100 ms or less
The reason is found in the human nature, as humans tend to perceive
anything faster than 100ms as "instant"[1], while a 100ms is a long time
for computers.
Now a small default time may lead to to little retries, so maybe it's worth
checking at the very end of the time again (ignoring the computed backoff
times). As pushes to $server usually take a while (connecting, packing packs,
writing objects etc), this may be overcautios bikeshedding from my side.
[1] http://stackoverflow.com/questions/536300/what-is-the-shortest-perceivable-application-response-delay
> +
> 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 aa9eb3a..d18b726 100755
> --- 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 "rm -f $LOCK" &&
> + {
> + ( sleep 1 ; rm -f $LOCK ) &
> + } &&
> + git -c core.packedrefstimeout=3000 pack-refs --all --prune
> +'
> +
> test_done
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html