[PATCH 0/2] Fixup for js/mingw-o-append

[PATCH 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffhost@microsoft.com
[jeffhost@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: 
peff@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile                           |  1 +
 compat/mingw.c                     |  2 +-
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 6 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/35
-- 
gitgitgadget

[PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create a test-tool helper to create the server side of
a windows named pipe, wait for a client connection, and
copy data written to the pipe to stdout.

Create t0051 test to route GIT_TRACE output of a command
to a named pipe using the above test-tool helper.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                           |  1 +
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 5 files changed, 96 insertions(+)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh

diff --git a/Makefile b/Makefile
index e4b503d259..b1b934d295 100644
--- a/Makefile
+++ b/Makefile
@@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
+TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
 
 TEST_PROGRAMS_NEED_X += test-dump-fsmonitor
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..7a9764cd5c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -41,6 +41,9 @@ static struct test_cmd cmds[] = {
 	{ "subprocess", cmd__subprocess },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
 	{ "wildmatch", cmd__wildmatch },
+#ifdef GIT_WINDOWS_NATIVE
+	{ "windows-named-pipe", cmd__windows_named_pipe },
+#endif
 	{ "write-cache", cmd__write_cache },
 };
 
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..01c34fe5e7 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
+#ifdef GIT_WINDOWS_NATIVE
+int cmd__windows_named_pipe(int argc, const char **argv);
+#endif
 int cmd__write_cache(int argc, const char **argv);
 
 #endif
diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c
new file mode 100644
index 0000000000..b4b752b01a
--- /dev/null
+++ b/t/helper/test-windows-named-pipe.c
@@ -0,0 +1,72 @@
+#include "test-tool.h"
+#include "git-compat-util.h"
+#include "strbuf.h"
+
+#ifdef GIT_WINDOWS_NATIVE
+static const char *usage_string = "<pipe-filename>";
+
+#define TEST_BUFSIZE (4096)
+
+int cmd__windows_named_pipe(int argc, const char **argv)
+{
+	const char *filename;
+	struct strbuf pathname = STRBUF_INIT;
+	int err;
+	HANDLE h;
+	BOOL connected;
+	char buf[TEST_BUFSIZE + 1];
+
+	if (argc < 2)
+		goto print_usage;
+	filename = argv[1];
+	if (strchr(filename, '/') || strchr(filename, '\\'))
+		goto print_usage;
+	strbuf_addf(&pathname, "//./pipe/%s", filename);
+
+	/*
+	 * Create a single instance of the server side of the named pipe.
+	 * This will allow exactly one client instance to connect to it.
+	 */
+	h = CreateNamedPipeA(
+		pathname.buf,
+		PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE,
+		PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+		PIPE_UNLIMITED_INSTANCES,
+		TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL);
+	if (h == INVALID_HANDLE_VALUE) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "CreateNamedPipe failed: %s\n",
+			strerror(err));
+		return err;
+	}
+
+	connected = ConnectNamedPipe(h, NULL)
+		? TRUE
+		: (GetLastError() == ERROR_PIPE_CONNECTED);
+	if (!connected) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "ConnectNamedPipe failed: %s\n",
+			strerror(err));
+		CloseHandle(h);
+		return err;
+	}
+
+	while (1) {
+		DWORD nbr;
+		BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL);
+		if (!success || nbr == 0)
+			break;
+		buf[nbr] = 0;
+
+		write(1, buf, nbr);
+	}
+
+	DisconnectNamedPipe(h);
+	CloseHandle(h);
+	return 0;
+
+print_usage:
+	fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);
+	return 1;
+}
+#endif
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
new file mode 100755
index 0000000000..10ac92d225
--- /dev/null
+++ b/t/t0051-windows-named-pipe.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='Windows named pipes'
+
+. ./test-lib.sh
+
+test_expect_success MINGW 'o_append write to named pipe' '
+	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
+	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
+	pid=$! &&
+	sleep 1 &&
+	GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning &&
+	wait $pid &&
+	test_cmp expect actual
+'
+
+test_done
-- 
gitgitgadget

[PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..ef03bbe5d2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	 * FILE_SHARE_WRITE is required to permit child processes
 	 * to append to the file.
 	 */
-	handle = CreateFileW(wfilename, FILE_APPEND_DATA,
+	handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
 			FILE_SHARE_WRITE | FILE_SHARE_READ,
 			NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
 	if (handle == INVALID_HANDLE_VALUE)
-- 
gitgitgadget

Re: [PATCH 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler]

GitGitGadget botched the CCs when I submitted this.
Replying here to add them.

Sorry,
Jeff

https://github.com/gitgitgadget/gitgitgadget/issues/35


On 9/7/2018 2:19 PM, Jeff Hostetler via GitGitGadget wrote:
> The recent change mingw O_APPEND change breaks writing to named pipes on
> Windows. The first commit adds a new test to confirm the breakage and the
> second commit fixes the problem. These could be squashed together or we can
> just keep the fix and omit the test if that would be better.
> 
> d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND
> 
> The new mingw_open_append() routine successfully opens the client side of
> the named pipe, but the first write() to it fails with EBADF. Adding the
> FILE_WRITE_DATA corrects the problem.
> 
>   Signed-off-by: Jeff Hostetler jeffhost@microsoft.com
> [jeffhost@microsoft.com]
> 
> Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc:
> peff@peff.net
> 
> Jeff Hostetler (2):
>    t0051: test GIT_TRACE to a windows named pipe
>    mingw: fix mingw_open_append to work with named pipes
> 
>   Makefile                           |  1 +
>   compat/mingw.c                     |  2 +-
>   t/helper/test-tool.c               |  3 ++
>   t/helper/test-tool.h               |  3 ++
>   t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
>   t/t0051-windows-named-pipe.sh      | 17 +++++++
>   6 files changed, 97 insertions(+), 1 deletion(-)
>   create mode 100644 t/helper/test-windows-named-pipe.c
>   create mode 100755 t/t0051-windows-named-pipe.sh
> 
> 
> base-commit: d641097589160eb795127d8dbcb14c877c217b60
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/35
> 

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>   compat/mingw.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 858ca14a57..ef03bbe5d2 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>   	 * FILE_SHARE_WRITE is required to permit child processes
>   	 * to append to the file.
>   	 */
> -	handle = CreateFileW(wfilename, FILE_APPEND_DATA,
> +	handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
>   			FILE_SHARE_WRITE | FILE_SHARE_READ,
>   			NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
>   	if (handle == INVALID_HANDLE_VALUE)
> 

I did not go with this version because the documentation 
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
says:

FILE_APPEND_DATA: For a file object, the right to append data to the 
file. (For local files, write operations will not overwrite existing 
data if this flag is specified without FILE_WRITE_DATA.) [...]

which could be interpreted as: Only if FILE_WRITE_DATA is not set, we 
have the guarantee that existing data in local files is not overwritten, 
i.e., new data is appended atomically.

Is this interpretation too narrow and we do get atomicity even when 
FILE_WRITE_DATA is set?

-- Hannes

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 08.09.2018 um 11:26 schrieb Johannes Sixt:
> Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 858ca14a57..ef03bbe5d2 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const 
>> *wfilename, int oflags, ...)
>>        * FILE_SHARE_WRITE is required to permit child processes
>>        * to append to the file.
>>        */
>> -    handle = CreateFileW(wfilename, FILE_APPEND_DATA,
>> +    handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
>>               FILE_SHARE_WRITE | FILE_SHARE_READ,
>>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
>>       if (handle == INVALID_HANDLE_VALUE)
>>
> 
> I did not go with this version because the documentation 
> https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
> says:
> 
> FILE_APPEND_DATA: For a file object, the right to append data to the 
> file. (For local files, write operations will not overwrite existing 
> data if this flag is specified without FILE_WRITE_DATA.) [...]
> 
> which could be interpreted as: Only if FILE_WRITE_DATA is not set, we 
> have the guarantee that existing data in local files is not overwritten, 
> i.e., new data is appended atomically.
> 
> Is this interpretation too narrow and we do get atomicity even when 
> FILE_WRITE_DATA is set?

Here is are some comments on stackoverflow which let me think that 
FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic:

https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249

-- Hannes

Re: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe

[Sebastian Schuberth]

On 9/7/2018 8:19 PM, Jeff Hostetler via GitGitGadget wrote:

> +test_expect_success MINGW 'o_append write to named pipe' '

Shouldn't this be "test_expect_failure" here, and then be changed to 
"test_expect_success" by your second patch?


-- 
Sebastian Schuberth

Re: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe

[Jeff Hostetler]

On 9/9/2018 3:28 AM, Sebastian Schuberth wrote:
> On 9/7/2018 8:19 PM, Jeff Hostetler via GitGitGadget wrote:
> 
>> +test_expect_success MINGW 'o_append write to named pipe' '
> 
> Shouldn't this be "test_expect_failure" here, and then be changed to 
> "test_expect_success" by your second patch?
> 
> 

yes. thanks!
Jeff

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler]

On 9/8/2018 2:31 PM, Johannes Sixt wrote:
> Am 08.09.2018 um 11:26 schrieb Johannes Sixt:
>> Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:
>>> diff --git a/compat/mingw.c b/compat/mingw.c
>>> index 858ca14a57..ef03bbe5d2 100644
>>> --- a/compat/mingw.c
>>> +++ b/compat/mingw.c
>>> @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const 
>>> *wfilename, int oflags, ...)
>>>        * FILE_SHARE_WRITE is required to permit child processes
>>>        * to append to the file.
>>>        */
>>> -    handle = CreateFileW(wfilename, FILE_APPEND_DATA,
>>> +    handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
>>>               FILE_SHARE_WRITE | FILE_SHARE_READ,
>>>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
>>>       if (handle == INVALID_HANDLE_VALUE)
>>>
>>
>> I did not go with this version because the documentation 
>> https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
>> says:
>>
>> FILE_APPEND_DATA: For a file object, the right to append data to the 
>> file. (For local files, write operations will not overwrite existing 
>> data if this flag is specified without FILE_WRITE_DATA.) [...]
>>
>> which could be interpreted as: Only if FILE_WRITE_DATA is not set, we 
>> have the guarantee that existing data in local files is not 
>> overwritten, i.e., new data is appended atomically.
>>
>> Is this interpretation too narrow and we do get atomicity even when 
>> FILE_WRITE_DATA is set?
> 
> Here is are some comments on stackoverflow which let me think that 
> FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic:
> 
> https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249 
> 
> 
> -- Hannes

Yeah, this whole thing is a little under-documented for my tastes.
Let's leave it as you have it.  I'll re-roll with a fix to route
named pipes to the existing _wopen() code.

thanks
Jeff

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> Yeah, this whole thing is a little under-documented for my tastes.
> Let's leave it as you have it.  I'll re-roll with a fix to route
> named pipes to the existing _wopen() code.

OK, I have these two patches in 'next', but let's postpone it for
now.  Windows port will be built with extra topics over what we have
a the release anyway, so your improved version may become part of
that and then can come back to benefit us in the next cycle ;-)

Thanks.

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler]

On 9/10/2018 12:42 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> Yeah, this whole thing is a little under-documented for my tastes.
>> Let's leave it as you have it.  I'll re-roll with a fix to route
>> named pipes to the existing _wopen() code.
> 
> OK, I have these two patches in 'next', but let's postpone it for
> now.  Windows port will be built with extra topics over what we have
> a the release anyway, so your improved version may become part of
> that and then can come back to benefit us in the next cycle ;-)
> 
> Thanks.
> 

That's fine.  This can easily wait until after 2.19.0.

Thanks
Jeff

[PATCH v2 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffhost@microsoft.com
[jeffhost@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: 
peff@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile                           |  1 +
 compat/mingw.c                     | 38 ++++++++++++++--
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 6 files changed, 131 insertions(+), 3 deletions(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/35

Range-diff vs v1:

 1:  03453cb521 ! 1:  ecb30eb47c t0051: test GIT_TRACE to a windows named pipe
     @@ -140,7 +140,7 @@
      +
      +. ./test-lib.sh
      +
     -+test_expect_success MINGW 'o_append write to named pipe' '
     ++test_expect_failure MINGW 'o_append write to named pipe' '
      +	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
      +	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
      +	pid=$! &&
 2:  f433937d55 < -:  ---------- mingw: fix mingw_open_append to work with named pipes
 -:  ---------- > 2:  f0361dd306 mingw: fix mingw_open_append to work with named pipes

-- 
gitgitgadget

[PATCH v2 1/2] t0051: test GIT_TRACE to a windows named pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create a test-tool helper to create the server side of
a windows named pipe, wait for a client connection, and
copy data written to the pipe to stdout.

Create t0051 test to route GIT_TRACE output of a command
to a named pipe using the above test-tool helper.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                           |  1 +
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 5 files changed, 96 insertions(+)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh

diff --git a/Makefile b/Makefile
index e4b503d259..b1b934d295 100644
--- a/Makefile
+++ b/Makefile
@@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
+TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
 
 TEST_PROGRAMS_NEED_X += test-dump-fsmonitor
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..7a9764cd5c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -41,6 +41,9 @@ static struct test_cmd cmds[] = {
 	{ "subprocess", cmd__subprocess },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
 	{ "wildmatch", cmd__wildmatch },
+#ifdef GIT_WINDOWS_NATIVE
+	{ "windows-named-pipe", cmd__windows_named_pipe },
+#endif
 	{ "write-cache", cmd__write_cache },
 };
 
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..01c34fe5e7 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
+#ifdef GIT_WINDOWS_NATIVE
+int cmd__windows_named_pipe(int argc, const char **argv);
+#endif
 int cmd__write_cache(int argc, const char **argv);
 
 #endif
diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c
new file mode 100644
index 0000000000..b4b752b01a
--- /dev/null
+++ b/t/helper/test-windows-named-pipe.c
@@ -0,0 +1,72 @@
+#include "test-tool.h"
+#include "git-compat-util.h"
+#include "strbuf.h"
+
+#ifdef GIT_WINDOWS_NATIVE
+static const char *usage_string = "<pipe-filename>";
+
+#define TEST_BUFSIZE (4096)
+
+int cmd__windows_named_pipe(int argc, const char **argv)
+{
+	const char *filename;
+	struct strbuf pathname = STRBUF_INIT;
+	int err;
+	HANDLE h;
+	BOOL connected;
+	char buf[TEST_BUFSIZE + 1];
+
+	if (argc < 2)
+		goto print_usage;
+	filename = argv[1];
+	if (strchr(filename, '/') || strchr(filename, '\\'))
+		goto print_usage;
+	strbuf_addf(&pathname, "//./pipe/%s", filename);
+
+	/*
+	 * Create a single instance of the server side of the named pipe.
+	 * This will allow exactly one client instance to connect to it.
+	 */
+	h = CreateNamedPipeA(
+		pathname.buf,
+		PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE,
+		PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+		PIPE_UNLIMITED_INSTANCES,
+		TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL);
+	if (h == INVALID_HANDLE_VALUE) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "CreateNamedPipe failed: %s\n",
+			strerror(err));
+		return err;
+	}
+
+	connected = ConnectNamedPipe(h, NULL)
+		? TRUE
+		: (GetLastError() == ERROR_PIPE_CONNECTED);
+	if (!connected) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "ConnectNamedPipe failed: %s\n",
+			strerror(err));
+		CloseHandle(h);
+		return err;
+	}
+
+	while (1) {
+		DWORD nbr;
+		BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL);
+		if (!success || nbr == 0)
+			break;
+		buf[nbr] = 0;
+
+		write(1, buf, nbr);
+	}
+
+	DisconnectNamedPipe(h);
+	CloseHandle(h);
+	return 0;
+
+print_usage:
+	fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);
+	return 1;
+}
+#endif
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
new file mode 100755
index 0000000000..e3c36341a0
--- /dev/null
+++ b/t/t0051-windows-named-pipe.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='Windows named pipes'
+
+. ./test-lib.sh
+
+test_expect_failure MINGW 'o_append write to named pipe' '
+	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
+	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
+	pid=$! &&
+	sleep 1 &&
+	GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning &&
+	wait $pid &&
+	test_cmp expect actual
+'
+
+test_done
-- 
gitgitgadget

[PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/mingw.c                | 38 ++++++++++++++++++++++++++++++++---
 t/t0051-windows-named-pipe.sh |  2 +-
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..f87376b26a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+/*
+ * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
+ *
+ * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes.  Instead it creates
+ * a named pipe client handle that cannot be written to.  Callers should
+ * just use the regular _wopen() for them.  (And since client handle gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
 static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 {
 	HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 			NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
 	if (handle == INVALID_HANDLE_VALUE)
 		return errno = err_win_to_posix(GetLastError()), -1;
+
 	/*
 	 * No O_APPEND here, because the CRT uses it only to reset the
-	 * file pointer to EOF on write(); but that is not necessary
-	 * for a file created with FILE_APPEND_DATA.
+	 * file pointer to EOF before each write(); but that is not
+	 * necessary (and may lead to races) for a file created with
+	 * FILE_APPEND_DATA.
 	 */
 	fd = _open_osfhandle((intptr_t)handle, O_BINARY);
 	if (fd < 0)
@@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	return fd;
 }
 
+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int mingw_is_local_named_pipe_path(const char *filename)
+{
+	return (IS_SBS(filename[0]) &&
+		IS_SBS(filename[1]) &&
+		filename[2] == '.'  &&
+		IS_SBS(filename[3]) &&
+		!strncasecmp(filename+4, "pipe", 4) &&
+		IS_SBS(filename[8]) &&
+		filename[9]);
+}
+#undef IS_SBS
+
 int mingw_open (const char *filename, int oflags, ...)
 {
 	typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
@@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
 	if (filename && !strcmp(filename, "/dev/null"))
 		filename = "nul";
 
-	if (oflags & O_APPEND)
+	if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
 	else
 		open_fn = _wopen;
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
index e3c36341a0..10ac92d225 100755
--- a/t/t0051-windows-named-pipe.sh
+++ b/t/t0051-windows-named-pipe.sh
@@ -4,7 +4,7 @@ test_description='Windows named pipes'
 
 . ./test-lib.sh
 
-test_expect_failure MINGW 'o_append write to named pipe' '
+test_expect_success MINGW 'o_append write to named pipe' '
 	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
 	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
 	pid=$! &&
-- 
gitgitgadget

Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 858ca14a57..f87376b26a 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
>   	return ret;
>   }
>   
> +/*
> + * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
> + * is documented in [1] as opening a writable file handle in append mode.
> + * (It is believed that) this is atomic since it is maintained by the
> + * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
> + *
> + * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
> + *
> + * This trick does not appear to work for named pipes.  Instead it creates
> + * a named pipe client handle that cannot be written to.  Callers should
> + * just use the regular _wopen() for them.  (And since client handle gets
> + * bound to a unique server handle, it isn't really an issue.)
> + */
>   static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>   {
>   	HANDLE handle;
> @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>   			NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
>   	if (handle == INVALID_HANDLE_VALUE)
>   		return errno = err_win_to_posix(GetLastError()), -1;
> +
>   	/*
>   	 * No O_APPEND here, because the CRT uses it only to reset the
> -	 * file pointer to EOF on write(); but that is not necessary
> -	 * for a file created with FILE_APPEND_DATA.
> +	 * file pointer to EOF before each write(); but that is not
> +	 * necessary (and may lead to races) for a file created with
> +	 * FILE_APPEND_DATA.
>   	 */
>   	fd = _open_osfhandle((intptr_t)handle, O_BINARY);
>   	if (fd < 0)
> @@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>   	return fd;
>   }
>   
> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
> +/*
> + * Does the pathname map to the local named pipe filesystem?
> + * That is, does it have a "//./pipe/" prefix?
> + */
> +static int mingw_is_local_named_pipe_path(const char *filename)
> +{
> +	return (IS_SBS(filename[0]) &&
> +		IS_SBS(filename[1]) &&
> +		filename[2] == '.'  &&
> +		IS_SBS(filename[3]) &&
> +		!strncasecmp(filename+4, "pipe", 4) &&
> +		IS_SBS(filename[8]) &&
> +		filename[9]);
> +}
> +#undef IS_SBS
> +
>   int mingw_open (const char *filename, int oflags, ...)
>   {
>   	typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
>   	if (filename && !strcmp(filename, "/dev/null"))
>   		filename = "nul";
>   
> -	if (oflags & O_APPEND)
> +	if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
>   		open_fn = mingw_open_append;
>   	else
>   		open_fn = _wopen;

This looks reasonable.

I wonder which part of the code uses local named pipes. Is it downstream 
in Git for Windows or one of the topics in flight?

-- Hannes

Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler]

On 9/10/2018 3:45 PM, Johannes Sixt wrote:
> Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 858ca14a57..f87376b26a 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
>>       return ret;
>>   }
>> +/*
>> + * Calling CreateFile() using FILE_APPEND_DATA and without 
>> FILE_WRITE_DATA
>> + * is documented in [1] as opening a writable file handle in append 
>> mode.
>> + * (It is believed that) this is atomic since it is maintained by the
>> + * kernel unlike the O_APPEND flag which is racily maintained by the 
>> CRT.
>> + *
>> + * [1] 
>> https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
>>
>> + *
>> + * This trick does not appear to work for named pipes.  Instead it 
>> creates
>> + * a named pipe client handle that cannot be written to.  Callers should
>> + * just use the regular _wopen() for them.  (And since client handle 
>> gets
>> + * bound to a unique server handle, it isn't really an issue.)
>> + */
>>   static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>>   {
>>       HANDLE handle;
>> @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const 
>> *wfilename, int oflags, ...)
>>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
>>       if (handle == INVALID_HANDLE_VALUE)
>>           return errno = err_win_to_posix(GetLastError()), -1;
>> +
>>       /*
>>        * No O_APPEND here, because the CRT uses it only to reset the
>> -     * file pointer to EOF on write(); but that is not necessary
>> -     * for a file created with FILE_APPEND_DATA.
>> +     * file pointer to EOF before each write(); but that is not
>> +     * necessary (and may lead to races) for a file created with
>> +     * FILE_APPEND_DATA.
>>        */
>>       fd = _open_osfhandle((intptr_t)handle, O_BINARY);
>>       if (fd < 0)
>> @@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const 
>> *wfilename, int oflags, ...)
>>       return fd;
>>   }
>> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
>> +/*
>> + * Does the pathname map to the local named pipe filesystem?
>> + * That is, does it have a "//./pipe/" prefix?
>> + */
>> +static int mingw_is_local_named_pipe_path(const char *filename)
>> +{
>> +    return (IS_SBS(filename[0]) &&
>> +        IS_SBS(filename[1]) &&
>> +        filename[2] == '.'  &&
>> +        IS_SBS(filename[3]) &&
>> +        !strncasecmp(filename+4, "pipe", 4) &&
>> +        IS_SBS(filename[8]) &&
>> +        filename[9]);
>> +}
>> +#undef IS_SBS
>> +
>>   int mingw_open (const char *filename, int oflags, ...)
>>   {
>>       typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, 
>> ...);
>> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, 
>> ...)
>>       if (filename && !strcmp(filename, "/dev/null"))
>>           filename = "nul";
>> -    if (oflags & O_APPEND)
>> +    if ((oflags & O_APPEND) && 
>> !mingw_is_local_named_pipe_path(filename))
>>           open_fn = mingw_open_append;
>>       else
>>           open_fn = _wopen;
> 
> This looks reasonable.

Thanks for the review.

> 
> I wonder which part of the code uses local named pipes. Is it downstream 
> in Git for Windows or one of the topics in flight?
> 
> -- Hannes

I'm wanting to use them as a tracing target option in my trace2 series
currently in progress.

Jeff

Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>>   +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))

I think you already have mingw_is_dir_sep() and its shorter alias
is_dir_sep() available to you.

>> +/*
>> + * Does the pathname map to the local named pipe filesystem?
>> + * That is, does it have a "//./pipe/" prefix?
>> + */
>> +static int mingw_is_local_named_pipe_path(const char *filename)

There is no need to prefix mingw_ to this function that is file
local static.  Isn't is_local_named_pipe() descriptive and unique
enough?

>> +{
>> +	return (IS_SBS(filename[0]) &&
>> +		IS_SBS(filename[1]) &&
>> +		filename[2] == '.'  &&
>> +		IS_SBS(filename[3]) &&
>> +		!strncasecmp(filename+4, "pipe", 4) &&
>> +		IS_SBS(filename[8]) &&
>> +		filename[9]);
>> +}
>> +#undef IS_SBS

It is kind-of surprising that there hasn't been any existing need
for a helper function that would allow us to write this function
like so:

	static int is_local_named_pipe(const char *path)
	{
		return path_is_in_directory(path, "//./pipe/");
	}

Not a suggestion to add such a thing; as long as we know there is no
other codepath that would benefit from having one, a generalization
like that can and should wait.

>>   int mingw_open (const char *filename, int oflags, ...)
>>   {
>>   	typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
>> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
>>   	if (filename && !strcmp(filename, "/dev/null"))
>>   		filename = "nul";
>>   -	if (oflags & O_APPEND)
>> +	if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
>>   		open_fn = mingw_open_append;
>>   	else
>>   		open_fn = _wopen;
>
> This looks reasonable.
>
> I wonder which part of the code uses local named pipes. Is it
> downstream in Git for Windows or one of the topics in flight?
>
> -- Hannes

Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler]

On 9/10/2018 6:00 PM, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>>    +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
> 
> I think you already have mingw_is_dir_sep() and its shorter alias
> is_dir_sep() available to you.

good catch. thanks.


>>> +/*
>>> + * Does the pathname map to the local named pipe filesystem?
>>> + * That is, does it have a "//./pipe/" prefix?
>>> + */
>>> +static int mingw_is_local_named_pipe_path(const char *filename)
> 
> There is no need to prefix mingw_ to this function that is file
> local static.  Isn't is_local_named_pipe() descriptive and unique
> enough?

right. will do.


>>> +{
>>> +	return (IS_SBS(filename[0]) &&
>>> +		IS_SBS(filename[1]) &&
>>> +		filename[2] == '.'  &&
>>> +		IS_SBS(filename[3]) &&
>>> +		!strncasecmp(filename+4, "pipe", 4) &&
>>> +		IS_SBS(filename[8]) &&
>>> +		filename[9]);
>>> +}
>>> +#undef IS_SBS
> 
> It is kind-of surprising that there hasn't been any existing need
> for a helper function that would allow us to write this function
> like so:
> 
> 	static int is_local_named_pipe(const char *path)
> 	{
> 		return path_is_in_directory(path, "//./pipe/");
> 	}
> 
> Not a suggestion to add such a thing; as long as we know there is no
> other codepath that would benefit from having one, a generalization
> like that can and should wait.

Yeah, I don't think we need something that general just yet.  Named
pipes exist in a special namespace using the UNC/network-share syntax
(rather than the DOS drive-letter syntax), and we don't do much with
UNC paths yet.

Perhaps, later we could have something to try splitting a UNC
path into <server>, <volume>, and <relative-path> and then have
is_local_named_pipe() verify the first 2 are as expected.
Or have a function like is_path_in_volume(path, server, volume)
that does the tests without cutting up strings and allocating.
We could do either, but I don't think we need to be that general
yet.

Thanks,
Jeff
		

[PATCH v3 0/2] Fixup for js/mingw-o-append

[Jeff Hostetler via GitGitGadget]

The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.

d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND

The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.

 Signed-off-by: Jeff Hostetler jeffhost@microsoft.com
[jeffhost@microsoft.com]

Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: 
peff@peff.net

Jeff Hostetler (2):
  t0051: test GIT_TRACE to a windows named pipe
  mingw: fix mingw_open_append to work with named pipes

 Makefile                           |  1 +
 compat/mingw.c                     | 36 +++++++++++++--
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 6 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh


base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/35

Range-diff vs v2:

 1:  ecb30eb47c = 1:  ecb30eb47c t0051: test GIT_TRACE to a windows named pipe
 2:  f0361dd306 ! 2:  5592300ca5 mingw: fix mingw_open_append to work with named pipes
     @@ -46,22 +46,20 @@
       	return fd;
       }
       
     -+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
      +/*
      + * Does the pathname map to the local named pipe filesystem?
      + * That is, does it have a "//./pipe/" prefix?
      + */
     -+static int mingw_is_local_named_pipe_path(const char *filename)
     ++static int is_local_named_pipe_path(const char *filename)
      +{
     -+	return (IS_SBS(filename[0]) &&
     -+		IS_SBS(filename[1]) &&
     ++	return (is_dir_sep(filename[0]) &&
     ++		is_dir_sep(filename[1]) &&
      +		filename[2] == '.'  &&
     -+		IS_SBS(filename[3]) &&
     ++		is_dir_sep(filename[3]) &&
      +		!strncasecmp(filename+4, "pipe", 4) &&
     -+		IS_SBS(filename[8]) &&
     ++		is_dir_sep(filename[8]) &&
      +		filename[9]);
      +}
     -+#undef IS_SBS
      +
       int mingw_open (const char *filename, int oflags, ...)
       {
     @@ -71,7 +69,7 @@
       		filename = "nul";
       
      -	if (oflags & O_APPEND)
     -+	if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
     ++	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
       		open_fn = mingw_open_append;
       	else
       		open_fn = _wopen;

-- 
gitgitgadget

[PATCH v3 1/2] t0051: test GIT_TRACE to a windows named pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create a test-tool helper to create the server side of
a windows named pipe, wait for a client connection, and
copy data written to the pipe to stdout.

Create t0051 test to route GIT_TRACE output of a command
to a named pipe using the above test-tool helper.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile                           |  1 +
 t/helper/test-tool.c               |  3 ++
 t/helper/test-tool.h               |  3 ++
 t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++
 t/t0051-windows-named-pipe.sh      | 17 +++++++
 5 files changed, 96 insertions(+)
 create mode 100644 t/helper/test-windows-named-pipe.c
 create mode 100755 t/t0051-windows-named-pipe.sh

diff --git a/Makefile b/Makefile
index e4b503d259..b1b934d295 100644
--- a/Makefile
+++ b/Makefile
@@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
+TEST_BUILTINS_OBJS += test-windows-named-pipe.o
 TEST_BUILTINS_OBJS += test-write-cache.o
 
 TEST_PROGRAMS_NEED_X += test-dump-fsmonitor
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..7a9764cd5c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -41,6 +41,9 @@ static struct test_cmd cmds[] = {
 	{ "subprocess", cmd__subprocess },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
 	{ "wildmatch", cmd__wildmatch },
+#ifdef GIT_WINDOWS_NATIVE
+	{ "windows-named-pipe", cmd__windows_named_pipe },
+#endif
 	{ "write-cache", cmd__write_cache },
 };
 
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..01c34fe5e7 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
+#ifdef GIT_WINDOWS_NATIVE
+int cmd__windows_named_pipe(int argc, const char **argv);
+#endif
 int cmd__write_cache(int argc, const char **argv);
 
 #endif
diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c
new file mode 100644
index 0000000000..b4b752b01a
--- /dev/null
+++ b/t/helper/test-windows-named-pipe.c
@@ -0,0 +1,72 @@
+#include "test-tool.h"
+#include "git-compat-util.h"
+#include "strbuf.h"
+
+#ifdef GIT_WINDOWS_NATIVE
+static const char *usage_string = "<pipe-filename>";
+
+#define TEST_BUFSIZE (4096)
+
+int cmd__windows_named_pipe(int argc, const char **argv)
+{
+	const char *filename;
+	struct strbuf pathname = STRBUF_INIT;
+	int err;
+	HANDLE h;
+	BOOL connected;
+	char buf[TEST_BUFSIZE + 1];
+
+	if (argc < 2)
+		goto print_usage;
+	filename = argv[1];
+	if (strchr(filename, '/') || strchr(filename, '\\'))
+		goto print_usage;
+	strbuf_addf(&pathname, "//./pipe/%s", filename);
+
+	/*
+	 * Create a single instance of the server side of the named pipe.
+	 * This will allow exactly one client instance to connect to it.
+	 */
+	h = CreateNamedPipeA(
+		pathname.buf,
+		PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE,
+		PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+		PIPE_UNLIMITED_INSTANCES,
+		TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL);
+	if (h == INVALID_HANDLE_VALUE) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "CreateNamedPipe failed: %s\n",
+			strerror(err));
+		return err;
+	}
+
+	connected = ConnectNamedPipe(h, NULL)
+		? TRUE
+		: (GetLastError() == ERROR_PIPE_CONNECTED);
+	if (!connected) {
+		err = err_win_to_posix(GetLastError());
+		fprintf(stderr, "ConnectNamedPipe failed: %s\n",
+			strerror(err));
+		CloseHandle(h);
+		return err;
+	}
+
+	while (1) {
+		DWORD nbr;
+		BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL);
+		if (!success || nbr == 0)
+			break;
+		buf[nbr] = 0;
+
+		write(1, buf, nbr);
+	}
+
+	DisconnectNamedPipe(h);
+	CloseHandle(h);
+	return 0;
+
+print_usage:
+	fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);
+	return 1;
+}
+#endif
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
new file mode 100755
index 0000000000..e3c36341a0
--- /dev/null
+++ b/t/t0051-windows-named-pipe.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='Windows named pipes'
+
+. ./test-lib.sh
+
+test_expect_failure MINGW 'o_append write to named pipe' '
+	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
+	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
+	pid=$! &&
+	sleep 1 &&
+	GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning &&
+	wait $pid &&
+	test_cmp expect actual
+'
+
+test_done
-- 
gitgitgadget

[PATCH v3 2/2] mingw: fix mingw_open_append to work with named pipes

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/mingw.c                | 36 ++++++++++++++++++++++++++++++++---
 t/t0051-windows-named-pipe.sh |  2 +-
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..18caf21969 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
 	return ret;
 }
 
+/*
+ * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
+ *
+ * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes.  Instead it creates
+ * a named pipe client handle that cannot be written to.  Callers should
+ * just use the regular _wopen() for them.  (And since client handle gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
 static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 {
 	HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 			NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
 	if (handle == INVALID_HANDLE_VALUE)
 		return errno = err_win_to_posix(GetLastError()), -1;
+
 	/*
 	 * No O_APPEND here, because the CRT uses it only to reset the
-	 * file pointer to EOF on write(); but that is not necessary
-	 * for a file created with FILE_APPEND_DATA.
+	 * file pointer to EOF before each write(); but that is not
+	 * necessary (and may lead to races) for a file created with
+	 * FILE_APPEND_DATA.
 	 */
 	fd = _open_osfhandle((intptr_t)handle, O_BINARY);
 	if (fd < 0)
@@ -371,6 +386,21 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	return fd;
 }
 
+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int is_local_named_pipe_path(const char *filename)
+{
+	return (is_dir_sep(filename[0]) &&
+		is_dir_sep(filename[1]) &&
+		filename[2] == '.'  &&
+		is_dir_sep(filename[3]) &&
+		!strncasecmp(filename+4, "pipe", 4) &&
+		is_dir_sep(filename[8]) &&
+		filename[9]);
+}
+
 int mingw_open (const char *filename, int oflags, ...)
 {
 	typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
@@ -387,7 +417,7 @@ int mingw_open (const char *filename, int oflags, ...)
 	if (filename && !strcmp(filename, "/dev/null"))
 		filename = "nul";
 
-	if (oflags & O_APPEND)
+	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
 	else
 		open_fn = _wopen;
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
index e3c36341a0..10ac92d225 100755
--- a/t/t0051-windows-named-pipe.sh
+++ b/t/t0051-windows-named-pipe.sh
@@ -4,7 +4,7 @@ test_description='Windows named pipes'
 
 . ./test-lib.sh
 
-test_expect_failure MINGW 'o_append write to named pipe' '
+test_expect_success MINGW 'o_append write to named pipe' '
 	GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
 	{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
 	pid=$! &&
-- 
gitgitgadget