{"thread":{"id":"45131","subject":"[PATCH] mingw: make stderr unbuffered again","startedAt":"2017-02-13T22:34:42Z","lastAt":"2017-02-17T23:49:50Z","messageCount":16,"participants":["Johannes Schindelin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311460","messageId":"c88612da0a62bfcbc3e278296f9d3eb010057071.1487025228.git.johannes.schindelin@gmx.de","threadId":"45131","inReplyTo":null,"subject":"[PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-13T22:34:06Z","receivedAt":"2017-02-13T22:34:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When removing the hack for isatty(), we actually removed more than just\nan isatty() hack: we removed the hack where internal data structures of\nthe MSVC runtime are modified in order to redirect stdout/stderr.\n\nInstead of using that hack (that does not work with newer versions of\nthe runtime, anyway), we replaced it by reopening the respective file\ndescriptors.\n\nWhat we forgot was to mark stderr as unbuffered again.\n\nReported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1\n\n compat/winansi.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 82b89ab1376..793420f9d0d 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n \t */\n \tclose(new_fd);\n \n+\tif (fd == 2)\n+\t\tsetvbuf(stderr, NULL, _IONBF, BUFSIZ);\n \tfd_is_interactive[fd] |= FD_SWAPPED;\n \n \treturn duplicate;\n@@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)\n \t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n+\tif (fd == 2)\n+\t\tsetvbuf(stderr, NULL, _IONBF, BUFSIZ);\n \tfd_is_interactive[fd] |= FD_MSYS;\n }\n \n\nbase-commit: 5588dbffbd61e4906e453808c6ad32f792fea521\n-- \n2.11.1.windows.1\n"},{"id":"311464","messageId":"xmqqlgt9btrv.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"c88612da0a62bfcbc3e278296f9d3eb010057071.1487025228.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T22:39:00Z","receivedAt":"2017-02-13T22:39:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When removing the hack for isatty(), we actually removed more than just\n> an isatty() hack: we removed the hack where internal data structures of\n> the MSVC runtime are modified in order to redirect stdout/stderr.\n>\n> Instead of using that hack (that does not work with newer versions of\n> the runtime, anyway), we replaced it by reopening the respective file\n> descriptors.\n>\n> What we forgot was to mark stderr as unbuffered again.\n>\n> Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1\n\nOK. Should this go directly to 'master', as the isatty thing is\nalready in?\n\n>\n> compat/winansi.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index 82b89ab1376..793420f9d0d 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n> \t */\n> \tclose(new_fd);\n> \n> +\tif (fd == 2)\n> +\t\tsetvbuf(stderr, NULL, _IONBF, BUFSIZ);\n> \tfd_is_interactive[fd] |= FD_SWAPPED;\n> \n> \treturn duplicate;\n> @@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)\n> \t\t\t!wcsstr(name, L\"-pty\"))\n> \t\treturn;\n> \n> +\tif (fd == 2)\n> +\t\tsetvbuf(stderr, NULL, _IONBF, BUFSIZ);\n> \tfd_is_interactive[fd] |= FD_MSYS;\n> }\n> \n>\n> base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521\n"},{"id":"311541","messageId":"alpine.DEB.2.20.1702141545380.3496@virtualbox","threadId":"45131","inReplyTo":"xmqqlgt9btrv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-14T14:47:05Z","receivedAt":"2017-02-14T14:47:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 13 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > When removing the hack for isatty(), we actually removed more than just\n> > an isatty() hack: we removed the hack where internal data structures of\n> > the MSVC runtime are modified in order to redirect stdout/stderr.\n> >\n> > Instead of using that hack (that does not work with newer versions of\n> > the runtime, anyway), we replaced it by reopening the respective file\n> > descriptors.\n> >\n> > What we forgot was to mark stderr as unbuffered again.\n> >\n> > Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1\n> > Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1\n> \n> OK. Should this go directly to 'master', as the isatty thing is\n> already in?\n\nFrom my point of view, it is not crucial. The next Git for Windows version\nwill have it, of course, and Hannes is always running with his set of\npatches, he can easily cherry-pick this one, too.\n\nCiao,\nJohannes\n\nP.S.: Could you please cut the remainder of the mail that you are not\nresponding to? Thanks.\n"},{"id":"311557","messageId":"ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702141545380.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-02-14T18:45:28Z","receivedAt":"2017-02-14T18:45:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:\n> On Mon, 13 Feb 2017, Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>>> When removing the hack for isatty(), we actually removed more than just\n>>> an isatty() hack: we removed the hack where internal data structures of\n>>> the MSVC runtime are modified in order to redirect stdout/stderr.\n>>>\n>>> Instead of using that hack (that does not work with newer versions of\n>>> the runtime, anyway), we replaced it by reopening the respective file\n>>> descriptors.\n>>>\n>>> What we forgot was to mark stderr as unbuffered again.\n\nI do not see how the earlier patch turned stderr from unbuffered to \nbuffered, as it did not add or remove any setvbuf() call. Can you explain?\n\n>> OK. Should this go directly to 'master', as the isatty thing is\n>> already in?\n>\n>>From my point of view, it is not crucial. The next Git for Windows version\n> will have it, of course, and Hannes is always running with his set of\n> patches, he can easily cherry-pick this one, too.\n\nThe patch fixes the problem for me here.\n\nI am a bit hesitant to call it \"not crucial\": The symptom I observed is \ncertainly not a show stopper; but who knows where else it is important \nthat stderr goes to the terminal earlier than other output. As it is a \nnew regression, it should be included in the next release.\n\nThanks,\n-- Hannes\n\n"},{"id":"311560","messageId":"xmqq37fga9rn.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702141545380.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T18:48:44Z","receivedAt":"2017-02-14T18:48:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> OK. Should this go directly to 'master', as the isatty thing is\n>> already in?\n>\n> From my point of view, it is not crucial. The next Git for Windows version\n> will have it, of course, and Hannes is always running with his set of\n> patches, he can easily cherry-pick this one, too.\n\nWhat hat were you wearing when you said the above \"From my point of\nview\"? Were you the \"Git for Windows\" maintainer, or were you a\ncontributor and a member of the Git development community that works\nto improve the upstream? If it was not clear, I was asking the\nquestion to you wearing the latter hat.\n\nTo put it differently, what is our position, as the upstream\ndevelopers, toward those who are on Windows and wants to build from\nthe source? It's not just Hannes.\n\nIs our position \"unless you are extremely competent and are willing\nto cherry-pick missing things from Git for Windows tree as needed,\nwe recommend you to build Git for Windows source instead\"?\n\nIt is inevitable that the upstream lags behind in Windows specific\nchanges. Even though you have been trickling Windows specific\nchanges (both things in compat/ and also changes to the generic\nparts, updating \"c == '/'\" to \"is_dir_sep(c)\") in, and I have been\naccepting them for the past few years, in order to reduce the size\nof the patch pile Git for Windows needs on top of the upstream,\nuntil the patch pile becomes empty, it will always be the case.\n\nSo I won't object if that were our position. I just need to know\nwhat it is to adjust my expectations, and as far as I am concerned,\nyou and Hannes are the two people whose thinking I'd trust regarding\nwhat to do with/to Windows users; even though I keep saying \"our\"\nposition, I am asking yours and Hannes's.\n\nThanks.\n\n"},{"id":"311563","messageId":"xmqqy3x88uqi.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T18:58:45Z","receivedAt":"2017-02-14T18:58:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:\n>\n>>>From my point of view, it is not crucial. The next Git for Windows version\n>> will have it, of course, and Hannes is always running with his set of\n>> patches, he can easily cherry-pick this one, too.\n>\n> The patch fixes the problem for me here.\n>\n> I am a bit hesitant to call it \"not crucial\": The symptom I observed\n> is certainly not a show stopper; but who knows where else it is\n> important that stderr goes to the terminal earlier than other\n> output. As it is a new regression, it should be included in the next\n> release.\n\nI tend to agree with you on the \"not crucial\" bit.\n\nI applied the patch with \"Tested-by: Johannes Sixt \"\non top of 1d3f065e0e (\"mingw: follow-up to \"replace isatty() hack\"\",\n2017-01-18), which was the tip of js/mingw-isatty topic that was\nmerged to both master and maint during this cycle. Unless I hear\nthe \"fix\" turns out to be undesirable in the coming few days, let's\nplan to merge it to 'master' by the final.\n\nThanks.\n\n"},{"id":"311644","messageId":"alpine.DEB.2.20.1702151312330.3496@virtualbox","threadId":"45131","inReplyTo":"ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-15T12:32:39Z","receivedAt":"2017-02-15T12:32:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 14 Feb 2017, Johannes Sixt wrote:\n\n> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:\n> > On Mon, 13 Feb 2017, Junio C Hamano wrote:\n> > > Johannes Schindelin writes:\n> > > > When removing the hack for isatty(), we actually removed more than\n> > > > just an isatty() hack: we removed the hack where internal data\n> > > > structures of the MSVC runtime are modified in order to redirect\n> > > > stdout/stderr.\n> > > >\n> > > > Instead of using that hack (that does not work with newer versions\n> > > > of the runtime, anyway), we replaced it by reopening the\n> > > > respective file descriptors.\n> > > >\n> > > > What we forgot was to mark stderr as unbuffered again.\n> \n> I do not see how the earlier patch turned stderr from unbuffered to\n> buffered, as it did not add or remove any setvbuf() call. Can you\n> explain?\n\nCertainly. I hoped that the commit message would do the job, but then, I\nam under time pressure these days, so it was probably a bit terse.\n\nThe hack we used to make isatty() work used to change data structures\ndirectly that are *internal* to the MSVC runtime. That is, instead of\n*really* redirecting stdout/stderr, we simply changed the target of the\nexisting stdout/stderr and thereby could fool MSVC into believing that it\nis *still* writing to the terminal (because the bit is set) and that it is\n*not* a pipe (because we forcibly unset that bit).\n\nNeedless to say, this meddling with internal data structures is not only\nprone to break with future updates of the MSVC runtime, it is also\ninappropriate because the implementation may rely on certain side effects\n(or not so side effects) that may very well cause crashes or data loss.\nImagine, for example, that the internal data structure were variable-size,\nbased on the HANDLE type. That is totally legitimate for an internal data\nstructure. And if we meddle with the HANDLE, we can easily cause data\ncorruption.\n\nAs GCC is basically tied to using an older version of the MSVC runtime\n(unlike GLIBC, multiple versions of the MSVC runtime can coexist happily,\nmaking it relatively easy to maintain backwards-compatibility, but that\nconcept is foreign to GCC), this used to not be a problem.\n\nHowever, with our recent push to allow developing, building, debugging and\nperformance profiling in Visual Studio, that limitation no longer holds\ntrue: if you develop with Visual Studio 2015, you will link to a newer\nMSVC runtime, and that runtime changed those internal data structures\nrather dramatically.\n\nThat means that we had to come up with a non-hacky way to redirect\nstdout/stderr (e.g. to parse ESC color sequences and apply them to the\nWin32 Console as appropriate) and still have isatty() report what we want\nit to report. That is, if we redirect stdout/stderr to a pipe that parses\nthe color sequences for use in the Win32 Console, we want isatty() to\nreport that we are writing to a Terminal Typewriter (a charming\nanachronism, isn't it?).\n\nMy colleague Jeff Hostetler worked on this and figured out that the only\nway to do this properly is to wrap isatty() and override it via a #define,\nand simply remember when stdout/stderr referred to a terminal before\nredirecting, say, to that pipe.\n\nAs my famously broken patch to override isatty() (so that /dev/null would\nnot be treated as a terminal) *already* overrode isatty() with a custom\nwrapper, and as it was broken and needed fixing, I decided to reconcile\nthe two approaches into what is now the version in `master`.\n\nSo instead of \"bending\" the target HANDLE of the existing stdout/stderr\n(which would *naturally* have kept the buffered/unbuffered nature as-is),\nwe now redirect with correct API calls. And the patch I provided at the\nbottom of this mail thread reinstates the unbuffered nature of stderr now\nthat it gets reopened.\n\nHopefully that makes it clear why the setvbuf() call is required now, but\nwas previously unnecessary?\n\nCiao,\nDscho\n"},{"id":"311645","messageId":"alpine.DEB.2.20.1702151332540.3496@virtualbox","threadId":"45131","inReplyTo":"xmqq37fga9rn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-15T12:48:46Z","receivedAt":"2017-02-15T12:49:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 14 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> OK. Should this go directly to 'master', as the isatty thing is\n> >> already in?\n> >\n> > From my point of view, it is not crucial. The next Git for Windows\n> > version will have it, of course, and Hannes is always running with his\n> > set of patches, he can easily cherry-pick this one, too.\n> \n> What hat were you wearing when you said the above \"From my point of\n> view\"?\n\nThe hat of a person who sees how patches are reviewed before they enter\npu/next/master/maint of git.git.\n\nIf that review had anything to do with Windows, and refused to accept\npatches unless they work correctly on Windows, I would agree that it is a\nwise idea to fast-track important fixes for Windows.\n\nBut that is not the case. Quite often `pu`, sometimes `next`, and rarely\neven `master` are regularly broken on Windows.\n\nThe only branch that is tested very stringently on Windows, and into which\nnothing is allowed that breaks on Windows, is git-for-windows/git's\n`master` branch.\n\nBTW this is not just an opinion, this is just an account of the current\nstate.\n\nOnce you accept that this is reality, you will understand why I *dared* to\nsay that a criticial Windows-specific fix needs to be fast-tracked to\ngit-for-windows/git's `master`, but not into git.git's `master` branch.\n\nFWIW I wish it were different, that git.git's `master` reflected more\nclosely what the current Git for Windows version has. If you are\nattentive, you will have noticed that I continuously work toward that\ngoal. I frequently \"upstream patches\" from Git for Windows[*1*], even if\nit seems that the influx of new patches is higher than the rate of patches\nthat make it into git.git's `master`. And even if I am often asked to\nchange these patches so much that it is virtually guaranteed that they\nregress (hence my recently increasing reluctance to accept each and every\nreviewer's suggestions as \"must implement\").\n\nCiao,\nJohannes\n\nFootnote *1*: Yes, I even \"upstream patches\" from developers other than\nmyself, trying to shield contributors from having to send their patches as\nmails and to cope with the reviewers' suggestions that may, or may not,\nmake sense. This makes my life harder, but I believe that the alternative\nwould be *not* to have those patches in git.git *at all*.\n"},{"id":"311670","messageId":"0275af7b-eb7a-1094-a891-674300175e56@kdbg.org","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702151312330.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-02-15T20:45:22Z","receivedAt":"2017-02-15T20:45:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:\n> On Tue, 14 Feb 2017, Johannes Sixt wrote:\n>> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:\n>>> On Mon, 13 Feb 2017, Junio C Hamano wrote:\n>>>> Johannes Schindelin writes:\n>>>>> What we forgot was to mark stderr as unbuffered again.\n>>\n>> I do not see how the earlier patch turned stderr from unbuffered to\n>> buffered, as it did not add or remove any setvbuf() call. Can you\n>> explain?\n>\n> [ motivation and history of js/mingw-isatty snipped ]\n>\n> So instead of \"bending\" the target HANDLE of the existing stdout/stderr\n> (which would *naturally* have kept the buffered/unbuffered nature as-is),\n> we now redirect with correct API calls.\n\nYour statement implies that at the time when winansi_init() begins, \nstdio is already initialized and the buffered/unbuffered state has been \nset for stderr. I would think that this is true.\n\nThen we swap out the file handle underlying stderr in swap_osfhnd() \nusing dup2(). Why would that change the buffered state of stdio?\n\n> And the patch I provided at the\n> bottom of this mail thread reinstates the unbuffered nature of stderr now\n> that it gets reopened.\n>\n> Hopefully that makes it clear why the setvbuf() call is required now, but\n> was previously unnecessary?\n\nUnfortunately, no. I do not see how dup2() causes a change in stdio \nstate. I must be missing something (and that may be a basic \nmisunderstanding of how stdio is initialized).\n\n-- Hannes\n\n"},{"id":"311688","messageId":"xmqqbmu32iyb.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702151332540.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T22:22:04Z","receivedAt":"2017-02-15T22:22:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n> ...\n> The hat of a person who sees how patches are reviewed before they enter\n> pu/next/master/maint of git.git.\n> ...\n> make sense. This makes my life harder, but I believe that the alternative\n> would be *not* to have those patches in git.git *at all*.\n\nYou wrote a lot, what you wrote were readable, and perhaps were good\nmaterial to support your answer/conclusion, but you forgot to answer\na simple question I asked. I think your description of where things\ncurrently are would support any of the possible answers below, and I\ncannot guess which one it would be.\n\nThe question was:\n\n Is our position \"unless you are extremely competent and are willing\n to cherry-pick missing things from Git for Windows tree as needed,\n we recommend you to build Git for Windows source instead\"?\n\nOr is it \"please ignore upstream and work off of Git for Windows?\"\n\nOr is it \"please try to work with upstream and if you find Windows\nspecific glitches, after checking to see if Git for Windows has\nalready a fix for it and otherwise feed your fix to Git for Windows,\nso that we can upstream your fix for you, together with changes from\nothers\"?\n\nOr is it \"please try to work with upstream and if you find Windows\nspecific glitches, after checking to see if Git for Windows has\nalready a fix for it and otherwise upstream your fix, so that next\npull from upstream into Git for Windows would have your fix\"?\n\nOr is it something else?\n\n\n> FWIW I wish it were different, that git.git's `master` reflected more\n> closely what the current Git for Windows version has.\n\nWell, we two wishing the same thing together without doing anything\nelse would make it happen.\n\nAs an experiment to see if our process can be improved, I've been\nmeaning to suggest (which is what was behind my \"question at a bit\nhigher level\" to Hannes [*1*]) asking you to throw me occasional\npull requests for changes that are only about Windows specific\nissues, bypassing \"patches on the list\" for things like a hotfix to\njs/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],\nessentially treating Windows specific changes as \"a sub-maintainer\nmakes pull requests\" we already do with Paul, Eric and Pat.\n\nInterested parties would instead see only the pull request sent by\nyou to me, either on the list or directly CC'ed to them, and do\ntheir own fetch to assess if it is a good idea for me to actually\npull. Suggestions to the changes from bystanders like Peff's\ncomment [*4*] may need to reproduce the patch text when sent to the\nlist, which would burden reviewers a bit more, but they still are\npossible.\n\nSuch a pull-request workflow would either hide the communication\nfrom the list or force people to go to multiple places (i.e. both to\nthe list and to GitHub comments) in order to view the whole picture,\nand I am still reluctant to extend it to other areas (e.g. a change\nthat adds \"#if WINDOWS\" to a more generic codepath), but a trial on\na limited scope may give us a better feel of how well such an\nupdated workflow would work for us.\n\n\n[References]\n\n*1* \n\n*2* \n\n*3* <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>\n\n*4* <20170210050237.gajicliueuvk6s5d@sigill.intra.peff.net>\n"},{"id":"311702","messageId":"xmqqlgt7110q.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"xmqqbmu32iyb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T23:34:45Z","receivedAt":"2017-02-15T23:35:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> FWIW I wish it were different, that git.git's `master` reflected more\n>> closely what the current Git for Windows version has.\n>\n> Well, we two wishing the same thing together without doing anything\n> else would make it happen.\n\nehh, would *not* make it happen, of course.\n\n> As an experiment to see if our process can be improved, I've been\n> meaning to suggest (which is what was behind my \"question at a bit\n> higher level\" to Hannes [*1*]) asking you to throw me occasional\n> pull requests for changes that are only about Windows specific\n> issues, bypassing \"patches on the list\" for things like a hotfix to\n> js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],\n> essentially treating Windows specific changes as \"a sub-maintainer\n> makes pull requests\" we already do with Paul, Eric and Pat.\n\nWhile this may ease the flow of upstreaming windows specific\nchanges, we need a separate thing to address the on-going issue you\nraised in your message. A Windows-less person would not know his\nchange to a generic code that is innocuous-looking has fallouts on\nWindows (read this sentence with \"Windows\" replaced with any\nspecific platform name). When somebody writes c == '/' that should\nhave been written as is_dir_sep(c), you or Hannes often finds it\nduring the review here, and after repeatedly seeing such reviews,\nthat (slowly) rubs off on other Window-less folks. A new code may\nstill hit 'next' and 'master' with such an issue if it goes\nunnoticed during the review.\n\nThe CI you are setting up [*1*] may certainly be a step in the good\ndirection. Having more people like Hannes working off of upstream\nmay also be a great way to help the \"forget 'next' and upstream in\ngeneral\" issue. Any other ideas?\n\n"},{"id":"311776","messageId":"alpine.DEB.2.20.1702161753050.3496@virtualbox","threadId":"45131","inReplyTo":"0275af7b-eb7a-1094-a891-674300175e56@kdbg.org","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-16T17:10:02Z","receivedAt":"2017-02-16T17:10:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Wed, 15 Feb 2017, Johannes Sixt wrote:\n\n> Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:\n> > On Tue, 14 Feb 2017, Johannes Sixt wrote:\n> > > Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:\n> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:\n> > > > > Johannes Schindelin writes:\n> > > > > > What we forgot was to mark stderr as unbuffered again.\n> > >\n> > > I do not see how the earlier patch turned stderr from unbuffered to\n> > > buffered, as it did not add or remove any setvbuf() call. Can you\n> > > explain?\n> >\n> > [ motivation and history of js/mingw-isatty snipped ]\n> >\n> > So instead of \"bending\" the target HANDLE of the existing\n> > stdout/stderr (which would *naturally* have kept the\n> > buffered/unbuffered nature as-is), we now redirect with correct API\n> > calls.\n> \n> Your statement implies that at the time when winansi_init() begins,\n> stdio is already initialized and the buffered/unbuffered state has been\n> set for stderr. I would think that this is true.\n> \n> Then we swap out the file handle underlying stderr in swap_osfhnd()\n> using dup2(). Why would that change the buffered state of stdio?\n\nThe file handle we swap in for stderr points to the pipe that a\nfreshly-started thread consumes for parsing the ANSI color sequences. This\nhandle is used both for stdout and stderr. The dup2() call then implicitly\nreopens stderr, with the default buffering.\n\n> > And the patch I provided at the bottom of this mail thread reinstates\n> > the unbuffered nature of stderr now that it gets reopened.\n> >\n> > Hopefully that makes it clear why the setvbuf() call is required now,\n> > but was previously unnecessary?\n> \n> Unfortunately, no. I do not see how dup2() causes a change in stdio state. I\n> must be missing something (and that may be a basic misunderstanding of how\n> stdio is initialized).\n\nIt appears that dup2()ing fd 2 resets that stdio state.\n\nCiao,\nDscho\n"},{"id":"311782","messageId":"d1c65a34-3809-ee76-5e00-69ba715e0093@kdbg.org","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702161753050.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-02-16T17:55:41Z","receivedAt":"2017-02-16T17:56:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:\n> On Wed, 15 Feb 2017, Johannes Sixt wrote:\n>> I do not see how dup2() causes a change in stdio state. I\n>> must be missing something (and that may be a basic misunderstanding of how\n>> stdio is initialized).\n>\n> It appears that dup2()ing fd 2 resets that stdio state.\n\nOK, thanks. It's surprising, but so be it.\n\nThank you for the quick fix!\n\n-- Hannes\n\n"},{"id":"311785","messageId":"xmqq1suyypyv.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"d1c65a34-3809-ee76-5e00-69ba715e0093@kdbg.org","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-16T18:01:44Z","receivedAt":"2017-02-16T18:01:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:\n>> On Wed, 15 Feb 2017, Johannes Sixt wrote:\n>>> I do not see how dup2() causes a change in stdio state. I\n>>> must be missing something (and that may be a basic misunderstanding of how\n>>> stdio is initialized).\n>>\n>> It appears that dup2()ing fd 2 resets that stdio state.\n>\n> OK, thanks. It's surprising, but so be it.\n>\n> Thank you for the quick fix!\n\nI am happy to see two Windows folks being happy. I tentatively\nmarked this as \"undecided\" after merging it to 'next', but let's\nhave it in the -rc2 and final. Two people familiar with Windows\nagreeing that there is no longer mystery why it fixes, after seeing\nthat the update fixes a regression, is a strong enough sign to me\nthat including it is better than leaving it out.\n\nThanks.\n"},{"id":"311897","messageId":"alpine.DEB.2.20.1702171626160.3496@virtualbox","threadId":"45131","inReplyTo":"xmqqlgt7110q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:00:36Z","receivedAt":"2017-02-17T16:00:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 15 Feb 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >> FWIW I wish it were different, that git.git's `master` reflected more\n> >> closely what the current Git for Windows version has.\n> >\n> > Well, we two wishing the same thing together without doing anything\n> > else would make it happen.\n> \n> ehh, would *not* make it happen, of course.\n\nThat is the reason why set aside a substantial part of my maintainer time\nto upstream those patches. You may not see how much time it costs me, say,\nto get the putty-w-args stuff upstream, but rest assured that I would not\nbe able to do this were it not for a company paying me to do exactly that.\n\n> > As an experiment to see if our process can be improved, I've been\n> > meaning to suggest (which is what was behind my \"question at a bit\n> > higher level\" to Hannes [*1*]) asking you to throw me occasional\n> > pull requests for changes that are only about Windows specific\n> > issues, bypassing \"patches on the list\" for things like a hotfix to\n> > js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],\n> > essentially treating Windows specific changes as \"a sub-maintainer\n> > makes pull requests\" we already do with Paul, Eric and Pat.\n\nThe problem here is that Git for Windows is not a subsystem. It touches\npretty much all of Git. That is very different from the Tcl/Tk code, or\nfrom git-svn (whose code is not shared by anything else in Git's source\ncode, despite the fact that it is written in Perl).\n\nAnd even if you were to accept the occasional Pull Request from me, it\nwould not solve the even bigger problem that we essentially reject so much\nexpertise out there, so many potential contributions by very competent\ndevelopers who just have no desire to fight the contribution process.\n\n> While this may ease the flow of upstreaming windows specific\n> changes, we need a separate thing to address the on-going issue you\n> raised in your message. A Windows-less person would not know his\n\n... or her...\n\n> change to a generic code that is innocuous-looking has fallouts on\n> Windows (read this sentence with \"Windows\" replaced with any\n> specific platform name). When somebody writes c == '/' that should\n> have been written as is_dir_sep(c), you or Hannes often finds it\n> during the review here, and after repeatedly seeing such reviews,\n> that (slowly) rubs off on other Window-less folks. A new code may\n> still hit 'next' and 'master' with such an issue if it goes\n> unnoticed during the review.\n\nApart from the fact that we have no prayer at coming up with a system that\nkeeps track of open issues (because we do not use any tracker, but instead\nrely on people to remember that some thing still may not have been\naddressed), there is a different problem here: you stated very explicitly\nthat it is okay for `pu` to be broken [*1*]. If it were different, if any\nbreakage would imply that a fix is really, really required lest the patch\nseries be evicted from `pu`, we could easily modify my Continuous Testing\nsetup to report more visibly what is broken. But since it is okay for `pu`\nto be broken, that would just annoy everybody and people would learn to\nignore/procmail those reports.\n\n> The CI you are setting up [*1*]\n\nwhich *1*?\n\nAnd... I already set it up. I just did not bother to make it more public\nbecause the builds were broken more often than not. IIRC the entire month\nof October was a solid red.\n\n> may certainly be a step in the good direction. Having more people like\n> Hannes working off of upstream may also be a great way to help the\n> \"forget 'next' and upstream in general\" issue. Any other ideas?\n\nThe Continuous Integration is actually not so much a Continuous\nIntegration as it is a Continuous Testing. If it became more of a CI, that\nwould certainly reduce the impression that Git's bleeding edge only ever\nworks on Linux.\n\nCiao,\nJohannes\n\nFootnote *1*: You probably read between the lines that this is\nunfortunate, in my opinion. It sets the mood that lets experimental (if\nuseful) features such as worktrees be broken for the better part of a\nyear.\n"},{"id":"311991","messageId":"xmqqvas8nzse.fsf@gitster.mtv.corp.google.com","threadId":"45131","inReplyTo":"alpine.DEB.2.20.1702171626160.3496@virtualbox","subject":"Re: [PATCH] mingw: make stderr unbuffered again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T23:49:37Z","receivedAt":"2017-02-17T23:49:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> ... there is a different problem here: you stated very explicitly\n> that it is okay for `pu` to be broken [*1*]. If it were different, if any\n> breakage would imply that a fix is really, really required lest the patch\n> series be evicted from `pu`, we could easily modify my Continuous Testing\n> setup to report more visibly what is broken.\n\nI think you misread me.\n\nRegarding 'pu', there is a chicken-and-egg problem involved. Most\nof the days, the tip of 'pu' passes the test at least for me.\n\nAt the end of day's integration cycle, I may find that 'pu' does not\npass the test for me [*1*]. It is not unusual to see new multiple\ntopics come to 'pu', each of which may test well in isolation but\nhave hidden interactions with topics already in 'next' or 'pu' that\nare exposed only when merged to 'pu', and it may be too late in the\nday for me to find which one of these new topics is problematic.\n\nI could choose not to merge any of them and push 'pu' out with no\nnew topics. Or I may choose to push it out anyway, so that other\npeople who are working during the remainder of 24-hour in different\ntimezones can notice and find which new topic is broken [*2*].\n\nAnd that is why I explicitly say that 'pu' may not even build. It\nshouldn't be taken as a discouragement to people from looking at it,\nwhich seems to be how I read you to misinterpret it. It is the\nopposite---a breakage at the tip of 'pu' is an invitation for people\nto help.\n\nIf nobody steps up and says \"the tip of 'pu' does not build and that\nis because of this topic\", I'd be irritated enough to find which\ntopic is broken myself and then ask the author of the offencing\ntopic for help. If the topic is left broken after that, it will be\nejected from 'pu', because I cannot use 'pu' for helping to polish\nother new topics that wants to cook there unless I do so.\n\nOf course there are platform- or environment-dependent breakages\nthat would not cause my tests to fail [*3*]. Again, people with\ndifferent environment can step up to help, if these topics are\nincluded in 'pu'. You would recall that you called out one topic\n[*4*] that did not build in your environment and the topic was\nejected after your message (but not before---there wasn't a way for\nWindow-less folks to tell that it was broken IIRC) from 'pu'.\n\nIn a near-by thread, somebody wondered if we can add an integration\nbranch 'pr' that is beyond 'pu' and includes everything ever posted\nto the list, and I explained why I won't have time for that. But I\nthink the spirit of suggested 'pr' is what 'pu' already is---it is a\ncollection of promising topics that can be merged into a seemingly\nconsistent whole, which may or may not build and the purpose of the\nbranch is to contain them in one place so that people can find which\nones need unbreaking. Testing the tip of it alone and complaining\nthat it is broken does not help much, but figuiring out which topic\nmerged to it is broken does, by unblocking other topics in 'pu'.\n\n\n[Footnote]\n\n*1* If any other integration branch (including 'jch', which is\n somewhere between 'master' and 'pu' and beyond the commit with a\n tree that matches 'next' aka \"pu^{/^### match next}\") does not\n build, I pull overtime ;-)\n\n\n*2* When there is only a new topic that breaks 'pu' for me, it is\n easy to decide not to queue it and send a note to the author of\n the offending topic, and that does happen a lot, but not always.\n\n*3* I do not have p4 and I may not have svn installed so my tests\n may not even cover them.\n\n*4* I think it was nd/worktree-move but may have been another topic.\n"}]}