[RFH] plumber's puzzle

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?

* Junio C. Hamano:

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.

On Sun, 22 Apr 2007, Junio C Hamano wrote:

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

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

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.

On Sun, 22 Apr 2007, Junio C Hamano wrote:

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 <torvalds@linux-foundation.org> writes:

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.

Linus Torvalds wrote:

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