{"thread":{"id":"7783","subject":"[RFH] plumber's puzzle","startedAt":"2007-04-22T19:53:37Z","lastAt":"2007-04-24T08:36:03Z","messageCount":8,"participants":["Junio C Hamano","Florian Weimer","Linus Torvalds","H. Peter Anvin","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"40148","messageId":"7v647ombi6.fsf@assigned-by-dhcp.cox.net","threadId":"7783","inReplyTo":null,"subject":"[RFH] plumber's puzzle","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-22T19:53:37Z","receivedAt":"2007-04-22T19:53:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I know people had trouble fetching from the official git.git\nrepository this morning; sorry about the gotcha.\n\nNow it has all been hopefully sorted out, here is one thing that\nI have been having trouble with lately, and can use some help.\n\nPlease try this, after getting the latest 'pu':\n\n\t$ git checkout origin/pu\n\t$ make all test-chmtime\n\t$ cd t\n        $ sh ./t0021-filter.sh\n        $ cd trash\n        $ rm -f test.t\n        $ ../../git-checkout-index -f -q -a\n\t$ ../../git-diff\n\nThis shows the single \"diff --git\" header line without anything,\nto show that the path is not stat-clean, but the contents are\nunchanged, which is what is expected.\n\nA bit of background.  test.t has a funny \"rot13\" filter set for\nboth smudge and clean.  So test.t file has \"a b c d e...\", but\nthe actual blob records \"n o p q r...\".  Since we teached git\nabout the filter, git-checkout-index and git-diff do the right\nthing.\n\nNow, go back to the toplevel, comment out \"close(1)\" in\nconvert.c::filter_buffer() around line 236, and rebuild.\n\n\t$ cd ../..\n        $ edit convert.c ;# comment out \"close(1)\"\n        $ make\n        $ cd t/trash\n\nWe get a quite _WRONG_ diff, like this:\n\n        $ ../../git-diff\n        diff --git a/test.t b/test.t\n        --- a/test.t\n        +++ b/test.t\n        @@ -1,3 +1,4 @@\n         n o p q r s t u v w x y z\n         a b c d e f g h i j k l m\n         $vqrag$\n        +diff --git a/test.t b/test.t\n\nand if you disable pager like this:\n\n\t$ PAGER= ../../git-diff\n\nthen you get the expected correct result.  Also you get this if\nwe disable the internal pager:\n\n        $ ../../git-diff | cat\n        diff --git a/test.t b/test.t\n        --- a/test.t\n        +++ b/test.t\n        @@ -1,3 +1,4 @@\n         n o p q r s t u v w x y z\n         a b c d e f g h i j k l m\n         $vqrag$\n        +diff --git a/test.t b/test.t\n\nI do not want to have a \"fix\" that I do not understand *why* it\nfixes things in the tree; I couldn't figure out why that\nclose(1) in that codepath matters, although I was the one who\nadded that line.\n\nInsights?\n"},{"id":"40149","messageId":"87mz10duen.fsf@mid.deneb.enyo.de","threadId":"7783","inReplyTo":"7v647ombi6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFH] plumber's puzzle","fromName":"Florian Weimer","fromEmail":"fw@deneb.enyo.de","sentAt":"2007-04-22T20:30:08Z","receivedAt":"2007-04-22T20:30:08Z","isPatch":false,"sender":{"key":"fw@deneb.enyo.de","avatar":null},"body":"* Junio C. Hamano:\n\n> Insights?\n\n5520  write(1, \"diff --git a/test.t b/test.t\\n\", 29) = -1 EBADF (Bad file descriptor)\n\nIt's not a fix, you just discard the output because you've closed\nstandard output.\n"},{"id":"40150","messageId":"alpine.LFD.0.98.0704221341080.9964@woody.linux-foundation.org","threadId":"7783","inReplyTo":"7v647ombi6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFH] plumber's puzzle","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-22T20:43:02Z","receivedAt":"2007-04-22T20:43:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 22 Apr 2007, Junio C Hamano wrote:\n> \n> This shows the single \"diff --git\" header line without anything,\n> to show that the path is not stat-clean, but the contents are\n> unchanged, which is what is expected.\n\nActually, I think the \"good\" case is the broken one.\n\nDo an \"strace -f\" on the two cases, and you'll see an EBADF in the case \nthat you think is good: the missing output *is* there, it's just that you \nclosed the file descriptor so you don't see it.\n\nSo if the output you want is with the close(1) (ie with the output \ndiscarded), then you have some other bug there.\n\n\t\tLinus\n"},{"id":"40161","messageId":"7vps5wkph5.fsf@assigned-by-dhcp.cox.net","threadId":"7783","inReplyTo":"alpine.LFD.0.98.0704221341080.9964@woody.linux-foundation.org","subject":"Re: [RFH] plumber's puzzle","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-22T22:34:46Z","receivedAt":"2007-04-22T22:34:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 22 Apr 2007, Junio C Hamano wrote:\n>> \n>> This shows the single \"diff --git\" header line without anything,\n>> to show that the path is not stat-clean, but the contents are\n>> unchanged, which is what is expected.\n>\n> Actually, I think the \"good\" case is the broken one.\n>\n> Do an \"strace -f\" on the two cases, and you'll see an EBADF in the case \n> that you think is good: the missing output *is* there, it's just that you \n> closed the file descriptor so you don't see it.\n>\n> So if the output you want is with the close(1) (ie with the output \n> discarded), then you have some other bug there.\n\nI think I figured it out.  The extra EBADF output comes from the\nprocess that calls finish_command() in filter_buffer().\n\nThat is because the caller is diff_flush() which prepares its\noutput using stdio, and when apply_filter -> filter_bufer\ncallchain forks, the unflushed stdout hangs around in the\nchild.  Then we call exit() in apply_filter() to terminate the\nchild we spawned to do the filtering.  It flushes its copy of\nstdio buffer.\n\nYuck.\n\nI should be happy that I figured out what is going on, but I am\nnot very happy with this patch.\n\ndiff --git a/convert.c b/convert.c\nindex 845825b..35bb8cf 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -233,7 +233,6 @@ static int filter_buffer(const char *path, const char *src,\n \t\treturn 1;\n \t}\n \tclose(pipe_feed[0]);\n-\tclose(1);\n \n \twrite_err = (write_in_full(pipe_feed[1], src, size) < 0);\n \tif (close(pipe_feed[1]))\n@@ -273,6 +272,7 @@ static char *apply_filter(const char *path, const char *src,\n \t\treturn NULL;\n \t}\n \n+\tfflush(stdout);\n \tchild_process.pid = fork();\n \tif (child_process.pid < 0) {\n \t\terror(\"cannot fork to run external filter %s\", cmd);\n"},{"id":"40163","messageId":"alpine.LFD.0.98.0704221546160.9964@woody.linux-foundation.org","threadId":"7783","inReplyTo":"7vps5wkph5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFH] plumber's puzzle","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-22T23:03:20Z","receivedAt":"2007-04-22T23:03:20Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 22 Apr 2007, Junio C Hamano wrote:\n> \n> I should be happy that I figured out what is going on, but I am\n> not very happy with this patch.\n\nThat actually looks like the right patch.\n\nThe \"fflush() before fork()\" thing is a real issue, and a real bug. Stdio \nis buffered, and yes, fork() will duplicate the buffer if not flushed.\n\nOf course, I'm not 100% sure that is the right _place_ for the fflush() \ncall. I wonder if we should just do the fflush() closer to the place that \ngenerates the data. As it is, we may have other things like that lurking.\n\nOf course, delaying the fflush as long as possible is likely good for \nperformance, so doing it just before the fork() (even if it may be ugly \nand somewhat unexpected at that point to have to do it) may just be the \nright thing regardless...\n\n\t\tLinus\n"},{"id":"40166","messageId":"7vejmcklv0.fsf@assigned-by-dhcp.cox.net","threadId":"7783","inReplyTo":"alpine.LFD.0.98.0704221546160.9964@woody.linux-foundation.org","subject":"Re: [RFH] plumber's puzzle","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-22T23:52:51Z","receivedAt":"2007-04-22T23:52:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 22 Apr 2007, Junio C Hamano wrote:\n>> \n>> I should be happy that I figured out what is going on, but I am\n>> not very happy with this patch.\n>\n> That actually looks like the right patch.\n>\n> The \"fflush() before fork()\" thing is a real issue, and a real bug. Stdio \n> is buffered, and yes, fork() will duplicate the buffer if not flushed.\n>\n> Of course, I'm not 100% sure that is the right _place_ for the fflush() \n> call. I wonder if we should just do the fflush() closer to the place that \n> generates the data. As it is, we may have other things like that lurking.\n>\n> Of course, delaying the fflush as long as possible is likely good for \n> performance, so doing it just before the fork() (even if it may be ugly \n> and somewhat unexpected at that point to have to do it) may just be the \n> right thing regardless...\n\nAnother possibility I considered is to call low-level _exit so\nthat we bypass not just stdio flushing but also atexit().  But I\nthink \"fflush() before fork()\" is cleaner.\n"},{"id":"40273","messageId":"462D5487.2000601@zytor.com","threadId":"7783","inReplyTo":"alpine.LFD.0.98.0704221546160.9964@woody.linux-foundation.org","subject":"Re: [RFH] plumber's puzzle","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2007-04-24T00:51:19Z","receivedAt":"2007-04-24T00:51:19Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> On Sun, 22 Apr 2007, Junio C Hamano wrote:\n>> I should be happy that I figured out what is going on, but I am\n>> not very happy with this patch.\n> \n> That actually looks like the right patch.\n> \n> The \"fflush() before fork()\" thing is a real issue, and a real bug. Stdio \n> is buffered, and yes, fork() will duplicate the buffer if not flushed.\n> \n> Of course, I'm not 100% sure that is the right _place_ for the fflush() \n> call. I wonder if we should just do the fflush() closer to the place that \n> generates the data. As it is, we may have other things like that lurking.\n> \n> Of course, delaying the fflush as long as possible is likely good for \n> performance, so doing it just before the fork() (even if it may be ugly \n> and somewhat unexpected at that point to have to do it) may just be the \n> right thing regardless...\n> \n\nIt might be worthwhile to have a wrapper function for fork() which adds \nfflush(NULL); before forking?\n\n\t-hpa\n"},{"id":"40307","messageId":"Pine.LNX.4.64.0704241034220.6954@racer.site","threadId":"7783","inReplyTo":"462D5487.2000601@zytor.com","subject":"Re: [RFH] plumber's puzzle","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-04-24T08:36:03Z","receivedAt":"2007-04-24T08:36:03Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 Apr 2007, H. Peter Anvin wrote:\n\n> It might be worthwhile to have a wrapper function for fork() which adds \n> fflush(NULL); before forking?\n\nIt might be worthwhile to have wrapper functions which do much more than \nthat. If only to help portability. FWIW, the MinGW port has some nice code \nwhich I'd like to see back in git.git.\n\nCiao,\nDscho\n"}]}