Linus Torvalds [off-list ref] writes:
On Sun, 22 Apr 2007, Junio C Hamano wrote:
quoted
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);
On Sun, 22 Apr 2007, Junio C Hamano wrote:
I should be happy that I figured out what is going on, but I am
not very happy with this patch.
That actually looks like the right patch.
The "fflush() before fork()" thing is a real issue, and a real bug. Stdio
is buffered, and yes, fork() will duplicate the buffer if not flushed.
Of course, I'm not 100% sure that is the right _place_ for the fflush()
call. I wonder if we should just do the fflush() closer to the place that
generates the data. As it is, we may have other things like that lurking.
Of course, delaying the fflush as long as possible is likely good for
performance, so doing it just before the fork() (even if it may be ugly
and somewhat unexpected at that point to have to do it) may just be the
right thing regardless...
Linus
Linus Torvalds wrote:
On Sun, 22 Apr 2007, Junio C Hamano wrote:
quoted
I should be happy that I figured out what is going on, but I am
not very happy with this patch.
That actually looks like the right patch.
The "fflush() before fork()" thing is a real issue, and a real bug. Stdio
is buffered, and yes, fork() will duplicate the buffer if not flushed.
Of course, I'm not 100% sure that is the right _place_ for the fflush()
call. I wonder if we should just do the fflush() closer to the place that
generates the data. As it is, we may have other things like that lurking.
Of course, delaying the fflush as long as possible is likely good for
performance, so doing it just before the fork() (even if it may be ugly
and somewhat unexpected at that point to have to do it) may just be the
right thing regardless...
It might be worthwhile to have a wrapper function for fork() which adds
fflush(NULL); before forking?
-hpa
Hi,
On Mon, 23 Apr 2007, H. Peter Anvin wrote:
It might be worthwhile to have a wrapper function for fork() which adds
fflush(NULL); before forking?
It might be worthwhile to have wrapper functions which do much more than
that. If only to help portability. FWIW, the MinGW port has some nice code
which I'd like to see back in git.git.
Ciao,
Dscho