{"thread":{"id":"63066","subject":"[PATCH 0/2] describe and diff: implement --no-optional-locks","startedAt":"2025-03-06T05:58:08Z","lastAt":"2025-03-11T02:10:53Z","messageCount":11,"participants":["Benjamin Woodruff via GitGitGadget","Junio C Hamano","Jeff King","Benjamin Woodruff"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"513631","messageId":"pull.1872.git.1741240685.gitgitgadget@gmail.com","threadId":"63066","inReplyTo":null,"subject":"[PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Benjamin Woodruff via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T05:58:03Z","receivedAt":"2025-03-06T05:58:08Z","isPatch":true,"sender":{"key":"github@benjam.info","avatar":null},"body":"git describe and git diff may update the index in the background for similar\nperformance reasons to git-status. These patches implement the\n--no-optional-locks option for these commands, which allows scripts to\nbypass this behavior.\n\nI'm implementing this as a solution to a stale lockfile issue we've\nsporadically encountered due to a build script that runs git describe. We're\nmitigating this issue by using libgit2 in our build script, which does not\nhave the same background update behavior for its git_describe_workdir\nimplementation, but it would be nice to have this option supported in the\ngit CLI.\n\nTests and documentation changes are included!\n\nBenjamin Woodruff (2):\n describe: implement --no-optional-locks\n diff: implement --no-optional-locks\n\n Documentation/config/diff.adoc | 4 ++-\n Documentation/git-describe.adoc | 12 ++++++++\n Documentation/git.adoc | 4 ++-\n builtin/describe.c | 12 ++++----\n builtin/diff.c | 4 +++\n t/meson.build | 1 +\n t/t4070-diff-auto-refresh-index.sh | 46 ++++++++++++++++++++++++++++++\n t/t6120-describe.sh | 8 ++++++\n 8 files changed, 84 insertions(+), 7 deletions(-)\n create mode 100755 t/t4070-diff-auto-refresh-index.sh\n\n\nbase-commit: e969bc875963a10890d61ba84eab3a460bd9e535\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1872%2Fbgw%2Fbgw%2Fdiff-describe-optional-locks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1872/bgw/bgw/diff-describe-optional-locks-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1872\n-- \ngitgitgadget\n"},{"id":"513632","messageId":"46c90ba12a5afd0e697a224c7951771b649b336b.1741240685.git.gitgitgadget@gmail.com","threadId":"63066","inReplyTo":"pull.1872.git.1741240685.gitgitgadget@gmail.com","subject":"[PATCH 1/2] describe: implement --no-optional-locks","fromName":"Benjamin Woodruff via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T05:58:04Z","receivedAt":"2025-03-06T05:58:09Z","isPatch":true,"sender":{"key":"github@benjam.info","avatar":null},"body":"From: Benjamin Woodruff \n\nWhen used with `--dirty`, describe may update the index in the\nbackground for similar performance reasons to `git-status`.\n\nThis commit implements the `--no-optional-locks` option for\n`git describe`, which allows scripts to bypass this behavior.\n\nSigned-off-by: Benjamin Woodruff \n---\n Documentation/git-describe.adoc | 12 ++++++++++++\n Documentation/git.adoc | 3 ++-\n builtin/describe.c | 12 +++++++-----\n t/t6120-describe.sh | 8 ++++++++\n 4 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-describe.adoc b/Documentation/git-describe.adoc\nindex 08ff715709c..97b4c656078 100644\n--- a/Documentation/git-describe.adoc\n+++ b/Documentation/git-describe.adoc\n@@ -198,6 +198,18 @@ selected and output. Here fewest commits different is defined as\n the number of commits which would be shown by `git log tag..input`\n will be the smallest number of commits possible.\n \n+BACKGROUND REFRESH\n+------------------\n+\n+By default, `git describe --dirty` will automatically refresh the index,\n+similar to the background refresh functionality of\n+linkgit:git-status[1]. Writing out the updated index is an optimization\n+that isn't strictly necessary. When `describe` is run in the background,\n+the lock held during the write may conflict with other simultaneous\n+processes, causing them to fail. Scripts running `describe` in the\n+background should consider using `git --no-optional-locks status` (see\n+linkgit:git[1] for details).\n+\n BUGS\n ----\n \ndiff --git a/Documentation/git.adoc b/Documentation/git.adoc\nindex 743b7b00e4d..f084b2f0f1e 100644\n--- a/Documentation/git.adoc\n+++ b/Documentation/git.adoc\n@@ -189,7 +189,8 @@ If you just want to run git as if it was started in `` then use\n \n --no-optional-locks::\n \tDo not perform optional operations that require locks. This is\n-\tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n+\tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`. This\n+\tfunctionality is implemented for `git status` and `git describe`.\n \n --no-advice::\n \tDisable all advice hints from being printed.\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex e2e73f3d757..d4fea55352d 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -704,7 +704,6 @@ int cmd_describe(int argc,\n \t\t} else if (dirty) {\n \t\t\tstruct lock_file index_lock = LOCK_INIT;\n \t\t\tstruct rev_info revs;\n-\t\t\tint fd;\n \n \t\t\tsetup_work_tree();\n \t\t\tprepare_repo_settings(the_repository);\n@@ -712,10 +711,13 @@ int cmd_describe(int argc,\n \t\t\trepo_read_index(the_repository);\n \t\t\trefresh_index(the_repository->index, REFRESH_QUIET|REFRESH_UNMERGED,\n \t\t\t\t NULL, NULL, NULL);\n-\t\t\tfd = repo_hold_locked_index(the_repository,\n-\t\t\t\t\t\t &index_lock, 0);\n-\t\t\tif (0 <= fd)\n-\t\t\t\trepo_update_index_if_able(the_repository, &index_lock);\n+\t\t\tif (use_optional_locks()) {\n+\t\t\t\tint fd = repo_hold_locked_index(the_repository,\n+\t\t\t\t\t\t\t\t&index_lock, 0);\n+\t\t\t\tif (0 <= fd)\n+\t\t\t\t\trepo_update_index_if_able(the_repository,\n+\t\t\t\t\t\t\t\t &index_lock);\n+\t\t\t}\n \n \t\t\trepo_init_revisions(the_repository, &revs, prefix);\n \ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 76843a61691..ae7b6901ed6 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -749,4 +749,12 @@ test_expect_success 'do not be fooled by invalid describe format ' '\n \ttest_must_fail git cat-file -t \"refs/tags/super-invalid/./../...../ ~^:/?*[////\\\\\\\\\\\\&}/busted.lock-42-g\"$(cat out)\n '\n \n+test_expect_success '--no-optional-locks prevents index update' '\n+\ttest_set_magic_mtime .git/index &&\n+\tgit --no-optional-locks describe --dirty &&\n+\ttest_is_magic_mtime .git/index &&\n+\tgit describe --dirty &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"513633","messageId":"3ba3113738e913d51156703b7cf86daf43c7d1b4.1741240685.git.gitgitgadget@gmail.com","threadId":"63066","inReplyTo":"pull.1872.git.1741240685.gitgitgadget@gmail.com","subject":"[PATCH 2/2] diff: implement --no-optional-locks","fromName":"Benjamin Woodruff via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T05:58:05Z","receivedAt":"2025-03-06T05:58:10Z","isPatch":true,"sender":{"key":"github@benjam.info","avatar":null},"body":"From: Benjamin Woodruff \n\nWhen used with `autoRefreshIndex`, `git diff` may update the index in\nthe background for similar performance reasons to `git-status`.\n\nThis commit implements the `--no-optional-locks` option for `git diff`,\nwhich allows scripts to bypass this behavior.\n\nSigned-off-by: Benjamin Woodruff \n---\n Documentation/config/diff.adoc | 4 ++-\n Documentation/git.adoc | 3 +-\n builtin/diff.c | 4 +++\n t/meson.build | 1 +\n t/t4070-diff-auto-refresh-index.sh | 46 ++++++++++++++++++++++++++++++\n 5 files changed, 56 insertions(+), 2 deletions(-)\n create mode 100755 t/t4070-diff-auto-refresh-index.sh\n\ndiff --git a/Documentation/config/diff.adoc b/Documentation/config/diff.adoc\nindex 1135a62a0ad..2dadcf7a1da 100644\n--- a/Documentation/config/diff.adoc\n+++ b/Documentation/config/diff.adoc\n@@ -6,7 +6,9 @@\n \tcontents in the work tree match the contents in the\n \tindex. This option defaults to `true`. Note that this\n \taffects only `git diff` Porcelain, and not lower level\n-\t`diff` commands such as `git diff-files`.\n+\t`diff` commands such as `git diff-files`. If\n+\t`--no-optional-locks` is set (see linkgit:git[1] for\n+\tdetails), the index file is not updated.\n \n `diff.dirstat`::\n ifdef::git-diff[]\ndiff --git a/Documentation/git.adoc b/Documentation/git.adoc\nindex f084b2f0f1e..754880b8672 100644\n--- a/Documentation/git.adoc\n+++ b/Documentation/git.adoc\n@@ -190,7 +190,8 @@ If you just want to run git as if it was started in `` then use\n --no-optional-locks::\n \tDo not perform optional operations that require locks. This is\n \tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`. This\n-\tfunctionality is implemented for `git status` and `git describe`.\n+\tfunctionality is implemented for `git status`, `git describe`,\n+\tand `git diff`.\n \n --no-advice::\n \tDisable all advice hints from being printed.\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex a4fffee42c6..5469e58bf5d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -9,6 +9,7 @@\n \n #include \"builtin.h\"\n #include \"config.h\"\n+#include \"environment.h\"\n #include \"ewah/ewok.h\"\n #include \"lockfile.h\"\n #include \"color.h\"\n@@ -239,6 +240,9 @@ static void refresh_index_quietly(void)\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint fd;\n \n+\tif (!use_optional_locks())\n+\t\treturn;\n+\n \tfd = repo_hold_locked_index(the_repository, &lock_file, 0);\n \tif (fd < 0)\n \t\treturn;\ndiff --git a/t/meson.build b/t/meson.build\nindex a59da26be3f..10d7cace3c6 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -500,6 +500,7 @@ integration_tests = [\n 't4067-diff-partial-clone.sh',\n 't4068-diff-symmetric-merge-base.sh',\n 't4069-remerge-diff.sh',\n+ 't4070-diff-auto-refresh-index.sh',\n 't4100-apply-stat.sh',\n 't4101-apply-nonl.sh',\n 't4102-apply-rename.sh',\ndiff --git a/t/t4070-diff-auto-refresh-index.sh b/t/t4070-diff-auto-refresh-index.sh\nnew file mode 100755\nindex 00000000000..9e38f1d3206\n--- /dev/null\n+++ b/t/t4070-diff-auto-refresh-index.sh\n@@ -0,0 +1,46 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2025 Benjamin Woodruff\n+#\n+\n+test_description='diff.autoRefreshIndex config option'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-diff.sh\n+\n+test_expect_success 'index is updated when autoRefreshIndex is true' '\n+\t>tracked &&\n+\tgit add tracked &&\n+\n+\t# stat() must change (but not file contents) to trigger an index update\n+\ttest_set_magic_mtime tracked &&\n+\n+\t# check the mtime of .git/index does not change without autoRefreshIndex\n+\ttest_set_magic_mtime .git/index &&\n+\tgit config diff.autoRefreshIndex false &&\n+\tgit diff &&\n+\ttest_is_magic_mtime .git/index &&\n+\n+\t# but it does change when autoRefreshIndex is true (the default)\n+\tgit config diff.autoRefreshIndex true &&\n+\tgit diff &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n+test_expect_success '--no-optional-locks overrides autoRefreshIndex' '\n+\t>tracked &&\n+\tgit add tracked &&\n+\ttest_set_magic_mtime tracked &&\n+\n+\t# `--no-optional-locks` overrides `autoRefreshIndex`\n+\ttest_set_magic_mtime .git/index &&\n+\tgit config diff.autoRefreshIndex true &&\n+\tgit --no-optional-locks diff &&\n+\n+\t# sanity check that without `--no-optional-locks` it still updates\n+\ttest_is_magic_mtime .git/index &&\n+\tgit diff &&\n+\t! test_is_magic_mtime .git/index\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"513715","messageId":"xmqqo6yejg0y.fsf@gitster.g","threadId":"63066","inReplyTo":"pull.1872.git.1741240685.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-06T16:11:09Z","receivedAt":"2025-03-06T16:11:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Benjamin Woodruff via GitGitGadget\" \nwrites:\n\n> git describe and git diff may update the index in the background for similar\n> performance reasons to git-status.\n\nThat is a wrong reasoning that is completely opposite, though.\n\nThe commands at the Porcelain level, like \"status\" and \"diff\",\nrefresh the index for the CORRECTNESS purposes.\n\nThe commands at the plumbing level, which are designed to be used in\nyour own scripts, like \"diff-files\" and \"diff-index\", do not refresh\nthe index for the performance purposes. If your own script wants to\nproduce correct result, your script IS responsible for refreshing\nthe index after its last modification to working tree files before\nit starts to use the plumbing commands to inspect which ones are\nmodified and which ones are not. This is so that your script has\nmore control over when the index is refreshed. It does not have to\npay cost to run refresh for each Git command it invokes, if it knows\nthat it does not make any modification between the two invocations;\nit can instead refresh just once and then run these two plumbing\ncommands.\n\nSo, it would be absolute no-no to make the Porcelain commands like\n\"describe\" and \"diff\" not to refresh the index before they work by\ndefault. As an optional behaviour, it might be acceptable if there\nis no other reasonable solutions (like, using plumbing commands if\nthe callers are not you typing but your scripts calling them).\n\n"},{"id":"513845","messageId":"20250309033908.GA2361799@coredump.intra.peff.net","threadId":"63066","inReplyTo":"xmqqo6yejg0y.fsf@gitster.g","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-09T03:39:08Z","receivedAt":"2025-03-09T03:39:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2025 at 08:11:09AM -0800, Junio C Hamano wrote:\n\n> \"Benjamin Woodruff via GitGitGadget\" \n> writes:\n> \n> > git describe and git diff may update the index in the background for similar\n> > performance reasons to git-status.\n> \n> That is a wrong reasoning that is completely opposite, though.\n> \n> The commands at the Porcelain level, like \"status\" and \"diff\",\n> refresh the index for the CORRECTNESS purposes.\n\nRight, but \"status\" supports --no-optional-locks already.\n\nSo I think these patches are wrong, but the goal is reasonable. What\ngit-status does with --no-optional-locks is to update the index\ninternally for its _own_ use (giving it the correct results), but not to\nlock nor write out the resulting index (to avoid conflicting with other\nrunning programs). So it's pessimal (losing the opportunity to share\nwhat it learned) but prevents lock contention.\n\nAnd I think other programs could do that. I even wrote back in\n27344d6a6c (git: add --no-optional-locks option, 2017-09-27):\n\n I've punted here on finding more callers to convert, since \"status\" is\n the obvious one to call as a repeated background job. But \"git diff\"'s\n opportunistic refresh of the index may be a good candidate.\n\nI must admit that I didn't imagine \"describe\" is something that somebody\nwould run a lot in the background, but there's not really any harm in\nhaving it support optional locks if somebody cares about that case.\n\n> The commands at the plumbing level, which are designed to be used in\n> your own scripts, like \"diff-files\" and \"diff-index\", do not refresh\n> the index for the performance purposes. If your own script wants to\n> produce correct result, your script IS responsible for refreshing\n> the index after its last modification to working tree files before\n> it starts to use the plumbing commands to inspect which ones are\n> modified and which ones are not. This is so that your script has\n> more control over when the index is refreshed. It does not have to\n> pay cost to run refresh for each Git command it invokes, if it knows\n> that it does not make any modification between the two invocations;\n> it can instead refresh just once and then run these two plumbing\n> commands.\n\nAssuming --no-optional-locks is being used as intended, the idea is that\nthe script cannot touch the index at all, because it is running in the\nbackground and does not want lock contention with things the user is\ndoing in the foreground. So it cannot naively do a single index refresh\nfollowed by plumbing commands like diff-files. Either it must:\n\n - accept that diff-files might return stat-dirty results (yuck)\n\n - use its own index that is separate from the regular .git/index file.\n But that may be overly slow, since the index \"update\" would rewrite\n the whole thing from scratch. Of course all of our index writes are\n from scratch, but you'd pay the price even when there is nothing to\n update.\n\n - use a command which operates all in a single process, with an\n in-memory index that is updated but not written out (e.g., \"git\n --no-optional-locks status\").\n\n-Peff\n"},{"id":"513895","messageId":"xmqqwmcx9io3.fsf@gitster.g","threadId":"63066","inReplyTo":"20250309033908.GA2361799@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T12:25:32Z","receivedAt":"2025-03-10T12:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> .... What\n> git-status does with --no-optional-locks is to update the index\n> internally for its _own_ use (giving it the correct results), but not to\n> lock nor write out the resulting index (to avoid conflicting with other\n> running programs). So it's pessimal (losing the opportunity to share\n> what it learned) but prevents lock contention.\n\nYup, that sounds somewhat sensible. I also have to wonder, other\nthan commands that are clearly about changing the repository state\nlike \"add\", the inspection commands like diff and status should\nalways opportunistically write the index back, without even being\nasked?\n\n> ... Either it must:\n>\n> - accept that diff-files might return stat-dirty results (yuck)\n>\n> - use its own index that is separate from the regular .git/index file.\n> But that may be overly slow, since the index \"update\" would rewrite\n> the whole thing from scratch. Of course all of our index writes are\n> from scratch, but you'd pay the price even when there is nothing to\n> update.\n>\n> - use a command which operates all in a single process, with an\n> in-memory index that is updated but not written out (e.g., \"git\n> --no-optional-locks status\").\n\n"},{"id":"513918","messageId":"20250310160837.GA26308@coredump.intra.peff.net","threadId":"63066","inReplyTo":"xmqqwmcx9io3.fsf@gitster.g","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-10T16:08:37Z","receivedAt":"2025-03-10T16:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2025 at 05:25:32AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > .... What\n> > git-status does with --no-optional-locks is to update the index\n> > internally for its _own_ use (giving it the correct results), but not to\n> > lock nor write out the resulting index (to avoid conflicting with other\n> > running programs). So it's pessimal (losing the opportunity to share\n> > what it learned) but prevents lock contention.\n> \n> Yup, that sounds somewhat sensible. I also have to wonder, other\n> than commands that are clearly about changing the repository state\n> like \"add\", the inspection commands like diff and status should\n> always opportunistically write the index back, without even being\n> asked?\n\nYeah, certainly it is a potential source of confusion to have a\nconceptually read-only operation take locks or modify the repo state.\n\nBut I'm not sure we have a sense of how valuable that optimization is in\npractice. After touching some files, every git-status, git-diff, etc\nwould end up re-hashing those files instead of using the stat cache.\nBut maybe that is lost in the noise of reading the files to actually do\ndiffs, etc? I dunno. I expect it is more important for status, which\nprobably does not need to read the whole file contents in most cases\n(and which may be run a lot from the user's prompt, etc).\n\nIt seems like a big and possibly risky departure from what we've done\nfor so many years. I'm inclined not to rock the boat too much. ;)\n\n-Peff\n"},{"id":"513930","messageId":"xmqqo6y87m4d.fsf@gitster.g","threadId":"63066","inReplyTo":"20250310160837.GA26308@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T18:53:54Z","receivedAt":"2025-03-10T18:53:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But maybe that is lost in the noise of reading the files to actually do\n> diffs, etc? I dunno. I expect it is more important for status, which\n> probably does not need to read the whole file contents in most cases\n> (and which may be run a lot from the user's prompt, etc).\n\nYeah, and old timers who run \"diff --raw\" as if it were a quick\nanalogue for \"status\" also would notice.\n\n> It seems like a big and possibly risky departure from what we've done\n> for so many years. I'm inclined not to rock the boat too much. ;)\n\nCertainly not right now. But adding a command line option is even\nworse as we would have to carry the support for it for practically\nforever X-<.\n\n\n"},{"id":"513939","messageId":"bbc8a0ef-737c-44ba-9786-f5456f5ce71b@app.fastmail.com","threadId":"63066","inReplyTo":"xmqqo6y87m4d.fsf@gitster.g","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Benjamin Woodruff","fromEmail":"github@benjam.info","sentAt":"2025-03-10T20:50:36Z","receivedAt":"2025-03-10T20:51:43Z","isPatch":true,"sender":{"key":"github@benjam.info","avatar":null},"body":"On Mon, Mar 10, 2025, at 11:53 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> But maybe that is lost in the noise of reading the files to actually do\n>> diffs, etc? I dunno. I expect it is more important for status, which\n>> probably does not need to read the whole file contents in most cases\n>> (and which may be run a lot from the user's prompt, etc).\n>\n> Yeah, and old timers who run \"diff --raw\" as if it were a quick\n> analogue for \"status\" also would notice.\n\nThanks for your reviews, Junio and Jeff.\n\n>>>> I must admit that I didn't imagine \"describe\" is something that\n>>>> somebody would run a lot in the background\n\nIt might help if I start with some more context of why I wrote this\npatch. We've got a tool that uses the Rust `vergen-gitcl` crate to call\n`git describe --dirty`. You can see that code here:\nhttps://github.com/vercel/next.js/pull/76889/files\n\nWe use `vergen-gitcl` to generate version identifiers for an on-disk\ncache. This cache stores results of thousands of functions and has no\nbackwards compatibility. We want to invalidate it when *any* of the code\nchanges. `git-describe` felt like a good fit for that, as it gives us a\nunique identifier that's still reasonably user-friendly.\n\nHowever, we discovered that we'd frequently end up with stale git\nlockfiles. This appeared to be due some combination of IDE tools that\nrun the build in the background (i.e. the rust-analyzer LSP), behavior\nthat causes builds to sometimes get killed before completion, and the\nfact that `git describe --dirty` takes a lock.\n\nI've worked around this on our end, by re-implementing `describe`'s\n`--dirty` flag on top of `status`:\n\n\nSo, from my end, there's no urgency to get this change in, or to add\nsupport for `diff` (we only care about `describe`). Rather, I felt like\nthis might be a footgun for other users, and wanted to do the right\nthing by submitting an upstream change.\n\n>>> git describe and git diff may update the index in the background for\n>>> similar performance reasons to git-status.\n>>\n>> That is a wrong reasoning that is completely opposite, though.\n>> \n>> The commands at the Porcelain level, like \"status\" and \"diff\",\n>> refresh the index for the CORRECTNESS purposes.\n>\n> Right, but \"status\" supports --no-optional-locks already.\n\nDoes this mean the documentation in `git-status` is incorrect? It\nimplies that the background refresh is only for performance reasons.\nThat's where I got this idea from:\n\n\nIt's also worth noting that libgit2 does not do this background refresh\nby default (`GIT_DIFF_UPDATE_INDEX` and `GIT_STATUS_OPT_UPDATE_INDEX`).\nI think that makes sense for libgit2's typical use-cases, but it is a\ndivergence in behavior.\n\n>> Yeah, certainly it is a potential source of confusion to have a\n>> conceptually read-only operation take locks or modify the repo state.\n>>\n>> [...]\n>>\n>> It seems like a big and possibly risky departure from what we've done\n>> for so many years. I'm inclined not to rock the boat too much. ;)\n>\n> Certainly not right now. But adding a command line option is even\n> worse as we would have to carry the support for it for practically\n> forever X-<.\n\nWhat about if we reduce this to documentation changes? That alone\nwould've saved me a lot of pain of trying to figure out what does and\ndoesn't take a lock:\n\n- Explicitly document that `--no-optional-locks` only changes behavior\n for `git status`.\n\n- Document that the `--dirty` flag on `describe` will lead to\n `status`-like background refresh behavior.\n\n- Change the documentation for status's background refresh to indicate\n that there is a subtle difference in behavior caused by\n `--no-optional-locks`? I lack sufficient context on how best to\n describe or explain this.\n\n- Leave `git-diff` documentation as-is. I think the current\n documentation of `diff.autoRefreshIndex` is sufficient?\n"},{"id":"513953","messageId":"xmqq34fk5vxz.fsf@gitster.g","threadId":"63066","inReplyTo":"bbc8a0ef-737c-44ba-9786-f5456f5ce71b@app.fastmail.com","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-10T23:04:40Z","receivedAt":"2025-03-10T23:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Benjamin Woodruff\" writes:\n\n>>> The commands at the Porcelain level, like \"status\" and \"diff\",\n>>> refresh the index for the CORRECTNESS purposes.\n>>\n>> Right, but \"status\" supports --no-optional-locks already.\n>\n> Does this mean the documentation in `git-status` is incorrect? It\n> implies that the background refresh is only for performance reasons.\n\nIt has to do, and it does, refresh the in-core index for correctness\nreasons. Writing the result out to the on-disk file for _other_\nprocesses that will use the index later is a performance hack, and\nmay or may not happen (it is up to what repo_update_index_if_able()\ndoes).\n\nIf we do not care about redundant refreshing these other processes\nneed to do before they start, we do not have to write the resulting\nin-core index out to the disk.\n\nWriting the in-core index out for other processes has one downside,\nwhich is that for correctness it has to be done under lock. We\ncannot say \"we tried to be nice to others by writing the index file\nout, but we just found out that there is a competing process trying\nto open/write the index so let's skip writing it and let them do\ntheir thing---they are responsible for refreshing the index for\ntheir own use\". There needs some coordination, i.e. when we start\nwriting the result of our refreshing the index for others, if other\nprocesses need to pay attention to that fact and wait for a while\nbefore reading the index, then they benefit because they do not have\nto refresh the index themselves. Even though our programs, by\ncalling hold_lock_file_for_update(), know how to wait for a bit when\nthis happens, not everybody cooperates.\n"},{"id":"513968","messageId":"20250311021049.GA33498@coredump.intra.peff.net","threadId":"63066","inReplyTo":"bbc8a0ef-737c-44ba-9786-f5456f5ce71b@app.fastmail.com","subject":"Re: [PATCH 0/2] describe and diff: implement --no-optional-locks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-11T02:10:49Z","receivedAt":"2025-03-11T02:10:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2025 at 01:50:36PM -0700, Benjamin Woodruff wrote:\n\n> It might help if I start with some more context of why I wrote this\n> patch. We've got a tool that uses the Rust `vergen-gitcl` crate to call\n> `git describe --dirty`. You can see that code here:\n> https://github.com/vercel/next.js/pull/76889/files\n> \n> We use `vergen-gitcl` to generate version identifiers for an on-disk\n> cache. This cache stores results of thousands of functions and has no\n> backwards compatibility. We want to invalidate it when *any* of the code\n> changes. `git-describe` felt like a good fit for that, as it gives us a\n> unique identifier that's still reasonably user-friendly.\n> \n> However, we discovered that we'd frequently end up with stale git\n> lockfiles. This appeared to be due some combination of IDE tools that\n> run the build in the background (i.e. the rust-analyzer LSP), behavior\n> that causes builds to sometimes get killed before completion, and the\n> fact that `git describe --dirty` takes a lock.\n\nYeah, that is not quite the original use case that --no-optional-locks\nwas designed for (i.e., simultaneous contention), but I think it is a\nreasonable application of the flag.\n\n> >>> git describe and git diff may update the index in the background for\n> >>> similar performance reasons to git-status.\n> >>\n> >> That is a wrong reasoning that is completely opposite, though.\n> >> \n> >> The commands at the Porcelain level, like \"status\" and \"diff\",\n> >> refresh the index for the CORRECTNESS purposes.\n> >\n> > Right, but \"status\" supports --no-optional-locks already.\n> \n> Does this mean the documentation in `git-status` is incorrect? It\n> implies that the background refresh is only for performance reasons.\n> That's where I got this idea from:\n> \n\nI think Junio gave an explanation here, so I won't repeat that. But I\nalso think both of us may have been a bit confused about the changes\nyour patches are making, because there's some subtlety.\n\nThe important thing to keep in mind is that there are _two_ steps:\nrefreshing the in-core index and writing the result out to the on-disk\nfile. With --no-optional-locks we must continue to do the first step\n(for correctness), and skip the second step.\n\nSo looking at your patch 1/2 for git-describe, it is doing the right\nthing: we still call refresh_index() always, and only skip the calls to\nrepo_hold_locked_index() and repo_update_index_if_able().\n\nBut one thing that puzzles me is that we read and refresh the index\nfirst and only _then_ take a lock. Which seems wrong to me, as we could\nracily overwrite an intermediate write from somebody else that we never\neven saw (e.g., imagine you call \"git add\" at just the wrong moment).\n\nThat is not a bug in your code, but an existing problem that I think\nmade it harder to understand your change (and probably one we should\nfix regardless).\n\nYour patch 2/2 for git-diff is what I thought was actually wrong, but\nafter digging further, I'm not so sure.\n\nIn your patch we return early from refresh_index_quietly(), without\nactually refreshing the in-core index. So I _thought_ that meant we'd\nproduce a wrong answer for something like this:\n\n $ touch git.c\n $ ./git --no-optional-locks diff\n\nwhere we should report \"no changes\", but would instead find the\nstat-dirty git.c (just like a plumbing \"git diff-files\" would). But\nthat doesn't happen!\n\nThat's because refresh_index_quietly() runs after the diff has completed\nanyway. The real magic is in diffcore_skip_stat_unmatch(), which\nprocesses individual stat-dirty entries and suppresses them (when\nthere's no actual content change).\n\nSo the call in refresh_index_quietly() really is just about updating\nwhat we're about to write out, and your patch is correct to bail from\nthe whole function (if we are not writing it out, there is no purpose in\nrefreshing at that point).\n\nSo as far as I can tell the patches are doing the right thing. But I\nthink the commit messages probably need to describe those subtleties and\nargue that the change is correct. Bonus points if a preparatory patch\nfixes the race in git-describe. ;)\n\n> It's also worth noting that libgit2 does not do this background refresh\n> by default (`GIT_DIFF_UPDATE_INDEX` and `GIT_STATUS_OPT_UPDATE_INDEX`).\n> I think that makes sense for libgit2's typical use-cases, but it is a\n> divergence in behavior.\n\nYes, I think that is probably a reasonable default for libgit2, where\nyou'd expect everything to happen in a single process (that can share\nthe in-core index).\n\n-Peff\n"}]}