{"thread":{"id":"52284","subject":"[PATCH] enable a timeout for hold_lock_file_for_update","startedAt":"2019-11-18T13:47:55Z","lastAt":"2019-11-20T02:38:21Z","messageCount":4,"participants":["Martin Nicolay","Denton Liu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"386452","messageId":"20191118134750.1901ED756F@wsmn.osm-gmbh.de","threadId":"52284","inReplyTo":null,"subject":"[PATCH] enable a timeout for hold_lock_file_for_update","fromName":"Martin Nicolay","fromEmail":"m.nicolay@osm-ag.de","sentAt":"2019-11-18T13:42:17Z","receivedAt":"2019-11-18T13:47:55Z","isPatch":true,"sender":{"key":"m.nicolay@osm-ag.de","avatar":null},"body":"The new funktion get_files_lock_timeout_ms reads the config\ncore.filesLockTimeout analog get_files_ref_lock_timeout_ms.\n\nThis value is used in hold_lock_file_for_update instead of the\nfixed value 0.\n---\nWhile working with complex scripts invoking git multiple times\nmy editor (emacs with standard version control) detects the\nchanges and apparently calls \"git status\". This leads to abort\nin \"git stash\". With this patch and an appropriate value\ncore.fileslocktimeout this problem goes away.\n\nWhile it may be possible to patch the elisp scripts of emacs (and\nall other similar callers) to call \"git status\" with\n--no-optional-locks it seems to me a better approarch to solve this\nproblem at its root: calling hold_lock_file_for_update_timeout with\na timeout of 0 ms.\n\nThe implementation with the function get_files_lock_timeout_ms is \nadopted from a similar usage of get_files_ref_lock_timeout_ms.\n\nBTW: is there a way to link this version of the patch to the previous\nversion to reduce the work for reviewers?\n\n Documentation/config/core.txt |  6 ++++++\n lockfile.c                    | 16 ++++++++++++++++\n lockfile.h                    |  4 +++-\n 3 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 852d2ba37a..230ea02560 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -482,6 +482,12 @@ core.packedRefsTimeout::\n \tall; -1 means to try indefinitely. Default is 1000 (i.e.,\n \tretry for 1 second).\n \n+core.filesLockTimeout::\n+\tThe length of time, in milliseconds, to retry when trying to\n+\tlock an individual file. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 0 (i.e., don't\n+\tretry at all).\n+\n core.pager::\n \tText viewer for use by Git commands (e.g., 'less').  The value\n \tis meant to be interpreted by the shell.  The order of preference\ndiff --git a/lockfile.c b/lockfile.c\nindex 8e8ab4f29f..7301f393d6 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -145,6 +145,22 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t}\n }\n \n+/*\n+ * Get timeout for hold_lock_file_for_update.\n+ */\n+long get_files_lock_timeout_ms(void)\n+{\n+\tstatic int configured = 0;\n+\n+\tstatic int timeout_ms = 0; /* default */\n+\tif (!configured) {\n+\t\tgit_config_get_int(\"core.fileslocktimeout\", &timeout_ms);\n+\t\tconfigured = 1;\n+\t}\n+\n+\treturn timeout_ms;\n+}\n+\n void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n {\n \tif (err == EEXIST) {\ndiff --git a/lockfile.h b/lockfile.h\nindex 9843053ce8..a0520e6a7b 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -163,6 +163,8 @@ int hold_lock_file_for_update_timeout(\n \t\tstruct lock_file *lk, const char *path,\n \t\tint flags, long timeout_ms);\n \n+long get_files_lock_timeout_ms(void);\n+\n /*\n  * Attempt to create a lockfile for the file at `path` and return a\n  * file descriptor for writing to it, or -1 on error. The flags\n@@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(\n \t\tstruct lock_file *lk, const char *path,\n \t\tint flags)\n {\n-\treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n+\treturn hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms() );\n }\n \n /*\n-- \n2.13.7\n\n"},{"id":"386460","messageId":"20191118175732.GA11649@generichostname","threadId":"52284","inReplyTo":"20191118134750.1901ED756F@wsmn.osm-gmbh.de","subject":"Re: [PATCH] enable a timeout for hold_lock_file_for_update","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-11-18T17:57:32Z","receivedAt":"2019-11-18T17:57:37Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Martin,\n\nThanks for the patch. I'm not too familiar with the lockfile codebase of\ngit but here are some general comments:\n\n> Subject: [PATCH] enable a timeout for hold_lock_file_for_update\n\nWe prefer subjects in the format of \"<area>: <brief subject>\" so\nperhaps, we could rewrite this to:\n\n\tlockfile: learn core.filesLockTimeout configuration\n\nOn Mon, Nov 18, 2019 at 02:42:17PM +0100, Martin Nicolay wrote:\n> The new funktion get_files_lock_timeout_ms reads the config\n\ns/funktion/function/\n\n> core.filesLockTimeout analog get_files_ref_lock_timeout_ms.\n\nPerhaps s/analog/similar to/ ?\n\n> \n> This value is used in hold_lock_file_for_update instead of the\n> fixed value 0.\n> ---\n> While working with complex scripts invoking git multiple times\n> my editor (emacs with standard version control) detects the\n> changes and apparently calls \"git status\". This leads to abort\n> in \"git stash\". With this patch and an appropriate value\n> core.fileslocktimeout this problem goes away.\n> \n> While it may be possible to patch the elisp scripts of emacs (and\n> all other similar callers) to call \"git status\" with\n> --no-optional-locks it seems to me a better approarch to solve this\n> problem at its root: calling hold_lock_file_for_update_timeout with\n> a timeout of 0 ms.\n> \n> The implementation with the function get_files_lock_timeout_ms is \n> adopted from a similar usage of get_files_ref_lock_timeout_ms.\n\nIt might be good to include the above three paragraphs in your commit\nmessage. Not only do they describe the change but, more importantly,\nthey describe _why_ the change is being made.\n\n> \n> BTW: is there a way to link this version of the patch to the previous\n> version to reduce the work for reviewers?\n\nWhen you generate your patches, run\n\n\tgit format-patch --in-reply-to=<r> -v<n>\n\nwhere <r> is the Message-ID of your last patch and where <n> is the\nversion of the patch (in this case, it should've been 2 since you sent\nout one before).\n\n> \n>  Documentation/config/core.txt |  6 ++++++\n>  lockfile.c                    | 16 ++++++++++++++++\n>  lockfile.h                    |  4 +++-\n>  3 files changed, 25 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index 852d2ba37a..230ea02560 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -482,6 +482,12 @@ core.packedRefsTimeout::\n>  \tall; -1 means to try indefinitely. Default is 1000 (i.e.,\n>  \tretry for 1 second).\n>  \n> +core.filesLockTimeout::\n> +\tThe length of time, in milliseconds, to retry when trying to\n> +\tlock an individual file. Value 0 means not to retry at\n> +\tall; -1 means to try indefinitely. Default is 0 (i.e., don't\n> +\tretry at all).\n> +\n>  core.pager::\n>  \tText viewer for use by Git commands (e.g., 'less').  The value\n>  \tis meant to be interpreted by the shell.  The order of preference\n> diff --git a/lockfile.c b/lockfile.c\n> index 8e8ab4f29f..7301f393d6 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -145,6 +145,22 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n>  \t}\n>  }\n>  \n> +/*\n> + * Get timeout for hold_lock_file_for_update.\n> + */\n> +long get_files_lock_timeout_ms(void)\n> +{\n> +\tstatic int configured = 0;\n> +\n> +\tstatic int timeout_ms = 0; /* default */\n> +\tif (!configured) {\n> +\t\tgit_config_get_int(\"core.fileslocktimeout\", &timeout_ms);\n> +\t\tconfigured = 1;\n> +\t}\n> +\n> +\treturn timeout_ms;\n> +}\n> +\n>  void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n>  {\n>  \tif (err == EEXIST) {\n> diff --git a/lockfile.h b/lockfile.h\n> index 9843053ce8..a0520e6a7b 100644\n> --- a/lockfile.h\n> +++ b/lockfile.h\n> @@ -163,6 +163,8 @@ int hold_lock_file_for_update_timeout(\n>  \t\tstruct lock_file *lk, const char *path,\n>  \t\tint flags, long timeout_ms);\n>  \n> +long get_files_lock_timeout_ms(void);\n> +\n>  /*\n>   * Attempt to create a lockfile for the file at `path` and return a\n>   * file descriptor for writing to it, or -1 on error. The flags\n> @@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(\n>  \t\tstruct lock_file *lk, const char *path,\n>  \t\tint flags)\n>  {\n> -\treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n> +\treturn hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms() );\n\nStyle nit: remove the space after the function call.\n\nThanks,\n\nDenton\n\n>  }\n>  \n>  /*\n> -- \n> 2.13.7\n> \n"},{"id":"386519","messageId":"20191119150747.E5AF2D756F@wsmn.osm-gmbh.de","threadId":"52284","inReplyTo":"20191118134750.1901ED756F@wsmn.osm-gmbh.de","subject":"[PATCH v3] lockfile: learn core.filesLockTimeout configuration","fromName":"Martin Nicolay","fromEmail":"m.nicolay@osm-ag.de","sentAt":"2019-11-19T14:56:05Z","receivedAt":"2019-11-19T15:07:53Z","isPatch":true,"sender":{"key":"m.nicolay@osm-ag.de","avatar":null},"body":"The new function get_files_lock_timeout_ms reads the config\ncore.filesLockTimeout similar to get_files_ref_lock_timeout_ms.\nThis value is used in hold_lock_file_for_update instead of the\nfixed value 0.\n\nWhile working with complex scripts invoking git multiple times\nmy editor (emacs with standard version control) detects the\nchanges and apparently calls \"git status\". This leads to abort\nin \"git stash\". With this patch and an appropriate value\ncore.filesLockTimeout this problem goes away.\n\nWhile it may be possible to patch the elisp scripts of emacs (and\nall other similar callers) to call \"git status\" with\n--no-optional-locks it seems to me a better approarch to solve this\nproblem at its root: calling hold_lock_file_for_update_timeout with\na timeout of 0 ms.\n\nThe implementation with the function get_files_lock_timeout_ms is\nadopted from a similar usage of get_files_ref_lock_timeout_ms.\n---\n Documentation/config/core.txt |  6 ++++++\n lockfile.c                    | 16 ++++++++++++++++\n lockfile.h                    |  4 +++-\n 3 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 852d2ba37a..230ea02560 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -482,6 +482,12 @@ core.packedRefsTimeout::\n \tall; -1 means to try indefinitely. Default is 1000 (i.e.,\n \tretry for 1 second).\n \n+core.filesLockTimeout::\n+\tThe length of time, in milliseconds, to retry when trying to\n+\tlock an individual file. Value 0 means not to retry at\n+\tall; -1 means to try indefinitely. Default is 0 (i.e., don't\n+\tretry at all).\n+\n core.pager::\n \tText viewer for use by Git commands (e.g., 'less').  The value\n \tis meant to be interpreted by the shell.  The order of preference\ndiff --git a/lockfile.c b/lockfile.c\nindex 8e8ab4f29f..7301f393d6 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -145,6 +145,22 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n \t}\n }\n \n+/*\n+ * Get timeout for hold_lock_file_for_update.\n+ */\n+long get_files_lock_timeout_ms(void)\n+{\n+\tstatic int configured = 0;\n+\n+\tstatic int timeout_ms = 0; /* default */\n+\tif (!configured) {\n+\t\tgit_config_get_int(\"core.fileslocktimeout\", &timeout_ms);\n+\t\tconfigured = 1;\n+\t}\n+\n+\treturn timeout_ms;\n+}\n+\n void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n {\n \tif (err == EEXIST) {\ndiff --git a/lockfile.h b/lockfile.h\nindex 9843053ce8..c7e7db0d5e 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -163,6 +163,8 @@ int hold_lock_file_for_update_timeout(\n \t\tstruct lock_file *lk, const char *path,\n \t\tint flags, long timeout_ms);\n \n+long get_files_lock_timeout_ms(void);\n+\n /*\n  * Attempt to create a lockfile for the file at `path` and return a\n  * file descriptor for writing to it, or -1 on error. The flags\n@@ -172,7 +174,7 @@ static inline int hold_lock_file_for_update(\n \t\tstruct lock_file *lk, const char *path,\n \t\tint flags)\n {\n-\treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n+\treturn hold_lock_file_for_update_timeout(lk, path, flags, get_files_lock_timeout_ms());\n }\n \n /*\n-- \n2.13.7\n\n"},{"id":"386600","messageId":"xmqqd0dne0hc.fsf@gitster-ct.c.googlers.com","threadId":"52284","inReplyTo":"20191119150747.E5AF2D756F@wsmn.osm-gmbh.de","subject":"Re: [PATCH v3] lockfile: learn core.filesLockTimeout configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-20T02:38:07Z","receivedAt":"2019-11-20T02:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Nicolay <m.nicolay@osm-ag.de> writes:\n\n> The new function get_files_lock_timeout_ms reads the config\n> core.filesLockTimeout similar to get_files_ref_lock_timeout_ms.\n> This value is used in hold_lock_file_for_update instead of the\n> fixed value 0.\n>\n> While working with complex scripts invoking git multiple times\n> my editor (emacs with standard version control) detects the\n> changes and apparently calls \"git status\". This leads to abort\n> in \"git stash\". With this patch and an appropriate value\n> core.filesLockTimeout this problem goes away.\n>\n> While it may be possible to patch the elisp scripts of emacs (and\n> all other similar callers) to call \"git status\" with\n> --no-optional-locks it seems to me a better approarch to solve this\n> problem at its root: calling hold_lock_file_for_update_timeout with\n> a timeout of 0 ms.\n>\n> The implementation with the function get_files_lock_timeout_ms is\n> adopted from a similar usage of get_files_ref_lock_timeout_ms.\n> ---\n\nMissing sign-off before the three-dash line.\n\nI think the last paragraph can be left without.  It is not like\nthere are many other sensible ways to get a configured value without\nmaking repeated calls to git_config_get_int().\n\n> +core.filesLockTimeout::\n> +\tThe length of time, in milliseconds, to retry when trying to\n> +\tlock an individual file. Value 0 means not to retry at\n> +\tall; -1 means to try indefinitely. Default is 0 (i.e., don't\n> +\tretry at all).\n\nWill there be *NO* callers of the lockfile API functions that do not\nhonor the value taken from this configuration variable after this\npatch is applied?\n\nOtherwise, users who set this configuration variable and hit a\ncodepath that locks files without asking get_files_lock_timeout_ms()\nhow it should retry would find the above description inaccurate,\nwouldn't they?\n\nThere is another question---is it safe to make all attempts to\ncreate a lockfile retry, possibly forever?  I do not offhand think\nof any concrete example, but I would not be surprised if there is a\ncodepath that would never want to retry but want to fail upon the\nfirst failure (i.e. wants to always use value 0 without allowing the\nusers to configure).\n\nSo, unless the answers to the above two questions are \"with this\npatch, all attempts to lock will honor this variable\" and \"yes it is\nsafe because ...\", some tweak of the description may be necessary to\nhint the readers that not all the locks will retry by honoring this\nvariable.\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 8e8ab4f29f..7301f393d6 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -145,6 +145,22 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,\n>  \t}\n>  }\n>  \n> +/*\n> + * Get timeout for hold_lock_file_for_update.\n> + */\n> +long get_files_lock_timeout_ms(void)\n\nShouldn't this return \"int\", which is the type you get from the\nunderlying configuration API?\n\nI also wondered if this has to be extern at all; the reason why this\npatch makes it extern is purely because hold_lock_file_for_update()\nis defined as a static inline in lockfile.h to expand to another\nfunction, so any file that includes lockfile.h and calls that static\ninline must be able to see this name.\n\nBecause all of the lockfile public API functions are about accessing\nfilesystem entities, I am not sure if making this many thin wrappers\nstatic inlines to potentially save one extra intermediate call is\nworth it (there are 10 of them).  For now, I think the organization\nthis patch leaves is OK, but we may later want to examine these\nstatic inline wrappers and consider turning them into a regular\nextern functions.\n\nI do not think other static inline wrappers in the lockfile.h is\nhurting right now, but with this change, hold_lock_file_for_update()\ncertainly is.  If it becomes just a usual extern function, we do not\nhave to expose the get-files-lock-timeout-ms helper at all (and\nworry about its name, as globally visible names needs extra care to\nhelp developers).  In any case, that is outside the scope of this\npatch, but a potential follow-on work after this patch stabilizes.\n\n> +{\n> +\tstatic int configured = 0;\n> +\n> +\tstatic int timeout_ms = 0; /* default */\n> +\tif (!configured) {\n\n - Do not explicitly initialize statics to zero (instead, let BSS take\n   care of it).\n - Lose the blank line between the declarations.\n - Have a blank line after the last decl and the first statement.\n\n> +\t\tgit_config_get_int(\"core.fileslocktimeout\", &timeout_ms);\n> +\t\tconfigured = 1;\n> +\t}\n> +\n> +\treturn timeout_ms;\n> +}\n\nBy the way, why are these called file*S*locktimeout (both the\nend-user facing configuration variable and the function name)?\n\nAlso, \"lock timeout\" is a misleading name for both the configuration\nvariable and the function.  It sounds as if after that many\nmilliseconds, the system will automatically break your lock if you\ndo not perform the action under the lock quickly enough, but that is\na wrong message to send to the end users.\n\nThe timeout is about retrying to acquire the lock, so the name most\nlikely needs to have words \"lock\", \"retry\", and \"timeout\" somewhere\nin it.\n\nPerhaps core.lockRetryTimeout or something?  I dunno.  You may want\nto wait before others offer a better name before rerolling, as I am\nnot very good at naming things.\n\nThanks.\n"}]}