Re: [BUG] Index.lock error message regression in git 2.11.0

[Junio C Hamano <gitster@pobox.com>]

Robbie Iannucci <iannucci@google.com> writes:

> I just upgraded to 2.11.0 from 2.10.2, and I noticed that some
> commands no longer print an error message when the `index.lock` file
> exists (such as `git merge --ff-only`).
>
> It appears this bug was introduced in
> 55f5704da69d3e6836620f01bee0093ad5e331e8 (sequencer: lib'ify
> checkout_fast_forward()). I determined this by running the attached
> bisect script (on OS X, but I don't think that matters; I've also seen
> the error message missing on Linux):

Yes, I can see that this is not limited to OSX.  The command does
exit with non-zero status, but that is not of much help for an
interactive user.

    $ git checkout v2.11.0^0
    $ >.git/index.lock
    $ git merge --ff-only master; echo $?
    Updating 454cb6bd52..8d7a455ed5
    1
    $ exit

We can see that it attempted to update, but because there is no
indication that it failed, the user can easily be misled to think
that we are now at the tip of the master branch.

We used to give "fatal: Unable to create ..." followed by "Another
git process seems to be running..." advice, that would have helped
the user from the confusion.

I do not think it is the right solution to call hold_locked_index()
with die_on_error=1 from the codepath changed by your 55f5704da6
("sequencer: lib'ify checkout_fast_forward()", 2016-09-09).  Perhaps
the caller of checkout_fast_forward() needs to learn how/why the
function returned error and diagnose this case and a message?  Or
perhaps turn that die_on_error parameter from boolean to tricolor
(i.e. the caller does not want you to die, but the caller knows that
you have more information to give better error message than it does,
so please show an error message instead of silently returning -1)?

Perhaps the attached would fix it (not even compile tested, though)?

I would prefer to make 0 to mean "show error but return -1", 1 to
mean "die on error", and 2 to mean "be silent and return -1 on
error", though.  Asking to be silent should be the exception for
this error from usability and safety's point of view, and requiring
such exceptional callers to pass LOCK_SILENT_ON_ERROR would be
easier to "git grep" for them.

Dscho, what do you think?  


 lockfile.c | 11 +++++++++--
 lockfile.h |  5 +++++
 merge.c    |  2 +-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f190caddb0 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,15 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
 				      int flags, long timeout_ms)
 {
 	int fd = lock_file_timeout(lk, path, flags, timeout_ms);
-	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		unable_to_lock_die(path, errno);
+	if (fd < 0) {
+		if (flags & LOCK_DIE_ON_ERROR)
+			unable_to_lock_die(path, errno);
+		else if (flags & LOCK_ERROR_ON_ERROR) {
+			struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(path, errno, &buf);
+			error("%s", buf.buf);
+		}
+	}
 	return fd;
 }
 
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..6cba9c8142 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -132,6 +132,11 @@ struct lock_file {
  * is already locked returns -1 to the caller.
  */
 #define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to show the usual error message
+ * and still return -1 to the caller with this flag
+ */
+#define LOCK_ERROR_ON_ERROR 2
 
 /*
  * Usually symbolic links in the destination path are resolved. This
diff --git a/merge.c b/merge.c
index 23866c9165..9e2e4f1a22 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,
 
 	refresh_cache(REFRESH_QUIET);
 
-	if (hold_locked_index(lock_file, 0) < 0)
+	if (hold_locked_index(lock_file, LOCK_ERROR_ON_ERROR) < 0)
 		return -1;
 
 	memset(&trees, 0, sizeof(trees));