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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2014, 17:59 UTC
Message-ID: <xmqqk38z5h0r.fsf@gitster.dls.corp.google.com>
In-Reply-To: <81E85F13A4BE084BAF4B1AA24173EDA0144E14B7@JWPKEXMBX03.corp.checkfree.com>

"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.

Show 5 quoted lines

> 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.

Show 20 quoted lines

> 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;