[PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Naumov, Michael (North Sydney)]

Some git tools such as GitExtensions for Windows use environment variable TERM=msys which causes the weird ANSI sequence shown for the messages returned from server-side hooks
We add those ANSI sequences to help format sideband data on the user's terminal. However, GitExtensions is not using a terminal, and the ANSI sequences just confuses it. We can recognize this use by checking isatty().
See https://github.com/gitextensions/gitextensions/issues/1313 for more details

NOTE: I considered to cover the case that a pager has already been started. But decided that is probably not worth worrying about here, though, as we shouldn't be using a pager for commands that do network communications (and if we do, omitting the magic line-clearing signal is probably a sane thing to do).

Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
Thanks-to: Jeff King <peff@peff.net>
---
 sideband.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sideband.c b/sideband.c
index d1125f5..7f9dc22 100644
--- a/sideband.c
+++ b/sideband.c
@@ -30,7 +30,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
-	if (term && strcmp(term, "dumb"))
+	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-- 
1.8.3.msysgit.0

P.S. I gave up trying to send this letter from gmail, it eats my tab character
P.S 2. Sorry, my tab character has been eaten again!

Regards,
Michael

Re: [PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Jeff King]

On Wed, May 28, 2014 at 03:12:15AM +0000, Naumov, Michael (North Sydney) wrote:

> Some git tools such as GitExtensions for Windows use environment
> variable TERM=msys which causes the weird ANSI sequence shown for the
> messages returned from server-side hooks
> We add those ANSI sequences to help format sideband data on the user's
> terminal. However, GitExtensions is not using a terminal, and the ANSI
> sequences just confuses it. We can recognize this use by checking
> isatty().
> See https://github.com/gitextensions/gitextensions/issues/1313 for
> more details

Please wrap your commit messages (and emails) at something reasonable to
fit on an 80-char terminal with indentation (I usually use 60, but
anything <= 72 is probably fine).

Other than that, I think the patch looks fine.

> P.S. I gave up trying to send this letter from gmail, it eats my tab character
> P.S 2. Sorry, my tab character has been eaten again!

This version looks OK to me (and applies via git-am).

-Peff

[PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Naumov, Michael (North Sydney)]

Some git tools such as GitExtensions for Windows use environment
variable TERM=msys which causes the weird ANSI sequence shown for the
messages returned from server-side hooks
We add those ANSI sequences to help format sideband data on the user's
terminal. However, GitExtensions is not using a terminal, and the ANSI
sequences just confuses it. We can recognize this use by checking
isatty().
See https://github.com/gitextensions/gitextensions/issues/1313 for
more details

NOTE: I considered to cover the case that a pager has already been
started. But decided that is probably not worth worrying about here,
though, as we shouldn't be using a pager for commands that do network
communications (and if we do, omitting the magic line-clearing signal
is probably a sane thing to do).

Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
Thanks-to: Jeff King <peff@peff.net>
---
 sideband.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sideband.c b/sideband.c
index d1125f5..7f9dc22 100644
--- a/sideband.c
+++ b/sideband.c
@@ -30,7 +30,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
-	if (term && strcmp(term, "dumb"))
+	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-- 
1.8.3.msysgit.0

P.S. I gave up trying to send this letter from gmail, it eats my tab
character
P.S 2. Sorry, my tab character has been eaten again!
P.S 3. Wrapped the letter lines to fit on the terminal

Regards,
Michael

Re: [PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Jeff King]

On Fri, May 30, 2014 at 11:10:51PM +0000, Naumov, Michael (North Sydney) wrote:

> Some git tools such as GitExtensions for Windows use environment
> variable TERM=msys which causes the weird ANSI sequence shown for the
> messages returned from server-side hooks
> We add those ANSI sequences to help format sideband data on the user's
> terminal. However, GitExtensions is not using a terminal, and the ANSI
> sequences just confuses it. We can recognize this use by checking
> isatty().
> See https://github.com/gitextensions/gitextensions/issues/1313 for
> more details
> 
> NOTE: I considered to cover the case that a pager has already been
> started. But decided that is probably not worth worrying about here,
> though, as we shouldn't be using a pager for commands that do network
> communications (and if we do, omitting the magic line-clearing signal
> is probably a sane thing to do).
> 
> Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
> Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
> Thanks-to: Jeff King <peff@peff.net>

This version looks fine to me.

-Peff

Re: [PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Junio C Hamano]

"Naumov, Michael (North Sydney)" <Michael.Naumov@Fiserv.com> writes:

You either want to correct the "From: " header that appears in
e-mails from you, or want to start your body of your patch message
like this:

	From: Michael Naumov <mnaoumov@gmail.com>

	Some git tools such as GitExtensions for Windows...

if you want the author of the patch and the name you used to sign it
off to match, which is almost always what you want.

> Some git tools such as GitExtensions for Windows use environment
> variable TERM=msys which causes the weird ANSI sequence shown for the
> messages returned from server-side hooks

The above sentence, while it may be telling a truth, feels more or
less irrelevant, especially the part that talks about TERM=msys.
Even if GitExtensions used TERM=vt100, the end result would be the
same, wouldn't it?

If you are suggesting a fix to GitExtensions to make it export
TERM=dumb like everybody else does, that would be a different
story.  Mentioning "TERM=msys causes the problem" would be a very
relevant thing to do.  But this patch is not that.

> We add those ANSI sequences to help format sideband data on the user's
> terminal. However, GitExtensions is not using a terminal, and the ANSI
> sequences just confuses it. We can recognize this use by checking
> isatty().

    This on the other hand is very readable.  How about rephrasing these
    two like so:

    Diagnostic messages received on the sideband #2 from the server side
    are sent to the standard error with ANSI terminal control sequence
    "\033[K" that erases to the end of line appended at the end of each
    line.

    However, some programs (e.g. GitExtensions for Windows) read and
    interpret and/or show the message without understanding the terminal
    control sequences, resulting them to be shown to their end users.
    To help these programs, squelch the control sequence when the
    standard error stream is not being sent to a tty.

There are programs that drive other programs (not limited to Git)
through pty (hence satisfying isatty(2)) without interpreting the
ANSI terminal control sequences, and it is conventional for these
programs to export TERM=dumb, so and your patch still checks for
TERM=dumb to help them, which is very good.

> See https://github.com/gitextensions/gitextensions/issues/1313 for
> more details

And if you explain it like the above, I do not think this external
reference is very useful.

> NOTE: I considered to cover the case that a pager has already been
> started. But decided that is probably not worth worrying about here,
> though, as we shouldn't be using a pager for commands that do network
> communications (and if we do, omitting the magic line-clearing signal
> is probably a sane thing to do).

Sensible.

> Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
> Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
> Thanks-to: Jeff King <peff@peff.net>
> ---
>  sideband.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sideband.c b/sideband.c
> index d1125f5..7f9dc22 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -30,7 +30,7 @@ int recv_sideband(const char *me, int in_stream, int out)
>  
>  	memcpy(buf, PREFIX, pf);
>  	term = getenv("TERM");
> -	if (term && strcmp(term, "dumb"))
> +	if (isatty(2) && term && strcmp(term, "dumb"))
>  		suffix = ANSI_SUFFIX;
>  	else
>  		suffix = DUMB_SUFFIX;

[PATCH] sideband.c: Get rid of ANSI sequences for non-terminal shell

[Naumov, Michael (North Sydney)]

From: Michael Naumov <mnaoumov@gmail.com>

Diagnostic messages received on the sideband #2 from the server side
are sent to the standard error with ANSI terminal control sequence
"\033[K" that erases to the end of line appended at the end of each
line.

However, some programs (e.g. GitExtensions for Windows) read and
interpret and/or show the message without understanding the terminal
control sequences, resulting them to be shown to their end users.
To help these programs, squelch the control sequence when the
standard error stream is not being sent to a tty.

NOTE: I considered to cover the case that a pager has already been 
started. But decided that is probably not worth worrying about here, 
though, as we shouldn't be using a pager for commands that do network 
communications (and if we do, omitting the magic line-clearing signal 
is probably a sane thing to do).

Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
Thanks-to: Jeff King <peff@peff.net>
---
 sideband.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sideband.c b/sideband.c
index d1125f5..7f9dc22 100644
--- a/sideband.c
+++ b/sideband.c
@@ -30,7 +30,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
-	if (term && strcmp(term, "dumb"))
+	if (isatty(2) && term && strcmp(term, "dumb"))
 		suffix = ANSI_SUFFIX;
 	else
 		suffix = DUMB_SUFFIX;
-- 
1.8.3.msysgit.0

P.S. I gave up trying to send this letter from gmail, it eats my tab
character
P.S 2. Sorry, my tab character has been eaten again!
P.S 3. Wrapped the letter lines to fit on the terminal
P.S 4. GitExtensions tried to use TERM=dumb but this caused process leakage for diff tools. See https://github.com/gitextensions/gitextensions/issues/1092

Regards,
Michael