{"thread":{"id":"44605","subject":"[BUG] Index.lock error message regression in git 2.11.0","startedAt":"2016-12-03T01:44:40Z","lastAt":"2016-12-08T18:22:13Z","messageCount":19,"participants":["Robbie Iannucci","Junio C Hamano","Stefan Beller","Paul Tan","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306853","messageId":"CA+q_oBdHytoeSD-hmLx_N473M8XinjqckvE35Re3eNpQRWYjHQ@mail.gmail.com","threadId":"44605","inReplyTo":null,"subject":"[BUG] Index.lock error message regression in git 2.11.0","fromName":"Robbie Iannucci","fromEmail":"iannucci@google.com","sentAt":"2016-12-03T01:44:33Z","receivedAt":"2016-12-03T01:44:40Z","isPatch":false,"sender":{"key":"iannucci@google.com","avatar":null},"body":"Hello,\n\nI just upgraded to 2.11.0 from 2.10.2, and I noticed that some\ncommands no longer print an error message when the `index.lock` file\nexists (such as `git merge --ff-only`).\n\nIt appears this bug was introduced in\n55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify\ncheckout_fast_forward()). I determined this by running the attached\nbisect script (on OS X, but I don't think that matters; I've also seen\nthe error message missing on Linux):\n\n$ cd /path/to/git/src\n$ git bisect start v2.11.0-rc0 v2.10.2\n$ git bisect run /path/to/bisect.test.sh\n\n(my original version of the script also includes some other makefile\nparameters to get a modern version of gettext and openssl too, but\nthey're not relevant to this ML).\n\nI'm not certain that I have enough context to propose a meaningful\npatch though :/.\n\nCheers,\nRobbie\n"},{"id":"306854","messageId":"CA+q_oBc77Z7YW0yqPPB-R_HkAUtno1bDDTx3tR22ScRpC96ZcA@mail.gmail.com","threadId":"44605","inReplyTo":"CA+q_oBdHytoeSD-hmLx_N473M8XinjqckvE35Re3eNpQRWYjHQ@mail.gmail.com","subject":"Re: [BUG] Index.lock error message regression in git 2.11.0","fromName":"Robbie Iannucci","fromEmail":"iannucci@google.com","sentAt":"2016-12-03T01:52:15Z","receivedAt":"2016-12-03T01:56:23Z","isPatch":false,"sender":{"key":"iannucci@google.com","avatar":null},"body":"Apparently I'm not supposed to send attachments >_<. Here's the script\nin non-attachement form:\n\n--------\n\n#!/bin/bash\n\nmake -j 8 2>&1 > /dev/null\nif [[ $? != 0 ]]; then\n  # skip this version\n  exit 125\nfi\ngit=`realpath ./git`\n\nrm -rf .test_repo || true\nmkdir .test_repo\n\ncd .test_repo\n$git init\n\necho HELLO > a\n$git add a\n$git commit -m 'initial_commit'\n$git branch --track new_branch\n\n$git checkout master\necho HELLO >> a\n$git add a\n$git commit -m 'second_commit'\n\n$git checkout new_branch\n\ntouch .git/index.lock\nif $git merge --ff-only master 2>&1 | grep -F \"index.lock\" ; then\n  echo OK\n  exit 0\nfi\necho Message is missing\nexit 1\n\nOn Fri, Dec 2, 2016 at 5:44 PM, Robbie Iannucci <iannucci@google.com> wrote:\n> Hello,\n>\n> I just upgraded to 2.11.0 from 2.10.2, and I noticed that some\n> commands no longer print an error message when the `index.lock` file\n> exists (such as `git merge --ff-only`).\n>\n> It appears this bug was introduced in\n> 55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify\n> checkout_fast_forward()). I determined this by running the attached\n> bisect script (on OS X, but I don't think that matters; I've also seen\n> the error message missing on Linux):\n>\n> $ cd /path/to/git/src\n> $ git bisect start v2.11.0-rc0 v2.10.2\n> $ git bisect run /path/to/bisect.test.sh\n>\n> (my original version of the script also includes some other makefile\n> parameters to get a modern version of gettext and openssl too, but\n> they're not relevant to this ML).\n>\n> I'm not certain that I have enough context to propose a meaningful\n> patch though :/.\n>\n> Cheers,\n> Robbie\n"},{"id":"307072","messageId":"xmqqbmwozppx.fsf@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"CA+q_oBdHytoeSD-hmLx_N473M8XinjqckvE35Re3eNpQRWYjHQ@mail.gmail.com","subject":"Re: [BUG] Index.lock error message regression in git 2.11.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-06T21:56:26Z","receivedAt":"2016-12-06T21:56:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robbie Iannucci <iannucci@google.com> writes:\n\n> I just upgraded to 2.11.0 from 2.10.2, and I noticed that some\n> commands no longer print an error message when the `index.lock` file\n> exists (such as `git merge --ff-only`).\n>\n> It appears this bug was introduced in\n> 55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify\n> checkout_fast_forward()). I determined this by running the attached\n> bisect script (on OS X, but I don't think that matters; I've also seen\n> the error message missing on Linux):\n\nYes, I can see that this is not limited to OSX.  The command does\nexit with non-zero status, but that is not of much help for an\ninteractive user.\n\n    $ git checkout v2.11.0^0\n    $ >.git/index.lock\n    $ git merge --ff-only master; echo $?\n    Updating 454cb6bd52..8d7a455ed5\n    1\n    $ exit\n\nWe can see that it attempted to update, but because there is no\nindication that it failed, the user can easily be misled to think\nthat we are now at the tip of the master branch.\n\nWe used to give \"fatal: Unable to create ...\" followed by \"Another\ngit process seems to be running...\" advice, that would have helped\nthe user from the confusion.\n\nI do not think it is the right solution to call hold_locked_index()\nwith die_on_error=1 from the codepath changed by your 55f5704da6\n(\"sequencer: lib'ify checkout_fast_forward()\", 2016-09-09).  Perhaps\nthe caller of checkout_fast_forward() needs to learn how/why the\nfunction returned error and diagnose this case and a message?  Or\nperhaps turn that die_on_error parameter from boolean to tricolor\n(i.e. the caller does not want you to die, but the caller knows that\nyou have more information to give better error message than it does,\nso please show an error message instead of silently returning -1)?\n\nPerhaps the attached would fix it (not even compile tested, though)?\n\nI would prefer to make 0 to mean \"show error but return -1\", 1 to\nmean \"die on error\", and 2 to mean \"be silent and return -1 on\nerror\", though.  Asking to be silent should be the exception for\nthis error from usability and safety's point of view, and requiring\nsuch exceptional callers to pass LOCK_SILENT_ON_ERROR would be\neasier to \"git grep\" for them.\n\nDscho, what do you think?  \n\n\n lockfile.c | 11 +++++++++--\n lockfile.h |  5 +++++\n merge.c    |  2 +-\n 3 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9268cdf325..f190caddb0 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n \t\t\t\t      int flags, long timeout_ms)\n {\n \tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n-\tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_die(path, errno);\n+\tif (fd < 0) {\n+\t\tif (flags & LOCK_DIE_ON_ERROR)\n+\t\t\tunable_to_lock_die(path, errno);\n+\t\telse if (flags & LOCK_ERROR_ON_ERROR) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(path, errno, &buf);\n+\t\t\terror(\"%s\", buf.buf);\n+\t\t}\n+\t}\n \treturn fd;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex d26ad27b2b..6cba9c8142 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -132,6 +132,11 @@ struct lock_file {\n  * is already locked returns -1 to the caller.\n  */\n #define LOCK_DIE_ON_ERROR 1\n+/*\n+ * ... or the function can be told to show the usual error message\n+ * and still return -1 to the caller with this flag\n+ */\n+#define LOCK_ERROR_ON_ERROR 2\n \n /*\n  * Usually symbolic links in the destination path are resolved. This\ndiff --git a/merge.c b/merge.c\nindex 23866c9165..9e2e4f1a22 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (hold_locked_index(lock_file, 0) < 0)\n+\tif (hold_locked_index(lock_file, LOCK_ERROR_ON_ERROR) < 0)\n \t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n"},{"id":"307073","messageId":"xmqq4m2gzotn.fsf_-_@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"xmqqbmwozppx.fsf@gitster.mtv.corp.google.com","subject":"Re* [BUG] Index.lock error message regression in git 2.11.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-06T22:15:48Z","receivedAt":"2016-12-06T22:15:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Perhaps the attached would fix it (not even compile tested, though)?\n>\n> I would prefer to make 0 to mean \"show error but return -1\", 1 to\n> mean \"die on error\", and 2 to mean \"be silent and return -1 on\n> error\", though.  Asking to be silent should be the exception for\n> this error from usability and safety's point of view, and requiring\n> such exceptional callers to pass LOCK_SILENT_ON_ERROR would be\n> easier to \"git grep\" for them.\n\nSo here is the \"You have to ask explicitly, if you know that it is\nsafe to be silent\" version with a proper log message.\n\n-- >8 --\nSubject: [PATCH] lockfile: LOCK_SILENT_ON_ERROR\n\nRecent \"libify merge machinery\" stopped from passing die_on_error\nbit to hold_locked_index(), and lost an error message when there are\ncompeting update in progress with \"git merge --ff-only $commit\", for\nexample.  The command still exits with a non-zero status, but that\nis not of much help for an interactive user.  The last thing the\ncommand says is \"Updating $from..$to\".  We used to follow it with a\nbig error message that makes it clear that \"merge --ff-only\" did not\nsucceed.\n\nIntroduce a new bit \"LOCK_SILENT_ON_ERROR\" that can be passed by\ncallers that do want to silence the message (because they either\nmake it a non-error by doing something else, or they show their own\nerror message to explain the situation), and show the error message\nwe used to give for everybody else, including the caller that was\ntouched by the libification in question.\n\nI would not be surprised if some existing calls to hold_lock*()\nfunctions that pass die_on_error=0 need to be updated to pass\nLOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look\nlike a regression, but we are better off starting louder and squelch\nthe ones that we find safe to make silent than the other way around.\n\nReported-by: Robbie Iannucci <iannucci@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n lockfile.c | 11 +++++++++--\n lockfile.h |  8 +++++++-\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9268cdf325..f7e8104449 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n \t\t\t\t      int flags, long timeout_ms)\n {\n \tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n-\tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_die(path, errno);\n+\tif (fd < 0) {\n+\t\tif (flags & LOCK_DIE_ON_ERROR)\n+\t\t\tunable_to_lock_die(path, errno);\n+\t\telse if (!(flags & LOCK_SILENT_ON_ERROR)) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(path, errno, &buf);\n+\t\t\terror(\"%s\", buf.buf);\n+\t\t}\n+\t}\n \treturn fd;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex d26ad27b2b..98b4862254 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -129,9 +129,15 @@ struct lock_file {\n /*\n  * If a lock is already taken for the file, `die()` with an error\n  * message. If this flag is not specified, trying to lock a file that\n- * is already locked returns -1 to the caller.\n+ * is already locked gives the same error message and returns -1 to\n+ * the caller.\n  */\n #define LOCK_DIE_ON_ERROR 1\n+/*\n+ * ... or the function can be told to be totally silent and return\n+ * -1 to the caller upon error with this flag\n+ */\n+#define LOCK_SILENT_ON_ERROR 2\n \n /*\n  * Usually symbolic links in the destination path are resolved. This\n-- \n2.11.0-270-g0b6beed61f\n\n"},{"id":"307142","messageId":"xmqqd1h3y506.fsf@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"xmqq4m2gzotn.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: Re* [BUG] Index.lock error message regression in git 2.11.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T18:21:29Z","receivedAt":"2016-12-07T18:21:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...\n> I would not be surprised if some existing calls to hold_lock*()\n> functions that pass die_on_error=0 need to be updated to pass\n> LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look\n> like a regression, but we are better off starting louder and squelch\n> the ones that we find safe to make silent than the other way around.\n\nI actually take this part back, for two reasons.\n\n * Before the recent js/sequencer-wo-die topic that made this\n   failure mode of 'git merge' more dangerous by accident, there\n   were already callers that passed die_on_error=0 to hold_lock*\n   family of functions, and we can trust these callers.  They either\n   have been silent upon lock failure sensibly (e.g. a caller that\n   tries to acquire the lock to opportunistically update the index\n   can safely choose not to do anything and be silent) or they have\n   had their own way of reporting the errors to the users.  The \"you\n   need to ask to be totally quiet\" approach in my rerolled patch\n   (the one I am responding to) will introduce new regressions to\n   these codepaths.\n\n * Among the ones that stopped passing die_on_error=1 when the topic\n   was merged, there are\n   ones that give sensible error messages.  Again, they do not need\n   extra message with the \"you need to ask to be totally quiet\"\n   approach [*1*].\n\nWe need to instead go through the latter, i.e. the ones that appear\nin \"git show --first-parent 2a4062a4a8\", with fine-toothed comb to\nsee which 0 made an error totally silent (like the one Robbie\nspotted in merge.c) and fix them to ask hold_lock*() functions not\nto die but still report an error.\n\n"},{"id":"307145","messageId":"20161207194105.25780-1-gitster@pobox.com","threadId":"44605","inReplyTo":"xmqqd1h3y506.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] Do not be totally silent upon lock error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T19:41:02Z","receivedAt":"2016-12-07T19:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robbie Iannucci reported that \"git merge\" that fast-forwards fails\nsilently when a competing or stale index.lock is present in recent\nGit:\n\n    $ git merge --ff-only master; echo $?\n    Updating 454cb6bd52..8d7a455ed5\n    1\n    $ exit\n\nDid the update happen?  We used to give \"fatal: Unable to create\n...\" followed by \"Another git process seems to be running...\"\nadvice, and that would have helped the user from the confusion.\n\nThis was because there were only two choices available to the\ncallers of the lockfile API--they can either ask it to die with\nmessage when the lock cannot be acquired, or ask it to silently\nreturn -1 to signal an error. The recent \"libify sequencer\" topic\nturned many existing \"please die\" calls to \"just silently return\n-1\", and while it added new \"unable to lock\" error messages to most\nof them, one spot didn't get any and now is allowed to just die\nsilently.\n\nThis series should fix it:\n\n - 1/3 is something I noticed while reading the existing callers of\n   the lockfile API, and is not a new issue with \"libify sequencer\"\n   topic.\n\n - 2/3 gives a new third option to callers of the lockfile API; they\n   can now say \"please emit the usual error message upon failing to\n   acquire the lock, but give control back to me\".\n\n - 3/3 uses the new option to resurrect the message.\n\nJunio C Hamano (3):\n  wt-status: implement opportunisitc index update correctly\n  hold_locked_index(): align error handling with hold_lockfile_for_update()\n  lockfile: LOCK_REPORT_ON_ERROR\n\n apply.c                          |  2 +-\n builtin/add.c                    |  2 +-\n builtin/am.c                     |  6 +++---\n builtin/checkout-index.c         |  2 +-\n builtin/checkout.c               |  4 ++--\n builtin/clone.c                  |  2 +-\n builtin/commit.c                 |  8 ++++----\n builtin/merge.c                  |  6 +++---\n builtin/mv.c                     |  2 +-\n builtin/read-tree.c              |  2 +-\n builtin/reset.c                  |  2 +-\n builtin/rm.c                     |  2 +-\n builtin/update-index.c           |  1 +\n lockfile.c                       | 12 ++++++++++--\n lockfile.h                       |  8 +++++++-\n merge-recursive.c                |  2 +-\n merge.c                          |  2 +-\n read-cache.c                     |  7 ++-----\n rerere.c                         |  2 +-\n sequencer.c                      |  2 +-\n t/helper/test-scrap-cache-tree.c |  2 +-\n wt-status.c                      |  7 ++++---\n 22 files changed, 49 insertions(+), 36 deletions(-)\n\n-- \n2.11.0-274-g0631465056\n\n"},{"id":"307146","messageId":"20161207194105.25780-2-gitster@pobox.com","threadId":"44605","inReplyTo":"20161207194105.25780-1-gitster@pobox.com","subject":"[PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T19:41:03Z","receivedAt":"2016-12-07T19:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The require_clean_work_tree() function calls hold_locked_index()\nwith die_on_error=0 to signal that it is OK if it fails to obtain\nthe lock, but unconditionally calls update_index_if_able(), which\nwill try to write into fd=-1.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n wt-status.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex a2e9d332d8..a715e71906 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)\n int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)\n {\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n-\tint err = 0;\n+\tint err = 0, fd;\n \n-\thold_locked_index(lock_file, 0);\n+\tfd = hold_locked_index(lock_file, 0);\n \trefresh_cache(REFRESH_QUIET);\n-\tupdate_index_if_able(&the_index, lock_file);\n+\tif (0 <= fd)\n+\t\tupdate_index_if_able(&the_index, lock_file);\n \trollback_lock_file(lock_file);\n \n \tif (has_unstaged_changes(ignore_submodules)) {\n-- \n2.11.0-274-g0631465056\n\n"},{"id":"307147","messageId":"20161207194105.25780-3-gitster@pobox.com","threadId":"44605","inReplyTo":"20161207194105.25780-1-gitster@pobox.com","subject":"[PATCH 2/3] hold_locked_index(): align error handling with hold_lockfile_for_update()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T19:41:04Z","receivedAt":"2016-12-07T19:41:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Callers of the hold_locked_index() function pass 0 when they want to\nprepare to write a new version of the index file without wishing to\ndie or emit an error message when the request fails (e.g. somebody\nelse already held the lock), and pass 1 when they want the call to\ndie upon failure.\n\nThis option is called LOCK_DIE_ON_ERROR by the underlying lockfile\nAPI, and the hold_locked_index() function translates the paramter to\nLOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update().\n\nReplace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop\ntranslating.  Callers other than the ones that are replaced with\nthis change pass '0' to the function; no behaviour change is\nintended with this patch.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nAmong the callers of hold_locked_index() that passes 0:\n\n - diff.c::refresh_index_quietly() at the end of \"git diff\" is an\n   opportunistic update; it leaks the lockfile structure but it is\n   just before the program exits and nobody should care.\n\n - builtin/describe.c::cmd_describe(),\n   builtin/commit.c::cmd_status(),\n   sequencer.c::read_and_refresh_cache() are all opportunistic\n   updates and they are OK.\n\n - builtin/update-index.c::cmd_update_index() takes a lock upfront\n   but we may end up not needing to update the index (i.e. the\n   entries may be fully up-to-date), in which case we do not need to\n   issue an error upon failure to acquire the lock.  We do diagnose\n   and die if we indeed need to update, so it is OK.\n\n - wt-status.c::require_clean_work_tree() IS BUGGY.  It asks\n   silence, does not check the returned value.  Compare with\n   callsites like cmd_describe() and cmd_status() to notice that it\n   is wrong to call update_index_if_able() unconditionally.\n---\n apply.c                          | 2 +-\n builtin/add.c                    | 2 +-\n builtin/am.c                     | 6 +++---\n builtin/checkout-index.c         | 2 +-\n builtin/checkout.c               | 4 ++--\n builtin/clone.c                  | 2 +-\n builtin/commit.c                 | 8 ++++----\n builtin/merge.c                  | 6 +++---\n builtin/mv.c                     | 2 +-\n builtin/read-tree.c              | 2 +-\n builtin/reset.c                  | 2 +-\n builtin/rm.c                     | 2 +-\n builtin/update-index.c           | 1 +\n merge-recursive.c                | 2 +-\n read-cache.c                     | 7 ++-----\n rerere.c                         | 2 +-\n sequencer.c                      | 2 +-\n t/helper/test-scrap-cache-tree.c | 2 +-\n 18 files changed, 27 insertions(+), 29 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 705cf562f0..2ed808d429 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4688,7 +4688,7 @@ static int apply_patch(struct apply_state *state,\n \t\t\t\t\t\t\t\t state->index_file,\n \t\t\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \t\telse\n-\t\t\tstate->newfd = hold_locked_index(state->lock_file, 1);\n+\t\t\tstate->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \tif (state->check_index && read_apply_cache(state) < 0) {\ndiff --git a/builtin/add.c b/builtin/add.c\nindex e8fb80b36e..9f53f020d0 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -361,7 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tadd_new_files = !take_worktree_changes && !refresh_only;\n \trequire_pathspec = !(take_worktree_changes || (0 < addremove_explicit));\n \n-\thold_locked_index(&lock_file, 1);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tflags = ((verbose ? ADD_CACHE_VERBOSE : 0) |\n \t\t (show_only ? ADD_CACHE_PRETEND : 0) |\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6981f42ce9..bb5da422fc 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1119,7 +1119,7 @@ static void refresh_and_write_cache(void)\n {\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n \tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write index file\"));\n@@ -1976,7 +1976,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n \t\treturn -1;\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \n \trefresh_cache(REFRESH_QUIET);\n \n@@ -2016,7 +2016,7 @@ static int merge_tree(struct tree *tree)\n \t\treturn -1;\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 30a49d9f42..07631d0c9c 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -205,7 +205,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tif (index_opt && !state.base_dir_len && !to_tempfile) {\n \t\tstate.refresh_cache = 1;\n \t\tstate.istate = &the_index;\n-\t\tnewfd = hold_locked_index(&lock_file, 1);\n+\t\tnewfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \t}\n \n \t/* Check out named files first */\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 512492aad9..bfe685c198 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -274,7 +274,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(&opts->pathspec) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \n@@ -467,7 +467,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tint ret;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n \t\treturn error(_(\"index file corrupt\"));\n \ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 6c76a6ed66..892bdbfe3f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -711,7 +711,7 @@ static int checkout(int submodule_progress)\n \tsetup_work_tree();\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n+\thold_locked_index(lock_file, LOCK_DIE_ON_ERROR);\n \n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8976c3d29b..b910e76017 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \n \tif (interactive) {\n \t\tchar *old_index_env = NULL;\n-\t\thold_locked_index(&index_lock, 1);\n+\t\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \n \t\trefresh_cache_or_die(refresh_flags);\n \n@@ -396,7 +396,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n-\t\thold_locked_index(&index_lock, 1);\n+\t\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n@@ -416,7 +416,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t * We still need to refresh the index here.\n \t */\n \tif (!only && !pathspec.nr) {\n-\t\thold_locked_index(&index_lock, 1);\n+\t\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tif (active_cache_changed\n \t\t    || !cache_tree_fully_valid(active_cache_tree))\n@@ -468,7 +468,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (read_cache() < 0)\n \t\tdie(_(\"cannot read the index\"));\n \n-\thold_locked_index(&index_lock, 1);\n+\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \tupdate_main_cache_tree(WRITE_TREE_SILENT);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b65eeaa87d..0070bf2556 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -634,7 +634,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n {\n \tstatic struct lock_file lock;\n \n-\thold_locked_index(&lock, 1);\n+\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n \t    write_locked_index(&the_index, &lock, COMMIT_LOCK))\n@@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tfor (j = common; j; j = j->next)\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n-\t\thold_locked_index(&lock, 1);\n+\t\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (clean < 0)\n@@ -781,7 +781,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n \tstruct commit_list *parents, **pptr = &parents;\n \tstatic struct lock_file lock;\n \n-\thold_locked_index(&lock, 1);\n+\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n \t    write_locked_index(&the_index, &lock, COMMIT_LOCK))\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 2f43877bc9..43adf92ba6 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -126,7 +126,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tif (--argc < 1)\n \t\tusage_with_options(builtin_mv_usage, builtin_mv_options);\n \n-\thold_locked_index(&lock_file, 1);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 9bd1fd755e..fa6edb35b2 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -150,7 +150,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n \t\t\t     read_tree_usage, 0);\n \n-\thold_locked_index(&lock_file, 1);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tprefix_set = opts.prefix ? 1 : 0;\n \tif (1 < opts.merge + opts.reset + prefix_set)\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex c04ac076dc..8ab915bfcb 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -354,7 +354,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tif (reset_type != SOFT) {\n \t\tstruct lock_file *lock = xcalloc(1, sizeof(*lock));\n-\t\thold_locked_index(lock, 1);\n+\t\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n \t\tif (reset_type == MIXED) {\n \t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 3f3e24eb36..7f15a3d7f8 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -292,7 +292,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (!index_only)\n \t\tsetup_work_tree();\n \n-\thold_locked_index(&lock_file, 1);\n+\thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex f3f07e7f1c..d530e89368 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1012,6 +1012,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n+\t/* we will diagnose later if it turns out that we need to update it */\n \tnewfd = hold_locked_index(lock_file, 0);\n \tif (newfd < 0)\n \t\tlock_error = errno;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9041c2f149..8442068716 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2124,7 +2124,7 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t}\n \t}\n \n-\thold_locked_index(lock, 1);\n+\thold_locked_index(lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n \tif (clean < 0)\ndiff --git a/read-cache.c b/read-cache.c\nindex db5d910642..f92a912dcb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1425,12 +1425,9 @@ static int read_index_extension(struct index_state *istate,\n \treturn 0;\n }\n \n-int hold_locked_index(struct lock_file *lk, int die_on_error)\n+int hold_locked_index(struct lock_file *lk, int lock_flags)\n {\n-\treturn hold_lock_file_for_update(lk, get_index_file(),\n-\t\t\t\t\t die_on_error\n-\t\t\t\t\t ? LOCK_DIE_ON_ERROR\n-\t\t\t\t\t : 0);\n+\treturn hold_lock_file_for_update(lk, get_index_file(), lock_flags);\n }\n \n int read_index(struct index_state *istate)\ndiff --git a/rerere.c b/rerere.c\nindex 5d083ca572..3bd55caf3b 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -708,7 +708,7 @@ static void update_paths(struct string_list *update)\n {\n \tint i;\n \n-\thold_locked_index(&index_lock, 1);\n+\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \n \tfor (i = 0; i < update->nr; i++) {\n \t\tstruct string_list_item *item = &update->items[i];\ndiff --git a/sequencer.c b/sequencer.c\nindex 30b10ba143..7fc1e2a5df 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -370,7 +370,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tchar **xopt;\n \tstatic struct lock_file index_lock;\n \n-\thold_locked_index(&index_lock, 1);\n+\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \n \tread_cache();\n \ndiff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c\nindex 27fe0405b8..d2a63bea43 100644\n--- a/t/helper/test-scrap-cache-tree.c\n+++ b/t/helper/test-scrap-cache-tree.c\n@@ -8,7 +8,7 @@ static struct lock_file index_lock;\n int cmd_main(int ac, const char **av)\n {\n \tsetup_git_directory();\n-\thold_locked_index(&index_lock, 1);\n+\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \tif (read_cache() < 0)\n \t\tdie(\"unable to read index file\");\n \tactive_cache_tree = NULL;\n-- \n2.11.0-274-g0631465056\n\n"},{"id":"307148","messageId":"20161207194105.25780-4-gitster@pobox.com","threadId":"44605","inReplyTo":"20161207194105.25780-1-gitster@pobox.com","subject":"[PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T19:41:05Z","receivedAt":"2016-12-07T19:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"libify sequencer\" topic stopped passing the die_on_error option\nto hold_locked_index(), and this lost an error message from \"git\nmerge --ff-only $commit\" when there are competing updates in\nprogress.\n\nThe command still exits with a non-zero status, but that is not of\nmuch help for an interactive user.  The last thing the command says\nis \"Updating $from..$to\".  We used to follow it with a big error\nmessage that makes it clear that \"merge --ff-only\" did not succeed.\n\nWhat is sad is that we should have noticed this regression while\nreviewing the change.  It was clear that the update to the\ncheckout_fast_forward() function made a failing hold_locked_index()\nsilent, but the only caller of the checkout_fast_forward() function\nhad this comment:\n\n\t    if (checkout_fast_forward(from, to, 1))\n    -               exit(128); /* the callee should have complained already */\n    +               return -1; /* the callee should have complained already */\n\nwhich clearly contradicted the assumption X-<.\n\nAdd a new option LOCK_REPORT_ON_ERROR that can be passed instead of\nLOCK_DIE_ON_ERROR to the hold_lock*() family of functions and teach\ncheckout_fast_forward() to use it to fix this regression.\n\nAfter going thourgh all calls to hold_lock*() family of functions\nthat used to pass LOCK_DIE_ON_ERROR but were modified to pass 0 in\nthe \"libify sequencer\" topic \"git show --first-parent 2a4062a4a8\",\nit appears that this is the only one that has become silent.  Many\nothers used to give detailed report that talked about \"there may be\ncompeting Git process running\" but with the series merged they now\nonly give a single liner \"Unable to lock ...\", some of which may\nhave to be tweaked further, but at least they say something, unlike\nthe one this patch fixes.\n\nReported-by: Robbie Iannucci <iannucci@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n lockfile.c | 12 ++++++++++--\n lockfile.h |  8 +++++++-\n merge.c    |  2 +-\n 3 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9268cdf325..aa69210d8b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -174,8 +174,16 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n \t\t\t\t      int flags, long timeout_ms)\n {\n \tint fd = lock_file_timeout(lk, path, flags, timeout_ms);\n-\tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_die(path, errno);\n+\tif (fd < 0) {\n+\t\tif (flags & LOCK_DIE_ON_ERROR)\n+\t\t\tunable_to_lock_die(path, errno);\n+\t\tif (flags & LOCK_REPORT_ON_ERROR) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(path, errno, &buf);\n+\t\t\terror(\"%s\", buf.buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n+\t}\n \treturn fd;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex d26ad27b2b..16775a7d79 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -129,11 +129,17 @@ struct lock_file {\n /*\n  * If a lock is already taken for the file, `die()` with an error\n  * message. If this flag is not specified, trying to lock a file that\n- * is already locked returns -1 to the caller.\n+ * is already locked silently returns -1 to the caller, or ...\n  */\n #define LOCK_DIE_ON_ERROR 1\n \n /*\n+ * ... this flag can be passed instead to return -1 and give the usual\n+ * error message upon an error.\n+ */\n+#define LOCK_REPORT_ON_ERROR 2\n+\n+/*\n  * Usually symbolic links in the destination path are resolved. This\n  * means that (1) the lockfile is created by adding \".lock\" to the\n  * resolved path, and (2) upon commit, the resolved path is\ndiff --git a/merge.c b/merge.c\nindex 23866c9165..04ee5fc911 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (hold_locked_index(lock_file, 0) < 0)\n+\tif (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n \n \tmemset(&trees, 0, sizeof(trees));\n-- \n2.11.0-274-g0631465056\n\n"},{"id":"307158","messageId":"CAPc5daX2CZ0=UBMuz70KwFPBDTUgAdi8WoVUJ7gNTq+QEXKxbg@mail.gmail.com","threadId":"44605","inReplyTo":"CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T20:53:14Z","receivedAt":"2016-12-07T20:53:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:\n>\n> So I would expect that we'd rather fix the update_index_if_able instead by\n> checking for the lockfile to be in the correct state?\n\nI actually don't expect that, after looking at other call sites of\nthat function.\n"},{"id":"307159","messageId":"CAGZ79kbZBzF20=cfF85_fSuFdpYZuNyhDNCSKCb9HBpLOSV0hA@mail.gmail.com","threadId":"44605","inReplyTo":"CAPc5daX2CZ0=UBMuz70KwFPBDTUgAdi8WoVUJ7gNTq+QEXKxbg@mail.gmail.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T20:56:55Z","receivedAt":"2016-12-07T20:57:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:\n>>\n>> So I would expect that we'd rather fix the update_index_if_able instead by\n>> checking for the lockfile to be in the correct state?\n>\n> I actually don't expect that, after looking at other call sites of\n> that function.\n\nYes I checked the other callers as well right now and you seem to be correct.\nMy initial response was based on the name of the function,\nspecifically the _if_able\npart as that hinted to me that I can call the function with no\nprecondition and the\n_if_able will figure out when to do the actual update_index.\n\nThe first part of the condition of the function\n    (istate->cache_changed || has_racy_timestamp(istate)\nreads rather as a _if_needed instead of an _if_able to me.\n"},{"id":"307160","messageId":"CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com","threadId":"44605","inReplyTo":"20161207194105.25780-2-gitster@pobox.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T20:48:34Z","receivedAt":"2016-12-07T20:59:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> The require_clean_work_tree() function calls hold_locked_index()\n> with die_on_error=0 to signal that it is OK if it fails to obtain\n> the lock, but unconditionally calls update_index_if_able(), which\n> will try to write into fd=-1.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n\nSo my first question I had to answer was if we do the right thing here,\ni.e. if we could just fail instead. But we want to continue and just\nnot write back the index, which is fine.\n\nSo we do not have to guard refresh_cache, but just call\nupdate_index_if_able conditionally.\n\nHowever I think the promise of that function is\nto take care of the fd == -1?\n\n    /*\n    * Opportunistically update the index but do not complain if we can't\n    */\n    void update_index_if_able(struct index_state *istate, struct\nlock_file *lockfile)\n    {\n        if ((istate->cache_changed || has_racy_timestamp(istate)) &&\n            verify_index(istate) &&\n            write_locked_index(istate, lockfile, COMMIT_LOCK))\n                rollback_lock_file(lockfile);\n    }\n\nSo I would expect that we'd rather fix the update_index_if_able instead by\nchecking for the lockfile to be in the correct state?\n\n\n>  wt-status.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index a2e9d332d8..a715e71906 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)\n>  int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)\n>  {\n>         struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));\n> -       int err = 0;\n> +       int err = 0, fd;\n>\n> -       hold_locked_index(lock_file, 0);\n> +       fd = hold_locked_index(lock_file, 0);\n>         refresh_cache(REFRESH_QUIET);\n> -       update_index_if_able(&the_index, lock_file);\n> +       if (0 <= fd)\n> +               update_index_if_able(&the_index, lock_file);\n>         rollback_lock_file(lock_file);\n>\n>         if (has_unstaged_changes(ignore_submodules)) {\n> --\n> 2.11.0-274-g0631465056\n>\n"},{"id":"307167","messageId":"xmqqmvg7wips.fsf@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T21:08:15Z","receivedAt":"2016-12-07T21:08:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So my first question I had to answer was if we do the right thing here,\n> i.e. if we could just fail instead. But we want to continue and just\n> not write back the index, which is fine.\n>\n> So we do not have to guard refresh_cache, but just call\n> update_index_if_able conditionally.\n\nAn explanation with stepping back a little bit may help.\n\nYou may be asked to visit a repository of a friend, to which you do\nnot have write access to but you can still read.  You may want to do\n\"diff\", \"status\" or \"describe\" there.\n\nIn order to avoid getting fooled into thinking some paths are dirty\nonly because the cached stat information does not match, these need\nto refresh the in-core index before doing their \"comparison\" to\nreport which paths are different (in \"diff\"), what are the modified\nbut not staged paths (in \"status\"), and if there is a need to add\nthe \"-dirty\" suffix (in \"describe\").\n\nSince we are doing the expensive \"bunch of lstat()\" anyway, if we\ncould write it back to the index, it would help future operations in\nthe same repository--that is the reasoning behind the opportunistic\nupdates.  It is perfectly OK if we do not have write access to the\nrepository and cannot write update the index.\n\n"},{"id":"307176","messageId":"CAGZ79ka_Sh7jppCVkAhFWkXNffEg_SxUGN9VULseJBpQkDSyww@mail.gmail.com","threadId":"44605","inReplyTo":"xmqqmvg7wips.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T22:06:08Z","receivedAt":"2016-12-07T22:06:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 1:08 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> So my first question I had to answer was if we do the right thing here,\n>> i.e. if we could just fail instead. But we want to continue and just\n>> not write back the index, which is fine.\n>>\n>> So we do not have to guard refresh_cache, but just call\n>> update_index_if_able conditionally.\n>\n> An explanation with stepping back a little bit may help.\n>\n> You may be asked to visit a repository of a friend, to which you do\n> not have write access to but you can still read.  You may want to do\n> \"diff\", \"status\" or \"describe\" there.\n>\n> In order to avoid getting fooled into thinking some paths are dirty\n> only because the cached stat information does not match, these need\n> to refresh the in-core index before doing their \"comparison\" to\n> report which paths are different (in \"diff\"), what are the modified\n> but not staged paths (in \"status\"), and if there is a need to add\n> the \"-dirty\" suffix (in \"describe\").\n>\n> Since we are doing the expensive \"bunch of lstat()\" anyway, if we\n> could write it back to the index, it would help future operations in\n> the same repository--that is the reasoning behind the opportunistic\n> updates.  It is perfectly OK if we do not have write access to the\n> repository and cannot write update the index.\n>\n\nThanks for that explanation!\nSo I agree that we should not call it _if_needed, but the _if_able\npart is still confusing, as we check for different parts here.\n\nThe case of not being able to write (read only) sounds similar to\nthe case of not being able to write (a concurrent lock),\nI'll think about it more.\n\nThanks,\nStefan\n"},{"id":"307228","messageId":"CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com","threadId":"44605","inReplyTo":"CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-12-08T10:18:39Z","receivedAt":"2016-12-08T10:18:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi Junio,\n\nOn Thu, Dec 8, 2016 at 4:48 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> The require_clean_work_tree() function calls hold_locked_index()\n>> with die_on_error=0 to signal that it is OK if it fails to obtain\n>> the lock, but unconditionally calls update_index_if_able(), which\n>> will try to write into fd=-1.\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n\nAh, sorry about this. I was indeed misled by the function naming and\nits comment (\"do not complain if we can't\"). Should have looked more\nclosely at the other call sites.\n\n> However I think the promise of that function is\n> to take care of the fd == -1?\n\nHmm, to add on, looking at the three other call sites of this\nfunction, two of them (builtin/commit.c and builtin/describe.c)\nbasically do:\n\n    if (0 <= fd)\n        update_index_if_able(...)\n\nwith that 0 <= fd conditional. With this patch it becomes three out of\nfour. Perhaps the repeated use of this conditional is a sign that the\n0 <= fd check could be built into update_index_if_able()? I think\nthere is precedent for building in these kind of checks --\nrollback_lock_file() also does not fail if the lock file was not\nsuccessfully opened.\n\nThat said, the number of call sites is quite low so it's probably not\nworth doing this.\n\nThanks,\nPaul\n"},{"id":"307238","messageId":"alpine.DEB.2.20.1612081252490.23160@virtualbox","threadId":"44605","inReplyTo":"20161207194105.25780-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-08T11:53:39Z","receivedAt":"2016-12-08T11:55:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Dec 2016, Junio C Hamano wrote:\n\n> The \"libify sequencer\" topic stopped passing the die_on_error option\n> to hold_locked_index(), and this lost an error message from \"git\n> merge --ff-only $commit\" when there are competing updates in\n> progress.\n\nSorry for the breakage.\n\nWhen libifying the code, I tried to be careful to retain the error\nmessages when not dying, and mistakenly assumed that hold_locked_index()\nwould do the same.\n\nCiao,\nDscho\n"},{"id":"307269","messageId":"xmqqmvg6ti3x.fsf@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com","subject":"Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-08T18:01:54Z","receivedAt":"2016-12-08T18:02:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> Hmm, to add on, looking at the three other call sites of this\n> function, two of them (builtin/commit.c and builtin/describe.c)\n> basically do:\n>\n>     if (0 <= fd)\n>         update_index_if_able(...)\n>\n> with that 0 <= fd conditional. With this patch it becomes three out of\n> four.\n\nThe other one is diff.c::refresh_index_quietly() that you are not\ncounting, I think, but if you look at it again, it also is not\ncalled after hold_locked_index() fails to acquire the lock, so with\nthis fix everybody refrains from calling it when it does not hold\nthe lock.\n\n> Perhaps the repeated use of this conditional is a sign that the\n> 0 <= fd check could be built into update_index_if_able()?\n\nThat condition is \"do we have the lock?  Otherwise we are not even\nallowed to update it\", so in that sense it may make sense.\n\n> I think there is precedent for building in these kind of checks --\n> rollback_lock_file() also does not fail if the lock file was not\n> successfully opened.\n>\n> That said, the number of call sites is quite low so it's probably not\n> worth doing this.\n\nYeah, I can go either way.  At least with the change things are\nconsistent.\n"},{"id":"307273","messageId":"CA+q_oBfTs7_-sNujG0wgUu7DDSyukx6DbYcNRTnKxZbc7LWNpQ@mail.gmail.com","threadId":"44605","inReplyTo":"alpine.DEB.2.20.1612081252490.23160@virtualbox","subject":"Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR","fromName":"Robbie Iannucci","fromEmail":"iannucci@google.com","sentAt":"2016-12-08T18:10:14Z","receivedAt":"2016-12-08T18:10:20Z","isPatch":true,"sender":{"key":"iannucci@google.com","avatar":null},"body":"Thanks all for taking a look at this, I didn't expect such a quick response :).\n\nNo hard feelings re: breakage; I know how gnarly these sorts of\nrefactors can be (and more libification is always better :)).\n\nRobbie\n\nOn Thu, Dec 8, 2016 at 3:53 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi Junio,\n>\n> On Wed, 7 Dec 2016, Junio C Hamano wrote:\n>\n>> The \"libify sequencer\" topic stopped passing the die_on_error option\n>> to hold_locked_index(), and this lost an error message from \"git\n>> merge --ff-only $commit\" when there are competing updates in\n>> progress.\n>\n> Sorry for the breakage.\n>\n> When libifying the code, I tried to be careful to retain the error\n> messages when not dying, and mistakenly assumed that hold_locked_index()\n> would do the same.\n>\n> Ciao,\n> Dscho\n"},{"id":"307276","messageId":"xmqqinquth69.fsf@gitster.mtv.corp.google.com","threadId":"44605","inReplyTo":"alpine.DEB.2.20.1612081252490.23160@virtualbox","subject":"Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-08T18:22:06Z","receivedAt":"2016-12-08T18:22:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Sorry for the breakage.\n\nApologies from me, too.  Once a topic is merged, the credit still\nremains with the contributor, but the blame is shared by the project\nas a whole, with those who missed breakages during their reviews,\nand those who didn't review or test and let breakages pass.\n\n> When libifying the code, I tried to be careful to retain the error\n> messages when not dying,...\n\nQuite honestly, I do not think either of us cared about preserving\nthe exact error message the end-user was getting from each failure\nsites that the series changed a call with die-on-error=1 to a call\nwith die-on-error=0 that is followed by a negative return while\nreviewing this series.  As I wrote in the proposed log message for\n3/3, this one was noticed as a end-user breaking change because it\nwas the only one that has become totally silent.  For example, this\nbit from sequencer.c::write_message() we can see in the output from\n\"git show --first-parent 2a4062a4a8\" does not preserve the message\nat all:\n\n    diff --git a/sequencer.c b/sequencer.c\n    index 3804fa931d..eec8a60d6b 100644\n    --- a/sequencer.c\n    +++ b/sequencer.c\n    @@ -180,17 +180,20 @@\n    ...\n    -static void write_message(struct strbuf *msgbuf, const char *filename)\n    +static int write_message(struct strbuf *msgbuf, const char *filename)\n     {\n            static struct lock_file msg_file;\n\n    -\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n    -\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n    +\tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n    +\tif (msg_fd < 0)\n    +\t\treturn error_errno(_(\"Could not lock '%s'\"), filename);\n\nAnd I do not think it is necessarily bad that the error message\nchanged with this conversion.  In other words, I do not think it\nshould have been the goal to preserve the exact error message.\nhold_lock*() can afford to give a detailed message that strongly\nsounds as being the final decision when called with die-on-error=1\nbecause it knows it is dying.  However, the message from the updated\nwrite_message(), \"could not lock\", cannot be final---the caller may\nwant to add something else after it to describe what failed in a\nlarger picture.\n"}]}