[PATCH] enable a timeout for hold_lock_file_for_update

[PATCH] enable a timeout for hold_lock_file_for_update

[Martin Nicolay]

The new funktion get_files_lock_timeout_ms reads the config
core.fileslocktimeout analog get_files_ref_lock_timeout_ms.

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 detects the changes and calls "git status". This leads to
aborts in "git-stash". With this patch and an appropriate value
core.fileslocktimeout this problem goes away.

 lockfile.c | 15 +++++++++++++++
 lockfile.h |  4 +++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lockfile.c b/lockfile.c
index 8e8ab4f29f..ac19de8937 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -145,6 +145,21 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 	}
 }
 
+long get_files_lock_timeout_ms(void)
+{
+	static int configured = 0;
+
+	/* The default timeout is 100 ms: */
+	static int timeout_ms = 100;
+
+	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() );
 }
 
 /*
-- 
2.13.7

[PATCH] enable a timeout for hold_lock_file_for_update

[Martin Nicolay]

The new funktion get_files_lock_timeout_ms reads the config
core.filesLockTimeout analog get_files_ref_lock_timeout_ms.

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.

BTW: is there a way to link this version of the patch to the previous
version to reduce the work for reviewers?

 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() );
 }
 
 /*
-- 
2.13.7

Re: [PATCH] enable a timeout for hold_lock_file_for_update

[Denton Liu]

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
>