Re: [PATCH v5 01/12] sequencer: avoid using errno clobbered by rollback_lock_file()

> On 26 February 2018 at 22:29, Johannes Schindelin
> <johannes.schindelin@gmx.de> 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@gmx.de>
> > ---
> >  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@gmail.com/T/#t