Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

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

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.