Better error-handling around revert

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
---
 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]