Better error-handling around revert

Better error-handling around revert

[Ramkumar Ramachandra]

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)

Re: Better error-handling around revert

[Ramkumar Ramachandra]

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)

Re: Better error-handling around revert

[Christian Couder]

Hi,

On Friday 20 May 2011 09:28:47 Ramkumar Ramachandra wrote:
> Hi,
> 
> Ugh, sorry about the whitespace breakage. Here's a fresh diff:

It looks like there is still whitespace breakage, because code is not indented 
anymore.

>     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.

Otherwise it looks good to me.

Thanks,
Christian.

[RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache

[Ramkumar Ramachandra]

If the read_cache() call succeeds, the function must call
discard_cache() before returning to the caller.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 cache-tree.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index f755590..17c5bab 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -573,8 +573,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 
 		if (cache_tree_update(active_cache_tree,
 				      active_cache, active_nr,
-				      missing_ok, 0) < 0)
+				      missing_ok, 0) < 0) {
+			discard_cache();
 			return WRITE_TREE_UNMERGED_INDEX;
+		}
 		if (0 <= newfd) {
 			if (!write_cache(newfd, active_cache, active_nr) &&
 			    !commit_lock_file(lock_file))
@@ -591,8 +593,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	if (prefix) {
 		struct cache_tree *subtree =
 			cache_tree_find(active_cache_tree, prefix);
-		if (!subtree)
+		if (!subtree) {
+			discard_cache();
 			return WRITE_TREE_PREFIX_ERROR;
+		}
 		hashcpy(sha1, subtree->sha1);
 	}
 	else
@@ -601,6 +605,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	if (0 <= newfd)
 		rollback_lock_file(lock_file);
 
+	discard_cache();
 	return 0;
 }
 
-- 
1.7.5.GIT

[RFC PATCH] revert: Use assert to catch inherent program bugs

[Ramkumar Ramachandra]

Instead of returning and error status or calling die, use an assert
statement to guard against callers who don't call the functions with
sane arguments.  This situation is hence treated as an inherent bug in
the program, rather than a runtime error.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..8102d77 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	int abbrev_len, subject_len;
 	char *q;
 
-	if (!raw_message)
-		return -1;
+	assert(raw_message);
 	encoding = get_encoding(raw_message);
 	if (!encoding)
 		encoding = "UTF-8";
@@ -167,9 +166,7 @@ static char *get_encoding(const char *message)
 {
 	const char *p = message, *eol;
 
-	if (!p)
-		die (_("Could not read commit message of %s"),
-				sha1_to_hex(commit->object.sha1));
+	assert(p);
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -444,9 +441,7 @@ static int do_pick_commit(void)
 		die(_("%s: cannot parse parent commit %s"),
 		    me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+	get_message(commit->buffer, &msg);
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
-- 
1.7.5.GIT

Re: [RFC PATCH] revert: Use assert to catch inherent program bugs

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Instead of returning and error status or calling die, use an assert
> statement to guard against callers who don't call the functions with
> sane arguments.

Please do not do this.  assert() is a mechanism to aid debugging in a
development build, and can be stripped away in the production build.

> diff --git a/builtin/revert.c b/builtin/revert.c
> index f697e66..8102d77 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	int abbrev_len, subject_len;
>  	char *q;
>  
> -	if (!raw_message)
> -		return -1;
> +	assert(raw_message);
>  	encoding = get_encoding(raw_message);

This change is wrong, as you could feed a NULL to get_encoding (and you
also broke that function in the later hunk).

If the calling convention of this internal function is that passing NULL
is a programming bug, please do something like this instead:

	if (!raw_message)
		die("BUG: get_message() called with NULL");

Re: [RFC PATCH] revert: Use assert to catch inherent program bugs

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> This situation is hence treated as an inherent bug in
> the program, rather than a runtime error.

Well, programming errors can be run-time errors, too. :)  But anyway,
it is not obvious at a glance to me that this assertion would always
hold, which makes it a bad candidate for an assert().

"Wait a second," you might say.  "You said in a previous message that
the check in get_encoding() could be an assert() or die("BUG: ...") to
avoid having to remember the commit id for use in the error message."

And that's still true:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	int abbrev_len, subject_len;
>  	char *q;
>  
> -	if (!raw_message)
> -		return -1;
> +	assert(raw_message);
>  	encoding = get_encoding(raw_message);

get_encoding() only gets called after the check here, so in the
existing code, the raw_message parameter to get_encoding() can never
be NULL.

By contrast, get_message() gets called by do_pick_commit() and passed
a buffer that is populated long before then.  Can it ever be NULL?  I
don't know, so the check is certainly doing something useful for my
peace of mind.

> @@ -444,9 +441,7 @@ static int do_pick_commit(void)
>  		die(_("%s: cannot parse parent commit %s"),
>  		    me, sha1_to_hex(parent->object.sha1));
>  
> -	if (get_message(commit->buffer, &msg) != 0)
> -		die(_("Cannot get commit message for %s"),
> -				sha1_to_hex(commit->object.sha1));
> +	get_message(commit->buffer, &msg);

Forgetting what's mentioned above, this change would be a regression.
What if get_message() learns to return error() for some other reason?
It is only safe to call a function like this if deliberately ignoring
errors (in which case a comment might be helpful for explaining why to
ignore the errors) or if the function returns "void".

[RFC PATCH] wrapper: Introduce xclose to restart close on EINTR

[Ramkumar Ramachandra]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-compat-util.h |    1 +
 wrapper.c         |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..6e06ad4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -428,6 +428,7 @@ extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
+extern int xclose(int fd);
 extern int xdup(int fd);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
diff --git a/wrapper.c b/wrapper.c
index 2829000..717e989 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -141,6 +141,21 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
+/*
+ * xclose() is the same a close(), but it automatically restarts close()
+ * operations with a recoverable error (EINTR).
+ */
+int xclose(int fd)
+{
+	int ret;
+	while (1) {
+		ret = close(fd);
+		if ((ret < 0) && errno == EINTR)
+			continue;
+		return ret;
+	}
+}
+
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;
-- 
1.7.5.GIT

Re: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> [Subject: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR]

close never returns with errno == EINTR on Linux, but it can happen on
Solaris, for example.

I can't think of a reason git would want to call close() without this
loop.  Maybe it would make sense to wrap close() unconditionally (see
below).

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -141,6 +141,21 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
>  	}
>  }
>  
> +/*
> + * xclose() is the same a close(), but it automatically restarts close()
> + * operations with a recoverable error (EINTR).
> + */

If there is to be an xclose, I think I'd say something like

	/*
	 * xclose() is like close(), but it retries if interrupted by
	 * a signal on platforms like Solaris that allow that to avoid
	 * unnecessarily leaking a file descriptor.  It quietly returns
	 * -1 with errno set appropriately on failure.
	 */

to avoid confusion with the semantics of xmalloc.

> +int xclose(int fd)
> +{
> +	int ret;
> +	while (1) {
> +		ret = close(fd);
> +		if ((ret < 0) && errno == EINTR)
> +			continue;
> +		return ret;
> +	}
> +}

Micronit: close() can only return 0 or -1, so this can be written more
simply as

	while (close(fd)) {
		if (errno == EINTR)
			continue;
		return -1;
	}
	return 0;

Untested.

-- >8 --
Subject: compat: wrap close() to avoid having to worry about EINTR

On some non-Linux platforms, close() can return EINTR to indicate
interruption by a signal.  In a poll or select loop, that could be
good behavior to prevent hangs, but that doesn't apply anywhere within
git.  Let close() loop unconditionally in this case to avoid
preventable file descriptor leaks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-compat-util.h |   12 +++++++++++-
 wrapper.c         |   15 ---------------
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 6e06ad4..1326edb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -428,7 +428,6 @@ extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
-extern int xclose(int fd);
 extern int xdup(int fd);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
@@ -450,6 +449,17 @@ static inline int has_extension(const char *filename, const char *ext)
 	return len > extlen && !memcmp(filename + len - extlen, ext, extlen);
 }
 
+/* Sane close - resumes after interruption by signals */
+static inline int git_close(int fd)
+{
+	while (close(fd)) {
+		if (errno != EINTR)
+			return -1;
+	}
+	return 0;
+}
+#define close git_close
+
 /* Sane ctype - no locale, and works with signed chars */
 #undef isascii
 #undef isspace
diff --git a/wrapper.c b/wrapper.c
index 717e989..2829000 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -141,21 +141,6 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 	}
 }
 
-/*
- * xclose() is the same a close(), but it automatically restarts close()
- * operations with a recoverable error (EINTR).
- */
-int xclose(int fd)
-{
-	int ret;
-	while (1) {
-		ret = close(fd);
-		if ((ret < 0) && errno == EINTR)
-			continue;
-		return ret;
-	}
-}
-
 ssize_t read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;
-- 
1.7.5.1

Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> If the read_cache() call succeeds, the function must call
> discard_cache() before returning to the caller.

Does this mean the callers (current ones and also future ones) cannot
continue using the index after writing it out as a tree?

I would imagine anything that wants to write more than one tree (imagine:
reimplementation of "git stash" in C) would want to do something like:

	read_cache();
        write_cache_as_tree(index_tree_sha1[]);
        create commit to record that tree, with HEAD as the parent,
            and call that i_commit;

	for (all path in work tree different from index)
	        add_file_to_index();
        write_cache_as_tree(worktree_tree_sha1[]);
        create commit to record that tree, with HEAD as the parent;

        create commit to record the same tree, with HEAD and i_commit,
            and return it as the result of "git stash create".

Of course you _could_ force the caller to read_cache() what you wrote out
again immediately, but I do not see why you want to do so.  Especially in
the non-error codepath, I do not see why this could be a good change.

> diff --git a/cache-tree.c b/cache-tree.c
> index f755590..17c5bab 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -573,8 +573,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  
>  		if (cache_tree_update(active_cache_tree,
>  				      active_cache, active_nr,
> -				      missing_ok, 0) < 0)
> +				      missing_ok, 0) < 0) {
> +			discard_cache();
>  			return WRITE_TREE_UNMERGED_INDEX;

Also this may not be what the callers want, either.

If your goal is to update the existing code even more reusable by new
callers, I would understand if you introduced a "quiet" mode to
cache_tree_update() codepath (especially, verify_cache()), so that the
callers can choose to re-inspect the index in the error case and give a
better diagnostics, suggestion, or even auto-correction, depending on the
application. That would be a good way to restructure the current API to
give more control to the application.

Running discard_cache() here, however, would forever forbid any such
callers to be written, without reverting this change.

> +		}
>  		if (0 <= newfd) {
>  			if (!write_cache(newfd, active_cache, active_nr) &&
>  			    !commit_lock_file(lock_file))
> @@ -591,8 +593,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  	if (prefix) {
>  		struct cache_tree *subtree =
>  			cache_tree_find(active_cache_tree, prefix);
> -		if (!subtree)
> +		if (!subtree) {
> +			discard_cache();
>  			return WRITE_TREE_PREFIX_ERROR;

Same here.

> +		}
>  		hashcpy(sha1, subtree->sha1);
>  	}
>  	else
> @@ -601,6 +605,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  	if (0 <= newfd)
>  		rollback_lock_file(lock_file);
>  
> +	discard_cache();
>  	return 0;
>  }

I am afraid that I have to say that the approach this patch takes is
totally backwards from the point of view of good API design.  Perhaps
there is an (unstated) other goal you are trying to achieve, but I cannot
read it from the patch.

Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:

> If the read_cache() call succeeds, the function must call
> discard_cache() before returning to the caller.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>

Sorry, I was unclear.  What I meant is this: currently all three
callers to write_cache_as_tree() call die() or exit() if it fails.
When a new caller wants to carry on after failure, that requires some
careful thinking about what the state should be when it resumes.

Toward that end, your patch "[PATCH 1/8] revert: Improve error
handling by..." included

> -		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +		if (write_cache_as_tree(head, 0, NULL)) {
> +			discard_cache();

but it doesn't feel right.  Why after trying to write-tree do we undo
the _reading_ of a tree?  It seemed strange.

So let's think carefully about what the state should be after
write-tree fails.  Does it discard_cache() to make valgrind happier?
Or do we keep the in-core index cached for use in later operations?
Whichever is the right choice would be likely to apply equally well to
"git cherry-pick" and other write_cache_as_tree callers.

I foolishly did not ask "do you really want to discard_cache() here?"
and instead said something to the effect of "if you are going to
discard_cache(), won't that apply equally to all callers?", while the
former is a better question.  Of course this would all be easier if we
had an example to think about that cared about the index state on
error one way or another.

If I understand correctly, the sequencer does not care about the state
at all, and just wants to write a few files under .git and print a
message.  It could do that just as well by keeping the die() and
setting up a die handler.