{"thread":{"id":"46794","subject":"[PATCH] git: add --no-optional-locks option","startedAt":"2017-09-21T04:32:22Z","lastAt":"2017-09-28T16:15:59Z","messageCount":26,"participants":["Jeff King","Junio C Hamano","Johannes Sixt","Daniel Santos","Jeff Hostetler","Stefan Beller","Kaartic Sivaraam","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328535","messageId":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","threadId":"46794","inReplyTo":null,"subject":"[PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T04:32:15Z","receivedAt":"2017-09-21T04:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not*\nto lock the index and update it, 2016-08-12). Folks working on GitHub\nDesktop complained to me that it's only available on Windows. :)\n\nI expanded the scope a bit to let us give the same treatment to more\ncommands in the long run.  I'd also be OK with just cherry-picking your\npatch to non-Windows Git if you don't find my reasoning below\ncompelling. But I think we need _something_ like this, as the other\nsolutions I could come up with don't seem very promising.\n\n-Peff\n\n-- >8 --\nSome tools like IDEs or fancy editors may periodically run\ncommands like \"git status\" in the background to keep track\nof the state of the repository. Some of these commands may\nrefresh the index and write out the result in an\nopportunistic way: if they can get the index lock, then they\nupdate the on-disk index with any updates they find. And if\nnot, then their in-core refresh is lost and just has to be\nrecomputed by the next caller.\n\nBut taking the index lock may conflict with other operations\nin the repository. Especially ones that the user is doing\nthemselves, which _aren't_ opportunistic. In other words,\n\"git status\" knows how to back off when somebody else is\nholding the lock, but other commands don't know that status\nwould be happy to drop the lock if somebody else wanted it.\n\nThere are a couple possible solutions:\n\n  1. Have some kind of \"pseudo-lock\" that allows other\n     commands to tell status that they want the lock.\n\n     This is likely to be complicated and error-prone to\n     implement (and maybe even impossible with just\n     dotlocks to work from, as it requires some\n     inter-process communication).\n\n  2. Avoid background runs of commands like \"git status\"\n     that want to do opportunistic updates, preferring\n     instead plumbing like diff-files, etc.\n\n     This is awkward for a couple of reasons. One is that\n     \"status --porcelain\" reports a lot more about the\n     repository state than is available from individual\n     plumbing commands. And two is that we actually _do_\n     want to see the refreshed index. We just don't want to\n     take a lock or write out the result. Whereas commands\n     like diff-files expect us to refresh the index\n     separately and write it to disk so that they can depend\n     on the result. But that write is exactly what we're\n     trying to avoid.\n\n  3. Ask \"status\" not to lock or write the index.\n\n     This is easy to implement. The big downside is that any\n     work done in refreshing the index for such a call is\n     lost when the process exits. So a background process\n     may end up re-hashing a changed file multiple times\n     until the user runs a command that does an index\n     refresh themselves.\n\nThis patch implements the option 3. The idea (and the test)\nis largely stolen from a Git for Windows patch by Johannes\nSchindelin, 67e5ce7f63 (status: offer *not* to lock the\nindex and update it, 2016-08-12). The twist here is that\ninstead of making this an option to \"git status\", it becomes\na \"git\" option and matching environment variable.\n\nThe reason there is two-fold:\n\n  1. An environment variable is carried through to\n     sub-processes. And whether an invocation is a\n     background process or not should apply to the whole\n     process tree. So you could do \"git --no-optional-locks\n     foo\", and if \"foo\" is a script or alias that calls\n     \"status\", you'll still get the effect.\n\n  2. There may be other programs that want the same\n     treatment.\n\n     I've punted here on finding more callers to convert,\n     since \"status\" is the obvious one to call as a repeated\n     background job. But \"git diff\"'s opportunistic refresh\n     of the index may be a good candidate.\n\nThe test is taken from 67e5ce7f63, and it's worth repeating\nJohannes's explanation:\n\n  Note that the regression test added in this commit does\n  not *really* verify that no index.lock file was written;\n  that test is not possible in a portable way. Instead, we\n  verify that .git/index is rewritten *only* when `git\n  status` is run without `--no-optional-locks`.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git.txt | 13 +++++++++++++\n builtin/commit.c      |  5 ++++-\n cache.h               |  6 ++++++\n environment.c         |  5 +++++\n git.c                 |  4 ++++\n t/t7508-status.sh     | 10 ++++++++++\n 6 files changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 6e3a6767e5..8dd3ae05ae 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -159,6 +159,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string which ` git config\n \tAdd \"icase\" magic to all pathspec. This is equivalent to setting\n \tthe `GIT_ICASE_PATHSPECS` environment variable to `1`.\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+\n GIT COMMANDS\n ------------\n \n@@ -697,6 +701,15 @@ of clones and fetches.\n \twhich feed potentially-untrusted URLS to git commands.  See\n \tlinkgit:git-config[1] for more details.\n \n+`GIT_OPTIONAL_LOCKS`::\n+\tIf set to `0`, Git will avoid performing any operations which\n+\trequire taking a lock and which are not required to complete the\n+\trequested operation. For example, this will prevent `git status`\n+\tfrom refreshing the index as a side effect. This is useful for\n+\tprocesses running in the background which do not want to cause\n+\tlock contention with other operations on the repository.\n+\tDefaults to `1`.\n+\n Discussion[[Discussion]]\n ------------------------\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 58f9747c2f..fafd492029 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1387,7 +1387,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tread_cache_preload(&s.pathspec);\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL);\n \n-\tfd = hold_locked_index(&index_lock, 0);\n+\tif (use_optional_locks())\n+\t\tfd = hold_locked_index(&index_lock, 0);\n+\telse\n+\t\tfd = -1;\n \n \ts.is_initial = get_oid(s.reference, &oid) ? 1 : 0;\n \tif (!s.is_initial)\ndiff --git a/cache.h b/cache.h\nindex a916bc79e3..fea400c5ec 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -443,6 +443,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_NOGLOB_PATHSPECS_ENVIRONMENT \"GIT_NOGLOB_PATHSPECS\"\n #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n #define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n+#define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n \n /*\n  * This environment variable is expected to contain a boolean indicating\n@@ -782,6 +783,11 @@ extern int protect_ntfs;\n  */\n extern int ref_paranoia;\n \n+/*\n+ * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value).\n+ */\n+int use_optional_locks(void);\n+\n /*\n  * The character that begins a commented line in user-editable file\n  * that is subject to stripspace.\ndiff --git a/environment.c b/environment.c\nindex f1f934b6fd..8289c25b44 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -338,3 +338,8 @@ void reset_shared_repository(void)\n {\n \tneed_shared_repository_from_config = 1;\n }\n+\n+int use_optional_locks(void)\n+{\n+\treturn git_env_bool(GIT_OPTIONAL_LOCKS_ENVIRONMENT, 1);\n+}\ndiff --git a/git.c b/git.c\nindex f31dca6962..9e96dd4090 100644\n--- a/git.c\n+++ b/git.c\n@@ -182,6 +182,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ICASE_PATHSPECS_ENVIRONMENT, \"1\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-optional-locks\")) {\n+\t\t\tsetenv(GIT_OPTIONAL_LOCKS_ENVIRONMENT, \"0\", 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 43d19a9b22..93f162a4f7 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1670,4 +1670,14 @@ test_expect_success '\"Initial commit\" should not be noted in commit template' '\n \ttest_i18ngrep ! \"Initial commit\" output\n '\n \n+test_expect_success '--no-optional-locks prevents index update' '\n+\ttest-chmtime =1234567890 .git/index &&\n+\tgit --no-optional-locks status &&\n+\ttest-chmtime -v +0 .git/index >out &&\n+\tgrep ^1234567890 out &&\n+\tgit status &&\n+\ttest-chmtime -v +0 .git/index >out &&\n+\t! grep ^1234567890 out\n+'\n+\n test_done\n-- \n2.14.1.1040.gcaf8795f39\n"},{"id":"328540","messageId":"xmqqbmm4lkf5.fsf@gitster.mtv.corp.google.com","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T04:55:58Z","receivedAt":"2017-09-21T04:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not*\n> to lock the index and update it, 2016-08-12). Folks working on GitHub\n> Desktop complained to me that it's only available on Windows. :)\n>\n> I expanded the scope a bit to let us give the same treatment to more\n> commands in the long run.  I'd also be OK with just cherry-picking your\n> patch to non-Windows Git if you don't find my reasoning below\n> compelling. But I think we need _something_ like this, as the other\n> solutions I could come up with don't seem very promising.\n\nThe phrase 'optional lock' does not answer this question clearly,\nthough: does it make sense to extend the coverage of this option in\nthe future to things more than the \"opportunistic update to the\nindex file\"?\n\nIf the answer is no, then having 'index' instead of 'lock' in the\nname of the option would make more sense (and 'opportunistic' over\n'optional', too), because what the change is about is to allow other\nprocesses that are directly interacting with the user to update the\nindex, and 'lock' being hindrance is merely an implementation\ndetail.  The comment on the \"test\" in the log message mentions as if\nit were a short-coming that it does not check the lock but checks\nif the index is written, but I think that is testing what matters\nand preferable than testing \"did we lock and then unlock it?\"\n\nOn the other hand, if the answer is yes, then I am curious what\nother things this may extend to, and if these other things are also\nopportunistic optimizations.\n\nOther than that, I think this change (including the part that this\nis done globally and down to subprocesses as needed) makes sense.\n\nThanks (and sorry for not being Johannes ;-).\n\n\n"},{"id":"328542","messageId":"20170921050835.mrbgx2zryy3jusdk@sigill.intra.peff.net","threadId":"46794","inReplyTo":"xmqqbmm4lkf5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T05:08:36Z","receivedAt":"2017-09-21T05:08:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2017 at 01:55:58PM +0900, Junio C Hamano wrote:\n\n> The phrase 'optional lock' does not answer this question clearly,\n> though: does it make sense to extend the coverage of this option in\n> the future to things more than the \"opportunistic update to the\n> index file\"?\n> \n> If the answer is no, then having 'index' instead of 'lock' in the\n> name of the option would make more sense (and 'opportunistic' over\n> 'optional', too), because what the change is about is to allow other\n> processes that are directly interacting with the user to update the\n> index, and 'lock' being hindrance is merely an implementation\n> detail.  The comment on the \"test\" in the log message mentions as if\n> it were a short-coming that it does not check the lock but checks\n> if the index is written, but I think that is testing what matters\n> and preferable than testing \"did we lock and then unlock it?\"\n> \n> On the other hand, if the answer is yes, then I am curious what\n> other things this may extend to, and if these other things are also\n> opportunistic optimizations.\n\nI left it intentionally vague exactly because I thought we might want to\nleave room for the answer to change to \"yes\" eventually.  For instance,\nimagine that we had a ref storage format that required periodic\ncompaction, and readers might sometimes choose to compact in order to\nsave future readers from repeating some work they've done. If that\ncompaction means holding a lock even for a brief period, I think it\nwould fall under this option.\n\nI admit that's just adding more hand-waving to the pile. But I don't\nthink it really _hurts_ to leave that door open (aside from making the\ndocumentation a bit wishy-washy). And it helps because callers would\npick up the new features automatically, without having to learn about a\nnew option.\n\nAnd I think that's really what this option is. It is less about the\ncaller asking for some specific behavior, and more about them telling\nGit about the context in which it's executing so it can make intelligent\ndecisions.\n\nAnd in that sense, something descriptive like --background-process\nperhaps would be a better name. Except that I couldn't come up with a\nname that isn't confusing (certainly --background-process implies to me\nthat Git would itself run in the background, which makes no sense here).\n\nI also considered something like \"--read-only\" to tell Git that we\nshould avoid writing to the repository. But that's really not what this\ndoes. It just avoids writes that may cause contention, not all writes.\n\nI also considered using the word \"opportunistic\" in the option name, but\ndecided it was too long and hard to spell.\n\nSo there. I am open to a better name, but I could not come up with one.\n\n> Thanks (and sorry for not being Johannes ;-).\n\nYou lack his rugged good looks, but your review was still welcome.\n\n-Peff\n"},{"id":"328544","messageId":"xmqq377gliv9.fsf@gitster.mtv.corp.google.com","threadId":"46794","inReplyTo":"20170921050835.mrbgx2zryy3jusdk@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T05:29:30Z","receivedAt":"2017-09-21T05:29:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I admit that's just adding more hand-waving to the pile. But I don't\n> think it really _hurts_ to leave that door open (aside from making the\n> documentation a bit wishy-washy). And it helps because callers would\n> pick up the new features automatically, without having to learn about a\n> new option.\n\nOh, I do agree it is a good idea to leave that door open, so that\nscripts that rely on today's --no-optional-locks option will not\nhave to be updated when a similar feature (like the \"ref compaction\"\nexample you mentioned) against which \"let's disable when we are not\nthe primary process, in order to keep the interference to the\nminimum\" would be something we would want to say.  The option being\nadded here should cover these future needs.\n\n> And I think that's really what this option is. It is less about the\n> caller asking for some specific behavior, and more about them telling\n> Git about the context in which it's executing so it can make intelligent\n> decisions.\n\nYes, indeed.\n"},{"id":"328585","messageId":"7a2d895c-df8c-1305-234f-7f8e2b271ec2@kdbg.org","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-09-21T18:25:50Z","receivedAt":"2017-09-21T18:25:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.09.2017 um 06:32 schrieb Jeff King:\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 6e3a6767e5..8dd3ae05ae 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -159,6 +159,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string which ` git config\n>   \tAdd \"icase\" magic to all pathspec. This is equivalent to setting\n>   \tthe `GIT_ICASE_PATHSPECS` environment variable to `1`.\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> +\n>   GIT COMMANDS\n>   ------------\n>   \n> @@ -697,6 +701,15 @@ of clones and fetches.\n>   \twhich feed potentially-untrusted URLS to git commands.  See\n>   \tlinkgit:git-config[1] for more details.\n>   \n> +`GIT_OPTIONAL_LOCKS`::\n> +\tIf set to `0`, Git will avoid performing any operations which\n> +\trequire taking a lock and which are not required to complete the\n> +\trequested operation. For example, this will prevent `git status`\n> +\tfrom refreshing the index as a side effect. This is useful for\n> +\tprocesses running in the background which do not want to cause\n> +\tlock contention with other operations on the repository.\n> +\tDefaults to `1`.\n\nI don't think we should pass this environment variable to remote \nrepositories. It should be listed in local_repo_env[] in environment.c.\n\n-- Hannes\n"},{"id":"328617","messageId":"20170922042539.fs37bauiabfqcjx5@sigill.intra.peff.net","threadId":"46794","inReplyTo":"7a2d895c-df8c-1305-234f-7f8e2b271ec2@kdbg.org","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T04:25:40Z","receivedAt":"2017-09-22T04:25:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2017 at 08:25:50PM +0200, Johannes Sixt wrote:\n\n> > +`GIT_OPTIONAL_LOCKS`::\n> > +\tIf set to `0`, Git will avoid performing any operations which\n> > +\trequire taking a lock and which are not required to complete the\n> > +\trequested operation. For example, this will prevent `git status`\n> > +\tfrom refreshing the index as a side effect. This is useful for\n> > +\tprocesses running in the background which do not want to cause\n> > +\tlock contention with other operations on the repository.\n> > +\tDefaults to `1`.\n> \n> I don't think we should pass this environment variable to remote\n> repositories. It should be listed in local_repo_env[] in environment.c.\n\nI'm not sure I agree. This is really about the context in which the\ncommand is executing, not anything about the particular repository\nyou're operating on.\n\nFor fetch/push operations that touch a remote, I doubt it would matter\neither way (and anyway, those often cross network boundaries that don't\npropagate environment variables anyway).\n\nBut imagine that \"git status\" learns to recurse into submodules and run\n\"git status\" inside them. Surely we would want the submodule repos to\nalso avoid taking any unnecessary locks?\n\n-Peff\n"},{"id":"328624","messageId":"c474bf47-5049-1425-d03c-2417eb53549e@pobox.com","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Daniel Santos","fromEmail":"daniel.santos@pobox.com","sentAt":"2017-09-22T06:42:10Z","receivedAt":"2017-09-22T06:36:10Z","isPatch":true,"sender":{"key":"daniel.santos@pobox.com","avatar":"https://gravatar.com/avatar/44d192e1fd815938960d07b2ba753ed185c8b37315c91919645c0a49a0d6be98?d=mp&s=160"},"body":"On 09/20/2017 11:32 PM, Jeff King wrote:\n> Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not*\n> to lock the index and update it, 2016-08-12). Folks working on GitHub\n> Desktop complained to me that it's only available on Windows. :)\n>\n> I expanded the scope a bit to let us give the same treatment to more\n> commands in the long run.  I'd also be OK with just cherry-picking your\n> patch to non-Windows Git if you don't find my reasoning below\n> compelling. But I think we need _something_ like this, as the other\n> solutions I could come up with don't seem very promising.\n>\n> -Peff\n>\n> -- >8 --\n> Some tools like IDEs or fancy editors may periodically run\n> commands like \"git status\" in the background to keep track\n> of the state of the repository. Some of these commands may\n> refresh the index and write out the result in an\n> opportunistic way: if they can get the index lock, then they\n> update the on-disk index with any updates they find. And if\n> not, then their in-core refresh is lost and just has to be\n> recomputed by the next caller.\n>\n> But taking the index lock may conflict with other operations\n> in the repository. Especially ones that the user is doing\n> themselves, which _aren't_ opportunistic. In other words,\n> \"git status\" knows how to back off when somebody else is\n> holding the lock, but other commands don't know that status\n> would be happy to drop the lock if somebody else wanted it.\n\nInterestingly, this usually slaps me when performing an _interactive_\nrebase.  It occurred to me that if I'm performing an interaction\noperation, it doesn't seem unreasonable for git wait up to 125ms or so\nfor the lock and then prompting the user to ask if they want to continue\nwaiting for the lock.\n\n> There are a couple possible solutions:\n>\n>   1. Have some kind of \"pseudo-lock\" that allows other\n>      commands to tell status that they want the lock.\n>\n>      This is likely to be complicated and error-prone to\n>      implement (and maybe even impossible with just\n>      dotlocks to work from, as it requires some\n>      inter-process communication).\n>\n>   2. Avoid background runs of commands like \"git status\"\n>      that want to do opportunistic updates, preferring\n>      instead plumbing like diff-files, etc.\n>\n>      This is awkward for a couple of reasons. One is that\n>      \"status --porcelain\" reports a lot more about the\n>      repository state than is available from individual\n>      plumbing commands. And two is that we actually _do_\n>      want to see the refreshed index. We just don't want to\n>      take a lock or write out the result. Whereas commands\n>      like diff-files expect us to refresh the index\n>      separately and write it to disk so that they can depend\n>      on the result. But that write is exactly what we're\n>      trying to avoid.\n>\n>   3. Ask \"status\" not to lock or write the index.\n>\n>      This is easy to implement. The big downside is that any\n>      work done in refreshing the index for such a call is\n>      lost when the process exits. So a background process\n>      may end up re-hashing a changed file multiple times\n>      until the user runs a command that does an index\n>      refresh themselves.\n\nThat is not necessarily the case.  I don't actually know git on the\ninside, but I would ask you to consider a read-write lock and a hybrid\nof one and three.\n\nI don't know what dotlocks are, but I'm certain that you can implement a\nrw lock using lock files and no other IPC, although it does increase the\ncomplexity.  The way this works is that `git status' acquires a read\nlock and does its thing.  If it has real changes, instead of discarding\nthem it attempts to upgrade to a write lock.  If that fails, you throw\nit away, otherwise you write them and release.\n\nIn order to implement rw locks with only lock files, \"off the cuff\" I\nsay you have a single \"lock list\" file that should never be deleted and\na \"lock lock\" file that is held in order to read or modify the list. \nThe format of the lock list would have a pair of 32-bit wrapping\nmodification counts (or versions) at the top -- one for modifications to\nthe lock list its self and another for modifications to the underlying\ndata (i.e., the number of times a write lock has been acquired).  This\nheader is followed by entries something like this:\n\n<operation> <pid> <version> <timestamp>\n\n<operation>  'r' if waiting for a read lock\n             'R' if actively reading\n             'w' if waiting for write lock\n             'W' if actively writing\n<pid>        The pid\n<version>    If active, the version of the data at the time lock acquired or zero.\n<timestamp>  Time began waiting or time lock acquired\n\n\nAn operation of 'r' or 'w' means that you are waiting and upper case\nmeans that you are active.  <version> is the version of the data at the\ntime the lock became active and writers increment it when they acquire a\nlock.  You wait with file alteration notification on the lock list (if\nthere is any doubt based upon timestamp precision then you can examine\nthe lock list version).  When you want to read or write, you lock the\nlock-lock (this sounds like a joke... \"Lock, lock.\", \"Who's there?\",\n\"Reader.\", \"Reader who?\"....) and examine the lock list.  If it's empty,\nyou add yourself as an active reader or writer with an upper case 'R' or\n'W' and release the lock-lock.  If there are only readers, and you want\nto read, you add yourself as an active reader.  The version of the lock\nlist is incremented every time it is modified.\n\nRead-write locks need to be given a priority policy of either readers,\nwriters, fifo or don't-care.  In this case that should probably be\nwriters.  So if you want to read and there is an active or waiting\nwriter, the prospective reader would either add themselves with an 'r'\nor fail if they don't want to wait.  If a process wants to write and\nthere are active readers or writers, it adds its self to the list or\nfails as well.  When all active readers have exited, then which ever\nprospective writers gets the lock-lock first can make themselves the\nactive writer.  When a process acquires a write lock, it increases the\ndata version number.  If a reader lock tries to upgrade to a writer lock\nbut the data version changed than it fails.\n\nIs there not already a library somewhere that does this?  Either way,\nyour current effort seems like a step in the right direction -- and\nthanks for that!\n\nDaniel\n\n"},{"id":"328629","messageId":"dcd542e9-ba92-4dc8-6929-2d0f02bd6307@jeffhostetler.com","threadId":"46794","inReplyTo":"20170922042539.fs37bauiabfqcjx5@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-22T11:22:28Z","receivedAt":"2017-09-22T11:22:35Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/22/2017 12:25 AM, Jeff King wrote:\n> On Thu, Sep 21, 2017 at 08:25:50PM +0200, Johannes Sixt wrote:\n> \n>>> +`GIT_OPTIONAL_LOCKS`::\n>>> +\tIf set to `0`, Git will avoid performing any operations which\n>>> +\trequire taking a lock and which are not required to complete the\n>>> +\trequested operation. For example, this will prevent `git status`\n>>> +\tfrom refreshing the index as a side effect. This is useful for\n>>> +\tprocesses running in the background which do not want to cause\n>>> +\tlock contention with other operations on the repository.\n>>> +\tDefaults to `1`. >>\n>> I don't think we should pass this environment variable to remote\n>> repositories. It should be listed in local_repo_env[] in environment.c.\n> \n> I'm not sure I agree. This is really about the context in which the\n> command is executing, not anything about the particular repository\n> you're operating on.\n> \n> For fetch/push operations that touch a remote, I doubt it would matter\n> either way (and anyway, those often cross network boundaries that don't\n> propagate environment variables anyway).\n> \n> But imagine that \"git status\" learns to recurse into submodules and run\n> \"git status\" inside them. Surely we would want the submodule repos to\n> also avoid taking any unnecessary locks?\n> \n> -Peff\n> \n\nhttps://github.com/git-for-windows/git/commit/ff63b51c22389139a864eb2e565c6cdc5a30f061\n\nhttps://github.com/git-for-windows/git/pull/1004/commits/45bad66192352481acbc826f11d90c8928b39a7a\n\nWe should compare this with what we did in Git for Windows last fall.\nI guess those commits didn't get pushed upstream.\n\nWe added '--no-lock-index' to keep status from locking the index\nduring status and effectively being read-only.  This helped with\nproblems with Visual Studio similar to the ones being described\nfor KDevelop.\n\nJeff\n\n"},{"id":"328636","messageId":"20170922150438.whslnn4qkamdhewr@sigill.intra.peff.net","threadId":"46794","inReplyTo":"dcd542e9-ba92-4dc8-6929-2d0f02bd6307@jeffhostetler.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T15:04:38Z","receivedAt":"2017-09-22T15:04:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 07:22:28AM -0400, Jeff Hostetler wrote:\n\n> > > I don't think we should pass this environment variable to remote\n> > > repositories. It should be listed in local_repo_env[] in environment.c.\n> > \n> > I'm not sure I agree. This is really about the context in which the\n> > command is executing, not anything about the particular repository\n> > you're operating on.\n> > \n> > For fetch/push operations that touch a remote, I doubt it would matter\n> > either way (and anyway, those often cross network boundaries that don't\n> > propagate environment variables anyway).\n> > \n> > But imagine that \"git status\" learns to recurse into submodules and run\n> > \"git status\" inside them. Surely we would want the submodule repos to\n> > also avoid taking any unnecessary locks?\n> > \n> > -Peff\n> > \n> \n> https://github.com/git-for-windows/git/commit/ff63b51c22389139a864eb2e565c6cdc5a30f061\n> \n> https://github.com/git-for-windows/git/pull/1004/commits/45bad66192352481acbc826f11d90c8928b39a7a\n> \n> We should compare this with what we did in Git for Windows last fall.\n> I guess those commits didn't get pushed upstream.\n\nRight. I think you missed the initial message in the thread that\nexplains how this is an expanded version of ff63b51c22. :)\n\nI didn't know about the environment thing in 45bad66192, though[1]. That\nmakes me even more confident that this is the right approach.\n\n-Peff\n\n[1] Sorry for not doing my homework more carefully on the existing\n    solution.  GitHub Desktop ran into the same situation and pointed me\n    at ff63b51c22. I extrapolated the rest of it on my own. ;)\n"},{"id":"328640","messageId":"20170922160413.nbqhmszvdfrvz3u4@sigill.intra.peff.net","threadId":"46794","inReplyTo":"c474bf47-5049-1425-d03c-2417eb53549e@pobox.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T16:04:14Z","receivedAt":"2017-09-22T16:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 01:42:10AM -0500, Daniel Santos wrote:\n\n> > But taking the index lock may conflict with other operations\n> > in the repository. Especially ones that the user is doing\n> > themselves, which _aren't_ opportunistic. In other words,\n> > \"git status\" knows how to back off when somebody else is\n> > holding the lock, but other commands don't know that status\n> > would be happy to drop the lock if somebody else wanted it.\n> \n> Interestingly, this usually slaps me when performing an _interactive_\n> rebase.  It occurred to me that if I'm performing an interaction\n> operation, it doesn't seem unreasonable for git wait up to 125ms or so\n> for the lock and then prompting the user to ask if they want to continue\n> waiting for the lock.\n\nYes, lock timeouts would help with this situation, though not eliminate\nit entirely (and we've been adding some lock timeouts in a few places\nfor related situations).\n\nWe generally avoid prompting, especially when a command can just be\nreissued. It sounds like \"git rebase\" gets into a funny state if it\ncannot grab the index lock. That's something that should be fixed. Even\nif it bails, you should be able to move forward with \"rebase --continue\"\nor similar.\n\n> That is not necessarily the case.  I don't actually know git on the\n> inside, but I would ask you to consider a read-write lock and a hybrid\n> of one and three.\n> \n> I don't know what dotlocks are, but I'm certain that you can implement a\n> rw lock using lock files and no other IPC, although it does increase the\n> complexity.  The way this works is that `git status' acquires a read\n> lock and does its thing.  If it has real changes, instead of discarding\n> them it attempts to upgrade to a write lock.  If that fails, you throw\n> it away, otherwise you write them and release.\n\nBy dotlocks, I just mean our strategy of creating O_EXCL \"index.lock\"\nfiles.\n\nYou're right that it can be done using two such locks, and communicating\nvia the lockfile contents. So my \"impossible\" was overstating it. I do\nstand by my contention that it's much more complex than the existing\nscheme. :)\n\nMore importantly, though, it changes the locking contract completely\nbetween old and new versions (and between other implementations).\nThere's probably only a small minority of users who might use two\nimplementations to simultaneously access the same repository, but it is\na use case we try to support. I think we'd have to require a\nrepository-version bump to tell programs to use the new scheme.\n\n-Peff\n"},{"id":"328667","messageId":"CAGZ79kaUTdFi5LkgR6zKeb+CH8vpfPQRZf6bFqRfYY-YXy_TNA@mail.gmail.com","threadId":"46794","inReplyTo":"20170922042539.fs37bauiabfqcjx5@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-22T20:09:32Z","receivedAt":"2017-09-22T20:09:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 21, 2017 at 9:25 PM, Jeff King <peff@peff.net> wrote:\n\n>\n> But imagine that \"git status\" learns to recurse into submodules and run\n> \"git status\" inside them. Surely we would want the submodule repos to\n> also avoid taking any unnecessary locks?\n\nYou can teach Git to recurse into submodules already at home,\njust 'git config status.submoduleSummary none'. ;)\n\nIt occurs to me that the config name is badly choosen, as it stores\nan argument for git status --ignore-submodules[=mode]\n"},{"id":"328688","messageId":"20170922212525.kxldygbjrmjqu7ci@sigill.intra.peff.net","threadId":"46794","inReplyTo":"CAGZ79kaUTdFi5LkgR6zKeb+CH8vpfPQRZf6bFqRfYY-YXy_TNA@mail.gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T21:25:25Z","receivedAt":"2017-09-22T21:25:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 01:09:32PM -0700, Stefan Beller wrote:\n\n> On Thu, Sep 21, 2017 at 9:25 PM, Jeff King <peff@peff.net> wrote:\n> \n> >\n> > But imagine that \"git status\" learns to recurse into submodules and run\n> > \"git status\" inside them. Surely we would want the submodule repos to\n> > also avoid taking any unnecessary locks?\n> \n> You can teach Git to recurse into submodules already at home,\n> just 'git config status.submoduleSummary none'. ;)\n> \n> It occurs to me that the config name is badly choosen, as it stores\n> an argument for git status --ignore-submodules[=mode]\n\nAh, thanks. I _thought_ we could already do that but when I went looking\nfor the standard --recursive option I couldn't find it.\n\nSo yes, I would think we would want this option to apply recursively in\nthat case, even when we cross repository boundaries.\n\n-Peff\n"},{"id":"328692","messageId":"CAGZ79kYJH-4qb5hiT1C_aCFXLcdiJs5h+Z7Goz4trp52C5s3Lw@mail.gmail.com","threadId":"46794","inReplyTo":"20170922212525.kxldygbjrmjqu7ci@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-22T21:41:06Z","receivedAt":"2017-09-22T21:41:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 22, 2017 at 2:25 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 22, 2017 at 01:09:32PM -0700, Stefan Beller wrote:\n>\n>> On Thu, Sep 21, 2017 at 9:25 PM, Jeff King <peff@peff.net> wrote:\n>>\n>> >\n>> > But imagine that \"git status\" learns to recurse into submodules and run\n>> > \"git status\" inside them. Surely we would want the submodule repos to\n>> > also avoid taking any unnecessary locks?\n>>\n>> You can teach Git to recurse into submodules already at home,\n>> just 'git config status.submoduleSummary none'. ;)\n>>\n>> It occurs to me that the config name is badly choosen, as it stores\n>> an argument for git status --ignore-submodules[=mode]\n>\n> Ah, thanks. I _thought_ we could already do that but when I went looking\n> for the standard --recursive option I couldn't find it.\n\nThanks for checking for submodules there.\n\nI personally prefer --recurse-submodules despite the longer name,\nbecause a plain recursive option can mean anything in a sufficiently\ncomplex program such as Git (recurse into the tree (c.f. ls-tree), or\nfor the algorithm used (c.f. merge, diff) or yet another dimension\nI did not think of).\n\n> So yes, I would think we would want this option to apply recursively in\n> that case, even when we cross repository boundaries.\n\nRegarding the actual patch which is heavily aimed at coping with IDEs\ndespite the command line being used, I wonder how many IDEs pass\n--ignore-submodules and recurse themselves (if needed). Reason for\nmy suspicion is [1] which does pay attention to submodules:\n\n>    Our application calls status including the following flags:\n>    --porcelain=v2 --ignored --untracked-files=all --ignore-submodules=none\n\n[1] https://public-inbox.org/git/2bbb1d0f-ae06-1878-d185-112bd51f75c9@gmail.com/\n\nThere might be another option to cope with the situation:\n\n 4. Teach all commands to spinlock / busywait shortly for important\n     locks instead of giving up. In that case git-status rewriting\n     the index ought to be fine?\n\nThanks,\nStefan\n"},{"id":"328710","messageId":"20170923033425.6ilykxl5pi7unlj6@sigill.intra.peff.net","threadId":"46794","inReplyTo":"CAGZ79kYJH-4qb5hiT1C_aCFXLcdiJs5h+Z7Goz4trp52C5s3Lw@mail.gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T03:34:26Z","receivedAt":"2017-09-23T03:34:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 02:41:06PM -0700, Stefan Beller wrote:\n\n> > Ah, thanks. I _thought_ we could already do that but when I went looking\n> > for the standard --recursive option I couldn't find it.\n> \n> Thanks for checking for submodules there.\n> \n> I personally prefer --recurse-submodules despite the longer name,\n> because a plain recursive option can mean anything in a sufficiently\n> complex program such as Git (recurse into the tree (c.f. ls-tree), or\n> for the algorithm used (c.f. merge, diff) or yet another dimension\n> I did not think of).\n\nYeah, I agree that's a better name (mentioning --recursive is probably\njust showing my general ignorance of submodules).\n\n> > So yes, I would think we would want this option to apply recursively in\n> > that case, even when we cross repository boundaries.\n> \n> Regarding the actual patch which is heavily aimed at coping with IDEs\n> despite the command line being used, I wonder how many IDEs pass\n> --ignore-submodules and recurse themselves (if needed). Reason for\n> my suspicion is [1] which does pay attention to submodules:\n> \n> >    Our application calls status including the following flags:\n> >    --porcelain=v2 --ignored --untracked-files=all --ignore-submodules=none\n> \n> [1] https://public-inbox.org/git/2bbb1d0f-ae06-1878-d185-112bd51f75c9@gmail.com/\n\nYeah, I think that's probably Visual Studio (which is what Johannes\npresumably wrote the original patch for). GitHub Desktop does not\ncurrently pass it, though perhaps should consider doing so (though of\ncourse the user could tweak their config, too).\n\n> There might be another option to cope with the situation:\n> \n>  4. Teach all commands to spinlock / busywait shortly for important\n>      locks instead of giving up. In that case git-status rewriting\n>      the index ought to be fine?\n\nWe do have all the infrastructure in place to do a reasonable busywait\nwith backoff. I think the patch is roughly just:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 65f4fe8375..fc1ba122a3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1563,7 +1563,8 @@ static int read_index_extension(struct index_state *istate,\n \n int hold_locked_index(struct lock_file *lk, int lock_flags)\n {\n-\treturn hold_lock_file_for_update(lk, get_index_file(), lock_flags);\n+\treturn hold_lock_file_for_update_timeout(lk, get_index_file(),\n+\t\t\t\t\t\t lock_flags, 500);\n }\n \n int read_index(struct index_state *istate)\n\nthough I think there are a few sites which manually call\nhold_lock_file_for_update() on the index that would need similar\ntreatment.\n\nI suspect it would work OK in practice, unless your index is so big that\n500ms isn't enough. The user may also see minor stalls when there's lock\ncontention. I'm not sure how annoying that would be.\n\n-Peff\n"},{"id":"328765","messageId":"1506252671.8678.3.camel@gmail.com","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-24T11:31:11Z","receivedAt":"2017-09-24T11:31:26Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thursday 21 September 2017 10:02 AM, Jeff King wrote:\n> Some tools like IDEs or fancy editors may periodically run\n> commands like \"git status\" in the background to keep track\n> of the state of the repository.\n\nI might be missing something, shouldn't the IDEs be encouraged to use\nlibgit2 instead? I thought it was meant for these use cases.\n\nNote: I assume getting the status through libgit2 doesn't create an \nindex.lock file.\n\n>    3. Ask \"status\" not to lock or write the index.\n> \n>       This is easy to implement. The big downside is that any\n>       work done in refreshing the index for such a call is\n>       lost when the process exits. So a background process\n>       may end up re-hashing a changed file multiple times\n>       until the user runs a command that does an index\n>       refresh themselves.\n> \n> This patch implements the option 3.\n\nSo, if I get that correctly \"git status --no-optional-locks\" is a way\nto get the \"current\" status without updating the on disk index file?\n\n> +`GIT_OPTIONAL_LOCKS`::\n> +\tIf set to `0`, Git will avoid performing any operations which\n> +\trequire taking a lock and which are not required to complete the\n> +\trequested operation.\n\nThe above sentence seems to be a little hard to interpret. How about,\n\n    If set to `0`, Git will complete the requested operation without\n    performing the optional sub-operations that require taking a lock.\n\n\n---\nKaartic\n"},{"id":"328856","messageId":"alpine.DEB.2.21.1.1709251808350.40514@virtualbox","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-25T16:10:12Z","receivedAt":"2017-09-25T16:10:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 21 Sep 2017, Jeff King wrote:\n\n> Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not*\n> to lock the index and update it, 2016-08-12). Folks working on GitHub\n> Desktop complained to me that it's only available on Windows. :)\n\nOkay, so this is trying to help me by upstreaming a patch from Git for\nWindows?\n\nIf so: thanks!\n\nThe changes, in particular the different semantics, will guarantee that I\nhave to work on the consumer's side here, though, leaving me even less\ntime for the Git mailing list, so I will need a lot more help with\nupstreaming patches.\n\nCiao,\nDscho\n"},{"id":"328857","messageId":"alpine.DEB.2.21.1.1709251816120.40514@virtualbox","threadId":"46794","inReplyTo":"1506252671.8678.3.camel@gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-25T16:17:12Z","receivedAt":"2017-09-25T16:17:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kaartic,\n\nOn Sun, 24 Sep 2017, Kaartic Sivaraam wrote:\n\n> On Thursday 21 September 2017 10:02 AM, Jeff King wrote:\n> > Some tools like IDEs or fancy editors may periodically run commands\n> > like \"git status\" in the background to keep track of the state of the\n> > repository.\n> \n> I might be missing something, shouldn't the IDEs be encouraged to use\n> libgit2 instead? I thought it was meant for these use cases.\n\nThere are pros and cons. Visual Studio moved away from libgit2 e.g. to\nsupport SSH (back then, libgit2 did not support spawning processes, I have\nno idea whether that changed in the meantime).\n\nCiao,\nJohannes\n"},{"id":"328860","messageId":"20170925170012.ik7lk35hhsp66tvh@sigill.intra.peff.net","threadId":"46794","inReplyTo":"alpine.DEB.2.21.1.1709251808350.40514@virtualbox","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T17:00:13Z","receivedAt":"2017-09-25T17:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 06:10:12PM +0200, Johannes Schindelin wrote:\n\n> On Thu, 21 Sep 2017, Jeff King wrote:\n> \n> > Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not*\n> > to lock the index and update it, 2016-08-12). Folks working on GitHub\n> > Desktop complained to me that it's only available on Windows. :)\n> \n> Okay, so this is trying to help me by upstreaming a patch from Git for\n> Windows?\n> \n> If so: thanks!\n\nYes, in a round-about way. :)\n\n> The changes, in particular the different semantics, will guarantee that I\n> have to work on the consumer's side here, though, leaving me even less\n> time for the Git mailing list, so I will need a lot more help with\n> upstreaming patches.\n\nI think you can get away with doing nothing for the time being. The two\npatches can co-exist in the GfW codebase[1], and people using the\nexisting option can switch over at their leisure. Eventually you may\ndecide to revert 67e5ce7f63.\n\n-Peff\n\n[1] Modulo some trivial conflict resolution when the two are merged.\n"},{"id":"328865","messageId":"CAGZ79kYOeJvQmw-h3GwFpv2w7AKtNWUxf96tUjPKPY1dMuzagA@mail.gmail.com","threadId":"46794","inReplyTo":"20170923033425.6ilykxl5pi7unlj6@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-25T18:51:31Z","receivedAt":"2017-09-25T18:51:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> There might be another option to cope with the situation:\n>>\n>>  4. Teach all commands to spinlock / busywait shortly for important\n>>      locks instead of giving up. In that case git-status rewriting\n>>      the index ought to be fine?\n>\n> We do have all the infrastructure in place to do a reasonable busywait\n> with backoff. I think the patch is roughly just:\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 65f4fe8375..fc1ba122a3 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1563,7 +1563,8 @@ static int read_index_extension(struct index_state *istate,\n>\n>  int hold_locked_index(struct lock_file *lk, int lock_flags)\n>  {\n> -       return hold_lock_file_for_update(lk, get_index_file(), lock_flags);\n> +       return hold_lock_file_for_update_timeout(lk, get_index_file(),\n> +                                                lock_flags, 500);\n>  }\n>\n>  int read_index(struct index_state *istate)\n>\n> though I think there are a few sites which manually call\n> hold_lock_file_for_update() on the index that would need similar\n> treatment.\n\nuh, too bad. The patch above looks really promising, though. :)\n\n>\n> I suspect it would work OK in practice, unless your index is so big that\n> 500ms isn't enough. The user may also see minor stalls when there's lock\n> contention. I'm not sure how annoying that would be.\n\nThere is only one way to find out. Though we don't want to volunteer\nall users into this experiment, I'd presume.\n\nRegarding larger indexes, I wonder if we can adapt the 500ms\nto the repo size. At first I thought the abbreviation length could be\na good proxy to determine the maximum waiting time, but now I am\nnot so sure any more.\n\nStefan\n"},{"id":"328946","messageId":"2646e18b-473e-17bc-fa92-1d7ef5f46055@jeffhostetler.com","threadId":"46794","inReplyTo":"alpine.DEB.2.21.1.1709251816120.40514@virtualbox","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-26T14:44:46Z","receivedAt":"2017-09-26T14:44:52Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/25/2017 12:17 PM, Johannes Schindelin wrote:\n> Hi Kaartic,\n> \n> On Sun, 24 Sep 2017, Kaartic Sivaraam wrote:\n> \n>> On Thursday 21 September 2017 10:02 AM, Jeff King wrote:\n>>> Some tools like IDEs or fancy editors may periodically run commands\n>>> like \"git status\" in the background to keep track of the state of the\n>>> repository.\n>>\n>> I might be missing something, shouldn't the IDEs be encouraged to use\n>> libgit2 instead? I thought it was meant for these use cases.\n> \n> There are pros and cons. Visual Studio moved away from libgit2 e.g. to\n> support SSH (back then, libgit2 did not support spawning processes, I have\n> no idea whether that changed in the meantime).\n\nThere were other issues besides feature parity.  The big one for VS\nwas that it moved the git computations into a separate process and\naddress space.  You can't easily read/modify/write a 500MB .git/index\nfile into memory (with however many copies of the data that that\ncreates) in a 32-bit process.\n\n"},{"id":"329020","messageId":"20170927064026.5xa3ydaggmknvulw@sigill.intra.peff.net","threadId":"46794","inReplyTo":"79ed4c34-1727-7c1e-868a-1206902638ad@gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:40:26Z","receivedAt":"2017-09-27T06:41:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 24, 2017 at 01:08:41PM +0530, Kaartic Sivaraam wrote:\n\n> On Thursday 21 September 2017 10:02 AM, Jeff King wrote:\n> > Some tools like IDEs or fancy editors may periodically run\n> > commands like \"git status\" in the background to keep track\n> > of the state of the repository.\n> \n> I might be missing something, shouldn't the IDEs be encouraged to use\n> libgit2 instead? I thought it was meant for these use cases.\n\nGitHub Desktop, at least, has actually moved _away_ from libgit2. I\nthink there were a number of reasons. Some match what Dscho said\nregarding Visual Studio. But I think a main one is just that libgit2\ndoesn't quite match regular git for feature parity or pace of\ndevelopment. So you're stuck shelling out to regular Git half the time\nanyway, and now you get to handle dependencies on _two_ systems. :)\n\n> Note: I assume getting the status through libgit2 doesn't create an\n> index.lock file.\n\nI don't know if that's the case or not.\n\n> > This patch implements the option 3.\n> \n> So, if I get that correctly \"git status --no-optional-locks\" is a way to get\n> the \"current\" status without updating the on disk index file?\n\nIt's actually \"git --no-optional-locks status\", since it's a git-wide\noption (it's just that \"status\" is the only one who respects it for\nnow).\n\n> > +`GIT_OPTIONAL_LOCKS`::\n> > +\tIf set to `0`, Git will avoid performing any operations which\n> > +\trequire taking a lock and which are not required to complete the\n> > +\trequested operation.\n> \n> The above sentence seems to be a little confusing. How about,\n> \n>    If set to `0`, Git will complete the requested operation without\n>    performing the optional sub-operations that require taking a lock.\n\nYeah, my original is pretty clunky. I ended up with:\n\n  If set to `0`, Git will complete any requested operation without\n  performing any optional sub-operations that require taking a lock.\n\nwhich swaps out \"the\" for \"any\".\n\nThanks.\n\n-Peff\n"},{"id":"329021","messageId":"20170927064423.twluii6jj57hyk5y@sigill.intra.peff.net","threadId":"46794","inReplyTo":"CAGZ79kYOeJvQmw-h3GwFpv2w7AKtNWUxf96tUjPKPY1dMuzagA@mail.gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:44:23Z","receivedAt":"2017-09-27T06:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 11:51:31AM -0700, Stefan Beller wrote:\n\n> > diff --git a/read-cache.c b/read-cache.c\n> > index 65f4fe8375..fc1ba122a3 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -1563,7 +1563,8 @@ static int read_index_extension(struct index_state *istate,\n> >\n> >  int hold_locked_index(struct lock_file *lk, int lock_flags)\n> >  {\n> > -       return hold_lock_file_for_update(lk, get_index_file(), lock_flags);\n> > +       return hold_lock_file_for_update_timeout(lk, get_index_file(),\n> > +                                                lock_flags, 500);\n> >  }\n> >\n> >  int read_index(struct index_state *istate)\n> >\n> > though I think there are a few sites which manually call\n> > hold_lock_file_for_update() on the index that would need similar\n> > treatment.\n> \n> uh, too bad. The patch above looks really promising, though. :)\n\nThere are probably only a handful of other callers, and they'd just need\nto swap out s/update/&_timeout/. So it really is pretty trivial.\n\n> > I suspect it would work OK in practice, unless your index is so big that\n> > 500ms isn't enough. The user may also see minor stalls when there's lock\n> > contention. I'm not sure how annoying that would be.\n> \n> There is only one way to find out. Though we don't want to volunteer\n> all users into this experiment, I'd presume.\n\nYes. One of the nice things about the optional-locks approach is that it\nonly affects callers who specify the option. And the general idea has\ngotten a year of testing in Visual Studio, which makes me feel good\nabout it.\n\n> Regarding larger indexes, I wonder if we can adapt the 500ms\n> to the repo size. At first I thought the abbreviation length could be\n> a good proxy to determine the maximum waiting time, but now I am\n> not so sure any more.\n\nI think madness that way lies.\n\n-Peff\n"},{"id":"329022","messageId":"20170927065430.cssyxxsxjhp7k4ye@sigill.intra.peff.net","threadId":"46794","inReplyTo":"20170921043214.pyhdsrpy4omy54rm@sigill.intra.peff.net","subject":"[PATCH v2] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:54:30Z","receivedAt":"2017-09-27T06:54:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is an update of my --no-optional-locks patch. The only difference\nis that I updated the documentation to be a bit less clunky (Thanks,\nKaartic).\n\nFor the other comments on v1, I decided not to make any changes:\n\n  - JSixt suggested marking this as a local-repo variable. I think we\n    really do want it to cross repo boundaries to handle submodules (and\n    the GfW version does the same).\n\n  - there was some discussion over the name. I didn't see other\n    suggestions, and I didn't come up with anything better.\n\n  - there was some discussion of alternatives, including reader/writer\n    locking schemes and lock-retry timeouts for other callers. This\n    approach has the benefit of simplicity and having been tested in the\n    real world by GfW/VS.  I wouldn't mind if somebody wanted to explore\n    lock retries, but I think we'd want to have this feature regardless.\n\nSo I'm happy with this version, though of course I'm open to more\ncomments.\n\n-- >8 --\nSubject: git: add --no-optional-locks option\n\nSome tools like IDEs or fancy editors may periodically run\ncommands like \"git status\" in the background to keep track\nof the state of the repository. Some of these commands may\nrefresh the index and write out the result in an\nopportunistic way: if they can get the index lock, then they\nupdate the on-disk index with any updates they find. And if\nnot, then their in-core refresh is lost and just has to be\nrecomputed by the next caller.\n\nBut taking the index lock may conflict with other operations\nin the repository. Especially ones that the user is doing\nthemselves, which _aren't_ opportunistic. In other words,\n\"git status\" knows how to back off when somebody else is\nholding the lock, but other commands don't know that status\nwould be happy to drop the lock if somebody else wanted it.\n\nThere are a couple possible solutions:\n\n  1. Have some kind of \"pseudo-lock\" that allows other\n     commands to tell status that they want the lock.\n\n     This is likely to be complicated and error-prone to\n     implement (and maybe even impossible with just\n     dotlocks to work from, as it requires some\n     inter-process communication).\n\n  2. Avoid background runs of commands like \"git status\"\n     that want to do opportunistic updates, preferring\n     instead plumbing like diff-files, etc.\n\n     This is awkward for a couple of reasons. One is that\n     \"status --porcelain\" reports a lot more about the\n     repository state than is available from individual\n     plumbing commands. And two is that we actually _do_\n     want to see the refreshed index. We just don't want to\n     take a lock or write out the result. Whereas commands\n     like diff-files expect us to refresh the index\n     separately and write it to disk so that they can depend\n     on the result. But that write is exactly what we're\n     trying to avoid.\n\n  3. Ask \"status\" not to lock or write the index.\n\n     This is easy to implement. The big downside is that any\n     work done in refreshing the index for such a call is\n     lost when the process exits. So a background process\n     may end up re-hashing a changed file multiple times\n     until the user runs a command that does an index\n     refresh themselves.\n\nThis patch implements the option 3. The idea (and the test)\nis largely stolen from a Git for Windows patch by Johannes\nSchindelin, 67e5ce7f63 (status: offer *not* to lock the\nindex and update it, 2016-08-12). The twist here is that\ninstead of making this an option to \"git status\", it becomes\na \"git\" option and matching environment variable.\n\nThe reason there is two-fold:\n\n  1. An environment variable is carried through to\n     sub-processes. And whether an invocation is a\n     background process or not should apply to the whole\n     process tree. So you could do \"git --no-optional-locks\n     foo\", and if \"foo\" is a script or alias that calls\n     \"status\", you'll still get the effect.\n\n  2. There may be other programs that want the same\n     treatment.\n\n     I've punted here on finding more callers to convert,\n     since \"status\" is the obvious one to call as a repeated\n     background job. But \"git diff\"'s opportunistic refresh\n     of the index may be a good candidate.\n\nThe test is taken from 67e5ce7f63, and it's worth repeating\nJohannes's explanation:\n\n  Note that the regression test added in this commit does\n  not *really* verify that no index.lock file was written;\n  that test is not possible in a portable way. Instead, we\n  verify that .git/index is rewritten *only* when `git\n  status` is run without `--no-optional-locks`.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git.txt | 12 ++++++++++++\n builtin/commit.c      |  5 ++++-\n cache.h               |  6 ++++++\n environment.c         |  5 +++++\n git.c                 |  4 ++++\n t/t7508-status.sh     | 10 ++++++++++\n 6 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 6e3a6767e5..f7e603bc82 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -159,6 +159,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string which ` git config\n \tAdd \"icase\" magic to all pathspec. This is equivalent to setting\n \tthe `GIT_ICASE_PATHSPECS` environment variable to `1`.\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+\n GIT COMMANDS\n ------------\n \n@@ -697,6 +701,14 @@ of clones and fetches.\n \twhich feed potentially-untrusted URLS to git commands.  See\n \tlinkgit:git-config[1] for more details.\n \n+`GIT_OPTIONAL_LOCKS`::\n+\tIf set to `0`, Git will complete any requested operation without\n+\tperforming any optional sub-operations that require taking a lock.\n+\tFor example, this will prevent `git status` from refreshing the\n+\tindex as a side effect. This is useful for processes running in\n+\tthe background which do not want to cause lock contention with\n+\tother operations on the repository.  Defaults to `1`.\n+\n Discussion[[Discussion]]\n ------------------------\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 58f9747c2f..fafd492029 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1387,7 +1387,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tread_cache_preload(&s.pathspec);\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL);\n \n-\tfd = hold_locked_index(&index_lock, 0);\n+\tif (use_optional_locks())\n+\t\tfd = hold_locked_index(&index_lock, 0);\n+\telse\n+\t\tfd = -1;\n \n \ts.is_initial = get_oid(s.reference, &oid) ? 1 : 0;\n \tif (!s.is_initial)\ndiff --git a/cache.h b/cache.h\nindex 49b083ee0a..75f8efc56f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -444,6 +444,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_NOGLOB_PATHSPECS_ENVIRONMENT \"GIT_NOGLOB_PATHSPECS\"\n #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n #define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n+#define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n \n /*\n  * This environment variable is expected to contain a boolean indicating\n@@ -783,6 +784,11 @@ extern int protect_ntfs;\n  */\n extern int ref_paranoia;\n \n+/*\n+ * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value).\n+ */\n+int use_optional_locks(void);\n+\n /*\n  * The character that begins a commented line in user-editable file\n  * that is subject to stripspace.\ndiff --git a/environment.c b/environment.c\nindex f1f934b6fd..8289c25b44 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -338,3 +338,8 @@ void reset_shared_repository(void)\n {\n \tneed_shared_repository_from_config = 1;\n }\n+\n+int use_optional_locks(void)\n+{\n+\treturn git_env_bool(GIT_OPTIONAL_LOCKS_ENVIRONMENT, 1);\n+}\ndiff --git a/git.c b/git.c\nindex f31dca6962..9e96dd4090 100644\n--- a/git.c\n+++ b/git.c\n@@ -182,6 +182,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ICASE_PATHSPECS_ENVIRONMENT, \"1\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-optional-locks\")) {\n+\t\t\tsetenv(GIT_OPTIONAL_LOCKS_ENVIRONMENT, \"0\", 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 43d19a9b22..93f162a4f7 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1670,4 +1670,14 @@ test_expect_success '\"Initial commit\" should not be noted in commit template' '\n \ttest_i18ngrep ! \"Initial commit\" output\n '\n \n+test_expect_success '--no-optional-locks prevents index update' '\n+\ttest-chmtime =1234567890 .git/index &&\n+\tgit --no-optional-locks status &&\n+\ttest-chmtime -v +0 .git/index >out &&\n+\tgrep ^1234567890 out &&\n+\tgit status &&\n+\ttest-chmtime -v +0 .git/index >out &&\n+\t! grep ^1234567890 out\n+'\n+\n test_done\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329037","messageId":"1506520205.6209.4.camel@gmail.com","threadId":"46794","inReplyTo":"20170927064026.5xa3ydaggmknvulw@sigill.intra.peff.net","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-27T13:50:05Z","receivedAt":"2017-09-27T13:50:15Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wed, 2017-09-27 at 02:40 -0400, Jeff King wrote:\n> On Sun, Sep 24, 2017 at 01:08:41PM +0530, Kaartic Sivaraam wrote:\n> \n> > \n> > So, if I get that correctly \"git status --no-optional-locks\" is a way to get\n> > the \"current\" status without updating the on disk index file?\n> \n> It's actually \"git --no-optional-locks status\", since it's a git-wide\n> option (it's just that \"status\" is the only one who respects it for\n> now).\n\nThanks for correcting the command. Now let me ask my (corrected)\nquestion again! So, if I get that correctly \"git --no-optional-\nlocks status\" is a way to get the \"current\" status *without updating*\nthe on disk index file?\n\n---\nKaartic\n"},{"id":"329040","messageId":"20170927162858.r6a3zx64bhqsoqzp@sigill.intra.peff.net","threadId":"46794","inReplyTo":"1506520205.6209.4.camel@gmail.com","subject":"Re: [PATCH] git: add --no-optional-locks option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T16:28:58Z","receivedAt":"2017-09-27T16:29:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 07:20:05PM +0530, Kaartic Sivaraam wrote:\n\n> On Wed, 2017-09-27 at 02:40 -0400, Jeff King wrote:\n> > On Sun, Sep 24, 2017 at 01:08:41PM +0530, Kaartic Sivaraam wrote:\n> > \n> > > \n> > > So, if I get that correctly \"git status --no-optional-locks\" is a way to get\n> > > the \"current\" status without updating the on disk index file?\n> > \n> > It's actually \"git --no-optional-locks status\", since it's a git-wide\n> > option (it's just that \"status\" is the only one who respects it for\n> > now).\n> \n> Thanks for correcting the command. Now let me ask my (corrected)\n> question again! So, if I get that correctly \"git --no-optional-\n> locks status\" is a way to get the \"current\" status *without updating*\n> the on disk index file?\n\nYes.\n\n-Peff\n"},{"id":"329109","messageId":"alpine.DEB.2.21.1.1709281813130.40514@virtualbox","threadId":"46794","inReplyTo":"20170927065430.cssyxxsxjhp7k4ye@sigill.intra.peff.net","subject":"Re: [PATCH v2] git: add --no-optional-locks option","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-28T16:15:35Z","receivedAt":"2017-09-28T16:15:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 27 Sep 2017, Jeff King wrote:\n\n> For the other comments on v1, I decided not to make any changes:\n> \n>   - JSixt suggested marking this as a local-repo variable. I think we\n>     really do want it to cross repo boundaries to handle submodules (and\n>     the GfW version does the same).\n\nIndeed. We had it originally as a local-repo only variable, and you\nprobably guessed that submodules made us rethink that approach.\n\n>   - there was some discussion of alternatives, including reader/writer\n>     locking schemes and lock-retry timeouts for other callers. This\n>     approach has the benefit of simplicity and having been tested in the\n>     real world by GfW/VS.  I wouldn't mind if somebody wanted to explore\n>     lock retries, but I think we'd want to have this feature regardless.\n\nWe also weighed other options, and took this route for simplicity. It's\npossible to explain during an uninterrupted elevator ride what it does.\nMakes for less bugs.\n\nAnd yes, we wanted to test it first before showing it to the Git mailing\nlist ;-)\n\nCiao,\nDscho\n"}]}