Re: [PATCH] enable a timeout for hold_lock_file_for_update

[Denton Liu <liu.denton@gmail.com>]

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
>