[PATCH] mingw: make stderr unbuffered again

[PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.

Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.

What we forgot was to mark stderr as unbuffered again.

Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

 compat/winansi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/compat/winansi.c b/compat/winansi.c
index 82b89ab1376..793420f9d0d 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 	 */
 	close(new_fd);
 
+	if (fd == 2)
+		setvbuf(stderr, NULL, _IONBF, BUFSIZ);
 	fd_is_interactive[fd] |= FD_SWAPPED;
 
 	return duplicate;
@@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
 			!wcsstr(name, L"-pty"))
 		return;
 
+	if (fd == 2)
+		setvbuf(stderr, NULL, _IONBF, BUFSIZ);
 	fd_is_interactive[fd] |= FD_MSYS;
 }
 

base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521
-- 
2.11.1.windows.1

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When removing the hack for isatty(), we actually removed more than just
> an isatty() hack: we removed the hack where internal data structures of
> the MSVC runtime are modified in order to redirect stdout/stderr.
>
> Instead of using that hack (that does not work with newer versions of
> the runtime, anyway), we replaced it by reopening the respective file
> descriptors.
>
> What we forgot was to mark stderr as unbuffered again.
>
> Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1

OK.  Should this go directly to 'master', as the isatty thing is
already in?

>
>  compat/winansi.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 82b89ab1376..793420f9d0d 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>  	 */
>  	close(new_fd);
>  
> +	if (fd == 2)
> +		setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>  	fd_is_interactive[fd] |= FD_SWAPPED;
>  
>  	return duplicate;
> @@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
>  			!wcsstr(name, L"-pty"))
>  		return;
>  
> +	if (fd == 2)
> +		setvbuf(stderr, NULL, _IONBF, BUFSIZ);
>  	fd_is_interactive[fd] |= FD_MSYS;
>  }
>  
>
> base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Junio,

On Mon, 13 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > When removing the hack for isatty(), we actually removed more than just
> > an isatty() hack: we removed the hack where internal data structures of
> > the MSVC runtime are modified in order to redirect stdout/stderr.
> >
> > Instead of using that hack (that does not work with newer versions of
> > the runtime, anyway), we replaced it by reopening the respective file
> > descriptors.
> >
> > What we forgot was to mark stderr as unbuffered again.
> >
> > Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
> > Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1
> 
> OK.  Should this go directly to 'master', as the isatty thing is
> already in?

From my point of view, it is not crucial. The next Git for Windows version
will have it, of course, and Hannes is always running with his set of
patches, he can easily cherry-pick this one, too.

Ciao,
Johannes

P.S.: Could you please cut the remainder of the mail that you are not
responding to? Thanks.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Sixt]

Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> On Mon, 13 Feb 2017, Junio C Hamano wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>> When removing the hack for isatty(), we actually removed more than just
>>> an isatty() hack: we removed the hack where internal data structures of
>>> the MSVC runtime are modified in order to redirect stdout/stderr.
>>>
>>> Instead of using that hack (that does not work with newer versions of
>>> the runtime, anyway), we replaced it by reopening the respective file
>>> descriptors.
>>>
>>> What we forgot was to mark stderr as unbuffered again.

I do not see how the earlier patch turned stderr from unbuffered to 
buffered, as it did not add or remove any setvbuf() call. Can you explain?

>> OK.  Should this go directly to 'master', as the isatty thing is
>> already in?
>
>>From my point of view, it is not crucial. The next Git for Windows version
> will have it, of course, and Hannes is always running with his set of
> patches, he can easily cherry-pick this one, too.

The patch fixes the problem for me here.

I am a bit hesitant to call it "not crucial": The symptom I observed is 
certainly not a show stopper; but who knows where else it is important 
that stderr goes to the terminal earlier than other output. As it is a 
new regression, it should be included in the next release.

Thanks,
-- Hannes

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> OK.  Should this go directly to 'master', as the isatty thing is
>> already in?
>
> From my point of view, it is not crucial. The next Git for Windows version
> will have it, of course, and Hannes is always running with his set of
> patches, he can easily cherry-pick this one, too.

What hat were you wearing when you said the above "From my point of
view"?  Were you the "Git for Windows" maintainer, or were you a
contributor and a member of the Git development community that works
to improve the upstream?  If it was not clear, I was asking the
question to you wearing the latter hat.

To put it differently, what is our position, as the upstream
developers, toward those who are on Windows and wants to build from
the source?  It's not just Hannes.

Is our position "unless you are extremely competent and are willing
to cherry-pick missing things from Git for Windows tree as needed,
we recommend you to build Git for Windows source instead"?

It is inevitable that the upstream lags behind in Windows specific
changes.  Even though you have been trickling Windows specific
changes (both things in compat/ and also changes to the generic
parts, updating "c == '/'" to "is_dir_sep(c)") in, and I have been
accepting them for the past few years, in order to reduce the size
of the patch pile Git for Windows needs on top of the upstream,
until the patch pile becomes empty, it will always be the case.

So I won't object if that were our position.  I just need to know
what it is to adjust my expectations, and as far as I am concerned,
you and Hannes are the two people whose thinking I'd trust regarding
what to do with/to Windows users; even though I keep saying "our"
position, I am asking yours and Hannes's.

Thanks.

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
>
>>>From my point of view, it is not crucial. The next Git for Windows version
>> will have it, of course, and Hannes is always running with his set of
>> patches, he can easily cherry-pick this one, too.
>
> The patch fixes the problem for me here.
>
> I am a bit hesitant to call it "not crucial": The symptom I observed
> is certainly not a show stopper; but who knows where else it is
> important that stderr goes to the terminal earlier than other
> output. As it is a new regression, it should be included in the next
> release.

I tend to agree with you on the "not crucial" bit.

I applied the patch with "Tested-by: Johannes Sixt <j6t@kdbg.org>"
on top of 1d3f065e0e ("mingw: follow-up to "replace isatty() hack"",
2017-01-18), which was the tip of js/mingw-isatty topic that was
merged to both master and maint during this cycle.  Unless I hear
the "fix" turns out to be undesirable in the coming few days, let's
plan to merge it to 'master' by the final.

Thanks.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Hannes,

On Tue, 14 Feb 2017, Johannes Sixt wrote:

> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > When removing the hack for isatty(), we actually removed more than
> > > > just an isatty() hack: we removed the hack where internal data
> > > > structures of the MSVC runtime are modified in order to redirect
> > > > stdout/stderr.
> > > >
> > > > Instead of using that hack (that does not work with newer versions
> > > > of the runtime, anyway), we replaced it by reopening the
> > > > respective file descriptors.
> > > >
> > > > What we forgot was to mark stderr as unbuffered again.
> 
> I do not see how the earlier patch turned stderr from unbuffered to
> buffered, as it did not add or remove any setvbuf() call. Can you
> explain?

Certainly. I hoped that the commit message would do the job, but then, I
am under time pressure these days, so it was probably a bit terse.

The hack we used to make isatty() work used to change data structures
directly that are *internal* to the MSVC runtime. That is, instead of
*really* redirecting stdout/stderr, we simply changed the target of the
existing stdout/stderr and thereby could fool MSVC into believing that it
is *still* writing to the terminal (because the bit is set) and that it is
*not* a pipe (because we forcibly unset that bit).

Needless to say, this meddling with internal data structures is not only
prone to break with future updates of the MSVC runtime, it is also
inappropriate because the implementation may rely on certain side effects
(or not so side effects) that may very well cause crashes or data loss.
Imagine, for example, that the internal data structure were variable-size,
based on the HANDLE type. That is totally legitimate for an internal data
structure. And if we meddle with the HANDLE, we can easily cause data
corruption.

As GCC is basically tied to using an older version of the MSVC runtime
(unlike GLIBC, multiple versions of the MSVC runtime can coexist happily,
making it relatively easy to maintain backwards-compatibility, but that
concept is foreign to GCC), this used to not be a problem.

However, with our recent push to allow developing, building, debugging and
performance profiling in Visual Studio, that limitation no longer holds
true: if you develop with Visual Studio 2015, you will link to a newer
MSVC runtime, and that runtime changed those internal data structures
rather dramatically.

That means that we had to come up with a non-hacky way to redirect
stdout/stderr (e.g. to parse ESC color sequences and apply them to the
Win32 Console as appropriate) and still have isatty() report what we want
it to report. That is, if we redirect stdout/stderr to a pipe that parses
the color sequences for use in the Win32 Console, we want isatty() to
report that we are writing to a Terminal Typewriter (a charming
anachronism, isn't it?).

My colleague Jeff Hostetler worked on this and figured out that the only
way to do this properly is to wrap isatty() and override it via a #define,
and simply remember when stdout/stderr referred to a terminal before
redirecting, say, to that pipe.

As my famously broken patch to override isatty() (so that /dev/null would
not be treated as a terminal) *already* overrode isatty() with a custom
wrapper, and as it was broken and needed fixing, I decided to reconcile
the two approaches into what is now the version in `master`.

So instead of "bending" the target HANDLE of the existing stdout/stderr
(which would *naturally* have kept the buffered/unbuffered nature as-is),
we now redirect with correct API calls. And the patch I provided at the
bottom of this mail thread reinstates the unbuffered nature of stderr now
that it gets reopened.

Hopefully that makes it clear why the setvbuf() call is required now, but
was previously unnecessary?

Ciao,
Dscho

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Junio,

On Tue, 14 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> OK.  Should this go directly to 'master', as the isatty thing is
> >> already in?
> >
> > From my point of view, it is not crucial. The next Git for Windows
> > version will have it, of course, and Hannes is always running with his
> > set of patches, he can easily cherry-pick this one, too.
> 
> What hat were you wearing when you said the above "From my point of
> view"?

The hat of a person who sees how patches are reviewed before they enter
pu/next/master/maint of git.git.

If that review had anything to do with Windows, and refused to accept
patches unless they work correctly on Windows, I would agree that it is a
wise idea to fast-track important fixes for Windows.

But that is not the case. Quite often `pu`, sometimes `next`, and rarely
even `master` are regularly broken on Windows.

The only branch that is tested very stringently on Windows, and into which
nothing is allowed that breaks on Windows, is git-for-windows/git's
`master` branch.

BTW this is not just an opinion, this is just an account of the current
state.

Once you accept that this is reality, you will understand why I *dared* to
say that a criticial Windows-specific fix needs to be fast-tracked to
git-for-windows/git's `master`, but not into git.git's `master` branch.

FWIW I wish it were different, that git.git's `master` reflected more
closely what the current Git for Windows version has. If you are
attentive, you will have noticed that I continuously work toward that
goal. I frequently "upstream patches" from Git for Windows[*1*], even if
it seems that the influx of new patches is higher than the rate of patches
that make it into git.git's `master`. And even if I am often asked to
change these patches so much that it is virtually guaranteed that they
regress (hence my recently increasing reluctance to accept each and every
reviewer's suggestions as "must implement").

Ciao,
Johannes

Footnote *1*: Yes, I even "upstream patches" from developers other than
myself, trying to shield contributors from having to send their patches as
mails and to cope with the reviewers' suggestions that may, or may not,
make sense. This makes my life harder, but I believe that the alternative
would be *not* to have those patches in git.git *at all*.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Sixt]

Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:
> On Tue, 14 Feb 2017, Johannes Sixt wrote:
>> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
>>> On Mon, 13 Feb 2017, Junio C Hamano wrote:
>>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>>> What we forgot was to mark stderr as unbuffered again.
>>
>> I do not see how the earlier patch turned stderr from unbuffered to
>> buffered, as it did not add or remove any setvbuf() call. Can you
>> explain?
>
> [ motivation and history of js/mingw-isatty snipped ]
>
> So instead of "bending" the target HANDLE of the existing stdout/stderr
> (which would *naturally* have kept the buffered/unbuffered nature as-is),
> we now redirect with correct API calls.

Your statement implies that at the time when winansi_init() begins, 
stdio is already initialized and the buffered/unbuffered state has been 
set for stderr. I would think that this is true.

Then we swap out the file handle underlying stderr in swap_osfhnd() 
using dup2(). Why would that change the buffered state of stdio?

> And the patch I provided at the
> bottom of this mail thread reinstates the unbuffered nature of stderr now
> that it gets reopened.
>
> Hopefully that makes it clear why the setvbuf() call is required now, but
> was previously unnecessary?

Unfortunately, no. I do not see how dup2() causes a change in stdio 
state. I must be missing something (and that may be a basic 
misunderstanding of how stdio is initialized).

-- Hannes

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
> ...
> The hat of a person who sees how patches are reviewed before they enter
> pu/next/master/maint of git.git.
> ...
> make sense. This makes my life harder, but I believe that the alternative
> would be *not* to have those patches in git.git *at all*.

You wrote a lot, what you wrote were readable, and perhaps were good
material to support your answer/conclusion, but you forgot to answer
a simple question I asked.  I think your description of where things
currently are would support any of the possible answers below, and I
cannot guess which one it would be.

The question was:

    Is our position "unless you are extremely competent and are willing
    to cherry-pick missing things from Git for Windows tree as needed,
    we recommend you to build Git for Windows source instead"?

Or is it "please ignore upstream and work off of Git for Windows?"

Or is it "please try to work with upstream and if you find Windows
specific glitches, after checking to see if Git for Windows has
already a fix for it and otherwise feed your fix to Git for Windows,
so that we can upstream your fix for you, together with changes from
others"?

Or is it "please try to work with upstream and if you find Windows
specific glitches, after checking to see if Git for Windows has
already a fix for it and otherwise upstream your fix, so that next
pull from upstream into Git for Windows would have your fix"?

Or is it something else?


> FWIW I wish it were different, that git.git's `master` reflected more
> closely what the current Git for Windows version has.

Well, we two wishing the same thing together without doing anything
else would make it happen.

As an experiment to see if our process can be improved, I've been
meaning to suggest (which is what was behind my "question at a bit
higher level" to Hannes [*1*]) asking you to throw me occasional
pull requests for changes that are only about Windows specific
issues, bypassing "patches on the list" for things like a hotfix to
js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
essentially treating Windows specific changes as "a sub-maintainer
makes pull requests" we already do with Paul, Eric and Pat.

Interested parties would instead see only the pull request sent by
you to me, either on the list or directly CC'ed to them, and do
their own fetch to assess if it is a good idea for me to actually
pull.  Suggestions to the changes from bystanders like Peff's
comment [*4*] may need to reproduce the patch text when sent to the
list, which would burden reviewers a bit more, but they still are
possible.

Such a pull-request workflow would either hide the communication
from the list or force people to go to multiple places (i.e. both to
the list and to GitHub comments) in order to view the whole picture,
and I am still reluctant to extend it to other areas (e.g. a change
that adds "#if WINDOWS" to a more generic codepath), but a trial on
a limited scope may give us a better feel of how well such an
updated workflow would work for us.


[References]

*1* <xmqq60kdev2r.fsf@gitster.mtv.corp.google.com>

*2* <c88612da0a62bfcbc3e278296f9d3eb010057071.1487025228.git.johannes.schindelin@gmx.de>

*3* <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>

*4* <20170210050237.gajicliueuvk6s5d@sigill.intra.peff.net>

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> FWIW I wish it were different, that git.git's `master` reflected more
>> closely what the current Git for Windows version has.
>
> Well, we two wishing the same thing together without doing anything
> else would make it happen.

ehh, would *not* make it happen, of course.

> As an experiment to see if our process can be improved, I've been
> meaning to suggest (which is what was behind my "question at a bit
> higher level" to Hannes [*1*]) asking you to throw me occasional
> pull requests for changes that are only about Windows specific
> issues, bypassing "patches on the list" for things like a hotfix to
> js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
> essentially treating Windows specific changes as "a sub-maintainer
> makes pull requests" we already do with Paul, Eric and Pat.

While this may ease the flow of upstreaming windows specific
changes, we need a separate thing to address the on-going issue you
raised in your message.  A Windows-less person would not know his
change to a generic code that is innocuous-looking has fallouts on
Windows (read this sentence with "Windows" replaced with any
specific platform name).  When somebody writes c == '/' that should
have been written as is_dir_sep(c), you or Hannes often finds it
during the review here, and after repeatedly seeing such reviews,
that (slowly) rubs off on other Window-less folks.  A new code may
still hit 'next' and 'master' with such an issue if it goes
unnoticed during the review.

The CI you are setting up [*1*] may certainly be a step in the good
direction.  Having more people like Hannes working off of upstream
may also be a great way to help the "forget 'next' and upstream in
general" issue.  Any other ideas?

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Hannes,

On Wed, 15 Feb 2017, Johannes Sixt wrote:

> Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:
> > On Tue, 14 Feb 2017, Johannes Sixt wrote:
> > > Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > > > What we forgot was to mark stderr as unbuffered again.
> > >
> > > I do not see how the earlier patch turned stderr from unbuffered to
> > > buffered, as it did not add or remove any setvbuf() call. Can you
> > > explain?
> >
> > [ motivation and history of js/mingw-isatty snipped ]
> >
> > So instead of "bending" the target HANDLE of the existing
> > stdout/stderr (which would *naturally* have kept the
> > buffered/unbuffered nature as-is), we now redirect with correct API
> > calls.
> 
> Your statement implies that at the time when winansi_init() begins,
> stdio is already initialized and the buffered/unbuffered state has been
> set for stderr.  I would think that this is true.
> 
> Then we swap out the file handle underlying stderr in swap_osfhnd()
> using dup2(). Why would that change the buffered state of stdio?

The file handle we swap in for stderr points to the pipe that a
freshly-started thread consumes for parsing the ANSI color sequences. This
handle is used both for stdout and stderr. The dup2() call then implicitly
reopens stderr, with the default buffering.

> > And the patch I provided at the bottom of this mail thread reinstates
> > the unbuffered nature of stderr now that it gets reopened.
> >
> > Hopefully that makes it clear why the setvbuf() call is required now,
> > but was previously unnecessary?
> 
> Unfortunately, no. I do not see how dup2() causes a change in stdio state. I
> must be missing something (and that may be a basic misunderstanding of how
> stdio is initialized).

It appears that dup2()ing fd 2 resets that stdio state.

Ciao,
Dscho

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Sixt]

Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>> I do not see how dup2() causes a change in stdio state. I
>> must be missing something (and that may be a basic misunderstanding of how
>> stdio is initialized).
>
> It appears that dup2()ing fd 2 resets that stdio state.

OK, thanks. It's surprising, but so be it.

Thank you for the quick fix!

-- Hannes

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
>> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>>> I do not see how dup2() causes a change in stdio state. I
>>> must be missing something (and that may be a basic misunderstanding of how
>>> stdio is initialized).
>>
>> It appears that dup2()ing fd 2 resets that stdio state.
>
> OK, thanks. It's surprising, but so be it.
>
> Thank you for the quick fix!

I am happy to see two Windows folks being happy.  I tentatively
marked this as "undecided" after merging it to 'next', but let's
have it in the -rc2 and final.  Two people familiar with Windows
agreeing that there is no longer mystery why it fixes, after seeing
that the update fixes a regression, is a strong enough sign to me
that including it is better than leaving it out.

Thanks.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Junio,

On Wed, 15 Feb 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> FWIW I wish it were different, that git.git's `master` reflected more
> >> closely what the current Git for Windows version has.
> >
> > Well, we two wishing the same thing together without doing anything
> > else would make it happen.
> 
> ehh, would *not* make it happen, of course.

That is the reason why set aside a substantial part of my maintainer time
to upstream those patches. You may not see how much time it costs me, say,
to get the putty-w-args stuff upstream, but rest assured that I would not
be able to do this were it not for a company paying me to do exactly that.

> > As an experiment to see if our process can be improved, I've been
> > meaning to suggest (which is what was behind my "question at a bit
> > higher level" to Hannes [*1*]) asking you to throw me occasional
> > pull requests for changes that are only about Windows specific
> > issues, bypassing "patches on the list" for things like a hotfix to
> > js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
> > essentially treating Windows specific changes as "a sub-maintainer
> > makes pull requests" we already do with Paul, Eric and Pat.

The problem here is that Git for Windows is not a subsystem. It touches
pretty much all of Git. That is very different from the Tcl/Tk code, or
from git-svn (whose code is not shared by anything else in Git's source
code, despite the fact that it is written in Perl).

And even if you were to accept the occasional Pull Request from me, it
would not solve the even bigger problem that we essentially reject so much
expertise out there, so many potential contributions by very competent
developers who just have no desire to fight the contribution process.

> While this may ease the flow of upstreaming windows specific
> changes, we need a separate thing to address the on-going issue you
> raised in your message.  A Windows-less person would not know his

... or her...

> change to a generic code that is innocuous-looking has fallouts on
> Windows (read this sentence with "Windows" replaced with any
> specific platform name).  When somebody writes c == '/' that should
> have been written as is_dir_sep(c), you or Hannes often finds it
> during the review here, and after repeatedly seeing such reviews,
> that (slowly) rubs off on other Window-less folks.  A new code may
> still hit 'next' and 'master' with such an issue if it goes
> unnoticed during the review.

Apart from the fact that we have no prayer at coming up with a system that
keeps track of open issues (because we do not use any tracker, but instead
rely on people to remember that some thing still may not have been
addressed), there is a different problem here: you stated very explicitly
that it is okay for `pu` to be broken [*1*]. If it were different, if any
breakage would imply that a fix is really, really required lest the patch
series be evicted from `pu`, we could easily modify my Continuous Testing
setup to report more visibly what is broken. But since it is okay for `pu`
to be broken, that would just annoy everybody and people would learn to
ignore/procmail those reports.

> The CI you are setting up [*1*]

which *1*?

And... I already set it up. I just did not bother to make it more public
because the builds were broken more often than not. IIRC the entire month
of October was a solid red.

> may certainly be a step in the good direction. Having more people like
> Hannes working off of upstream may also be a great way to help the
> "forget 'next' and upstream in general" issue. Any other ideas?

The Continuous Integration is actually not so much a Continuous
Integration as it is a Continuous Testing. If it became more of a CI, that
would certainly reduce the impression that Git's bleeding edge only ever
works on Linux.

Ciao,
Johannes

Footnote *1*: You probably read between the lines that this is
unfortunate, in my opinion. It sets the mood that lets experimental (if
useful) features such as worktrees be broken for the better part of a
year.

Re: [PATCH] mingw: make stderr unbuffered again

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> ... there is a different problem here: you stated very explicitly
> that it is okay for `pu` to be broken [*1*]. If it were different, if any
> breakage would imply that a fix is really, really required lest the patch
> series be evicted from `pu`, we could easily modify my Continuous Testing
> setup to report more visibly what is broken.

I think you misread me.

Regarding 'pu', there is a chicken-and-egg problem involved.  Most
of the days, the tip of 'pu' passes the test at least for me.

At the end of day's integration cycle, I may find that 'pu' does not
pass the test for me [*1*].  It is not unusual to see new multiple
topics come to 'pu', each of which may test well in isolation but
have hidden interactions with topics already in 'next' or 'pu' that
are exposed only when merged to 'pu', and it may be too late in the
day for me to find which one of these new topics is problematic.

I could choose not to merge any of them and push 'pu' out with no
new topics.  Or I may choose to push it out anyway, so that other
people who are working during the remainder of 24-hour in different
timezones can notice and find which new topic is broken [*2*].

And that is why I explicitly say that 'pu' may not even build.  It
shouldn't be taken as a discouragement to people from looking at it,
which seems to be how I read you to misinterpret it.  It is the
opposite---a breakage at the tip of 'pu' is an invitation for people
to help.

If nobody steps up and says "the tip of 'pu' does not build and that
is because of this topic", I'd be irritated enough to find which
topic is broken myself and then ask the author of the offencing
topic for help.  If the topic is left broken after that, it will be
ejected from 'pu', because I cannot use 'pu' for helping to polish
other new topics that wants to cook there unless I do so.

Of course there are platform- or environment-dependent breakages
that would not cause my tests to fail [*3*]. Again, people with
different environment can step up to help, if these topics are
included in 'pu'.  You would recall that you called out one topic
[*4*] that did not build in your environment and the topic was
ejected after your message (but not before---there wasn't a way for
Window-less folks to tell that it was broken IIRC) from 'pu'.

In a near-by thread, somebody wondered if we can add an integration
branch 'pr' that is beyond 'pu' and includes everything ever posted
to the list, and I explained why I won't have time for that.  But I
think the spirit of suggested 'pr' is what 'pu' already is---it is a
collection of promising topics that can be merged into a seemingly
consistent whole, which may or may not build and the purpose of the
branch is to contain them in one place so that people can find which
ones need unbreaking.  Testing the tip of it alone and complaining
that it is broken does not help much, but figuiring out which topic
merged to it is broken does, by unblocking other topics in 'pu'.


[Footnote]

*1* If any other integration branch (including 'jch', which is
    somewhere between 'master' and 'pu' and beyond the commit with a
    tree that matches 'next' aka "pu^{/^### match next}") does not
    build, I pull overtime ;-)


*2* When there is only a new topic that breaks 'pu' for me, it is
    easy to decide not to queue it and send a note to the author of
    the offending topic, and that does happen a lot, but not always.

*3* I do not have p4 and I may not have svn installed so my tests
    may not even cover them.

*4* I think it was nd/worktree-move but may have been another topic.