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

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

[Robbie Iannucci]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

Hello,

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):

$ cd /path/to/git/src
$ git bisect start v2.11.0-rc0 v2.10.2
$ git bisect run /path/to/bisect.test.sh

(my original version of the script also includes some other makefile
parameters to get a modern version of gettext and openssl too, but
they're not relevant to this ML).

I'm not certain that I have enough context to propose a meaningful
patch though :/.

Cheers,
Robbie

[-- Attachment #2: bisect.test.sh --]
[-- Type: application/octet-stream, Size: 527 bytes --]

#!/bin/bash

make -j 8 2>&1 > /dev/null
if [[ $? != 0 ]]; then
  # skip this version
  exit 125
fi
git=`realpath ./git`

rm -rf .test_repo || true
mkdir .test_repo

cd .test_repo
$git init

echo HELLO > a
$git add a
$git commit -m 'initial_commit'
$git branch --track new_branch

$git checkout master
echo HELLO >> a
$git add a
$git commit -m 'second_commit'

$git checkout new_branch

touch .git/index.lock
if $git merge --ff-only master 2>&1 | grep -F "index.lock" ; then
  echo OK
  exit 0
fi
echo Message is missing
exit 1

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

[Robbie Iannucci]

Apparently I'm not supposed to send attachments >_<. Here's the script
in non-attachement form:

--------

#!/bin/bash

make -j 8 2>&1 > /dev/null
if [[ $? != 0 ]]; then
  # skip this version
  exit 125
fi
git=`realpath ./git`

rm -rf .test_repo || true
mkdir .test_repo

cd .test_repo
$git init

echo HELLO > a
$git add a
$git commit -m 'initial_commit'
$git branch --track new_branch

$git checkout master
echo HELLO >> a
$git add a
$git commit -m 'second_commit'

$git checkout new_branch

touch .git/index.lock
if $git merge --ff-only master 2>&1 | grep -F "index.lock" ; then
  echo OK
  exit 0
fi
echo Message is missing
exit 1

On Fri, Dec 2, 2016 at 5:44 PM, Robbie Iannucci <iannucci@google.com> wrote:
> Hello,
>
> 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):
>
> $ cd /path/to/git/src
> $ git bisect start v2.11.0-rc0 v2.10.2
> $ git bisect run /path/to/bisect.test.sh
>
> (my original version of the script also includes some other makefile
> parameters to get a modern version of gettext and openssl too, but
> they're not relevant to this ML).
>
> I'm not certain that I have enough context to propose a meaningful
> patch though :/.
>
> Cheers,
> Robbie

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

[Junio C Hamano]

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));

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

So here is the "You have to ask explicitly, if you know that it is
safe to be silent" version with a proper log message.

-- >8 --
Subject: [PATCH] lockfile: LOCK_SILENT_ON_ERROR

Recent "libify merge machinery" stopped from passing die_on_error
bit to hold_locked_index(), and lost an error message when there are
competing update in progress with "git merge --ff-only $commit", for
example.  The command still exits with a non-zero status, but that
is not of much help for an interactive user.  The last thing the
command says is "Updating $from..$to".  We used to follow it with a
big error message that makes it clear that "merge --ff-only" did not
succeed.

Introduce a new bit "LOCK_SILENT_ON_ERROR" that can be passed by
callers that do want to silence the message (because they either
make it a non-error by doing something else, or they show their own
error message to explain the situation), and show the error message
we used to give for everybody else, including the caller that was
touched by the libification in question.

I would not be surprised if some existing calls to hold_lock*()
functions that pass die_on_error=0 need to be updated to pass
LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look
like a regression, but we are better off starting louder and squelch
the ones that we find safe to make silent than the other way around.

Reported-by: Robbie Iannucci <iannucci@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 lockfile.c | 11 +++++++++--
 lockfile.h |  8 +++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..f7e8104449 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_SILENT_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..98b4862254 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,9 +129,15 @@ struct lock_file {
 /*
  * If a lock is already taken for the file, `die()` with an error
  * message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked gives the same error message and returns -1 to
+ * the caller.
  */
 #define LOCK_DIE_ON_ERROR 1
+/*
+ * ... or the function can be told to be totally silent and return
+ * -1 to the caller upon error with this flag
+ */
+#define LOCK_SILENT_ON_ERROR 2
 
 /*
  * Usually symbolic links in the destination path are resolved. This
-- 
2.11.0-270-g0b6beed61f

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> I would not be surprised if some existing calls to hold_lock*()
> functions that pass die_on_error=0 need to be updated to pass
> LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look
> like a regression, but we are better off starting louder and squelch
> the ones that we find safe to make silent than the other way around.

I actually take this part back, for two reasons.

 * Before the recent js/sequencer-wo-die topic that made this
   failure mode of 'git merge' more dangerous by accident, there
   were already callers that passed die_on_error=0 to hold_lock*
   family of functions, and we can trust these callers.  They either
   have been silent upon lock failure sensibly (e.g. a caller that
   tries to acquire the lock to opportunistically update the index
   can safely choose not to do anything and be silent) or they have
   had their own way of reporting the errors to the users.  The "you
   need to ask to be totally quiet" approach in my rerolled patch
   (the one I am responding to) will introduce new regressions to
   these codepaths.

 * Among the ones that stopped passing die_on_error=1 when the topic
   was merged, there are
   ones that give sensible error messages.  Again, they do not need
   extra message with the "you need to ask to be totally quiet"
   approach [*1*].

We need to instead go through the latter, i.e. the ones that appear
in "git show --first-parent 2a4062a4a8", with fine-toothed comb to
see which 0 made an error totally silent (like the one Robbie
spotted in merge.c) and fix them to ask hold_lock*() functions not
to die but still report an error.

[PATCH 0/3] Do not be totally silent upon lock error

[Junio C Hamano]

Robbie Iannucci reported that "git merge" that fast-forwards fails
silently when a competing or stale index.lock is present in recent
Git:

    $ git merge --ff-only master; echo $?
    Updating 454cb6bd52..8d7a455ed5
    1
    $ exit

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

This was because there were only two choices available to the
callers of the lockfile API--they can either ask it to die with
message when the lock cannot be acquired, or ask it to silently
return -1 to signal an error. The recent "libify sequencer" topic
turned many existing "please die" calls to "just silently return
-1", and while it added new "unable to lock" error messages to most
of them, one spot didn't get any and now is allowed to just die
silently.

This series should fix it:

 - 1/3 is something I noticed while reading the existing callers of
   the lockfile API, and is not a new issue with "libify sequencer"
   topic.

 - 2/3 gives a new third option to callers of the lockfile API; they
   can now say "please emit the usual error message upon failing to
   acquire the lock, but give control back to me".

 - 3/3 uses the new option to resurrect the message.

Junio C Hamano (3):
  wt-status: implement opportunisitc index update correctly
  hold_locked_index(): align error handling with hold_lockfile_for_update()
  lockfile: LOCK_REPORT_ON_ERROR

 apply.c                          |  2 +-
 builtin/add.c                    |  2 +-
 builtin/am.c                     |  6 +++---
 builtin/checkout-index.c         |  2 +-
 builtin/checkout.c               |  4 ++--
 builtin/clone.c                  |  2 +-
 builtin/commit.c                 |  8 ++++----
 builtin/merge.c                  |  6 +++---
 builtin/mv.c                     |  2 +-
 builtin/read-tree.c              |  2 +-
 builtin/reset.c                  |  2 +-
 builtin/rm.c                     |  2 +-
 builtin/update-index.c           |  1 +
 lockfile.c                       | 12 ++++++++++--
 lockfile.h                       |  8 +++++++-
 merge-recursive.c                |  2 +-
 merge.c                          |  2 +-
 read-cache.c                     |  7 ++-----
 rerere.c                         |  2 +-
 sequencer.c                      |  2 +-
 t/helper/test-scrap-cache-tree.c |  2 +-
 wt-status.c                      |  7 ++++---
 22 files changed, 49 insertions(+), 36 deletions(-)

-- 
2.11.0-274-g0631465056

[PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

The require_clean_work_tree() function calls hold_locked_index()
with die_on_error=0 to signal that it is OK if it fails to obtain
the lock, but unconditionally calls update_index_if_able(), which
will try to write into fd=-1.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index a2e9d332d8..a715e71906 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
 int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int err = 0;
+	int err = 0, fd;
 
-	hold_locked_index(lock_file, 0);
+	fd = hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
-	update_index_if_able(&the_index, lock_file);
+	if (0 <= fd)
+		update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
 	if (has_unstaged_changes(ignore_submodules)) {
-- 
2.11.0-274-g0631465056

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The require_clean_work_tree() function calls hold_locked_index()
> with die_on_error=0 to signal that it is OK if it fails to obtain
> the lock, but unconditionally calls update_index_if_able(), which
> will try to write into fd=-1.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

So my first question I had to answer was if we do the right thing here,
i.e. if we could just fail instead. But we want to continue and just
not write back the index, which is fine.

So we do not have to guard refresh_cache, but just call
update_index_if_able conditionally.

However I think the promise of that function is
to take care of the fd == -1?

    /*
    * Opportunistically update the index but do not complain if we can't
    */
    void update_index_if_able(struct index_state *istate, struct
lock_file *lockfile)
    {
        if ((istate->cache_changed || has_racy_timestamp(istate)) &&
            verify_index(istate) &&
            write_locked_index(istate, lockfile, COMMIT_LOCK))
                rollback_lock_file(lockfile);
    }

So I would expect that we'd rather fix the update_index_if_able instead by
checking for the lockfile to be in the correct state?


>  wt-status.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index a2e9d332d8..a715e71906 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
>  int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
>  {
>         struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -       int err = 0;
> +       int err = 0, fd;
>
> -       hold_locked_index(lock_file, 0);
> +       fd = hold_locked_index(lock_file, 0);
>         refresh_cache(REFRESH_QUIET);
> -       update_index_if_able(&the_index, lock_file);
> +       if (0 <= fd)
> +               update_index_if_able(&the_index, lock_file);
>         rollback_lock_file(lock_file);
>
>         if (has_unstaged_changes(ignore_submodules)) {
> --
> 2.11.0-274-g0631465056
>

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:
>
> So I would expect that we'd rather fix the update_index_if_able instead by
> checking for the lockfile to be in the correct state?

I actually don't expect that, after looking at other call sites of
that function.

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>> So I would expect that we'd rather fix the update_index_if_able instead by
>> checking for the lockfile to be in the correct state?
>
> I actually don't expect that, after looking at other call sites of
> that function.

Yes I checked the other callers as well right now and you seem to be correct.
My initial response was based on the name of the function,
specifically the _if_able
part as that hinted to me that I can call the function with no
precondition and the
_if_able will figure out when to do the actual update_index.

The first part of the condition of the function
    (istate->cache_changed || has_racy_timestamp(istate)
reads rather as a _if_needed instead of an _if_able to me.

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So my first question I had to answer was if we do the right thing here,
> i.e. if we could just fail instead. But we want to continue and just
> not write back the index, which is fine.
>
> So we do not have to guard refresh_cache, but just call
> update_index_if_able conditionally.

An explanation with stepping back a little bit may help.

You may be asked to visit a repository of a friend, to which you do
not have write access to but you can still read.  You may want to do
"diff", "status" or "describe" there.

In order to avoid getting fooled into thinking some paths are dirty
only because the cached stat information does not match, these need
to refresh the in-core index before doing their "comparison" to
report which paths are different (in "diff"), what are the modified
but not staged paths (in "status"), and if there is a need to add
the "-dirty" suffix (in "describe").

Since we are doing the expensive "bunch of lstat()" anyway, if we
could write it back to the index, it would help future operations in
the same repository--that is the reasoning behind the opportunistic
updates.  It is perfectly OK if we do not have write access to the
repository and cannot write update the index.

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 1:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So my first question I had to answer was if we do the right thing here,
>> i.e. if we could just fail instead. But we want to continue and just
>> not write back the index, which is fine.
>>
>> So we do not have to guard refresh_cache, but just call
>> update_index_if_able conditionally.
>
> An explanation with stepping back a little bit may help.
>
> You may be asked to visit a repository of a friend, to which you do
> not have write access to but you can still read.  You may want to do
> "diff", "status" or "describe" there.
>
> In order to avoid getting fooled into thinking some paths are dirty
> only because the cached stat information does not match, these need
> to refresh the in-core index before doing their "comparison" to
> report which paths are different (in "diff"), what are the modified
> but not staged paths (in "status"), and if there is a need to add
> the "-dirty" suffix (in "describe").
>
> Since we are doing the expensive "bunch of lstat()" anyway, if we
> could write it back to the index, it would help future operations in
> the same repository--that is the reasoning behind the opportunistic
> updates.  It is perfectly OK if we do not have write access to the
> repository and cannot write update the index.
>

Thanks for that explanation!
So I agree that we should not call it _if_needed, but the _if_able
part is still confusing, as we check for different parts here.

The case of not being able to write (read only) sounds similar to
the case of not being able to write (a concurrent lock),
I'll think about it more.

Thanks,
Stefan

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Paul Tan]

Hi Junio,

On Thu, Dec 8, 2016 at 4:48 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The require_clean_work_tree() function calls hold_locked_index()
>> with die_on_error=0 to signal that it is OK if it fails to obtain
>> the lock, but unconditionally calls update_index_if_able(), which
>> will try to write into fd=-1.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---

Ah, sorry about this. I was indeed misled by the function naming and
its comment ("do not complain if we can't"). Should have looked more
closely at the other call sites.

> However I think the promise of that function is
> to take care of the fd == -1?

Hmm, to add on, looking at the three other call sites of this
function, two of them (builtin/commit.c and builtin/describe.c)
basically do:

    if (0 <= fd)
        update_index_if_able(...)

with that 0 <= fd conditional. With this patch it becomes three out of
four. Perhaps the repeated use of this conditional is a sign that the
0 <= fd check could be built into update_index_if_able()? I think
there is precedent for building in these kind of checks --
rollback_lock_file() also does not fail if the lock file was not
successfully opened.

That said, the number of call sites is quite low so it's probably not
worth doing this.

Thanks,
Paul

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Hmm, to add on, looking at the three other call sites of this
> function, two of them (builtin/commit.c and builtin/describe.c)
> basically do:
>
>     if (0 <= fd)
>         update_index_if_able(...)
>
> with that 0 <= fd conditional. With this patch it becomes three out of
> four.

The other one is diff.c::refresh_index_quietly() that you are not
counting, I think, but if you look at it again, it also is not
called after hold_locked_index() fails to acquire the lock, so with
this fix everybody refrains from calling it when it does not hold
the lock.

> Perhaps the repeated use of this conditional is a sign that the
> 0 <= fd check could be built into update_index_if_able()?

That condition is "do we have the lock?  Otherwise we are not even
allowed to update it", so in that sense it may make sense.

> I think there is precedent for building in these kind of checks --
> rollback_lock_file() also does not fail if the lock file was not
> successfully opened.
>
> That said, the number of call sites is quite low so it's probably not
> worth doing this.

Yeah, I can go either way.  At least with the change things are
consistent.

[PATCH 2/3] hold_locked_index(): align error handling with hold_lockfile_for_update()

[Junio C Hamano]

Callers of the hold_locked_index() function pass 0 when they want to
prepare to write a new version of the index file without wishing to
die or emit an error message when the request fails (e.g. somebody
else already held the lock), and pass 1 when they want the call to
die upon failure.

This option is called LOCK_DIE_ON_ERROR by the underlying lockfile
API, and the hold_locked_index() function translates the paramter to
LOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update().

Replace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop
translating.  Callers other than the ones that are replaced with
this change pass '0' to the function; no behaviour change is
intended with this patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Among the callers of hold_locked_index() that passes 0:

 - diff.c::refresh_index_quietly() at the end of "git diff" is an
   opportunistic update; it leaks the lockfile structure but it is
   just before the program exits and nobody should care.

 - builtin/describe.c::cmd_describe(),
   builtin/commit.c::cmd_status(),
   sequencer.c::read_and_refresh_cache() are all opportunistic
   updates and they are OK.

 - builtin/update-index.c::cmd_update_index() takes a lock upfront
   but we may end up not needing to update the index (i.e. the
   entries may be fully up-to-date), in which case we do not need to
   issue an error upon failure to acquire the lock.  We do diagnose
   and die if we indeed need to update, so it is OK.

 - wt-status.c::require_clean_work_tree() IS BUGGY.  It asks
   silence, does not check the returned value.  Compare with
   callsites like cmd_describe() and cmd_status() to notice that it
   is wrong to call update_index_if_able() unconditionally.
---
 apply.c                          | 2 +-
 builtin/add.c                    | 2 +-
 builtin/am.c                     | 6 +++---
 builtin/checkout-index.c         | 2 +-
 builtin/checkout.c               | 4 ++--
 builtin/clone.c                  | 2 +-
 builtin/commit.c                 | 8 ++++----
 builtin/merge.c                  | 6 +++---
 builtin/mv.c                     | 2 +-
 builtin/read-tree.c              | 2 +-
 builtin/reset.c                  | 2 +-
 builtin/rm.c                     | 2 +-
 builtin/update-index.c           | 1 +
 merge-recursive.c                | 2 +-
 read-cache.c                     | 7 ++-----
 rerere.c                         | 2 +-
 sequencer.c                      | 2 +-
 t/helper/test-scrap-cache-tree.c | 2 +-
 18 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/apply.c b/apply.c
index 705cf562f0..2ed808d429 100644
--- a/apply.c
+++ b/apply.c
@@ -4688,7 +4688,7 @@ static int apply_patch(struct apply_state *state,
 								 state->index_file,
 								 LOCK_DIE_ON_ERROR);
 		else
-			state->newfd = hold_locked_index(state->lock_file, 1);
+			state->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);
 	}
 
 	if (state->check_index && read_apply_cache(state) < 0) {
diff --git a/builtin/add.c b/builtin/add.c
index e8fb80b36e..9f53f020d0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -361,7 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
 
-	hold_locked_index(&lock_file, 1);
+	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
 		 (show_only ? ADD_CACHE_PRETEND : 0) |
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce9..bb5da422fc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1119,7 +1119,7 @@ static void refresh_and_write_cache(void)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
 	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 		die(_("unable to write index file"));
@@ -1976,7 +1976,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 		return -1;
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 
 	refresh_cache(REFRESH_QUIET);
 
@@ -2016,7 +2016,7 @@ static int merge_tree(struct tree *tree)
 		return -1;
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 1;
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 30a49d9f42..07631d0c9c 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -205,7 +205,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	if (index_opt && !state.base_dir_len && !to_tempfile) {
 		state.refresh_cache = 1;
 		state.istate = &the_index;
-		newfd = hold_locked_index(&lock_file, 1);
+		newfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 	}
 
 	/* Check out named files first */
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 512492aad9..bfe685c198 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -274,7 +274,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 	if (read_cache_preload(&opts->pathspec) < 0)
 		return error(_("index file corrupt"));
 
@@ -467,7 +467,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 	int ret;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 	if (read_cache_preload(NULL) < 0)
 		return error(_("index file corrupt"));
 
diff --git a/builtin/clone.c b/builtin/clone.c
index 6c76a6ed66..892bdbfe3f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -711,7 +711,7 @@ static int checkout(int submodule_progress)
 	setup_work_tree();
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
+	hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 
 	memset(&opts, 0, sizeof opts);
 	opts.update = 1;
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29b..b910e76017 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 
 	if (interactive) {
 		char *old_index_env = NULL;
-		hold_locked_index(&index_lock, 1);
+		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 
 		refresh_cache_or_die(refresh_flags);
 
@@ -396,7 +396,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * (B) on failure, rollback the real index.
 	 */
 	if (all || (also && pathspec.nr)) {
-		hold_locked_index(&index_lock, 1);
+		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
@@ -416,7 +416,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * We still need to refresh the index here.
 	 */
 	if (!only && !pathspec.nr) {
-		hold_locked_index(&index_lock, 1);
+		hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed
 		    || !cache_tree_fully_valid(active_cache_tree))
@@ -468,7 +468,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (read_cache() < 0)
 		die(_("cannot read the index"));
 
-	hold_locked_index(&index_lock, 1);
+	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 	update_main_cache_tree(WRITE_TREE_SILENT);
diff --git a/builtin/merge.c b/builtin/merge.c
index b65eeaa87d..0070bf2556 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -634,7 +634,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 {
 	static struct lock_file lock;
 
-	hold_locked_index(&lock, 1);
+	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
@@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		for (j = common; j; j = j->next)
 			commit_list_insert(j->item, &reversed);
 
-		hold_locked_index(&lock, 1);
+		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
 		if (clean < 0)
@@ -781,7 +781,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 	struct commit_list *parents, **pptr = &parents;
 	static struct lock_file lock;
 
-	hold_locked_index(&lock, 1);
+	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877bc9..43adf92ba6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -126,7 +126,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (--argc < 1)
 		usage_with_options(builtin_mv_usage, builtin_mv_options);
 
-	hold_locked_index(&lock_file, 1);
+	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 9bd1fd755e..fa6edb35b2 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -150,7 +150,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	hold_locked_index(&lock_file, 1);
+	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	prefix_set = opts.prefix ? 1 : 0;
 	if (1 < opts.merge + opts.reset + prefix_set)
diff --git a/builtin/reset.c b/builtin/reset.c
index c04ac076dc..8ab915bfcb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -354,7 +354,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	if (reset_type != SOFT) {
 		struct lock_file *lock = xcalloc(1, sizeof(*lock));
-		hold_locked_index(lock, 1);
+		hold_locked_index(lock, LOCK_DIE_ON_ERROR);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..7f15a3d7f8 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -292,7 +292,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (!index_only)
 		setup_work_tree();
 
-	hold_locked_index(&lock_file, 1);
+	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
diff --git a/builtin/update-index.c b/builtin/update-index.c
index f3f07e7f1c..d530e89368 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1012,6 +1012,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
+	/* we will diagnose later if it turns out that we need to update it */
 	newfd = hold_locked_index(lock_file, 0);
 	if (newfd < 0)
 		lock_error = errno;
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..8442068716 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2124,7 +2124,7 @@ int merge_recursive_generic(struct merge_options *o,
 		}
 	}
 
-	hold_locked_index(lock, 1);
+	hold_locked_index(lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
 			result);
 	if (clean < 0)
diff --git a/read-cache.c b/read-cache.c
index db5d910642..f92a912dcb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1425,12 +1425,9 @@ static int read_index_extension(struct index_state *istate,
 	return 0;
 }
 
-int hold_locked_index(struct lock_file *lk, int die_on_error)
+int hold_locked_index(struct lock_file *lk, int lock_flags)
 {
-	return hold_lock_file_for_update(lk, get_index_file(),
-					 die_on_error
-					 ? LOCK_DIE_ON_ERROR
-					 : 0);
+	return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
 }
 
 int read_index(struct index_state *istate)
diff --git a/rerere.c b/rerere.c
index 5d083ca572..3bd55caf3b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -708,7 +708,7 @@ static void update_paths(struct string_list *update)
 {
 	int i;
 
-	hold_locked_index(&index_lock, 1);
+	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 
 	for (i = 0; i < update->nr; i++) {
 		struct string_list_item *item = &update->items[i];
diff --git a/sequencer.c b/sequencer.c
index 30b10ba143..7fc1e2a5df 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -370,7 +370,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	char **xopt;
 	static struct lock_file index_lock;
 
-	hold_locked_index(&index_lock, 1);
+	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 
 	read_cache();
 
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index 27fe0405b8..d2a63bea43 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -8,7 +8,7 @@ static struct lock_file index_lock;
 int cmd_main(int ac, const char **av)
 {
 	setup_git_directory();
-	hold_locked_index(&index_lock, 1);
+	hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
 	if (read_cache() < 0)
 		die("unable to read index file");
 	active_cache_tree = NULL;
-- 
2.11.0-274-g0631465056

[PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

[Junio C Hamano]

The "libify sequencer" topic stopped passing the die_on_error option
to hold_locked_index(), and this lost an error message from "git
merge --ff-only $commit" when there are competing updates in
progress.

The command still exits with a non-zero status, but that is not of
much help for an interactive user.  The last thing the command says
is "Updating $from..$to".  We used to follow it with a big error
message that makes it clear that "merge --ff-only" did not succeed.

What is sad is that we should have noticed this regression while
reviewing the change.  It was clear that the update to the
checkout_fast_forward() function made a failing hold_locked_index()
silent, but the only caller of the checkout_fast_forward() function
had this comment:

	    if (checkout_fast_forward(from, to, 1))
    -               exit(128); /* the callee should have complained already */
    +               return -1; /* the callee should have complained already */

which clearly contradicted the assumption X-<.

Add a new option LOCK_REPORT_ON_ERROR that can be passed instead of
LOCK_DIE_ON_ERROR to the hold_lock*() family of functions and teach
checkout_fast_forward() to use it to fix this regression.

After going thourgh all calls to hold_lock*() family of functions
that used to pass LOCK_DIE_ON_ERROR but were modified to pass 0 in
the "libify sequencer" topic "git show --first-parent 2a4062a4a8",
it appears that this is the only one that has become silent.  Many
others used to give detailed report that talked about "there may be
competing Git process running" but with the series merged they now
only give a single liner "Unable to lock ...", some of which may
have to be tweaked further, but at least they say something, unlike
the one this patch fixes.

Reported-by: Robbie Iannucci <iannucci@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 lockfile.c | 12 ++++++++++--
 lockfile.h |  8 +++++++-
 merge.c    |  2 +-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..aa69210d8b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,16 @@ 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);
+		if (flags & LOCK_REPORT_ON_ERROR) {
+			struct strbuf buf = STRBUF_INIT;
+			unable_to_lock_message(path, errno, &buf);
+			error("%s", buf.buf);
+			strbuf_release(&buf);
+		}
+	}
 	return fd;
 }
 
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..16775a7d79 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,11 +129,17 @@ struct lock_file {
 /*
  * If a lock is already taken for the file, `die()` with an error
  * message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked silently returns -1 to the caller, or ...
  */
 #define LOCK_DIE_ON_ERROR 1
 
 /*
+ * ... this flag can be passed instead to return -1 and give the usual
+ * error message upon an error.
+ */
+#define LOCK_REPORT_ON_ERROR 2
+
+/*
  * Usually symbolic links in the destination path are resolved. This
  * means that (1) the lockfile is created by adding ".lock" to the
  * resolved path, and (2) upon commit, the resolved path is
diff --git a/merge.c b/merge.c
index 23866c9165..04ee5fc911 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_REPORT_ON_ERROR) < 0)
 		return -1;
 
 	memset(&trees, 0, sizeof(trees));
-- 
2.11.0-274-g0631465056

Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

[Johannes Schindelin]

Hi Junio,

On Wed, 7 Dec 2016, Junio C Hamano wrote:

> The "libify sequencer" topic stopped passing the die_on_error option
> to hold_locked_index(), and this lost an error message from "git
> merge --ff-only $commit" when there are competing updates in
> progress.

Sorry for the breakage.

When libifying the code, I tried to be careful to retain the error
messages when not dying, and mistakenly assumed that hold_locked_index()
would do the same.

Ciao,
Dscho

Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

[Robbie Iannucci]

Thanks all for taking a look at this, I didn't expect such a quick response :).

No hard feelings re: breakage; I know how gnarly these sorts of
refactors can be (and more libification is always better :)).

Robbie

On Thu, Dec 8, 2016 at 3:53 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Wed, 7 Dec 2016, Junio C Hamano wrote:
>
>> The "libify sequencer" topic stopped passing the die_on_error option
>> to hold_locked_index(), and this lost an error message from "git
>> merge --ff-only $commit" when there are competing updates in
>> progress.
>
> Sorry for the breakage.
>
> When libifying the code, I tried to be careful to retain the error
> messages when not dying, and mistakenly assumed that hold_locked_index()
> would do the same.
>
> Ciao,
> Dscho

Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Sorry for the breakage.

Apologies from me, too.  Once a topic is merged, the credit still
remains with the contributor, but the blame is shared by the project
as a whole, with those who missed breakages during their reviews,
and those who didn't review or test and let breakages pass.

> When libifying the code, I tried to be careful to retain the error
> messages when not dying,...

Quite honestly, I do not think either of us cared about preserving
the exact error message the end-user was getting from each failure
sites that the series changed a call with die-on-error=1 to a call
with die-on-error=0 that is followed by a negative return while
reviewing this series.  As I wrote in the proposed log message for
3/3, this one was noticed as a end-user breaking change because it
was the only one that has become totally silent.  For example, this
bit from sequencer.c::write_message() we can see in the output from
"git show --first-parent 2a4062a4a8" does not preserve the message
at all:

    diff --git a/sequencer.c b/sequencer.c
    index 3804fa931d..eec8a60d6b 100644
    --- a/sequencer.c
    +++ b/sequencer.c
    @@ -180,17 +180,20 @@
    ...
    -static void write_message(struct strbuf *msgbuf, const char *filename)
    +static int write_message(struct strbuf *msgbuf, const char *filename)
     {
            static struct lock_file msg_file;

    -	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
    -					       LOCK_DIE_ON_ERROR);
    +	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
    +	if (msg_fd < 0)
    +		return error_errno(_("Could not lock '%s'"), filename);

And I do not think it is necessarily bad that the error message
changed with this conversion.  In other words, I do not think it
should have been the goal to preserve the exact error message.
hold_lock*() can afford to give a detailed message that strongly
sounds as being the final decision when called with die-on-error=1
because it knows it is dying.  However, the message from the updated
write_message(), "could not lock", cannot be final---the caller may
want to add something else after it to describe what failed in a
larger picture.