GNU bug report logs - #22067
[bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color

Previous Next

Full log

Message #26 received at 22067 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: Gisle Vanem <gvanem <at> yahoo.no>, 22067 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Thu, 11 Feb 2016 13:43:22 +0100

Jim Meyering <jim <at> meyering.net> writes:

> On Wed, Feb 10, 2016 at 8:45 AM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
>> Gisle Vanem <gvanem <at> yahoo.no> writes:
>>
>>> Giuseppe Scrivano wrote:
>>>
>>>> thanks for your patches.  Is it fine for you if we keep bug-diffutils in
>>>> the loop?
>>>
>>> Sure. I forgot it was in the CC-list.
>>> Attached again; wincolor.c + diff-1.txt.
>>> Excused my diff format; I'm a "git n00b". Hope you figure it out.
>>
>> cannot talk for the diff maintainers, but personally I would split it
>> two patches: one that fixes the build on Windows, I would not care to
>> disable them as in any case the user specifies when to use them or not;
>> and another patch that adds the support for colors on Windows.
>>
>> In this way the first one could be applied immediately and there is more
>> time to review the second one which adds support for colors on Windows.
>>
>> Anyway, let's wait for the maintainers' opinion about it.  My previous
>> patch is also waiting to be applied.
>
> Hi Giuseppe,
> The only thing missing from your infloop-fixing patch is an
> addition to the regression test suite. Can you contrive an
> example that induces the infinite recursion? It's not an
> absolute requirement in this case, but would be nice...

I can write one, but it will need a change in the code as well, since
the signals are installed only when outputting to a tty:

diff --git a/src/util.c b/src/util.c
index bf9ed97..be7d436 100644
--- a/src/util.c
+++ b/src/util.c
@@ -718,7 +718,7 @@ check_color_output (bool is_pipe)
   if (colors_enabled)
     parse_diff_color ();
 
-  if (output_is_tty)
+  if (output_is_tty || getenv ("DIFF_INSTALL_SIGNALS"))
     install_signal_handlers ();
 }

Is that fine?

Regards,
Giuseppe

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.