{"thread":{"id":"65429","subject":"[PATCH] config: retry acquiring config.lock for 100ms","startedAt":"2026-04-03T10:07:43Z","lastAt":"2026-07-23T19:54:03Z","messageCount":18,"participants":["Joerg Thalheim","Junio C Hamano","Patrick Steinhardt","Jörg Thalheim","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540846","messageId":"20260403100135.3901610-1-joerg@thalheim.io","threadId":"65429","inReplyTo":null,"subject":"[PATCH] config: retry acquiring config.lock for 100ms","fromName":"Joerg Thalheim","fromEmail":"joerg@thalheim.io","sentAt":"2026-04-03T10:01:35Z","receivedAt":"2026-04-03T10:07:43Z","isPatch":true,"body":"From: Jörg Thalheim \n\nWhen multiple processes write to a config file concurrently, they\ncontend on its \".lock\" file, which is acquired via open(O_EXCL) with\nno retry. The losers fail immediately with \"could not lock config\nfile\". Two processes writing unrelated keys (say, \"branch.a.remote\"\nand \"branch.b.remote\") have no semantic conflict, yet one of them\nfails for a purely mechanical reason.\n\nThis bites in practice when running `git worktree add -b` concurrently\nagainst the same repository. Each invocation makes several writes to\n\".git/config\" to set up branch tracking, and tooling that creates\nworktrees in parallel sees intermittent failures. Worse, `git worktree\nadd` does not propagate the failed config write to its exit code: the\nworktree is created and the command exits 0, but tracking\nconfiguration is silently dropped.\n\nThe lock is held only for the duration of rewriting a small file, so\nretrying for 100 ms papers over any realistic contention while still\nfailing fast if a stale lock has been left behind by a crashed\nprocess. This mirrors what we already do for individual reference\nlocks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n2017-08-21)).\n\nSigned-off-by: Jörg Thalheim \n---\n config.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 156f2a24fa..f7aff8725d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)\n \treturn prepared;\n }\n \n+/*\n+ * How long to retry acquiring config.lock when another process holds it.\n+ * The lock is held only for the duration of rewriting a small file, so\n+ * 100 ms covers any realistic contention while still failing fast if\n+ * a stale lock has been left behind by a crashed process.\n+ */\n+#define CONFIG_LOCK_TIMEOUT_MS 100\n+\n static void validate_comment_string(const char *comment)\n {\n \tsize_t leading_blanks;\n@@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n+\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n \tif (fd < 0) {\n \t\terror_errno(_(\"could not lock config file %s\"), config_filename);\n \t\tret = CONFIG_NO_LOCK;\n@@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(\n \tif (!config_filename)\n \t\tconfig_filename = filename_buf = repo_git_path(r, \"config\");\n \n-\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n+\t\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n \tif (out_fd < 0) {\n \t\tret = error(_(\"could not lock config file %s\"), config_filename);\n \t\tgoto out;\n-- \n2.53.0\n\n"},{"id":"540860","messageId":"xmqqzf3klym1.fsf@gitster.g","threadId":"65429","inReplyTo":"20260403100135.3901610-1-joerg@thalheim.io","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-03T17:53:10Z","receivedAt":"2026-04-03T17:53:13Z","isPatch":true,"body":"Joerg Thalheim writes:\n\n> From: Jörg Thalheim \n>\n> When multiple processes write to a config file concurrently, they\n> contend on its \".lock\" file, which is acquired via open(O_EXCL) with\n> no retry. The losers fail immediately with \"could not lock config\n> file\". Two processes writing unrelated keys (say, \"branch.a.remote\"\n> and \"branch.b.remote\") have no semantic conflict, yet one of them\n> fails for a purely mechanical reason.\n>\n> This bites in practice when running `git worktree add -b` concurrently\n> against the same repository. Each invocation makes several writes to\n> \".git/config\" to set up branch tracking, and tooling that creates\n> worktrees in parallel sees intermittent failures. Worse, `git worktree\n> add` does not propagate the failed config write to its exit code: the\n> worktree is created and the command exits 0, but tracking\n> configuration is silently dropped.\n>\n> The lock is held only for the duration of rewriting a small file, so\n> retrying for 100 ms papers over any realistic contention while still\n> failing fast if a stale lock has been left behind by a crashed\n> process. This mirrors what we already do for individual reference\n> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> 2017-08-21)).\n>\n> Signed-off-by: Jörg Thalheim \n> ---\n> config.c | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n\nOK. This is consistent with the default used for a ref update with\nfiles backend. Various subsystems use their own value randomly\nchosen out of thin air, it seems. credential-store uses 1000ms, gc\nuses 150ms to interact with launchctl, refs have their own per\nbackend, etc.\n\n> diff --git a/config.c b/config.c\n> index 156f2a24fa..f7aff8725d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)\n> \treturn prepared;\n> }\n> \n> +/*\n> + * How long to retry acquiring config.lock when another process holds it.\n> + * The lock is held only for the duration of rewriting a small file, so\n> + * 100 ms covers any realistic contention while still failing fast if\n> + * a stale lock has been left behind by a crashed process.\n> + */\n> +#define CONFIG_LOCK_TIMEOUT_MS 100\n> +\n\nMaking this configurable would make a cute chicken-and-egg problem?\n\n;-) No, no need for that---just kidding.\n\nWill queue. Thanks.\n\n> @@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n> \t * The lock serves a purpose in addition to locking: the new\n> \t * contents of .git/config will be written into it.\n> \t */\n> -\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n> +\tfd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n> +\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n> \tif (fd < 0) {\n> \t\terror_errno(_(\"could not lock config file %s\"), config_filename);\n> \t\tret = CONFIG_NO_LOCK;\n> @@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(\n> \tif (!config_filename)\n> \t\tconfig_filename = filename_buf = repo_git_path(r, \"config\");\n> \n> -\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n> +\tout_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n> +\t\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n> \tif (out_fd < 0) {\n> \t\tret = error(_(\"could not lock config file %s\"), config_filename);\n> \t\tgoto out;\n"},{"id":"541129","messageId":"adYvSZeN0ZVqwRhi@pks.im","threadId":"65429","inReplyTo":"20260403100135.3901610-1-joerg@thalheim.io","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-08T10:34:49Z","receivedAt":"2026-04-08T10:34:55Z","isPatch":true,"body":"On Fri, Apr 03, 2026 at 12:01:35PM +0200, Joerg Thalheim wrote:\n> From: Jörg Thalheim \n> \n> When multiple processes write to a config file concurrently, they\n> contend on its \".lock\" file, which is acquired via open(O_EXCL) with\n> no retry. The losers fail immediately with \"could not lock config\n> file\". Two processes writing unrelated keys (say, \"branch.a.remote\"\n> and \"branch.b.remote\") have no semantic conflict, yet one of them\n> fails for a purely mechanical reason.\n> \n> This bites in practice when running `git worktree add -b` concurrently\n> against the same repository. Each invocation makes several writes to\n> \".git/config\" to set up branch tracking, and tooling that creates\n> worktrees in parallel sees intermittent failures. Worse, `git worktree\n> add` does not propagate the failed config write to its exit code: the\n> worktree is created and the command exits 0, but tracking\n> configuration is silently dropped.\n\nThis very much sounds like a bug that is worth fixing independently.\n\n> The lock is held only for the duration of rewriting a small file, so\n> retrying for 100 ms papers over any realistic contention while still\n> failing fast if a stale lock has been left behind by a crashed\n> process. This mirrors what we already do for individual reference\n> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> 2017-08-21)).\n\nFamous last words :) Experience tells me that any timeout value that\nisn't excessive will eventually be hit in some production system. Which\nraises the question whether we want to make the timeout configurable,\nsimilar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n\n> diff --git a/config.c b/config.c\n> index 156f2a24fa..f7aff8725d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2903,6 +2903,14 @@ char *git_config_prepare_comment_string(const char *comment)\n> \treturn prepared;\n> }\n> \n> +/*\n> + * How long to retry acquiring config.lock when another process holds it.\n> + * The lock is held only for the duration of rewriting a small file, so\n> + * 100 ms covers any realistic contention while still failing fast if\n> + * a stale lock has been left behind by a crashed process.\n> + */\n> +#define CONFIG_LOCK_TIMEOUT_MS 100\n> +\n> static void validate_comment_string(const char *comment)\n> {\n> \tsize_t leading_blanks;\n> @@ -2986,7 +2994,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n> \t * The lock serves a purpose in addition to locking: the new\n> \t * contents of .git/config will be written into it.\n> \t */\n> -\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n> +\tfd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n> +\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n> \tif (fd < 0) {\n> \t\terror_errno(_(\"could not lock config file %s\"), config_filename);\n> \t\tret = CONFIG_NO_LOCK;\n> @@ -3331,7 +3340,8 @@ static int repo_config_copy_or_rename_section_in_file(\n> \tif (!config_filename)\n> \t\tconfig_filename = filename_buf = repo_git_path(r, \"config\");\n> \n> -\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n> +\tout_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n> +\t\t\t\t\t\t CONFIG_LOCK_TIMEOUT_MS);\n> \tif (out_fd < 0) {\n> \t\tret = error(_(\"could not lock config file %s\"), config_filename);\n> \t\tgoto out;\n\nOkay, so we now handle this more gracefully with concurrent writers. But\neven if we handle that more gracefully, we still don't really have any\nguarantee that the outcome will be reasonable, or even close.\n\nThe main difference to the locking timeouts we have for refs is that we\nhave an easy way to check for conflicts there: we simpliy verify that\nthe refs we want to update haven't moved from their expected value. We\nhave no such thing for our configuration though, so two processes that\nrace with one another will happily overwrite each other's changes.\n\nHonestly though, I'm not really sure what to make with this. We could\nof course also add some validation that the configuration we want to set\nhasn't been modified meanwhile. But that would now lead to a situation\nwhere we have to update every single caller in our tree to make use of\nthe new mechanism, which would be a bunch of work.\n\nAnd adding the timeout doesn't really change the status quo, either. We\nalready have the case that we'll happily overwrite changes made by\nconcurrent processes. The only thing that changes is that we make it\nmore likely for concurrent changes to succeed.\n\nSo I'm a bit cautious, but I think overall I'm fine with this?\n\nThanks!\n\nPatrick\n"},{"id":"542999","messageId":"xmqqcxz2vfpa.fsf@gitster.g","threadId":"65429","inReplyTo":"adYvSZeN0ZVqwRhi@pks.im","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-11T02:32:33Z","receivedAt":"2026-05-11T02:32:36Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n>> This bites in practice when running `git worktree add -b` concurrently\n>> against the same repository. Each invocation makes several writes to\n>> \".git/config\" to set up branch tracking, and tooling that creates\n>> worktrees in parallel sees intermittent failures. Worse, `git worktree\n>> add` does not propagate the failed config write to its exit code: the\n>> worktree is created and the command exits 0, but tracking\n>> configuration is silently dropped.\n>\n> This very much sounds like a bug that is worth fixing independently.\n>\n>> The lock is held only for the duration of rewriting a small file, so\n>> retrying for 100 ms papers over any realistic contention while still\n>> failing fast if a stale lock has been left behind by a crashed\n>> process. This mirrors what we already do for individual reference\n>> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n>> 2017-08-21)).\n>\n> Famous last words :) Experience tells me that any timeout value that\n> isn't excessive will eventually be hit in some production system. Which\n> raises the question whether we want to make the timeout configurable,\n> similar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n> ...\n> Honestly though, I'm not really sure what to make with this. We could\n> of course also add some validation that the configuration we want to set\n> hasn't been modified meanwhile. But that would now lead to a situation\n> where we have to update every single caller in our tree to make use of\n> the new mechanism, which would be a bunch of work.\n>\n> And adding the timeout doesn't really change the status quo, either. We\n> already have the case that we'll happily overwrite changes made by\n> concurrent processes. The only thing that changes is that we make it\n> more likely for concurrent changes to succeed.\n\nWe haven't heard any response to these points raised in the message\nI am responding to. Should I still keep the patch in my tree,\nhoping that a responses may come some day? I am tempted to discard\nthe topic as it has been quite a while since we last looked at it.\n\nThanks.\n"},{"id":"543026","messageId":"agGGP39V3_mZl7mp@pks.im","threadId":"65429","inReplyTo":"xmqqcxz2vfpa.fsf@gitster.g","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T07:33:19Z","receivedAt":"2026-05-11T07:33:25Z","isPatch":true,"body":"On Mon, May 11, 2026 at 11:32:33AM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> This bites in practice when running `git worktree add -b` concurrently\n> >> against the same repository. Each invocation makes several writes to\n> >> \".git/config\" to set up branch tracking, and tooling that creates\n> >> worktrees in parallel sees intermittent failures. Worse, `git worktree\n> >> add` does not propagate the failed config write to its exit code: the\n> >> worktree is created and the command exits 0, but tracking\n> >> configuration is silently dropped.\n> >\n> > This very much sounds like a bug that is worth fixing independently.\n> >\n> >> The lock is held only for the duration of rewriting a small file, so\n> >> retrying for 100 ms papers over any realistic contention while still\n> >> failing fast if a stale lock has been left behind by a crashed\n> >> process. This mirrors what we already do for individual reference\n> >> locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> >> 2017-08-21)).\n> >\n> > Famous last words :) Experience tells me that any timeout value that\n> > isn't excessive will eventually be hit in some production system. Which\n> > raises the question whether we want to make the timeout configurable,\n> > similar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n> > ...\n> > Honestly though, I'm not really sure what to make with this. We could\n> > of course also add some validation that the configuration we want to set\n> > hasn't been modified meanwhile. But that would now lead to a situation\n> > where we have to update every single caller in our tree to make use of\n> > the new mechanism, which would be a bunch of work.\n> >\n> > And adding the timeout doesn't really change the status quo, either. We\n> > already have the case that we'll happily overwrite changes made by\n> > concurrent processes. The only thing that changes is that we make it\n> > more likely for concurrent changes to succeed.\n> \n> We haven't heard any response to these points raised in the message\n> I am responding to. Should I still keep the patch in my tree,\n> hoping that a responses may come some day? I am tempted to discard\n> the topic as it has been quite a while since we last looked at it.\n\nSame here, let's discard it for now as it can be easily added back in\nonce a new version was posted or the feedback has been addressed.\n\nPatrick\n"},{"id":"543034","messageId":"91335804a092b09757331cac72092a3835020b3a@thalheim.io","threadId":"65429","inReplyTo":"xmqqcxz2vfpa.fsf@gitster.g","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Jörg Thalheim","fromEmail":"joerg@thalheim.io","sentAt":"2026-05-11T09:06:00Z","receivedAt":"2026-05-11T09:06:10Z","isPatch":true,"body":"I am not really sure what you want me to do here.\nI don't see how git can have this value configurable, given it's about reading the configuration itself.\nIs the user supposed via command line?\n\nMeanwhile the project I was facing this issue, added the required file lock in its own code,\nwhich has since then worked perfectly to fix my use case: https://github.com/raine/workmux/issues/116\n\n\n\nMay 11, 2026 at 4:32 AM, \"Junio C Hamano\" wrote:\n\n\n> \n> Patrick Steinhardt writes:\n> \n> > \n> > > \n> > > This bites in practice when running `git worktree add -b` concurrently\n> > > against the same repository. Each invocation makes several writes to\n> > > \".git/config\" to set up branch tracking, and tooling that creates\n> > > worktrees in parallel sees intermittent failures. Worse, `git worktree\n> > > add` does not propagate the failed config write to its exit code: the\n> > > worktree is created and the command exits 0, but tracking\n> > > configuration is silently dropped.\n> > > \n> > This very much sounds like a bug that is worth fixing independently.\n> > \n> > > \n> > > The lock is held only for the duration of rewriting a small file, so\n> > > retrying for 100 ms papers over any realistic contention while still\n> > > failing fast if a stale lock has been left behind by a crashed\n> > > process. This mirrors what we already do for individual reference\n> > > locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> > > 2017-08-21)).\n> > > \n> > Famous last words :) Experience tells me that any timeout value that\n> > isn't excessive will eventually be hit in some production system. Which\n> > raises the question whether we want to make the timeout configurable,\n> > similar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n> > ...\n> > Honestly though, I'm not really sure what to make with this. We could\n> > of course also add some validation that the configuration we want to set\n> > hasn't been modified meanwhile. But that would now lead to a situation\n> > where we have to update every single caller in our tree to make use of\n> > the new mechanism, which would be a bunch of work.\n> > \n> > And adding the timeout doesn't really change the status quo, either. We\n> > already have the case that we'll happily overwrite changes made by\n> > concurrent processes. The only thing that changes is that we make it\n> > more likely for concurrent changes to succeed.\n> > \n> We haven't heard any response to these points raised in the message\n> I am responding to. Should I still keep the patch in my tree,\n> hoping that a responses may come some day? I am tempted to discard\n> the topic as it has been quite a while since we last looked at it.\n> \n> Thanks.\n>\n"},{"id":"543041","messageId":"agGo9Prt8Hs2gbic@pks.im","threadId":"65429","inReplyTo":"91335804a092b09757331cac72092a3835020b3a@thalheim.io","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-11T10:01:24Z","receivedAt":"2026-05-11T10:01:31Z","isPatch":true,"body":"On Mon, May 11, 2026 at 09:06:00AM +0000, Jörg Thalheim wrote:\n> May 11, 2026 at 4:32 AM, \"Junio C Hamano\" wrote:\n> > Patrick Steinhardt writes:\n> > > > This bites in practice when running `git worktree add -b` concurrently\n> > > > against the same repository. Each invocation makes several writes to\n> > > > \".git/config\" to set up branch tracking, and tooling that creates\n> > > > worktrees in parallel sees intermittent failures. Worse, `git worktree\n> > > > add` does not propagate the failed config write to its exit code: the\n> > > > worktree is created and the command exits 0, but tracking\n> > > > configuration is silently dropped.\n> > > > \n> > > This very much sounds like a bug that is worth fixing independently.\n> > > \n> > > > \n> > > > The lock is held only for the duration of rewriting a small file, so\n> > > > retrying for 100 ms papers over any realistic contention while still\n> > > > failing fast if a stale lock has been left behind by a crashed\n> > > > process. This mirrors what we already do for individual reference\n> > > > locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> > > > 2017-08-21)).\n> > > > \n> > > Famous last words :) Experience tells me that any timeout value that\n> > > isn't excessive will eventually be hit in some production system. Which\n> > > raises the question whether we want to make the timeout configurable,\n> > > similar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n> > > ...\n> > > Honestly though, I'm not really sure what to make with this. We could\n> > > of course also add some validation that the configuration we want to set\n> > > hasn't been modified meanwhile. But that would now lead to a situation\n> > > where we have to update every single caller in our tree to make use of\n> > > the new mechanism, which would be a bunch of work.\n> > > \n> > > And adding the timeout doesn't really change the status quo, either. We\n> > > already have the case that we'll happily overwrite changes made by\n> > > concurrent processes. The only thing that changes is that we make it\n> > > more likely for concurrent changes to succeed.\n> > > \n> > We haven't heard any response to these points raised in the message\n> > I am responding to. Should I still keep the patch in my tree,\n> > hoping that a responses may come some day? I am tempted to discard\n> > the topic as it has been quite a while since we last looked at it.\n> \n> I am not really sure what you want me to do here.\n\nIn general, the idea here is to engage in a discussion that can\nultimately lead to one of two outcomes:\n\n - The discussion surfaces an area the author hasn't thought about, so\n the patch is adapted accordingly.\n\n - The discussion shows that the author already did think about the\n issue, but hasn't documented the assumptions. In this case, it\n should be the commit message that gets adapted.\n\n> I don't see how git can have this value configurable, given it's about\n> reading the configuration itself. Is the user supposed via command\n> line?\n\nThis is a fair point indeed. But if it's not possible to change via the\nconfiguration itself, then the next-best thing might be to introduce an\nenvironment variable that allows configuring it.\n\nThe other aspect that wasn't discussed in the commit message is how\nconcurrent writes are handled, both when they are non-conflicting\n(updating different keys) and when they are conflicting (updating the\nsame key). After spending some more time in the code I think it's\nultimately nothing we have to worry about too much, as we only start\nreading the configuration after we've locked it.\n\nSo in the semantically non-conflicting case there isn't really much of a\nrace, because things already work as expected. But in the semantically\nconflicting case it's a bit different, as the latter writer will\noverwrite the result of the former one. In theory it would be possible\nto detect such conflicts by:\n\n - Reading the configuration file.\n\n - Taking the lock.\n\n - Rereading the configuration to check for conflicts.\n\nBut even that is racy as the first writer might have succeeded before we\nread the configuration the first time. So I'm not sure whether we can do\nanything about that in the first place, as the race basically exists in\nthe outer loop controlled by the caller.\n\nSo there probably isn't much we can do about that, and unless I missed\nsomething I think your timeout is sensible. But ideally, such nuances\nwould be discussed as part of the commit message so that reviewers and\nfuture readers are made aware of them.\n\nThanks!\n\nPatrick\n"},{"id":"543474","messageId":"409d05a5-235b-6b19-5a33-a4e613dd447c@gmx.de","threadId":"65429","inReplyTo":"agGo9Prt8Hs2gbic@pks.im","subject":"Re: [PATCH] config: retry acquiring config.lock for 100ms","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2026-05-17T11:27:55Z","receivedAt":"2026-05-17T11:28:03Z","isPatch":true,"body":"Hi Patrick & Jörg,\n\nOn Mon, 11 May 2026, Patrick Steinhardt wrote:\n\n> On Mon, May 11, 2026 at 09:06:00AM +0000, Jörg Thalheim wrote:\n> > May 11, 2026 at 4:32 AM, \"Junio C Hamano\" > mailto:gitster@pobox.com?to=%22Junio%20C%20Hamano%22%20%3Cgitster%40pobox.com%3E\n> > > wrote:\n> > > Patrick Steinhardt writes:\n> > > > > This bites in practice when running `git worktree add -b` concurrently\n> > > > > against the same repository. Each invocation makes several writes to\n> > > > > \".git/config\" to set up branch tracking, and tooling that creates\n> > > > > worktrees in parallel sees intermittent failures. Worse, `git worktree\n> > > > > add` does not propagate the failed config write to its exit code: the\n> > > > > worktree is created and the command exits 0, but tracking\n> > > > > configuration is silently dropped.\n> > > > > \n> > > > This very much sounds like a bug that is worth fixing independently.\n> > > > \n> > > > > \n> > > > > The lock is held only for the duration of rewriting a small file, so\n> > > > > retrying for 100 ms papers over any realistic contention while still\n> > > > > failing fast if a stale lock has been left behind by a crashed\n> > > > > process. This mirrors what we already do for individual reference\n> > > > > locks (4ff0f01cb7 (refs: retry acquiring reference locks for 100ms,\n> > > > > 2017-08-21)).\n> > > > > \n> > > > Famous last words :) Experience tells me that any timeout value that\n> > > > isn't excessive will eventually be hit in some production system. Which\n> > > > raises the question whether we want to make the timeout configurable,\n> > > > similar to \"core.filesRefLockTimeout\" and \"core.packedRefsTimeout\".\n> > > > ...\n> > > > Honestly though, I'm not really sure what to make with this. We could\n> > > > of course also add some validation that the configuration we want to set\n> > > > hasn't been modified meanwhile. But that would now lead to a situation\n> > > > where we have to update every single caller in our tree to make use of\n> > > > the new mechanism, which would be a bunch of work.\n> > > > \n> > > > And adding the timeout doesn't really change the status quo, either. We\n> > > > already have the case that we'll happily overwrite changes made by\n> > > > concurrent processes. The only thing that changes is that we make it\n> > > > more likely for concurrent changes to succeed.\n> > > > \n> > > We haven't heard any response to these points raised in the message\n> > > I am responding to. Should I still keep the patch in my tree,\n> > > hoping that a responses may come some day? I am tempted to discard\n> > > the topic as it has been quite a while since we last looked at it.\n> > \n> > I am not really sure what you want me to do here.\n> \n> In general, the idea here is to engage in a discussion that can\n> ultimately lead to one of two outcomes:\n> \n> - The discussion surfaces an area the author hasn't thought about, so\n> the patch is adapted accordingly.\n> \n> - The discussion shows that the author already did think about the\n> issue, but hasn't documented the assumptions. In this case, it\n> should be the commit message that gets adapted.\n\nFor what it's worth, I meant to chime in earlier, but obligations kept\npreventing me from setting aside the time to do so. Well, better late than\nnever.\n\n> > I don't see how git can have this value configurable, given it's about\n> > reading the configuration itself. Is the user supposed via command\n> > line?\n> \n> This is a fair point indeed. But if it's not possible to change via the\n> configuration itself, then the next-best thing might be to introduce an\n> environment variable that allows configuring it.\n\nWell, given that the config is read first before it's written, it is\ntotally possible to configure a timeout via the config, and I have some\nreal-world proof that this works as intended (see below).\n\n> The other aspect that wasn't discussed in the commit message is how\n> concurrent writes are handled, both when they are non-conflicting\n> (updating different keys) and when they are conflicting (updating the\n> same key). After spending some more time in the code I think it's\n> ultimately nothing we have to worry about too much, as we only start\n> reading the configuration after we've locked it.\n\nCorrect. I had performed this analysis myself when writing a similar patch\nto fix problems in Scalar's Functional Test suite, which wants to register\n_many_ Scalar repositories with ~/.gitconfig concurrently. The current\niteration of the patch can be found here:\n\nhttps://github.com/microsoft/git/commit/a1c2d97cb61bc3697086d1749de848586df2ec54\n\nIt does include the config setting, leaving the default as \"off\" (but I\nmissed the separate code path to rename sections, which has _independent_\ncode that also wants to lock the config file, which your patch did not\nmiss). The subsequent child commit\n\nhttps://github.com/microsoft/git/commit/5d365c1f332b8d2214ae9c44970d6370ed9caffc\n\nconfigures it to 150ms in Scalar repositories only. This is notably larger\nthan the 100ms you suggested, and it is rooted in the fact that NTFS I/O\ncharacteristics are unfortunately in need of a wider margin. In other\nwords, the optimal value depends on the operating system (and the CPU\nload, as Junio had pointed out).\n\nFor the record, feel free to adopt whatever you want from my patches for\nyour next iteration (but also feel free to ignore all of it).\n\n> So in the semantically non-conflicting case there isn't really much of a\n> race, because things already work as expected. But in the semantically\n> conflicting case it's a bit different, as the latter writer will\n> overwrite the result of the former one. In theory it would be possible\n> to detect such conflicts by:\n> \n> - Reading the configuration file.\n> \n> - Taking the lock.\n> \n> - Rereading the configuration to check for conflicts.\n> \n> But even that is racy as the first writer might have succeeded before we\n> read the configuration the first time. So I'm not sure whether we can do\n> anything about that in the first place, as the race basically exists in\n> the outer loop controlled by the caller.\n> \n> So there probably isn't much we can do about that, and unless I missed\n> something I think your timeout is sensible. But ideally, such nuances\n> would be discussed as part of the commit message so that reviewers and\n> future readers are made aware of them.\n\nI agree. Complex cases like this would require a sort of transactional\nsupport to be added to `git config`, and that would in and of itself open\na can of worms I'm not sure we should open unless there is a concrete use\ncase that bites enough real-world scenarios to require acting upon.\n\nCiao,\nJohannes\n"},{"id":"543477","messageId":"20260517132111.1014901-1-joerg@thalheim.io","threadId":"65429","inReplyTo":"409d05a5-235b-6b19-5a33-a4e613dd447c@gmx.de","subject":"[PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Joerg Thalheim","fromEmail":"joerg@thalheim.io","sentAt":"2026-05-17T13:21:11Z","receivedAt":"2026-05-17T13:21:25Z","isPatch":true,"body":"From: Jörg Thalheim \n\nConcurrent config writers race for the \".lock\" file, which is taken\nwith open(O_EXCL) and no retry, so the losers fail right away with\n\"could not lock config file\".\n\nThis shows up with parallel \"git worktree add -b\" against the same\nrepository: each one writes a couple of branch.* keys and the losers\nfail at random. Worse, \"git worktree add\" doesn't propagate that\nfailure to its exit code, so the tracking config is silently dropped.\n(The swallowed error is a separate bug.)\n\nRetry instead of giving up on the first EEXIST. The lock is only held\nwhile rewriting a small file, so the loser only has to wait out the\nother writers. Same approach as 4ff0f01cb7 (refs: retry acquiring\nreference locks for 100ms, 2017-08-21).\n\nOn the semantics: the on-disk config is read only after the lock is\ntaken, so writers touching different keys can't lose each other's\nchange. Writers touching the same key still get last-writer-wins, but\nthat is already the case today and would need a compare-and-swap config\nAPI to fix. The retry only turns hard failures into successes.\n\nDefault to 1000ms, like core.packedRefsTimeout: same shape of problem,\none shared file everyone serializes through. A larger timeout only\ncosts anything when a stale lock is left behind by a crash, which is\nrare; a smaller one fails spuriously on slow filesystems (NTFS has\nbeen seen needing more than 100ms). Make it configurable as\ncore.configLockTimeout. There is no chicken-and-egg problem: we read\nthe config before we lock it.\n\nmicrosoft/git carries a similar patch (core.configWriteLockTimeoutMS,\ndefault off) for Scalar's tests. Defaulting to non-zero here because\nthe worktree case fails silently.\n\nHelped-by: Patrick Steinhardt \nHelped-by: Johannes Schindelin \nSigned-off-by: Jörg Thalheim \n---\nThanks for the review and for poking me, this had fallen off my radar.\n\nv1 -> v2:\n\n- added core.configLockTimeout. Johannes is right that there is no\n chicken-and-egg problem (config is read before the lock), so no env\n var needed.\n- default bumped to 1000ms; packed-refs is the closer precedent and it\n keeps NTFS out of trouble.\n- commit message now covers the read-after-lock / last-writer-wins\n semantics Patrick asked about.\n- added tests; existing stale-lock tests in t3200/t5505 now pass\n -c core.configLockTimeout=0 so they still fail fast.\n\nI matched the core.filesRefLockTimeout naming rather than reusing\nmicrosoft/git's core.configWriteLockTimeoutMS, but can switch if the\ndownstream compat matters more.\n\n Documentation/config/core.adoc | 8 ++++++++\n config.c | 24 ++++++++++++++++++++++--\n t/t1300-config.sh | 17 +++++++++++++++++\n t/t3200-branch.sh | 6 ++++--\n t/t5505-remote.sh | 3 ++-\n 5 files changed, 53 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc\nindex a0ebf03e2e..340329edc3 100644\n--- a/Documentation/config/core.adoc\n+++ b/Documentation/config/core.adoc\n@@ -589,6 +589,14 @@ core.packedRefsTimeout::\n \tall; -1 means to try indefinitely. Default is 1000 (i.e.,\n \tretry for 1 second).\n \n+core.configLockTimeout::\n+\tThe length of time, in milliseconds, to retry when trying to\n+\tlock a configuration file for writing. Value 0 means not to\n+\tretry at all; -1 means to try indefinitely. Default is 1000\n+\t(i.e., retry for 1 second). This is read from the configuration\n+\tthat is already on disk before the lock is taken, so it can be\n+\tset persistently like any other option.\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/config.c b/config.c\nindex 156f2a24fa..ac9563781b 100644\n--- a/config.c\n+++ b/config.c\n@@ -2903,6 +2903,24 @@ char *git_config_prepare_comment_string(const char *comment)\n \treturn prepared;\n }\n \n+/*\n+ * How long to retry acquiring config.lock when another process holds\n+ * it. Default matches core.packedRefsTimeout; override via\n+ * core.configLockTimeout.\n+ */\n+static long config_lock_timeout_ms(struct repository *r)\n+{\n+\tstatic int configured;\n+\tstatic int timeout_ms = 1000;\n+\n+\tif (!configured) {\n+\t\trepo_config_get_int(r, \"core.configlocktimeout\", &timeout_ms);\n+\t\tconfigured = 1;\n+\t}\n+\n+\treturn timeout_ms;\n+}\n+\n static void validate_comment_string(const char *comment)\n {\n \tsize_t leading_blanks;\n@@ -2986,7 +3004,8 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tfd = hold_lock_file_for_update(&lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n+\t\t\t\t\t config_lock_timeout_ms(r));\n \tif (fd < 0) {\n \t\terror_errno(_(\"could not lock config file %s\"), config_filename);\n \t\tret = CONFIG_NO_LOCK;\n@@ -3331,7 +3350,8 @@ static int repo_config_copy_or_rename_section_in_file(\n \tif (!config_filename)\n \t\tconfig_filename = filename_buf = repo_git_path(r, \"config\");\n \n-\tout_fd = hold_lock_file_for_update(&lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update_timeout(&lock, config_filename, 0,\n+\t\t\t\t\t\t config_lock_timeout_ms(r));\n \tif (out_fd < 0) {\n \t\tret = error(_(\"could not lock config file %s\"), config_filename);\n \t\tgoto out;\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 128971ee12..12a1cb8580 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -2939,4 +2939,21 @@ test_expect_success 'writing value with trailing CR not stripped on read' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'writing config fails immediately with core.configLockTimeout=0' '\n+\ttest_when_finished \"rm -f .git/config.lock\" &&\n+\t>.git/config.lock &&\n+\ttest_must_fail git -c core.configLockTimeout=0 config foo.bar baz 2>err &&\n+\ttest_grep \"could not lock config file\" err\n+'\n+\n+test_expect_success 'writing config retries until lock is released' '\n+\ttest_when_finished \"rm -f .git/config.lock\" &&\n+\t>.git/config.lock &&\n+\t{\n+\t\t( sleep 1 && rm -f .git/config.lock ) &\n+\t} &&\n+\tgit -c core.configLockTimeout=5000 config retried.key value &&\n+\ttest \"$(git config retried.key)\" = value\n+'\n+\n test_done\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e7829c2c4b..5f8a31c21d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1037,7 +1037,8 @@ test_expect_success '--set-upstream-to fails on locked config' '\n \ttest_when_finished \"rm -f .git/config.lock\" &&\n \t>.git/config.lock &&\n \tgit branch locked &&\n-\ttest_must_fail git branch --set-upstream-to locked 2>err &&\n+\ttest_must_fail git -c core.configLockTimeout=0 \\\n+\t\tbranch --set-upstream-to locked 2>err &&\n \ttest_grep \"could not lock config file .git/config\" err\n '\n \n@@ -1068,7 +1069,8 @@ test_expect_success '--unset-upstream should fail if config is locked' '\n \ttest_when_finished \"rm -f .git/config.lock\" &&\n \tgit branch --set-upstream-to locked &&\n \t>.git/config.lock &&\n-\ttest_must_fail git branch --unset-upstream 2>err &&\n+\ttest_must_fail git -c core.configLockTimeout=0 \\\n+\t\tbranch --unset-upstream 2>err &&\n \ttest_grep \"could not lock config file .git/config\" err\n '\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex e592c0bcde..aea9222649 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1327,7 +1327,8 @@ test_expect_success 'remote set-url with locked config' '\n \ttest_when_finished \"rm -f .git/config.lock\" &&\n \tgit config --get-all remote.someremote.url >expect &&\n \t>.git/config.lock &&\n-\ttest_must_fail git remote set-url someremote baz &&\n+\ttest_must_fail git -c core.configLockTimeout=0 \\\n+\t\tremote set-url someremote baz &&\n \tgit config --get-all remote.someremote.url >actual &&\n \tcmp expect actual\n '\n-- \n2.53.0\n\n"},{"id":"543495","messageId":"xmqqzf1xbl4i.fsf@gitster.g","threadId":"65429","inReplyTo":"20260517132111.1014901-1-joerg@thalheim.io","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-18T00:46:05Z","receivedAt":"2026-05-18T00:46:08Z","isPatch":true,"body":"Joerg Thalheim writes:\n\n> +/*\n> + * How long to retry acquiring config.lock when another process holds\n> + * it. Default matches core.packedRefsTimeout; override via\n> + * core.configLockTimeout.\n> + */\n> +static long config_lock_timeout_ms(struct repository *r)\n> +{\n> +\tstatic int configured;\n> +\tstatic int timeout_ms = 1000;\n> +\n> +\tif (!configured) {\n> +\t\trepo_config_get_int(r, \"core.configlocktimeout\", &timeout_ms);\n> +\t\tconfigured = 1;\n> +\t}\n> +\n> +\treturn timeout_ms;\n> +}\n\nThe above design means whichever repository happens to be passed for\nthe first time as \"r\" to this call will fix the return value from\nthe function for the rest of the system, meaning that the lock timeout\nis a per-process property and the repository parameter passed to the\nfunction does not matter all that much.\n\nIt may make sense to admit that this is not a per-repository\nproperty (due to the use of local caching), have the function take\nno parameter and use the_repository to the config_get call. That\nwould make the intention more clear.\n\nOf course the other end of the spectrum is to get rid of the\n\"configured\" caching here, and ask the config system to make a\nhashtable look-up every time the function is called. That will keep\nthe lock timeout per-repository, which is closer to what the current\nfunction signature suggests.\n\nI dunno. My gut feeling is that there aren't valid reasons why you\nwould want to specifically set different timeout values per\nrepository, so the simplicity of using the_repository (i.e. the\nprimary repository instance this process deals with) sounds like a\nbetter way to go.\n\nThanks.\n"},{"id":"543504","messageId":"agrIrGwSMFlKTx9x@pks.im","threadId":"65429","inReplyTo":"xmqqzf1xbl4i.fsf@gitster.g","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-18T08:07:08Z","receivedAt":"2026-05-18T08:07:14Z","isPatch":true,"body":"On Mon, May 18, 2026 at 09:46:05AM +0900, Junio C Hamano wrote:\n> Joerg Thalheim writes:\n> \n> > +/*\n> > + * How long to retry acquiring config.lock when another process holds\n> > + * it. Default matches core.packedRefsTimeout; override via\n> > + * core.configLockTimeout.\n> > + */\n> > +static long config_lock_timeout_ms(struct repository *r)\n> > +{\n> > +\tstatic int configured;\n> > +\tstatic int timeout_ms = 1000;\n> > +\n> > +\tif (!configured) {\n> > +\t\trepo_config_get_int(r, \"core.configlocktimeout\", &timeout_ms);\n> > +\t\tconfigured = 1;\n> > +\t}\n> > +\n> > +\treturn timeout_ms;\n> > +}\n> \n> The above design means whichever repository happens to be passed for\n> the first time as \"r\" to this call will fix the return value from\n> the function for the rest of the system, meaning that the lock timeout\n> is a per-process property and the repository parameter passed to the\n> function does not matter all that much.\n> \n> It may make sense to admit that this is not a per-repository\n> property (due to the use of local caching), have the function take\n> no parameter and use the_repository to the config_get call. That\n> would make the intention more clear.\n> \n> Of course the other end of the spectrum is to get rid of the\n> \"configured\" caching here, and ask the config system to make a\n> hashtable look-up every time the function is called. That will keep\n> the lock timeout per-repository, which is closer to what the current\n> function signature suggests.\n> \n> I dunno. My gut feeling is that there aren't valid reasons why you\n> would want to specifically set different timeout values per\n> repository, so the simplicity of using the_repository (i.e. the\n> primary repository instance this process deals with) sounds like a\n> better way to go.\n\nThere probably is no reason to have different values per repo. But to\nme the question is whether there even are any use cases where we have to\nlock the config file so often in quick succession that the caching\nmechanism even matters. My gut feeling says no, also because parsing the\nvalue from the configuration is going to be drowned out by actually\nwriting the lockfile and renaming it into place.\n\nSo I'd rather lean towards dropping the cache and keeping the repository\nparameter.\n\nPatrick\n"},{"id":"544224","messageId":"f449d0db-0434-f870-c69f-793f2b096816@gmx.de","threadId":"65429","inReplyTo":"20260517132111.1014901-1-joerg@thalheim.io","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2026-05-28T11:51:42Z","receivedAt":"2026-05-28T11:51:45Z","isPatch":true,"body":"Hi Joerg,\n\nOn Sun, 17 May 2026, Joerg Thalheim wrote:\n\n> I matched the core.filesRefLockTimeout naming rather than reusing\n> microsoft/git's core.configWriteLockTimeoutMS, but can switch if the\n> downstream compat matters more.\n\nI see that there is quite a bit of precedent for naming a config setting\n`*Timeout` and implying that it specifies milliseconds, e.g.\nhttps://git-scm.com/docs/git-config#Documentation/git-config.txt-corefilesRefLockTimeout\n\nIn general, I am pretty wary of unit-less numbers [*1*], that's why I\nchose that \"MS\" suffix. However, the prior art in Git is clear, and I\nshould not have missed it. Therefore, I have no objections against\n`core.configLockTimeout` as-is; I'll take care of providing a smooth\nupgrade path in Microsoft Git.\n\nThank you,\nJohannes\n\nFootnote *1*: In general, I am quite wary of unit-less numbers in\nconfigurations. There's not only the precedent of Mars Climate Orbiter\nhttps://en.wikipedia.org/wiki/Mars_Climate_Orbiter#Cause_of_failure, I\nalso heard a story that seemed to be entertaining only in hindsight where\na German car's software improperly interpreted a British speed limit sign\nto mean km/h instead of mph.\n"},{"id":"544225","messageId":"875x47u3il.fsf@gitster.g","threadId":"65429","inReplyTo":"f449d0db-0434-f870-c69f-793f2b096816@gmx.de","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-28T12:23:14Z","receivedAt":"2026-05-28T12:23:21Z","isPatch":true,"body":"Johannes Schindelin writes:\n\n> Footnote *1*: In general, I am quite wary of unit-less numbers in\n> configurations.\n\nHear, hear.\n\nI do not like unit-less names for variables that store numbers, not\njust configuration but in-code variables. In the longer run we want\nto clean these up. In-code variables are much easier to clean-up\nthan end-user facing configuration variable names, of course.\n\n"},{"id":"547321","messageId":"10bb26f4-38e7-1bb8-d2d9-4d3e2ef52adc@gmx.de","threadId":"65429","inReplyTo":"f449d0db-0434-f870-c69f-793f2b096816@gmx.de","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2026-07-07T11:39:07Z","receivedAt":"2026-07-07T11:39:10Z","isPatch":true,"body":"Hi,\n\nOn Thu, 28 May 2026, Johannes Schindelin wrote:\n\n> On Sun, 17 May 2026, Joerg Thalheim wrote:\n> \n> > I matched the core.filesRefLockTimeout naming rather than reusing\n> > microsoft/git's core.configWriteLockTimeoutMS, but can switch if the\n> > downstream compat matters more.\n> \n> I see that there is quite a bit of precedent for naming a config setting\n> `*Timeout` and implying that it specifies milliseconds, e.g.\n> https://git-scm.com/docs/git-config#Documentation/git-config.txt-corefilesRefLockTimeout\n> \n> In general, I am pretty wary of unit-less numbers [*1*], that's why I\n> chose that \"MS\" suffix. However, the prior art in Git is clear, and I\n> should not have missed it. Therefore, I have no objections against\n> `core.configLockTimeout` as-is; I'll take care of providing a smooth\n> upgrade path in Microsoft Git.\n\nFor the record: I meant this feedback as _supporting_ the patch. Now I see\nit is stalled... I do not really see any reason for this to be blocked\nfrom promoting to `next` and then `master`, though.\n\nCiao,\nJohannes\n"},{"id":"547379","messageId":"xmqq8q7m4qe5.fsf@gitster.g","threadId":"65429","inReplyTo":"10bb26f4-38e7-1bb8-d2d9-4d3e2ef52adc@gmx.de","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-07T18:16:02Z","receivedAt":"2026-07-07T18:16:04Z","isPatch":true,"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Thu, 28 May 2026, Johannes Schindelin wrote:\n>\n>> On Sun, 17 May 2026, Joerg Thalheim wrote:\n>> \n>> > I matched the core.filesRefLockTimeout naming rather than reusing\n>> > microsoft/git's core.configWriteLockTimeoutMS, but can switch if the\n>> > downstream compat matters more.\n>> \n>> I see that there is quite a bit of precedent for naming a config setting\n>> `*Timeout` and implying that it specifies milliseconds, e.g.\n>> https://git-scm.com/docs/git-config#Documentation/git-config.txt-corefilesRefLockTimeout\n>> \n>> In general, I am pretty wary of unit-less numbers [*1*], that's why I\n>> chose that \"MS\" suffix. However, the prior art in Git is clear, and I\n>> should not have missed it. Therefore, I have no objections against\n>> `core.configLockTimeout` as-is; I'll take care of providing a smooth\n>> upgrade path in Microsoft Git.\n>\n> For the record: I meant this feedback as _supporting_ the patch. Now I see\n> it is stalled... I do not really see any reason for this to be blocked\n> from promoting to `next` and then `master`, though.\n>\n> Ciao,\n> Johannes\n\nHeh, this paragraph\n\n microsoft/git carries a similar patch (core.configWriteLockTimeoutMS,\n default off) for Scalar's tests. Defaulting to non-zero here because\n the worktree case fails silently.\n\nin the proposed log message was enough to convince me that you'd be\nfavor of it.\n\nI think the \"Waiting for response(s) to review comment(s).\" is for\n\n So I'd rather lean towards dropping the cache and keeping the\n repository parameter.\n\nthat was expressed in a separate review in \nand haven't been responded to.\n\nThanks for pinging.\n"},{"id":"547475","messageId":"b5c80d76-5ef4-cf1f-f4e1-78e63cfea81b@gmx.de","threadId":"65429","inReplyTo":"agrIrGwSMFlKTx9x@pks.im","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2026-07-08T07:32:41Z","receivedAt":"2026-07-08T07:32:53Z","isPatch":true,"body":"Hi Patrick,\n\nOn Mon, 18 May 2026, Patrick Steinhardt wrote:\n\n> On Mon, May 18, 2026 at 09:46:05AM +0900, Junio C Hamano wrote:\n> > Joerg Thalheim writes:\n> > \n> > > +/*\n> > > + * How long to retry acquiring config.lock when another process holds\n> > > + * it. Default matches core.packedRefsTimeout; override via\n> > > + * core.configLockTimeout.\n> > > + */\n> > > +static long config_lock_timeout_ms(struct repository *r)\n> > > +{\n> > > +\tstatic int configured;\n> > > +\tstatic int timeout_ms = 1000;\n> > > +\n> > > +\tif (!configured) {\n> > > +\t\trepo_config_get_int(r, \"core.configlocktimeout\", &timeout_ms);\n> > > +\t\tconfigured = 1;\n> > > +\t}\n> > > +\n> > > +\treturn timeout_ms;\n> > > +}\n> > \n> > The above design means whichever repository happens to be passed for\n> > the first time as \"r\" to this call will fix the return value from\n> > the function for the rest of the system, meaning that the lock timeout\n> > is a per-process property and the repository parameter passed to the\n> > function does not matter all that much.\n> > \n> > It may make sense to admit that this is not a per-repository\n> > property (due to the use of local caching), have the function take\n> > no parameter and use the_repository to the config_get call. That\n> > would make the intention more clear.\n> > \n> > Of course the other end of the spectrum is to get rid of the\n> > \"configured\" caching here, and ask the config system to make a\n> > hashtable look-up every time the function is called. That will keep\n> > the lock timeout per-repository, which is closer to what the current\n> > function signature suggests.\n> > \n> > I dunno. My gut feeling is that there aren't valid reasons why you\n> > would want to specifically set different timeout values per\n> > repository, so the simplicity of using the_repository (i.e. the\n> > primary repository instance this process deals with) sounds like a\n> > better way to go.\n> \n> There probably is no reason to have different values per repo. But to\n> me the question is whether there even are any use cases where we have to\n> lock the config file so often in quick succession that the caching\n> mechanism even matters. My gut feeling says no, also because parsing the\n> value from the configuration is going to be drowned out by actually\n> writing the lockfile and renaming it into place.\n> \n> So I'd rather lean towards dropping the cache and keeping the repository\n> parameter.\n\nWhile the question whether or not to spend 5-ish lines on caching sounds\nlike a topic that could be debated in splendor and at length over a couple\nof beverages in a cozy bar, I am starting to grow a suspicion that I want\nto doubt whether the cost of that cache was worth blocking this patch for\nover a month. Lacking such a cozy setting and at this time also lacking\nthe leisure to enjoy said beverages, I'd rather go forward with the\nproposed version and move on to more exciting things.\n\nIn other words: I consider this patch fine as-is, and in the event that I\nwould consider highly unlikely where the cache _really_ bothers anyone, it\nwill be an easy patch to remove it.\n\nCiao,\nJohannes\n"},{"id":"547492","messageId":"xmqqa4s1wn7y.fsf@gitster.g","threadId":"65429","inReplyTo":"b5c80d76-5ef4-cf1f-f4e1-78e63cfea81b@gmx.de","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T14:49:21Z","receivedAt":"2026-07-08T14:49:30Z","isPatch":true,"body":"Johannes Schindelin writes:\n\n> ... doubt whether the cost of that cache was worth blocking this patch for\n> over a month. Lacking such a cozy setting and at this time also lacking\n> the leisure to enjoy said beverages, I'd rather go forward with the\n> proposed version and move on to more exciting things.\n>\n> In other words: I consider this patch fine as-is, and in the event that I\n> would consider highly unlikely where the cache _really_ bothers anyone, it\n> will be an easy patch to remove it.\n\nAnd until now, we had over a month to see such an add-on patch, or\nhear argument like the above that such a patch is not needed. That\nis what I find disturbing the most here.\n\n"},{"id":"548826","messageId":"xmqq33x9h487.fsf@gitster.g","threadId":"65429","inReplyTo":"20260517132111.1014901-1-joerg@thalheim.io","subject":"Re: [PATCH v2] config: retry acquiring config.lock, configurable via core.configLockTimeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-23T19:54:00Z","receivedAt":"2026-07-23T19:54:03Z","isPatch":true,"body":"Joerg Thalheim writes:\n\n> From: Jörg Thalheim \n>\n> Concurrent config writers race for the \".lock\" file, which is taken\n> with open(O_EXCL) and no retry, so the losers fail right away with\n> \"could not lock config file\".\n>\n> This shows up with parallel \"git worktree add -b\" against the same\n> repository: each one writes a couple of branch.* keys and the losers\n> fail at random. Worse, \"git worktree add\" doesn't propagate that\n> failure to its exit code, so the tracking config is silently dropped.\n> (The swallowed error is a separate bug.)\n>\n> Retry instead of giving up on the first EEXIST. The lock is only held\n> while rewriting a small file, so the loser only has to wait out the\n> other writers. Same approach as 4ff0f01cb7 (refs: retry acquiring\n> reference locks for 100ms, 2017-08-21).\n>\n> On the semantics: the on-disk config is read only after the lock is\n> taken, so writers touching different keys can't lose each other's\n> change. Writers touching the same key still get last-writer-wins, but\n> that is already the case today and would need a compare-and-swap config\n> API to fix. The retry only turns hard failures into successes.\n>\n> Default to 1000ms, like core.packedRefsTimeout: same shape of problem,\n> one shared file everyone serializes through. A larger timeout only\n> costs anything when a stale lock is left behind by a crash, which is\n> rare; a smaller one fails spuriously on slow filesystems (NTFS has\n> been seen needing more than 100ms). Make it configurable as\n> core.configLockTimeout. There is no chicken-and-egg problem: we read\n> the config before we lock it.\n>\n> microsoft/git carries a similar patch (core.configWriteLockTimeoutMS,\n> default off) for Scalar's tests. Defaulting to non-zero here because\n> the worktree case fails silently.\n>\n> Helped-by: Patrick Steinhardt \n> Helped-by: Johannes Schindelin \n> Signed-off-by: Jörg Thalheim \n> ---\n> Thanks for the review and for poking me, this had fallen off my radar.\n>\n> v1 -> v2:\n>\n> - added core.configLockTimeout. Johannes is right that there is no\n> chicken-and-egg problem (config is read before the lock), so no env\n> var needed.\n> - default bumped to 1000ms; packed-refs is the closer precedent and it\n> keeps NTFS out of trouble.\n> - commit message now covers the read-after-lock / last-writer-wins\n> semantics Patrick asked about.\n> - added tests; existing stale-lock tests in t3200/t5505 now pass\n> -c core.configLockTimeout=0 so they still fail fast.\n>\n> I matched the core.filesRefLockTimeout naming rather than reusing\n> microsoft/git's core.configWriteLockTimeoutMS, but can switch if the\n> downstream compat matters more.\n\nI was reviewing the whats-cooking and noticed there are a handful of\nstalled topics that are not going anywhere, and this is one of them.\n\nThis time it had fallen off my radar, sorry about that. All the\noutstanding issues seem to have been resolved, so let's merge it\ndown to 'next'.\n\nThanks.\n"}]}