Re: [PATCH v2 5/6] Win32: Thread-safe windows console output

2014-06-13 Thread Johannes Sixt

Am 07.06.2014 09:57, schrieb Stepan Kasal:

From: Karsten Blees bl...@dcon.de
Date: Sat, 14 Jan 2012 22:24:19 +0100

Winansi.c has many static variables that are accessed and modified from
the [v][f]printf / fputs functions overridden in the file. This may cause
multi threaded git commands that print to the console to produce corrupted
output or even crash.

Additionally, winansi.c doesn't override all functions that can be used to
print to the console (e.g. fwrite, write, fputc are missing), so that ANSI
escapes don't work properly for some git commands (e.g. git-grep).

Instead of doing ANSI emulation in just a few wrapped functions on top of
the IO API, let's plug into the IO system and take advantage of the thread
safety inherent to the IO system.

Redirect stdout and stderr to a pipe if they point to the console. A
background thread reads from the pipe, handles ANSI escape sequences and
UTF-8 to UTF-16 conversion, then writes to the console.


There's something fishy with this patch. Please checkout and build 
eac14f8909d9. Then run t5000-tar-tree.sh like so from a CMD prompt:


  sh t5000-tar-tree.sh -v -i

Notice that in test 36 (invoke tar filter by extension) the tar file is 
written to the console instead of the file. Hit Ctrl-C to interrupt the 
test; do not remove the trash directory. You can verify the incorrect 
behavior like this:


  cd t\trash directory.t5000-tar-tree
  ..\..\git archive -o config-implicit.tar.foo HEAD

It writes the tar file to the console. When you build the parent commit, 
and repeat these two commands, there is no unexpected console output.


The patch fcd428f4a952 (Win32: fix broken pipe detection) does not fix the 
incorrect behavior.


I haven't dug (and won't dig) deeper than that.

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 5/6] Win32: Thread-safe windows console output

2014-06-07 Thread Stepan Kasal
From: Karsten Blees bl...@dcon.de
Date: Sat, 14 Jan 2012 22:24:19 +0100

Winansi.c has many static variables that are accessed and modified from
the [v][f]printf / fputs functions overridden in the file. This may cause
multi threaded git commands that print to the console to produce corrupted
output or even crash.

Additionally, winansi.c doesn't override all functions that can be used to
print to the console (e.g. fwrite, write, fputc are missing), so that ANSI
escapes don't work properly for some git commands (e.g. git-grep).

Instead of doing ANSI emulation in just a few wrapped functions on top of
the IO API, let's plug into the IO system and take advantage of the thread
safety inherent to the IO system.

Redirect stdout and stderr to a pipe if they point to the console. A
background thread reads from the pipe, handles ANSI escape sequences and
UTF-8 to UTF-16 conversion, then writes to the console.

The pipe-based stdout and stderr replacements must be set to unbuffered, as
MSVCRT doesn't support line buffering and fully buffered streams are
inappropriate for console output.

Due to the byte-oriented pipe, ANSI escape sequences and multi-byte UTF-8
sequences can no longer be expected to arrive in one piece. Replace the
string-based ansi_emulate() with a simple stateful parser (this also fixes
colored diff hunk headers, which were broken as of commit 2efcc977).

Override isatty to return true for the pipes redirecting to the console.

Exec/spawn obtain the original console handle to pass to the next process
via winansi_get_osfhandle().

All other overrides are gone, the default stdio implementations work as
expected with the piped stdout/stderr descriptors.

Global variables are either initialized on startup (single threaded) or
exclusively modified by the background thread. Threads communicate through
the pipe, no further synchronization is necessary.

The background thread is terminated by disonnecting the pipe after flushing
the stdio and pipe buffers. This doesn't work for anonymous pipes (created
via CreatePipe), as DisconnectNamedPipe only works on the read end, which
discards remaining data. Thus we have to setup the pipe manually, with the
write end beeing the server (opened with CreateNamedPipe) and the read end
the client (opened with CreateFile).

Limitations: doesn't track reopened or duped file descriptors, i.e.:
- fdopen(1/2) returns fully buffered streams
- dup(1/2), dup2(1/2) returns normal pipe descriptors (i.e. isatty() =
  false, winansi_get_osfhandle won't return the original console handle)

Currently, only the git-format-patch command uses xfdopen(xdup(1)) (see
realstdout in builtin/log.c), but works well with these limitations.

Many thanks to Atsushi Nakagawa at...@chejz.com for suggesting and
reviewing the thread-exit-mechanism.

Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
 compat/mingw.c   |   9 +-
 compat/mingw.h   |  12 +-
 compat/winansi.c | 401 ---
 3 files changed, 273 insertions(+), 149 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6f1fb10..d242557 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -865,9 +865,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char 
**argv, char **env,
memset(si, 0, sizeof(si));
si.cb = sizeof(si);
si.dwFlags = STARTF_USESTDHANDLES;
-   si.hStdInput = (HANDLE) _get_osfhandle(fhin);
-   si.hStdOutput = (HANDLE) _get_osfhandle(fhout);
-   si.hStdError = (HANDLE) _get_osfhandle(fherr);
+   si.hStdInput = winansi_get_osfhandle(fhin);
+   si.hStdOutput = winansi_get_osfhandle(fhout);
+   si.hStdError = winansi_get_osfhandle(fherr);
 
/* concatenate argv, quoting args as we go */
strbuf_init(args, 0);
@@ -1946,4 +1946,7 @@ void mingw_startup()
_setmode(_fileno(stdin), _O_BINARY);
_setmode(_fileno(stdout), _O_BINARY);
_setmode(_fileno(stderr), _O_BINARY);
+
+   /* initialize Unicode console */
+   winansi_init();
 }
diff --git a/compat/mingw.h b/compat/mingw.h
index 921ba08..4b638d8 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -317,14 +317,10 @@ int mingw_raise(int sig);
  * ANSI emulation wrappers
  */
 
-int winansi_fputs(const char *str, FILE *stream);
-int winansi_printf(const char *format, ...) __attribute__((format (printf, 1, 
2)));
-int winansi_fprintf(FILE *stream, const char *format, ...) 
__attribute__((format (printf, 2, 3)));
-int winansi_vfprintf(FILE *stream, const char *format, va_list list);
-#define fputs winansi_fputs
-#define printf(...) winansi_printf(__VA_ARGS__)
-#define fprintf(...) winansi_fprintf(__VA_ARGS__)
-#define vfprintf winansi_vfprintf
+void winansi_init(void);
+int winansi_isatty(int fd);
+HANDLE winansi_get_osfhandle(int fd);
+#define isatty winansi_isatty
 
 /*
  * git specific compatibility
diff --git a/compat/winansi.c b/compat/winansi.c
index bec6713..fcdd6dc 100644
---