Hi Martin,
On Tue, 27 Feb 2018, Martin Ågren wrote:
> On 26 February 2018 at 22:29, Johannes Schindelin
> <johannes.schindelin@xxxxxx> wrote:
> > As pointed out in a review of the `--recreate-merges` patch series,
> > `rollback_lock_file()` clobbers errno. Therefore, we have to report the
> > error message that uses errno before calling said function.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> > sequencer.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index e9baaf59bd9..5aa3dc3c95c 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -345,12 +345,14 @@ static int write_message(const void *buf, size_t len, const char *filename,
> > if (msg_fd < 0)
> > return error_errno(_("could not lock '%s'"), filename);
> > if (write_in_full(msg_fd, buf, len) < 0) {
> > + error_errno(_("could not write to '%s'"), filename);
> > rollback_lock_file(&msg_file);
> > - return error_errno(_("could not write to '%s'"), filename);
> > + return -1;
> > }
> > if (append_eol && write(msg_fd, "\n", 1) < 0) {
> > + error_errno(_("could not write eol to '%s'"), filename);
> > rollback_lock_file(&msg_file);
> > - return error_errno(_("could not write eol to '%s'"), filename);
> > + return -1;
> > }
> > if (commit_lock_file(&msg_file) < 0) {
> > rollback_lock_file(&msg_file);
> > @@ -2106,16 +2108,17 @@ static int save_head(const char *head)
> >
> > fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
> > if (fd < 0) {
> > + error_errno(_("could not lock HEAD"));
> > rollback_lock_file(&head_lock);
> > - return error_errno(_("could not lock HEAD"));
> > + return -1;
> > }
>
> I just noticed this when test-merging my series of lockfile-fixes to pu.
> This `rollback_lock_file()` is not needed, since failure to take the
> lock leaves it unlocked. If one wants to roll back the lock "for
> clarity" or "just to be safe", then the same should arguably be done in
> `write_message()`, just barely visible at the top of this diff.
>
> Perhaps not worth a reroll. The conflict resolution between this and my
> patch would be to take my hunk.
>
> https://public-inbox.org/git/cover.1519763396.git.martin.agren@xxxxxxxxx/T/#t
Thank you for working on this!
Dscho
