Better error-handling around revert
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- May 20, 2011, 07:16 UTC
- Message-ID
- <20110520071609.GA6755@domU-12-31-39-06-A8-0A.compute-1.internal>
Hi,
Instead of hijacking the Sequencer thread with tons of emails related to error-handling, I thought I'd start a new thread. I'm currently preparing a large series to fix error-handling issues around revert, but I'm afraid I need a lot of feedback to get it done right. Expect lots of RFC patches and diffs, and other discussions related to error-handling in revert on this thread.
Ref: http://thread.gmane.org/gmane.comp.version-control.git/173408/focus=173413
Anyway, here's the first:
Does this look like something callers can use? What should callers do when an error is returned? Should I also modify callers in the same patch?
- do_pick_commit calls write_cherry_pick_head, so I think it should propgate an error upwards to revert_or_cherry_pick which should ultimately handle the error. - do_pick_commit also calls write_message in two places. What should happen when the recursive merge fails and the write_message succeeds, and viceversa? Again, I think we should propgate the error updwards and let revert_or_cherry_pick exit(128).
write_cherry_pick_head and write_message used to die when an operation such as write_in_full failed. Instead, clean up correctly, and return an error to be handled by the caller.
Mentored-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com> --- builtin/revert.c | 41 +++++++++++++++++++++++++++-------------- 1 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c index 138485f..bb0db66 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -195,20 +195,29 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message) strbuf_addstr(msgbuf, p); } -static void write_cherry_pick_head(void) +static int write_cherry_pick_head(void) { - int fd; + int fd, ret = 0; struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1)); fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666); - if (fd < 0) - die_errno(_("Could not open '%s' for writing"), - git_path("CHERRY_PICK_HEAD")); - if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) - die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD")); + if (fd < 0) { + strbuf_release(&buf); + return error(_("Could not open '%s' for writing: %s"), + git_path("CHERRY_PICK_HEAD"), strerror(errno)); + } + if (write_in_full(fd, buf.buf, buf.len) != buf.len) + ret = error(_("Could not write to '%s': %s"), + git_path("CHERRY_PICK_HEAD"), strerror(errno)); + if (xclose(fd)) { + ret = error(_("Could not close '%s': %s"), + git_path("CHERRY_PICK_HEAD"), strerror(errno)); + unlink_or_warn(git_path("CHERRY_PICK_HEAD")); + } strbuf_release(&buf); + return ret; } static void advise(const char *advice, ...) @@ -240,17 +249,21 @@ static void print_advice(void) advise("and commit the result with 'git commit'"); } -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 ret = 0; - int msg_fd = hold_lock_file_for_update(&msg_file, filename, - LOCK_DIE_ON_ERROR); - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) - die_errno(_("Could not write to %s."), filename); + int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0); + if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) { + rollback_lock_file(&msg_fd); + ret = error(_("Could not write to %s: %s"), filename, + strerror(errno)); + } + else if (commit_lock_file(&msg_file) < 0) + ret = error(_("Error wrapping up %s"), filename); strbuf_release(msgbuf); - if (commit_lock_file(&msg_file) < 0) - die(_("Error wrapping up %s"), filename); + return ret; } static struct tree *empty_tree(void)