threads / patch / 42732

patchRemove obsolete comment from color.h

Subject: [PATCH] Remove obsolete comment from color.h

## tl;dr

5 messages between Jun 28, 2016 and Jun 28, 2016. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Johannes Schindelin· Jun 28, 2016, 10:02 UTC · lore

Originally, ANSI color sequences were supported on Windows only by overriding the printf() and fprintf() functions, as mentioned in e7821d7 (Add a notice that only certain functions can print color escape codes, 2009-11-27).

As of eac14f8 (Win32: Thread-safe windows console output, 2012-01-14), however, this is no longer the case, as the ANSI color sequence support code needed to be replaced with a thread-safe version, one side effect being that stdout and stderr handled no matter which function is used to write to it.

So let's just remove the comment that is now obsolete.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/winansi-color-v1
 color.h | 5 -----
 1 file changed, 5 deletions(-)
Show changes to color.h +0 −5
diff --git a/color.h b/color.h
index e155d13..b9ead16 100644
--- a/color.h
+++ b/color.h
@@ -18,11 +18,6 @@ struct strbuf;
  */
 #define COLOR_MAXLEN 70
 
-/*
- * IMPORTANT: Due to the way these color codes are emulated on Windows,
- * write them only using printf(), fprintf(), and fputs(). In particular,
- * do not use puts() or write().
- */
 #define GIT_COLOR_NORMAL	""
 #define GIT_COLOR_RESET		"\033[m"
 #define GIT_COLOR_BOLD		"\033[1m"
-- 
2.9.0.118.g0e1a633

base-commit: cf4c2cfe52be5bd973a4838f73a35d3959ce2f43
Junio C Hamano· Jun 28, 2016, 15:42 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Remove obsolete comment from color.h

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 10 quoted lines
> Originally, ANSI color sequences were supported on Windows only by
> overriding the printf() and fprintf() functions, as mentioned in e7821d7
> (Add a notice that only certain functions can print color escape codes,
> 2009-11-27).
>
> As of eac14f8 (Win32: Thread-safe windows console output, 2012-01-14),
> however, this is no longer the case, as the ANSI color sequence support
> code needed to be replaced with a thread-safe version, one side effect
> being that stdout and stderr handled no matter which function is used to
> write to it.

So as long as we write via stdio to stdout/stderr, you can show colors? Or is it now stronger, in that as long as we do anything that ends up writing to file descriptors 1 or 2, you can show colors?

> So let's just remove the comment that is now obsolete.
Thanks.
Show 22 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/winansi-color-v1
>  color.h | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/color.h b/color.h
> index e155d13..b9ead16 100644
> --- a/color.h
> +++ b/color.h
> @@ -18,11 +18,6 @@ struct strbuf;
>   */
>  #define COLOR_MAXLEN 70
>  
> -/*
> - * IMPORTANT: Due to the way these color codes are emulated on Windows,
> - * write them only using printf(), fprintf(), and fputs(). In particular,
> - * do not use puts() or write().
> - */
>  #define GIT_COLOR_NORMAL	""
>  #define GIT_COLOR_RESET		"\033[m"
>  #define GIT_COLOR_BOLD		"\033[1m"
Johannes Schindelin· Jun 28, 2016, 16:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] Remove obsolete comment from color.h

Hi Junio,
On Tue, 28 Jun 2016, Junio C Hamano wrote:
Show 17 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Originally, ANSI color sequences were supported on Windows only by
> > overriding the printf() and fprintf() functions, as mentioned in e7821d7
> > (Add a notice that only certain functions can print color escape codes,
> > 2009-11-27).
> >
> > As of eac14f8 (Win32: Thread-safe windows console output, 2012-01-14),
> > however, this is no longer the case, as the ANSI color sequence support
> > code needed to be replaced with a thread-safe version, one side effect
> > being that stdout and stderr handled no matter which function is used to
> > write to it.
> 
> So as long as we write via stdio to stdout/stderr, you can show
> colors?  Or is it now stronger, in that as long as we do anything
> that ends up writing to file descriptors 1 or 2, you can show
> colors?

It means that we override stdout/stderr with a pipe that is parsed for the color sequences. If stdout or stderr are reopened later, this color handling will not apply to the new file descriptor/stream.

Essentially, the caveat in color.h that one should use fprintf() and printf() when outputting color sequences to a terminal no longer holds true. fwrite() or write() work just as well.

Ciao, Dscho

Junio C Hamano· Jun 28, 2016, 16:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Remove obsolete comment from color.h

On Tue, Jun 28, 2016 at 9:24 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines
>
> On Tue, 28 Jun 2016, Junio C Hamano wrote:
>>
>> So as long as we write via stdio to stdout/stderr, you can show
>> colors?  Or is it now stronger, in that as long as we do anything
>> that ends up writing to file descriptors 1 or 2, you can show
>> colors?
>
> Essentially, the caveat in color.h that one should use fprintf() and
> printf() when outputting color sequences to a terminal no longer holds
> true. fwrite() or write() work just as well.

In short, the answer is "the latter"? That is a great news. It means that Lukas can use write(2) in the recv_sideband() patch to ensure atomicity, without having to rely on the observed behaviour of fprintf(stderr, "%s", ...) that as long as the output is within reasonable size the call seems to result in a single write(2).

Thanks for a clarification.
Jeff King· Jun 28, 2016, 16:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Remove obsolete comment from color.h

On Tue, Jun 28, 2016 at 06:24:05PM +0200, Johannes Schindelin wrote:
> Essentially, the caveat in color.h that one should use fprintf() and
> printf() when outputting color sequences to a terminal no longer holds
> true. fwrite() or write() work just as well.
Cool. One less thing to worry about.
Thanks for digging into this.
-Peff

← back to recent threads