[PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Johannes Schindelin]

Particularly when calling Git from applications, such as Visual Studio,
it is important that stdin/stdout/stderr are closed properly. However,
when spawning processes on Windows, those handles must be marked as
inheritable if we want to use them, but that flag is a global flag and
may very well be used by other spawned processes which then do not know
to close those handles.

As a workaround, introduce handling for the environment variables
GIT_REDIRECT_STD* to read/write from/to named pipes instead
(conceptually similar to Unix sockets, for you Linux folks). These do
not need to be marked as inheritable, as the process can simply open the
named pipe. No global flags. No problems.

This feature was introduced as an experimental feature into Git for
Windows v2.11.0(2) and has been tested ever since. I feel it is
well-tested enough that it can be integrated into core Git.

Once it has been reviewed enough, I will gladly remove the
"experimental" adjectives and warnings.


Johannes Schindelin (3):
  mingw: add experimental feature to redirect standard handles
  mingw: special-case GIT_REDIRECT_STDERR=2>&1
  mingw: document the experimental standard handle redirection

 Documentation/git.txt | 17 +++++++++++++++
 compat/mingw.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0001-init.sh       | 12 +++++++++++
 3 files changed, 87 insertions(+)


base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
Published-As: https://github.com/dscho/git/releases/tag/redirect-std-handles-v1
Fetch-It-Via: git fetch https://github.com/dscho/git redirect-std-handles-v1
-- 
2.15.0.windows.1

[PATCH 1/3] mingw: add experimental feature to redirect standard handles

[Johannes Schindelin]

On Windows, file handles need to be marked inheritable when they need to
be used as standard input/output/error handles for a newly spawned
process. The problem with that, of course, is that the "inheritable" flag
is global and therefore can wreak havoc with highly multi-threaded
applications: other spawned processes will *also* inherit those file
handles, despite having *other* input/output/error handles, and never
close the former handles because they do not know about them.

Let's introduce a set of environment variables (GIT_REDIRECT_STDIN and
friends) that point to files, or even better, named pipes and that are
used by the spawned Git process. This helps work around above-mentioned
issue: those named pipes will be opened in a non-inheritable way upon
startup, and no handles are passed around.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 t/t0001-init.sh |  6 ++++++
 2 files changed, 49 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db446..6c6c7795a70 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2139,6 +2139,47 @@ static char *wcstoutfdup_startup(char *buffer, const wchar_t *wcs, size_t len)
 	return memcpy(malloc_startup(len), buffer, len);
 }
 
+static void maybe_redirect_std_handle(const wchar_t *key, DWORD std_id, int fd,
+				      DWORD desired_access, DWORD flags)
+{
+	DWORD create_flag = fd ? OPEN_ALWAYS : OPEN_EXISTING;
+	wchar_t buf[MAX_PATH];
+	DWORD max = ARRAY_SIZE(buf);
+	HANDLE handle;
+	DWORD ret = GetEnvironmentVariableW(key, buf, max);
+
+	if (!ret || ret >= max)
+		return;
+
+	/* make sure this does not leak into child processes */
+	SetEnvironmentVariableW(key, NULL);
+	if (!wcscmp(buf, L"off")) {
+		close(fd);
+		handle = GetStdHandle(std_id);
+		if (handle != INVALID_HANDLE_VALUE)
+			CloseHandle(handle);
+		return;
+	}
+	handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
+			     flags, NULL);
+	if (handle != INVALID_HANDLE_VALUE) {
+		int new_fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+		SetStdHandle(std_id, handle);
+		dup2(new_fd, fd);
+		close(new_fd);
+	}
+}
+
+static void maybe_redirect_std_handles(void)
+{
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDIN", STD_INPUT_HANDLE, 0,
+				  GENERIC_READ, FILE_ATTRIBUTE_NORMAL);
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDOUT", STD_OUTPUT_HANDLE, 1,
+				  GENERIC_WRITE, FILE_ATTRIBUTE_NORMAL);
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDERR", STD_ERROR_HANDLE, 2,
+				  GENERIC_WRITE, FILE_FLAG_NO_BUFFERING);
+}
+
 void mingw_startup(void)
 {
 	int i, maxlen, argc;
@@ -2146,6 +2187,8 @@ void mingw_startup(void)
 	wchar_t **wenv, **wargv;
 	_startupinfo si;
 
+	maybe_redirect_std_handles();
+
 	/* get wide char arguments and environment */
 	si.newmode = 0;
 	if (__wgetmainargs(&argc, &wargv, &wenv, _CRT_glob, &si) < 0)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 86c1a51654f..0fd2fc45385 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -453,4 +453,10 @@ test_expect_success 're-init from a linked worktree' '
 	)
 '
 
+test_expect_success MINGW 'redirect std handles' '
+	GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
+	test .git = "$(cat output.txt)" &&
+	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+'
+
 test_done
-- 
2.15.0.windows.1

[PATCH 2/3] mingw: special-case GIT_REDIRECT_STDERR=2>&1

[Johannes Schindelin]

The "2>&1" notation in POSIX shells implies that stderr is redirected to
stdout. Let's special-case this value for the environment variable
GIT_REDIRECT_STDERR to allow writing to the same destination as stdout.

The functionality was suggested by Jeff Hostetler.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c  | 15 +++++++++++++++
 t/t0001-init.sh |  8 +++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6c6c7795a70..2d44d21aca8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2160,6 +2160,21 @@ static void maybe_redirect_std_handle(const wchar_t *key, DWORD std_id, int fd,
 			CloseHandle(handle);
 		return;
 	}
+	if (std_id == STD_ERROR_HANDLE && !wcscmp(buf, L"2>&1")) {
+		handle = GetStdHandle(STD_OUTPUT_HANDLE);
+		if (handle == INVALID_HANDLE_VALUE) {
+			close(fd);
+			handle = GetStdHandle(std_id);
+			if (handle != INVALID_HANDLE_VALUE)
+				CloseHandle(handle);
+		} else {
+			int new_fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+			SetStdHandle(std_id, handle);
+			dup2(new_fd, fd);
+			/* do *not* close the new_fd: that would close stdout */
+		}
+		return;
+	}
 	handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
 			     flags, NULL);
 	if (handle != INVALID_HANDLE_VALUE) {
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 0fd2fc45385..c413bff9cf1 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -456,7 +456,13 @@ test_expect_success 're-init from a linked worktree' '
 test_expect_success MINGW 'redirect std handles' '
 	GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
 	test .git = "$(cat output.txt)" &&
-	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)" &&
+	test_must_fail env \
+		GIT_REDIRECT_STDOUT=output.txt \
+		GIT_REDIRECT_STDERR="2>&1" \
+		git rev-parse --git-dir --verify refs/invalid &&
+	printf ".git\nfatal: Needed a single revision\n" >expect &&
+	test_cmp expect output.txt
 '
 
 test_done
-- 
2.15.0.windows.1

[PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

This feature is still highly experimental and has not even been
contributed to the Git mailing list yet: the feature still needs to be
battle-tested more.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1d629ca06..10a98603b39 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -709,6 +709,23 @@ of clones and fetches.
 	the background which do not want to cause lock contention with
 	other operations on the repository.  Defaults to `1`.
 
+`GIT_REDIRECT_STDIN`::
+`GIT_REDIRECT_STDOUT`::
+`GIT_REDIRECT_STDERR`::
+	(EXPERIMENTAL) Windows-only: allow redirecting the standard
+	input/output/error handles. This is particularly useful in
+	multi-threaded applications where the canonical way to pass
+	standard handles via `CreateProcess()` is not an option because
+	it would require the handles to be marked inheritable (and
+	consequently *every* spawned process would inherit them, possibly
+	blocking regular Git operations). The primary intended use case
+	is to use named pipes for communication.
++
+Two special values are supported: `off` will simply close the
+corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
+`2>&1`, standard error will be redirected to the same handle as
+standard output.
+
 Discussion[[Discussion]]
 ------------------------
 
-- 
2.15.0.windows.1

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Eric Sunshine]

On Mon, Oct 30, 2017 at 1:10 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> This feature is still highly experimental and has not even been
> contributed to the Git mailing list yet: the feature still needs to be
> battle-tested more.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> +`GIT_REDIRECT_STDIN`::
> +`GIT_REDIRECT_STDOUT`::
> +`GIT_REDIRECT_STDERR`::
> +       (EXPERIMENTAL) Windows-only: allow redirecting the standard
> +       input/output/error handles. This is particularly useful in
> +       multi-threaded applications where the canonical way to pass
> +       standard handles via `CreateProcess()` is not an option because
> +       it would require the handles to be marked inheritable (and
> +       consequently *every* spawned process would inherit them, possibly
> +       blocking regular Git operations). The primary intended use case
> +       is to use named pipes for communication.
> ++
> +Two special values are supported: `off` will simply close the
> +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
> +`2>&1`, standard error will be redirected to the same handle as
> +standard output.

Consistent with the Unixy special-case for '2>&1', I wonder if the
'off' case would be more intuitively stated as '>/dev/null' or just
'/dev/null'...

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Eric,

On Mon, 30 Oct 2017, Eric Sunshine wrote:

> On Mon, Oct 30, 2017 at 1:10 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This feature is still highly experimental and has not even been
> > contributed to the Git mailing list yet: the feature still needs to be
> > battle-tested more.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > +`GIT_REDIRECT_STDIN`::
> > +`GIT_REDIRECT_STDOUT`::
> > +`GIT_REDIRECT_STDERR`::
> > +       (EXPERIMENTAL) Windows-only: allow redirecting the standard
> > +       input/output/error handles. This is particularly useful in
> > +       multi-threaded applications where the canonical way to pass
> > +       standard handles via `CreateProcess()` is not an option because
> > +       it would require the handles to be marked inheritable (and
> > +       consequently *every* spawned process would inherit them, possibly
> > +       blocking regular Git operations). The primary intended use case
> > +       is to use named pipes for communication.
> > ++
> > +Two special values are supported: `off` will simply close the
> > +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
> > +`2>&1`, standard error will be redirected to the same handle as
> > +standard output.
> 
> Consistent with the Unixy special-case for '2>&1', I wonder if the
> 'off' case would be more intuitively stated as '>/dev/null' or just
> '/dev/null'...

I feel this is the wrong way round. `>/dev/null` may sound very intuitive
to you, but this feature is Windows only. Guess three times how intuitive
it sounds to Windows developers to write `>/dev/null` if you want to
suppress output...

:0)

Ciao,
Dscho

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Junio C Hamano]

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

>> > +Two special values are supported: `off` will simply close the
>> > +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
>> > +`2>&1`, standard error will be redirected to the same handle as
>> > +standard output.
>> 
>> Consistent with the Unixy special-case for '2>&1', I wonder if the
>> 'off' case would be more intuitively stated as '>/dev/null' or just
>> '/dev/null'...
>
> I feel this is the wrong way round. `>/dev/null` may sound very intuitive
> to you, but this feature is Windows only. Guess three times how intuitive
> it sounds to Windows developers to write `>/dev/null` if you want to
> suppress output...

It would be just as intuitive to write '2>&1' for dup-redirection,
so I tend to agree with both of you in that perhaps '2>&1' may have
to become less Unix-y (or more Windows-y) to make these special
cases more consistent.  Perhaps "dup-to-stdout" or even just "stdout".

	Side note: if we really wanted to go in the other direction
	Eric suggests, "off" probably should be spelled as ">&-" ;-)

By the way, the description talks about "special values", but it
leaves it completely unclear what their normal values mean.  Are
they filenames, or integers that denote file descriptors, or
something else?  To those who read, wrote, or reviewed the code in
these patches, the answer is obvious (and I do *not* want you to
give your answer to *me* in your response for that reason).  But
we'd want to give the answer to future readers by clarifying this
documentation.

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Junio,

On Wed, 1 Nov 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +Two special values are supported: `off` will simply close the
> >> > +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
> >> > +`2>&1`, standard error will be redirected to the same handle as
> >> > +standard output.
> >> 
> >> Consistent with the Unixy special-case for '2>&1', I wonder if the
> >> 'off' case would be more intuitively stated as '>/dev/null' or just
> >> '/dev/null'...
> >
> > I feel this is the wrong way round. `>/dev/null` may sound very intuitive
> > to you, but this feature is Windows only. Guess three times how intuitive
> > it sounds to Windows developers to write `>/dev/null` if you want to
> > suppress output...
> 
> It would be just as intuitive to write '2>&1' for dup-redirection,

No. You misunderstand. I was mainly concerned with the `/dev/null`. Every
Windows developer knows what `>file.txt` means, and many know what
`2>error.txt` means. But `/dev/null` is not Windows, period. It is so not
Windows that Git itself translates it to `NUL` (which you Linux die-hards
won't have a clue about, I would wager a bet).

> Perhaps "dup-to-stdout" or even just "stdout".

No. The value is a path. I can special-case values that are not even valid
Windows file names (such as `2>&1`). I cannot special-case values that are
perfectly valid paths.

> By the way, the description talks about "special values", but it
> leaves it completely unclear what their normal values mean.

True. Fixed. I also threw in an example for a pipe.

Ciao,
Dscho

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Junio C Hamano]

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

>> > I feel this is the wrong way round. `>/dev/null` may sound very intuitive
>> > to you, but this feature is Windows only. Guess three times how intuitive
>> > it sounds to Windows developers to write `>/dev/null` if you want to
>> > suppress output...
>> 
>> It would be just as intuitive to write '2>&1' for dup-redirection,
>
> No. You misunderstand. I was mainly concerned with the `/dev/null`. Every
> Windows developer knows what `>file.txt` means, and many know what
> `2>error.txt` means. But `/dev/null` is not Windows, period.

Actually I did know that much.  

If I was correct in assuming that "2>&1" is just as foreign as
">/dev/null", then we should be shunning "2>&1" just like we shun
">/dev/null".  That was all I meant to say.

Are you saying "2>&1" is just as likely to be known as ">file.txt"
and my assumption of foreignness of "2>&1" was incorrect?

	Side note: would ">NUL" look more familiar, I wonder, and
	can stand for ">/dev/null" for the target audience?

> ... It is so not
> Windows that Git itself translates it to `NUL` (which you Linux die-hards
> won't have a clue about, I would wager a bet).

Ah, you lost your bet.  When can I collect ;-)?

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Nov 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > I feel this is the wrong way round. `>/dev/null` may sound very intuitive
> >> > to you, but this feature is Windows only. Guess three times how intuitive
> >> > it sounds to Windows developers to write `>/dev/null` if you want to
> >> > suppress output...
> >> 
> >> It would be just as intuitive to write '2>&1' for dup-redirection,
> >
> > No. You misunderstand. I was mainly concerned with the `/dev/null`. Every
> > Windows developer knows what `>file.txt` means, and many know what
> > `2>error.txt` means. But `/dev/null` is not Windows, period.
> 
> Actually I did know that much.  
> 
> If I was correct in assuming that "2>&1" is just as foreign as
> ">/dev/null", then we should be shunning "2>&1" just like we shun
> ">/dev/null".  That was all I meant to say.

Did you know that `2>&1` works in Powershell?

> Are you saying "2>&1" is just as likely to be known as ">file.txt"
> and my assumption of foreignness of "2>&1" was incorrect?
> 
> 	Side note: would ">NUL" look more familiar, I wonder, and
> 	can stand for ">/dev/null" for the target audience?
> 
> > ... It is so not
> > Windows that Git itself translates it to `NUL` (which you Linux die-hards
> > won't have a clue about, I would wager a bet).
> 
> Ah, you lost your bet.  When can I collect ;-)?

As soon as we meet in person again.

Ciao,
Dscho

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Junio C Hamano]

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

>> If I was correct in assuming that "2>&1" is just as foreign as
>> ">/dev/null", then we should be shunning "2>&1" just like we shun
>> ">/dev/null".  That was all I meant to say.
>
> Did you know that `2>&1` works in Powershell?

No.  And that makes me curious if ">&-" is also there to replace
"off" ;-)

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Nov 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> If I was correct in assuming that "2>&1" is just as foreign as
> >> ">/dev/null", then we should be shunning "2>&1" just like we shun
> >> ">/dev/null".  That was all I meant to say.
> >
> > Did you know that `2>&1` works in Powershell?
> 
> No.  And that makes me curious if ">&-" is also there to replace
> "off" ;-)

No, it does not:

-- snip --
PS C:\Users\me> echo 123 >&-
At line:1 char:11
+ echo 123 >&-
+           ~
Missing file specification after redirection operator.
At line:1 char:11
+ echo 123 >&-
+           ~
The ampersand (&) character is not allowed. The & operator is reserved for future use; wrap an ampersand in double
quotation marks ("&") to pass it as part of a string.
    + CategoryInfo          : ParserError: (:) [],
ParentContainsErrorRecordException
    + FullyQualifiedErrorId : MissingFileSpecification
-- snap --

Besides, we're really getting off-track here. I do not *like* `2>&1` as
quite cryptic placeholder for `redirect stderr into the same handle as
stdout was already redirected`. It is Perl-level obscurity.

Adding even more of those "let's string together non-alphanumerical
characters together and declare that they have some special meaning that
nobody would guess so they have to ask us and thereby make us feel smarter
than we are" is definitely not anything I want.

In my opinion, `off` is kind of a compromise that is both easy to
understand and hard to confuse.

If there was a short, succinct and easy-to-understand textual
representation of `same as stdout` that would not be easily confused for a
real file path, I would rather use that instead.

Please note, though, that I am again very reluctant to change things for
less than really compelling reasons at this stage. I have just burned two
days as a consequence of Peff's decision to take my --no-lock-index work
and turn it into something different enough that I had to put in more work
to adjust it, only to introduce a bug in something that worked without any
problem for over one entire year.

It is quite a bit ridiculous just how much bug hunting time I have to
spend lately on stuff that used to work and that got broken on transit
into git.git. It adds a whole new stress level to my work.

Ciao,
Dscho

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

> Particularly when calling Git from applications, such as Visual Studio,
> it is important that stdin/stdout/stderr are closed properly. However,
> when spawning processes on Windows, those handles must be marked as
> inheritable if we want to use them, but that flag is a global flag and
> may very well be used by other spawned processes which then do not know
> to close those handles.
>
> As a workaround, introduce handling for the environment variables
> GIT_REDIRECT_STD* to read/write from/to named pipes instead
> (conceptually similar to Unix sockets, for you Linux folks). These do
> not need to be marked as inheritable, as the process can simply open the
> named pipe. No global flags. No problems.
>
> This feature was introduced as an experimental feature into Git for
> Windows v2.11.0(2) and has been tested ever since. I feel it is
> well-tested enough that it can be integrated into core Git.

Can this rationale go in the commit messages?

Actually I wouldn't mind if this were all a single patch, with such a
rationale in the commit message.

The patches' concept seems sane.  I haven't looked closely at the
implementation.

Thanks,
Jonathan

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> This feature was introduced as an experimental feature into Git for
>> Windows v2.11.0(2) and has been tested ever since. I feel it is
>> well-tested enough that it can be integrated into core Git.
>
> Can this rationale go in the commit messages?
>
> Actually I wouldn't mind if this were all a single patch, with such a
> rationale in the commit message.
>
> The patches' concept seems sane.  I haven't looked closely at the
> implementation.

+1.

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Johannes Schindelin]

Hi Jonathan,

On Mon, 30 Oct 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > Particularly when calling Git from applications, such as Visual Studio,
> > it is important that stdin/stdout/stderr are closed properly. However,
> > when spawning processes on Windows, those handles must be marked as
> > inheritable if we want to use them, but that flag is a global flag and
> > may very well be used by other spawned processes which then do not know
> > to close those handles.
> >
> > As a workaround, introduce handling for the environment variables
> > GIT_REDIRECT_STD* to read/write from/to named pipes instead
> > (conceptually similar to Unix sockets, for you Linux folks). These do
> > not need to be marked as inheritable, as the process can simply open the
> > named pipe. No global flags. No problems.
> >
> > This feature was introduced as an experimental feature into Git for
> > Windows v2.11.0(2) and has been tested ever since. I feel it is
> > well-tested enough that it can be integrated into core Git.
> 
> Can this rationale go in the commit messages?

I thought I had done exactly that in 1/3...

> Actually I wouldn't mind if this were all a single patch, with such a
> rationale in the commit message.

Quite honestly, I'd rather not. There is a logical structure to the three
patches (and I left a Duck in 3/3, to see who bothers to actually read
what I wrote).

The redirection, for example, is a very special thing that I would like to
keep in a separate commit, for clarity.

> The patches' concept seems sane.  I haven't looked closely at the
> implementation.

Thanks,
Dscho

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Mon, 30 Oct 2017, Jonathan Nieder wrote:

>> Can this rationale go in the commit messages?
>
> I thought I had done exactly that in 1/3...

Okay, I'll be more specific.  This cover letter includes some
information about the rationale and motivation for the series.  That's
great: it makes reading the patches easier.  But TBH I'd rather that
it hadn't included that information at all, since if it said "see
patch 1/3 for rationale" then I could save the trouble of reading the
same information twice.

And unfortunately much of the relevant information is not repeated
there.  The cover letter mentions:

- that Visual Studio is a motivating example
- that this is conceptually similar to Unix sockets
- that those do not need to be marked as inheritable, as the process
  can simply open the named pipe. No global flags. No problems.
- that this has already seem some testing in Git for Windows (i.e.
  analagous information to what a Tested-by footer would say)

It is also just more readable than patch 1/3's commit message.  That's
to be expected, since it was written later.  My second draft of
something is often clearer than the first draft.

Thanks and hope that helps,
Jonathan

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Johannes Schindelin]

Hi Jonahtan,

On Tue, 31 Oct 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> > On Mon, 30 Oct 2017, Jonathan Nieder wrote:
> 
> >> Can this rationale go in the commit messages?
> >
> > I thought I had done exactly that in 1/3...
> 
> Okay, I'll be more specific.  This cover letter includes some
> information about the rationale and motivation for the series.  That's
> great: it makes reading the patches easier.  But TBH I'd rather that
> it hadn't included that information at all, since if it said "see
> patch 1/3 for rationale" then I could save the trouble of reading the
> same information twice.

Alas, I am the exact opposite. You see, I am seriously short on time, and
if the cover letter of a patch series leaves everything about the changes
unclear, I throw my laptop out the window (actually, I suppress the urge
and just delete the mail thread in my mail reader) and move to the next
mail.

It sounds a bit stupid to cater to myself in patches *I* submit, but I
refuse to believe that there are many people with more time on their hands
than myself (last time I tried to research this, it looked as everybody
has the same 86,400 seconds per day available, give or take the occasional
leap second).

> And unfortunately much of the relevant information is not repeated
> there.  The cover letter mentions:
> 
> - that Visual Studio is a motivating example

That was actually on purpose. Personally, I want to read the motivation in
the cover letter, and not get distracted by it when reading the commit
logs.

To make you happy, I added this, though.

> - that this is conceptually similar to Unix sockets

To make you happy, I added this, too.

> - that those do not need to be marked as inheritable, as the process
>   can simply open the named pipe. No global flags. No problems.

I just added "(and therefore no inherited handles need to be closed)" to
the last sentence of 1/3's commit message that already mentioned this.

> - that this has already seem some testing in Git for Windows (i.e.
>   analagous information to what a Tested-by footer would say)

I mentioned this twice, in 1/3's and in 3/3's commit message.

> It is also just more readable than patch 1/3's commit message.  That's
> to be expected, since it was written later.  My second draft of
> something is often clearer than the first draft.

I took your cue and simply replaced the first paragraph of 1/3's commit
message by the first paragraph of the cover letter.

Ciao,
Dscho

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Stefan Beller]

> It sounds a bit stupid to cater to myself in patches *I* submit, but I
> refuse to believe that there are many people with more time on their hands
> than myself (last time I tried to research this, it looked as everybody
> has the same 86,400 seconds per day available, give or take the occasional
> leap second).

Try traveling west bound every day.
This one weird trick with time zones will amaze you!

[PATCH v2 0/3] mingw: introduce a way to avoid std handle inheritance

[Johannes Schindelin]

Particularly when calling Git from applications, such as Visual Studio,
it is important that stdin/stdout/stderr are closed properly. However,
when spawning processes on Windows, those handles must be marked as
inheritable if we want to use them, but that flag is a global flag and
may very well be used by other spawned processes which then do not know
to close those handles.

As a workaround, introduce handling for the environment variables
GIT_REDIRECT_STD* to read/write from/to named pipes instead
(conceptually similar to Unix sockets, for you Linux folks). These do
not need to be marked as inheritable, as the process can simply open the
named pipe. No global flags. No problems.

This feature was introduced as an experimental feature into Git for
Windows v2.11.0(2) and has been tested ever since. I feel it is
well-tested enough that it can be integrated into core Git.

Changes since v1:

- clarified what the values of the environment variables mean.

- the feature is no longer marked experimental (I actually expected *some*
  remark about this, but it seems nobody looked at that patch all that closely
  *frown*)

- edited all commit messages heavily (primarily prodded by Jonathan's feedback).


Johannes Schindelin (3):
  mingw: add experimental feature to redirect standard handles
  mingw: optionally redirect stderr/stdout via the same handle
  mingw: document the standard handle redirection

 Documentation/git.txt | 18 ++++++++++++++++
 compat/mingw.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0001-init.sh       | 12 +++++++++++
 3 files changed, 88 insertions(+)


base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
Published-As: https://github.com/dscho/git/releases/tag/redirect-std-handles-v2
Fetch-It-Via: git fetch https://github.com/dscho/git redirect-std-handles-v2

Interdiff vs v1:
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index 10a98603b39..463b0eb0f5c 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -712,14 +712,15 @@ of clones and fetches.
  `GIT_REDIRECT_STDIN`::
  `GIT_REDIRECT_STDOUT`::
  `GIT_REDIRECT_STDERR`::
 -	(EXPERIMENTAL) Windows-only: allow redirecting the standard
 -	input/output/error handles. This is particularly useful in
 -	multi-threaded applications where the canonical way to pass
 -	standard handles via `CreateProcess()` is not an option because
 -	it would require the handles to be marked inheritable (and
 -	consequently *every* spawned process would inherit them, possibly
 -	blocking regular Git operations). The primary intended use case
 -	is to use named pipes for communication.
 +	Windows-only: allow redirecting the standard input/output/error
 +	handles to paths specified by the environment variables. This is
 +	particularly useful in multi-threaded applications where the
 +	canonical way to pass standard handles via `CreateProcess()` is
 +	not an option because it would require the handles to be marked
 +	inheritable (and consequently *every* spawned process would
 +	inherit them, possibly blocking regular Git operations). The
 +	primary intended use case is to use named pipes for communication
 +	(e.g. `\\.\pipe\my-git-stdin-123`).
  +
  Two special values are supported: `off` will simply close the
  corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
-- 
2.15.0.windows.1

[PATCH v2 1/3] mingw: add experimental feature to redirect standard handles

[Johannes Schindelin]

Particularly when calling Git from applications, such as Visual Studio's
Team Explorer, it is important that stdin/stdout/stderr are closed
properly. However, when spawning processes on Windows, those handles
must be marked as inheritable if we want to use them, but that flag is a
global flag and may very well be used by other spawned processes which
then do not know to close those handles.

Let's introduce a set of environment variables (GIT_REDIRECT_STDIN and
friends) that specify paths to files, or even better, named pipes (which
are similar to Unix sockets) and that are used by the spawned Git
process.  This helps work around above-mentioned issue: those named
pipes will be opened in a non-inheritable way upon startup, and no
handles are passed around (and therefore no inherited handles need to be
closed by any spawned child).

This feature shipped with Git for Windows (marked as experimental) since
v2.11.0(2), so it has seen some serious testing in the meantime.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 t/t0001-init.sh |  6 ++++++
 2 files changed, 49 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db446..6c6c7795a70 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2139,6 +2139,47 @@ static char *wcstoutfdup_startup(char *buffer, const wchar_t *wcs, size_t len)
 	return memcpy(malloc_startup(len), buffer, len);
 }
 
+static void maybe_redirect_std_handle(const wchar_t *key, DWORD std_id, int fd,
+				      DWORD desired_access, DWORD flags)
+{
+	DWORD create_flag = fd ? OPEN_ALWAYS : OPEN_EXISTING;
+	wchar_t buf[MAX_PATH];
+	DWORD max = ARRAY_SIZE(buf);
+	HANDLE handle;
+	DWORD ret = GetEnvironmentVariableW(key, buf, max);
+
+	if (!ret || ret >= max)
+		return;
+
+	/* make sure this does not leak into child processes */
+	SetEnvironmentVariableW(key, NULL);
+	if (!wcscmp(buf, L"off")) {
+		close(fd);
+		handle = GetStdHandle(std_id);
+		if (handle != INVALID_HANDLE_VALUE)
+			CloseHandle(handle);
+		return;
+	}
+	handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
+			     flags, NULL);
+	if (handle != INVALID_HANDLE_VALUE) {
+		int new_fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+		SetStdHandle(std_id, handle);
+		dup2(new_fd, fd);
+		close(new_fd);
+	}
+}
+
+static void maybe_redirect_std_handles(void)
+{
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDIN", STD_INPUT_HANDLE, 0,
+				  GENERIC_READ, FILE_ATTRIBUTE_NORMAL);
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDOUT", STD_OUTPUT_HANDLE, 1,
+				  GENERIC_WRITE, FILE_ATTRIBUTE_NORMAL);
+	maybe_redirect_std_handle(L"GIT_REDIRECT_STDERR", STD_ERROR_HANDLE, 2,
+				  GENERIC_WRITE, FILE_FLAG_NO_BUFFERING);
+}
+
 void mingw_startup(void)
 {
 	int i, maxlen, argc;
@@ -2146,6 +2187,8 @@ void mingw_startup(void)
 	wchar_t **wenv, **wargv;
 	_startupinfo si;
 
+	maybe_redirect_std_handles();
+
 	/* get wide char arguments and environment */
 	si.newmode = 0;
 	if (__wgetmainargs(&argc, &wargv, &wenv, _CRT_glob, &si) < 0)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 86c1a51654f..0fd2fc45385 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -453,4 +453,10 @@ test_expect_success 're-init from a linked worktree' '
 	)
 '
 
+test_expect_success MINGW 'redirect std handles' '
+	GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
+	test .git = "$(cat output.txt)" &&
+	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+'
+
 test_done
-- 
2.15.0.windows.1

[PATCH v2 2/3] mingw: optionally redirect stderr/stdout via the same handle

[Johannes Schindelin]

The "2>&1" notation in Powershell and in Unix shells implies that stderr
is redirected to the same handle into which stdout is already written.

Let's use this special value to allow the same trick with
GIT_REDIRECT_STDERR and GIT_REDIRECT_STDOUT: if the former's value is
`2>&1`, then stderr will simply be written to the same handle as stdout.

The functionality was suggested by Jeff Hostetler.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c  | 15 +++++++++++++++
 t/t0001-init.sh |  8 +++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6c6c7795a70..2d44d21aca8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2160,6 +2160,21 @@ static void maybe_redirect_std_handle(const wchar_t *key, DWORD std_id, int fd,
 			CloseHandle(handle);
 		return;
 	}
+	if (std_id == STD_ERROR_HANDLE && !wcscmp(buf, L"2>&1")) {
+		handle = GetStdHandle(STD_OUTPUT_HANDLE);
+		if (handle == INVALID_HANDLE_VALUE) {
+			close(fd);
+			handle = GetStdHandle(std_id);
+			if (handle != INVALID_HANDLE_VALUE)
+				CloseHandle(handle);
+		} else {
+			int new_fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+			SetStdHandle(std_id, handle);
+			dup2(new_fd, fd);
+			/* do *not* close the new_fd: that would close stdout */
+		}
+		return;
+	}
 	handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
 			     flags, NULL);
 	if (handle != INVALID_HANDLE_VALUE) {
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 0fd2fc45385..c413bff9cf1 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -456,7 +456,13 @@ test_expect_success 're-init from a linked worktree' '
 test_expect_success MINGW 'redirect std handles' '
 	GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
 	test .git = "$(cat output.txt)" &&
-	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+	test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)" &&
+	test_must_fail env \
+		GIT_REDIRECT_STDOUT=output.txt \
+		GIT_REDIRECT_STDERR="2>&1" \
+		git rev-parse --git-dir --verify refs/invalid &&
+	printf ".git\nfatal: Needed a single revision\n" >expect &&
+	test_cmp expect output.txt
 '
 
 test_done
-- 
2.15.0.windows.1

[PATCH v2 3/3] mingw: document the standard handle redirection

[Johannes Schindelin]

This feature has been in Git for Windows since v2.11.0(2), as an
experimental option. Now it is considered mature, and it is high time to
document it properly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1d629ca06..463b0eb0f5c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -709,6 +709,24 @@ of clones and fetches.
 	the background which do not want to cause lock contention with
 	other operations on the repository.  Defaults to `1`.
 
+`GIT_REDIRECT_STDIN`::
+`GIT_REDIRECT_STDOUT`::
+`GIT_REDIRECT_STDERR`::
+	Windows-only: allow redirecting the standard input/output/error
+	handles to paths specified by the environment variables. This is
+	particularly useful in multi-threaded applications where the
+	canonical way to pass standard handles via `CreateProcess()` is
+	not an option because it would require the handles to be marked
+	inheritable (and consequently *every* spawned process would
+	inherit them, possibly blocking regular Git operations). The
+	primary intended use case is to use named pipes for communication
+	(e.g. `\\.\pipe\my-git-stdin-123`).
++
+Two special values are supported: `off` will simply close the
+corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
+`2>&1`, standard error will be redirected to the same handle as
+standard output.
+
 Discussion[[Discussion]]
 ------------------------
 
-- 
2.15.0.windows.1