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 > wrote: > > To be truly useful, the sequencer should never die() but always return > > an error. > > > > Signed-off-by: Johannes Schindelin > > --- > > 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! > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] sequencer: lib'ify save_todo()
On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin wrote: > To be truly useful, the sequencer should never die() but always return > an error. > > Signed-off-by: Johannes Schindelin > --- > 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? > } > if (commit_lock_file(&todo_lock) < 0) { > strbuf_release(&buf); > - die(_("Error wrapping up %s."), git_path_todo_file()); > + return error(_("Error wrapping up %s."), > git_path_todo_file()); > } > strbuf_release(&buf); > + return 0; > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/15] sequencer: lib'ify save_todo()
To be truly useful, the sequencer should never die() but always return an error. Signed-off-by: Johannes Schindelin --- sequencer.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3e07aa0..17f2c8b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts) return -1; } -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()); 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()); } if (commit_lock_file(&todo_lock) < 0) { strbuf_release(&buf); - die(_("Error wrapping up %s."), git_path_todo_file()); + return error(_("Error wrapping up %s."), git_path_todo_file()); } strbuf_release(&buf); + return 0; } static void save_opts(struct replay_opts *opts) @@ -995,7 +1000,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) return -1; for (cur = todo_list; cur; cur = cur->next) { - save_todo(cur, opts); + if (save_todo(cur, opts)) + return -1; res = do_pick_commit(cur->item, opts); if (res) return res; -- 2.10.0.rc1.99.gcd66998 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html