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

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <redacted>
---

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?

Yes, it should. Thank you!

quoted

        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?

As before, atexit() handler to the rescue ;-)

Thanks,
Dscho