Re: [RFH] plumber's puzzle

From: Junio C Hamano <junkio@cox.net>
Date: Apr 22, 2007, 22:34 UTC
Message-ID: <7vps5wkph5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <alpine.LFD.0.98.0704221341080.9964@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 14 quoted lines

> On Sun, 22 Apr 2007, Junio C Hamano wrote:
>> 
>> This shows the single "diff --git" header line without anything,
>> to show that the path is not stat-clean, but the contents are
>> unchanged, which is what is expected.
>
> Actually, I think the "good" case is the broken one.
>
> Do an "strace -f" on the two cases, and you'll see an EBADF in the case 
> that you think is good: the missing output *is* there, it's just that you 
> closed the file descriptor so you don't see it.
>
> So if the output you want is with the close(1) (ie with the output 
> discarded), then you have some other bug there.

I think I figured it out. The extra EBADF output comes from the process that calls finish_command() in filter_buffer().

That is because the caller is diff_flush() which prepares its output using stdio, and when apply_filter -> filter_bufer callchain forks, the unflushed stdout hangs around in the child. Then we call exit() in apply_filter() to terminate the child we spawned to do the filtering. It flushes its copy of stdio buffer.

Yuck.

I should be happy that I figured out what is going on, but I am not very happy with this patch.

diff --git a/convert.c b/convert.c
index 845825b..35bb8cf 100644
--- a/convert.c
+++ b/convert.c
@@ -233,7 +233,6 @@ static int filter_buffer(const char *path, const char *src,
 		return 1;
 	}
 	close(pipe_feed[0]);
-	close(1);
 
 	write_err = (write_in_full(pipe_feed[1], src, size) < 0);
 	if (close(pipe_feed[1]))
@@ -273,6 +272,7 @@ static char *apply_filter(const char *path, const char *src,
 		return NULL;
 	}
 
+	fflush(stdout);
 	child_process.pid = fork();
 	if (child_process.pid < 0) {
 		error("cannot fork to run external filter %s", cmd);