Re: [PATCH 01/15] sequencer: lib'ify write_message()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> > -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);
> >         if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> > -               die_errno(_("Could not write to %s"), filename);
> > +               return error_errno(_("Could not write to %s"), filename);
> 
> Does this need to rollback the lockfile before returning[*]?
> 
> [*] I'm not terribly familiar with the lockfile mechanism and I don't
> have a lot of time to study it presently, so ignore me if this is a
> stupid question.

Not a stupid question at all.

The way lockfiles work is that there is an atexit() handler that ensures
that uncommitted lockfiles get rolled back automatically. So it happens to
work correctly right now, because all (direct and transitive) callers
simply stop doing things as quickly as possible. That means that no
subsequent attempt is made to write to the same file.

Technically, I agree with you that it may look a bit cleaner to roll back
at this point. However, this is outside the purview of this here patch
series, as it would actually change the behavior (the previous die() would
rely on the atexit() handler to roll back the locked file, too). So I
think if this is to be changed, it has to be in its own, separate patch
series.

Ciao,
Dscho