Hi Martin,
Thanks for the patch. I'm not too familiar with the lockfile codebase of
git but here are some general comments:
> Subject: [PATCH] enable a timeout for hold_lock_file_for_update
We prefer subjects in the format of "<area>: <brief subject>" so
perhaps, we could rewrite this to:
lockfile: learn core.filesLockTimeout configuration
On Mon, Nov 18, 2019 at 02:42:17PM +0100, Martin Nicolay wrote:
> The new funktion get_files_lock_timeout_ms reads the config
s/funktion/function/
> core.filesLockTimeout analog get_files_ref_lock_timeout_ms.
Perhaps s/analog/similar to/ ?
>
> This value is used in hold_lock_file_for_update instead of the
> fixed value 0.
> ---
> While working with complex scripts invoking git multiple times
> my editor (emacs with standard version control) detects the
> changes and apparently calls "git status". This leads to abort
> in "git stash". With this patch and an appropriate value
> core.fileslocktimeout this problem goes away.
>
> While it may be possible to patch the elisp scripts of emacs (and
> all other similar callers) to call "git status" with
> --no-optional-locks it seems to me a better approarch to solve this
> problem at its root: calling hold_lock_file_for_update_timeout with
> a timeout of 0 ms.
>
> The implementation with the function get_files_lock_timeout_ms is
> adopted from a similar usage of get_files_ref_lock_timeout_ms.
It might be good to include the above three paragraphs in your commit
message. Not only do they describe the change but, more importantly,
they describe _why_ the change is being made.
>
> BTW: is there a way to link this version of the patch to the previous
> version to reduce the work for reviewers?
When you generate your patches, run
git format-patch --in-reply-to=<r> -v<n>
where <r> is the Message-ID of your last patch and where <n> is the
version of the patch (in this case, it should've been 2 since you sent
out one before).
>
> Documentation/config/core.txt | 6 ++++++
> lockfile.c | 16 ++++++++++++++++
> lockfile.h | 4 +++-
> 3 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 852d2ba37a..230ea02560 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -482,6 +482,12 @@ core.packedRefsTimeout::
> all; -1 means to try indefinitely. Default is 1000 (i.e.,
> retry for 1 second).
>
> +core.filesLockTimeout::
> + The length of time, in milliseconds, to retry when trying to
> + lock an individual file. Value 0 means not to retry at
> + all; -1 means to try indefinitely. Default is 0 (i.e., don't
> + retry at all).
> +
> 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/lockfile.c b/lockfile.c
> index 8e8ab4f29f..7301f393d6 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -145,6 +145,22 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
> }
> }
>
> +/*
> + * Get timeout for hold_lock_file_for_update.
> + */
> +long get_files_lock_timeout_ms(void)
> +{
> + static int configured = 0;
> +
> + static int timeout_ms = 0; /* default */
> + if (!configured) {
> + git_config_get_int("core.fileslocktimeout", &timeout_ms);
> + configured = 1;
> + }
> +
> + return timeout_ms;
> +}
> +
> void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
> {
> if (err == EEXIST) {
> diff --git a/lockfile.h b/lockfile.h
> index 9843053ce8..a0520e6a7b 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -163,6 +163,8 @@ int hold_lock_file_for_update_timeout(
> struct lock_file *lk, const char *path,
> int flags, long timeout_ms);
>
> +long get_files_lock_timeout_ms(void);
> +
> /*
> * Attempt to create a lockfile for the file at `path` and return a
> * file descriptor for writing to it, or -1 on error. The flags
> @@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(
> struct lock_file *lk, const char *path,
> int flags)
> {
> - return hold_lock_file_for_update_timeout(lk, path, flags, 0);
> + return hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms() );
Style nit: remove the space after the function call.
Thanks,
Denton
> }
>
> /*
> --
> 2.13.7
>
