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

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

[Jeff King]

On Tue, May 27, 2014 at 01:27:12PM +1000, Michael Naumov wrote:

> From ae8d04fdbd71cf322e67903826544d5431f2866d Mon Sep 17 00:00:00 2001
> From: Michael Naumov <mnaoumov@gmail.com>
> Date: Tue, 27 May 2014 12:45:06 +1000
> Subject: [PATCH] sideband.c: Get rid of ANSI sequence for non-terminal shell

You can drop these lines; they are redundant with your email's headers
(but see below for more email tips).

> 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
> 
> See https://github.com/gitextensions/gitextensions/issues/1313 for more
> details

It's nice to give a pointer to more discussion, but it's also a good
idea to give a one- or two-sentence summary. Something like (after your
first paragraph):

  We add those ANSI sequences to help format sideband data on the user's
  terminal. However, these extensions are not using a terminal, and the
  sequences just confuse them. We can recognize this use by checking
  isatty().

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

I was nodding my head in agreement at this, and then went back and
realized that it is paraphrasing me. So I definitely agree with it. :)

> Signed-off-by: Michael Naumov <mnaoumov@gmail.com>
> 
> Thanks-to: Erik Faye-Lund <kusmabite@gmail.com>
> Thanks-to: Jeff King <peff@peff.net>

A minor nit, but please keep these "trailers" together in a single
paragraph. Elsewhere on the list people are developing tools to write
and parse them, and I believe that they only look backwards up to the
last empty line.

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

Your patch looks whitespace damaged. It was also sent as a
multipart/alternative with html, which I suspect means it did not make
it to the list. It looks like you're using gmail. The simplest thing is
to just use git-send-email to send it; there are tips for configuring
send-email with gmail in "git help send-email".

-Peff