Re: Better error-handling around revert

From

Ramkumar Ramachandra <artagnon@gmail.com>

Date

May 20, 2011, 07:28 UTC

Message-ID

<20110520072846.GB6755@domU-12-31-39-06-A8-0A.compute-1.internal>

In-Reply-To

<20110520071609.GA6755@domU-12-31-39-06-A8-0A.compute-1.internal>

Hi,

Ugh, sorry about the whitespace breakage. Here's a fresh diff:

    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.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

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)