[RFH] plumber's puzzle

[RFH] plumber's puzzle

[Junio C Hamano]

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?

Re: [RFH] plumber's puzzle

[Florian Weimer]

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

Re: [RFH] plumber's puzzle

[Linus Torvalds]

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

Re: [RFH] plumber's puzzle

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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);

Re: [RFH] plumber's puzzle

[Linus Torvalds]

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

Re: [RFH] plumber's puzzle

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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.

Re: [RFH] plumber's puzzle

[H. Peter Anvin]

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

Re: [RFH] plumber's puzzle

[Johannes Schindelin]

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