# [RFH] plumber's puzzle 8 messages from 2007-04-22 to 2007-04-24. Participants: Junio C Hamano, Florian Weimer, Linus Torvalds, H. Peter Anvin, Johannes Schindelin. Thread: https://gitlist.dev/t/7783 ## Junio C Hamano, 2007-04-22 19:53 Subject: [RFH] plumber's puzzle Message-ID: <7v647ombi6.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v647ombi6.fsf%40assigned-by-dhcp.cox.net ``` I know people had trouble fetching from the official git.git repository this morning; sorry about the gotcha. Now it has all been hopefully sorted out, here is one thing that I have been having trouble with lately, and can use some help. Please try this, after getting the latest 'pu': $ git checkout origin/pu $ make all test-chmtime $ cd t $ sh ./t0021-filter.sh $ cd trash $ rm -f test.t $ ../../git-checkout-index -f -q -a $ ../../git-diff 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. A bit of background. test.t has a funny "rot13" filter set for both smudge and clean. So test.t file has "a b c d e...", but the actual blob records "n o p q r...". Since we teached git about the filter, git-checkout-index and git-diff do the right thing. Now, go back to the toplevel, comment out "close(1)" in convert.c::filter_buffer() around line 236, and rebuild. $ cd ../.. $ edit convert.c ;# comment out "close(1)" $ make $ cd t/trash We get a quite _WRONG_ diff, like this: $ ../../git-diff diff --git a/test.t b/test.t --- a/test.t +++ b/test.t @@ -1,3 +1,4 @@ n o p q r s t u v w x y z a b c d e f g h i j k l m $vqrag$ +diff --git a/test.t b/test.t and if you disable pager like this: $ PAGER= ../../git-diff then you get the expected correct result. Also you get this if we disable the internal pager: $ ../../git-diff | cat diff --git a/test.t b/test.t --- a/test.t +++ b/test.t @@ -1,3 +1,4 @@ n o p q r s t u v w x y z a b c d e f g h i j k l m $vqrag$ +diff --git a/test.t b/test.t I do not want to have a "fix" that I do not understand *why* it fixes things in the tree; I couldn't figure out why that close(1) in that codepath matters, although I was the one who added that line. Insights? ``` ## Florian Weimer, 2007-04-22 20:30 Subject: Re: [RFH] plumber's puzzle Message-ID: <87mz10duen.fsf@mid.deneb.enyo.de> URL: https://gitlist.dev/e/87mz10duen.fsf%40mid.deneb.enyo.de In-Reply-To: <7v647ombi6.fsf@assigned-by-dhcp.cox.net> ``` * Junio C. Hamano: > Insights? 5520 write(1, "diff --git a/test.t b/test.t\n", 29) = -1 EBADF (Bad file descriptor) It's not a fix, you just discard the output because you've closed standard output. ``` ## Linus Torvalds, 2007-04-22 20:43 Subject: Re: [RFH] plumber's puzzle Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704221341080.9964%40woody.linux-foundation.org In-Reply-To: <7v647ombi6.fsf@assigned-by-dhcp.cox.net> ``` 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. Linus ``` ## Junio C Hamano, 2007-04-22 22:34 Subject: Re: [RFH] plumber's puzzle Message-ID: <7vps5wkph5.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vps5wkph5.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > 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); ``` ## Linus Torvalds, 2007-04-22 23:03 Subject: Re: [RFH] plumber's puzzle Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704221546160.9964%40woody.linux-foundation.org In-Reply-To: <7vps5wkph5.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Junio C Hamano, 2007-04-22 23:52 Subject: Re: [RFH] plumber's puzzle Message-ID: <7vejmcklv0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vejmcklv0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > 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... Another possibility I considered is to call low-level _exit so that we bypass not just stdio flushing but also atexit(). But I think "fflush() before fork()" is cleaner. ``` ## H. Peter Anvin, 2007-04-24 00:51 Subject: Re: [RFH] plumber's puzzle Message-ID: <462D5487.2000601@zytor.com> URL: https://gitlist.dev/e/462D5487.2000601%40zytor.com In-Reply-To: ``` Linus Torvalds wrote: > > 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... > It might be worthwhile to have a wrapper function for fork() which adds fflush(NULL); before forking? -hpa ``` ## Johannes Schindelin, 2007-04-24 08:36 Subject: Re: [RFH] plumber's puzzle Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0704241034220.6954%40racer.site In-Reply-To: <462D5487.2000601@zytor.com> ``` 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 ```