[PATCH] diff: fix textconv error zombies

[Johannes Sixt <j6t@kdbg.org>]

To make the code simpler, run_textconv lumps all of its
error checking into one conditional. However, the
short-circuit means that an error in reading will prevent us
from calling finish_command, leaving a zombie child.
Clean up properly after errors.

Based-on-work-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Jeff,

> Yes, there are clever ways to make this shorter, but I think it is
> clearer just written out.

Thanks for your work, but I'm worried that in your version the close()
call is not before the finish_command (but that's really not _that_
important in this case). My version is perhaps not as easy to read, but
has a slightly better diff-stat.

Oh, I removed the error messages after start_command and finish_command,
because these two print one themselves; Junio, if you disagree with
this change, ditch my version and take Jeff's.

-- Hannes

 diff.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index dfdfa1a..b96faea 100644
--- a/diff.c
+++ b/diff.c
@@ -3874,6 +3874,7 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	const char **arg = argv;
 	struct child_process child;
 	struct strbuf buf = STRBUF_INIT;
+	int err = 0;
 
 	temp = prepare_temp_file(spec->path, spec);
 	*arg++ = pgm;
@@ -3884,16 +3885,20 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	child.use_shell = 1;
 	child.argv = argv;
 	child.out = -1;
-	if (start_command(&child) != 0 ||
-	    strbuf_read(&buf, child.out, 0) < 0 ||
-	    finish_command(&child) != 0) {
-		close(child.out);
-		strbuf_release(&buf);
+	if (start_command(&child)) {
 		remove_tempfile();
-		error("error running textconv command '%s'", pgm);
 		return NULL;
 	}
+
+	if (strbuf_read(&buf, child.out, 0) < 0)
+		err = error("error reading from textconv command '%s'", pgm);
 	close(child.out);
+
+	if (finish_command(&child) || err) {
+		strbuf_release(&buf);
+		remove_tempfile();
+		return NULL;
+	}
 	remove_tempfile();
 
 	return strbuf_detach(&buf, outsize);
-- 
1.7.0.2.250.ge5578