threads / patch / 47073

patch, 3 partsmingw: introduce a way to avoid std handle inheritance

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

## tl;dr

22 messages between Oct 30, 2017 and Nov 3, 2017. Diffs are folded; open one to read it.

replies: 21people: 5as markdown or json

Johannes Schindelin· Oct 30, 2017, 17:10 UTC · lore

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
Johannes Schindelin· Oct 30, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to 2 files +49 −0

compat/mingw.c, t/t0001-init.sh

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
Johannes Schindelin· Oct 30, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +22 −1

compat/mingw.c, t/t0001-init.sh

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
Johannes Schindelin· Oct 30, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to Documentation/git.txt +17 −0
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
Eric Sunshine· Oct 30, 2017, 18:58 UTC · re: Johannes Schindelin · lore

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

On Mon, Oct 30, 2017 at 1:10 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 22 quoted lines
> 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'...

Johannes Schindelin· Oct 31, 2017, 17:08 UTC · re: Eric Sunshine · lore

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

Hi Eric,
On Mon, 30 Oct 2017, Eric Sunshine wrote:
Show 28 quoted lines
> 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

Junio C Hamano· Nov 1, 2017, 04:58 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
>> > +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.

Johannes Schindelin· Nov 1, 2017, 16:37 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Wed, 1 Nov 2017, Junio C Hamano wrote:
Show 17 quoted lines
> 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

Junio C Hamano· Nov 2, 2017, 01:31 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
>> > 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 ;-)?
Johannes Schindelin· Nov 2, 2017, 17:20 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Thu, 2 Nov 2017, Junio C Hamano wrote:
Show 18 quoted lines
> 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?
Show 11 quoted lines
> 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

Junio C Hamano· Nov 3, 2017, 01:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> 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" ;-)

Johannes Schindelin· Nov 3, 2017, 11:51 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Fri, 3 Nov 2017, Junio C Hamano wrote:
Show 10 quoted lines
> 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

Jonathan Nieder· Oct 30, 2017, 20:55 UTC · re: Johannes Schindelin · lore

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

Hi,
Johannes Schindelin wrote:
Show 16 quoted lines
> 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

Junio C Hamano· Oct 31, 2017, 05:48 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 11 quoted lines
>> 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.
Johannes Schindelin· Oct 31, 2017, 17:12 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
On Mon, 30 Oct 2017, Jonathan Nieder wrote:
Show 20 quoted lines
> 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

Jonathan Nieder· Oct 31, 2017, 18:09 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Nov 1, 2017, 17:07 UTC · re: Jonathan Nieder · lore

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

Hi Jonahtan,
On Tue, 31 Oct 2017, Jonathan Nieder wrote:
Show 13 quoted lines
> 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

Stefan Beller· Nov 1, 2017, 17:17 UTC · re: Johannes Schindelin · lore

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

Show 5 quoted lines
> 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!

Johannes Schindelin· Nov 1, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Nov 1, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to 2 files +49 −0

compat/mingw.c, t/t0001-init.sh

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
Johannes Schindelin· Nov 1, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +22 −1

compat/mingw.c, t/t0001-init.sh

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
Johannes Schindelin· Nov 1, 2017, 17:10 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to Documentation/git.txt +18 −0
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

← back to recent threads