Re: [PATCH 11/15] sequencer: lib'ify save_todo()

> On Tue, Aug 23, 2016 at 12:07 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
> > @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)
> > -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> > +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> >  {
> >         static struct lock_file todo_lock;
> >         struct strbuf buf = STRBUF_INIT;
> >         int fd;
> >
> > -       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
> > +       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
> > +       if (fd < 0)
> > +               return error_errno(_("Could not lock '%s'"),
> > +                                  git_path_todo_file());
> >         if (format_todo(&buf, todo_list, opts) < 0)
> > -               die(_("Could not format %s."), git_path_todo_file());
> > +               return error(_("Could not format %s."), git_path_todo_file());
> 
> format_todo() doesn't seem to make any promises about the state of the
> strbuf upon error, so should this be releasing the strbuf before
> returning?

> >         if (write_in_full(fd, buf.buf, buf.len) < 0) {
> >                 strbuf_release(&buf);
> > -               die_errno(_("Could not write to %s"), git_path_todo_file());
> > +               return error_errno(_("Could not write to %s"),
> > +                                  git_path_todo_file());
> 
> Do the above two new error returns need to rollback the lockfile?