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

Previous Next

Package: diffutils;

Reported by: Gisle Vanem <gvanem <at> yahoo.no>

Date: Tue, 1 Dec 2015 09:58:02 UTC

Severity: normal

Tags: patch

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

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 22067 in the body.
You can then email your comments to 22067 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Tue, 01 Dec 2015 09:58:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Gisle Vanem <gvanem <at> yahoo.no>:
New bug report received and forwarded. Copy sent to bug-diffutils <at> gnu.org. (Tue, 01 Dec 2015 09:58:02 GMT) Full text and rfc822 format available.

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

From: Gisle Vanem <gvanem <at> yahoo.no>
To: "bug-diffutils <at> gnu.org" <bug-diffutils <at> gnu.org>
Subject: [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for
 --color
Date: Tue, 1 Dec 2015 10:57:08 +0100
Jim Meyering wrote:

> Good! Worthwhile after all. Thank you.
> I have just pushed those commits.

Trying this color-patch on Windows, there are a few issues:

1) ANSI-sequences are no good on Windows.
2) Some signals are not in MSVC, MinGW nor in Gnulib; i.e. SIGTSTP,
   SIGSTOP etc.
3) I'm getting a stack-overflow in handling a SIGINT.
   AFAICS there is a infinite recursion in process_signals().
   This function calls 'set_color_context (RESET_CONTEXT)'. But
   'set_color_context()' again calls 'process_signals()'. How can
   that *not* stack-fault on any platform?

1+2) I've patched here to get colors. Although hard-coded; ignoring
the '--palette' option etc.

-- 
--gv




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Sat, 30 Jan 2016 10:03:02 GMT) Full text and rfc822 format available.

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

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Gisle Vanem <gvanem <at> yahoo.no>
Cc: 22067 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#20062: bug#20062: [PATCH] diff:
 add support for --color
Date: Sat, 30 Jan 2016 11:02:19 +0100
Gisle Vanem <gvanem <at> yahoo.no> writes:

> Jim Meyering wrote:
>
>> Good! Worthwhile after all. Thank you.
>> I have just pushed those commits.
>
> Trying this color-patch on Windows, there are a few issues:
>
> 1) ANSI-sequences are no good on Windows.
> 2) Some signals are not in MSVC, MinGW nor in Gnulib; i.e. SIGTSTP,
>    SIGSTOP etc.
> 3) I'm getting a stack-overflow in handling a SIGINT.
>    AFAICS there is a infinite recursion in process_signals().
>    This function calls 'set_color_context (RESET_CONTEXT)'. But
>    'set_color_context()' again calls 'process_signals()'. How can
>    that *not* stack-fault on any platform?
>
> 1+2) I've patched here to get colors. Although hard-coded; ignoring
> the '--palette' option etc.

Could you share your patch?  I've no Windows machine, altough I am
interested in how that could be done.

Bug 3 is a bug on all platforms, set_color_context should call
process_signals only when color_context != RESET_CONTEXT.  I'll send a
patch for that.

Thanks,
Giuseppe




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 01 Feb 2016 09:11:02 GMT) Full text and rfc822 format available.

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

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Gisle Vanem <gvanem <at> yahoo.no>
Cc: Jim Meyering <jim <at> meyering.net>, 22067 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#20062: bug#20062: [PATCH] diff:
 add support for --color
Date: Mon, 01 Feb 2016 10:10:05 +0100
[Message part 1 (text/plain, inline)]
Giuseppe Scrivano <gscrivano <at> gnu.org> writes:

> Bug 3 is a bug on all platforms, set_color_context should call
> process_signals only when color_context != RESET_CONTEXT.  I'll send a
> patch for that.

I've attached a patch for this bug.

Regards,
Giuseppe

[0001-Fix-an-infinite-recursion-with-color.patch (text/x-patch, inline)]
From c9326d016a05c594b8cd2f19effe792c23fde3ef Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano <at> gnu.org>
Date: Mon, 1 Feb 2016 09:58:52 +0100
Subject: [PATCH] Fix an infinite recursion with --color

* src/util.c: Call process_signals only when color_context is not
RESET_CONTEXT.

Reported by Gisle Vanem in http://debbugs.gnu.org/22067
---
 src/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util.c b/src/util.c
index bf9ed97..26615ca 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1349,7 +1349,8 @@ static enum color_context last_context = RESET_CONTEXT;
 void
 set_color_context (enum color_context color_context)
 {
-  process_signals ();
+  if (color_context != RESET_CONTEXT)
+    process_signals ();
   if (colors_enabled && last_context != color_context)
     {
       put_indicator (&color_indicator[C_LEFT]);
-- 
2.5.0


Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Sat, 06 Feb 2016 06:17:01 GMT) Full text and rfc822 format available.

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

From: KW Kam <nkwkam <at> gmail.com>
To: 22067 <at> debbugs.gnu.org
Subject: [PATCH] color for windows and fix sigxxx handling
Date: Sat, 6 Feb 2016 13:25:51 +0800
[Message part 1 (text/plain, inline)]
w32_sgr2attr is copied from grep
[Message part 2 (text/html, inline)]
[22067-diffutils.patch (application/octet-stream, attachment)]

Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 08 Feb 2016 13:30:02 GMT) Full text and rfc822 format available.

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

From: Gisle Vanem <gvanem <at> yahoo.no>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>,
 "bug-diffutils <at> gnu.org" <bug-diffutils <at> gnu.org>
Subject: Re: [bug-diffutils] bug#22067: bug#20062: bug#20062: [PATCH] diff:
 add support for --color
Date: Mon, 8 Feb 2016 14:29:11 +0100
[Message part 1 (text/plain, inline)]
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.

-- 
--gv


[wincolor.c (text/plain, attachment)]
[diff-1.txt (text/plain, attachment)]

Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Wed, 10 Feb 2016 16:47:02 GMT) Full text and rfc822 format available.

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

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Gisle Vanem <gvanem <at> yahoo.no>
Cc: "bug-diffutils <at> gnu.org" <bug-diffutils <at> gnu.org>
Subject: Re: [bug-diffutils] bug#22067: bug#20062: bug#20062: [PATCH] diff:
 add support for --color
Date: Wed, 10 Feb 2016 17:45:56 +0100
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.

Regards,
Giuseppe




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Wed, 10 Feb 2016 18:43:02 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
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: Wed, 10 Feb 2016 10:41:38 -0800
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...

Gisle, thanks for the patches. Giuseppe is right: we
would much prefer to keep patches as small as possible,
with separable changes in separate commits, so if you
can adjust that would be great.




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Thu, 11 Feb 2016 12:44:02 GMT) Full text and rfc822 format available.

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




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Thu, 11 Feb 2016 19:15:02 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
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 11:14:23 -0800
On Thu, Feb 11, 2016 at 4:43 AM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
> 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,

Great! Thank you.

> but it will need a change in the code as well, since
> the signals are installed only when outputting to a tty:
...
> -  if (output_is_tty)
> +  if (output_is_tty || getenv ("DIFF_INSTALL_SIGNALS"))
>      install_signal_handlers ();

However, we try very hard to avoid making tools depend on
environment variable settings more than they already do,
so how about a hidden, three-hyphen option, say,
---presume-output-tty, analogous to rm's ---presume-input-tty?




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Fri, 12 Feb 2016 15:15:01 GMT) Full text and rfc822 format available.

Message #32 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: Fri, 12 Feb 2016 16:14:04 +0100
[Message part 1 (text/plain, inline)]
Jim Meyering <jim <at> meyering.net> writes:

> Great! Thank you.
>
>> but it will need a change in the code as well, since
>> the signals are installed only when outputting to a tty:
> ...
>> -  if (output_is_tty)
>> +  if (output_is_tty || getenv ("DIFF_INSTALL_SIGNALS"))
>>      install_signal_handlers ();
>
> However, we try very hard to avoid making tools depend on
> environment variable settings more than they already do,
> so how about a hidden, three-hyphen option, say,
> ---presume-output-tty, analogous to rm's ---presume-input-tty?

I have added a test that uses the new option ---presume-output-tty.

[0001-Fix-an-infinite-recursion-with-color.patch (text/x-patch, inline)]
From 4aea918d31454fdeaada5e2453bea3dfc2e25f8b Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano <at> gnu.org>
Date: Mon, 1 Feb 2016 09:58:52 +0100
Subject: [PATCH] Fix an infinite recursion with --color

* src/diff.h: New extern variable `presume_output_tty'.
* src/diff.c: New enum PRESUME_OUTPUT_TTY_OPTION.
(group_format_option): Add '-presume-output-tty'.
(main): Handle PRESUME_OUTPUT_TTY_OPTION.
* src/util.c: New variable `presume_output_tty'.
(check_color_output): Handle presume_output_tty.
(set_color_context): Call process_signals only when color_context is
not RESET_CONTEXT.
* tests/colors: Check that diff doesn't crash when interrupted
in the middle of a color sequence.

Reported by Gisle Vanem in http://debbugs.gnu.org/22067
---
 src/diff.c   | 9 +++++++++
 src/diff.h   | 2 ++
 src/util.c   | 7 +++++--
 tests/colors | 7 +++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/diff.c b/src/diff.c
index 3a566b8..9bc1d96 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -141,6 +141,8 @@ enum
 
   COLOR_OPTION,
   COLOR_PALETTE_OPTION,
+
+  PRESUME_OUTPUT_TTY_OPTION,
 };
 
 static char const group_format_option[][sizeof "--unchanged-group-format"] =
@@ -219,6 +221,9 @@ static struct option const longopts[] =
   {"unified", 2, 0, 'U'},
   {"version", 0, 0, 'v'},
   {"width", 1, 0, 'W'},
+
+  /* This is solely for testing.  Do not document.  */
+  {"-presume-output-tty", no_argument, NULL, PRESUME_OUTPUT_TTY_OPTION},
   {0, 0, 0, 0}
 };
 
@@ -641,6 +646,10 @@ main (int argc, char **argv)
 	  set_color_palette (optarg);
 	  break;
 
+        case PRESUME_OUTPUT_TTY_OPTION:
+          presume_output_tty = true;
+          break;
+
 	default:
 	  try_help (NULL, NULL);
 	}
diff --git a/src/diff.h b/src/diff.h
index e4c138c..0983e7c 100644
--- a/src/diff.h
+++ b/src/diff.h
@@ -417,5 +417,7 @@ enum color_context
   LINE_NUMBER_CONTEXT,
 };
 
+XTERN bool presume_output_tty;
+
 extern void set_color_context (enum color_context color_context);
 extern void set_color_palette (char const *palette);
diff --git a/src/util.c b/src/util.c
index bf9ed97..d7b8925 100644
--- a/src/util.c
+++ b/src/util.c
@@ -44,6 +44,8 @@
 
 char const pr_program[] = PR_PROGRAM;
 
+bool presume_output_tty;
+
 /* Queue up one-line messages to be printed at the end,
    when -l is specified.  Each message is recorded with a 'struct msg'.  */
 
@@ -710,7 +712,7 @@ check_color_output (bool is_pipe)
   if (! outfile || colors_style == NEVER)
     return;
 
-  output_is_tty = !is_pipe && isatty (fileno (outfile));
+  output_is_tty = presume_output_tty || (!is_pipe && isatty (fileno (outfile)));
 
   colors_enabled = (colors_style == ALWAYS
                     || (colors_style == AUTO && output_is_tty));
@@ -1349,7 +1351,8 @@ static enum color_context last_context = RESET_CONTEXT;
 void
 set_color_context (enum color_context color_context)
 {
-  process_signals ();
+  if (color_context != RESET_CONTEXT)
+    process_signals ();
   if (colors_enabled && last_context != color_context)
     {
       put_indicator (&color_indicator[C_LEFT]);
diff --git a/tests/colors b/tests/colors
index facfd8d..df395ec 100755
--- a/tests/colors
+++ b/tests/colors
@@ -116,4 +116,11 @@ test $? = 1 || fail=1
 gen_exp_u > exp || framework_failure_
 compare exp out || fail=1
 
+mkfifo fifo
+printf '%*s' 1000000 | tr ' ' a > a
+printf '%*s' 1000000 | tr ' ' b > b
+head -c 10 < fifo > /dev/null &
+diff --color=always ---presume-output-tty a b > fifo
+test $? = 141 || fail=1
+
 Exit $fail
-- 
2.5.0

[Message part 3 (text/plain, inline)]
Thanks,
Giuseppe

Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 22 Feb 2016 14:40:01 GMT) Full text and rfc822 format available.

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

From: Gisle Vanem <gvanem <at> yahoo.no>
To: "bug-diffutils <at> gnu.org" <bug-diffutils <at> gnu.org>
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Mon, 22 Feb 2016 15:39:40 +0100
Sorry for the late reply. Jim Meyering wrote:

> Gisle, thanks for the patches. Giuseppe is right: we
> would much prefer to keep patches as small as possible,
> with separable changes in separate commits, so if you
> can adjust that would be great.

I'm not really sure what you mean here.

I've provided all patches [1] needed to enable coloured diffs
for the Windows console API (disregarding CygWin which has
ANSI-decoding built-in).

So it would just takes someone with a bit more "git-skills" than
I have, to do make a proper "git email-formatted" patch or whatever
you mean. And frankly I don't care if you do enable this feature
or not. So there you have it. It's your call.

[1] in an private email to Guiseppe.

-- 
--gv




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Tue, 01 Mar 2016 08:26:02 GMT) Full text and rfc822 format available.

Message #38 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: Tue, 01 Mar 2016 09:25:01 +0100
Giuseppe Scrivano <gscrivano <at> gnu.org> writes:

> Jim Meyering <jim <at> meyering.net> writes:
>
>> Great! Thank you.
>>
>>> but it will need a change in the code as well, since
>>> the signals are installed only when outputting to a tty:
>> ...
>>> -  if (output_is_tty)
>>> +  if (output_is_tty || getenv ("DIFF_INSTALL_SIGNALS"))
>>>      install_signal_handlers ();
>>
>> However, we try very hard to avoid making tools depend on
>> environment variable settings more than they already do,
>> so how about a hidden, three-hyphen option, say,
>> ---presume-output-tty, analogous to rm's ---presume-input-tty?
>
> I have added a test that uses the new option ---presume-output-tty.
>
> From 4aea918d31454fdeaada5e2453bea3dfc2e25f8b Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <gscrivano <at> gnu.org>
> Date: Mon, 1 Feb 2016 09:58:52 +0100
> Subject: [PATCH] Fix an infinite recursion with --color
>
> * src/diff.h: New extern variable `presume_output_tty'.
> * src/diff.c: New enum PRESUME_OUTPUT_TTY_OPTION.
> (group_format_option): Add '-presume-output-tty'.
> (main): Handle PRESUME_OUTPUT_TTY_OPTION.
> * src/util.c: New variable `presume_output_tty'.
> (check_color_output): Handle presume_output_tty.
> (set_color_context): Call process_signals only when color_context is
> not RESET_CONTEXT.
> * tests/colors: Check that diff doesn't crash when interrupted
> in the middle of a color sequence.
>
> Reported by Gisle Vanem in http://debbugs.gnu.org/22067
> ---

ping.

Regards,
Giuseppe




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 07 Mar 2016 15:20:01 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
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: Mon, 7 Mar 2016 07:19:06 -0800
[Message part 1 (text/plain, inline)]
On Tue, Mar 1, 2016 at 12:25 AM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
> Giuseppe Scrivano <gscrivano <at> gnu.org> writes:
>
>> Jim Meyering <jim <at> meyering.net> writes:
>>
>>> Great! Thank you.
>>>
>>>> but it will need a change in the code as well, since
>>>> the signals are installed only when outputting to a tty:
>>> ...
>>>> -  if (output_is_tty)
>>>> +  if (output_is_tty || getenv ("DIFF_INSTALL_SIGNALS"))
>>>>      install_signal_handlers ();
>>>
>>> However, we try very hard to avoid making tools depend on
>>> environment variable settings more than they already do,
>>> so how about a hidden, three-hyphen option, say,
>>> ---presume-output-tty, analogous to rm's ---presume-input-tty?
>>
>> I have added a test that uses the new option ---presume-output-tty.
>>
>> From 4aea918d31454fdeaada5e2453bea3dfc2e25f8b Mon Sep 17 00:00:00 2001
>> From: Giuseppe Scrivano <gscrivano <at> gnu.org>
>> Date: Mon, 1 Feb 2016 09:58:52 +0100
>> Subject: [PATCH] Fix an infinite recursion with --color
>>
>> * src/diff.h: New extern variable `presume_output_tty'.
>> * src/diff.c: New enum PRESUME_OUTPUT_TTY_OPTION.
>> (group_format_option): Add '-presume-output-tty'.
>> (main): Handle PRESUME_OUTPUT_TTY_OPTION.
>> * src/util.c: New variable `presume_output_tty'.
>> (check_color_output): Handle presume_output_tty.
>> (set_color_context): Call process_signals only when color_context is
>> not RESET_CONTEXT.
>> * tests/colors: Check that diff doesn't crash when interrupted
>> in the middle of a color sequence.
>>
>> Reported by Gisle Vanem in http://debbugs.gnu.org/22067

Thank you, and sorry about the delay.
I noticed that on OS X, the new test passed even without
the fix, but didn't want to let that hold up the fix any longer.
I've adjusted the test slightly, to eliminate the tr pipes.
Will wait for your "ack", before pushing.
[0001-diff-color-fix-an-infinite-recursion-bug.patch (text/x-patch, attachment)]

Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 07 Mar 2016 16:18:01 GMT) Full text and rfc822 format available.

Message #44 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: Mon, 07 Mar 2016 17:17:48 +0100
Jim Meyering <jim <at> meyering.net> writes:

> Thank you, and sorry about the delay.
> I noticed that on OS X, the new test passed even without
> the fix, but didn't want to let that hold up the fix any longer.
> I've adjusted the test slightly, to eliminate the tr pipes.
> Will wait for your "ack", before pushing.

Unfortunately I have no access to OS X to test it out by myself.

It is fine by me without the 'tr' so ACK from me.

Thanks for the review,
Giuseppe






Reply sent to Jim Meyering <jim <at> meyering.net>:
You have taken responsibility. (Mon, 07 Mar 2016 16:33:02 GMT) Full text and rfc822 format available.

Notification sent to Gisle Vanem <gvanem <at> yahoo.no>:
bug acknowledged by developer. (Mon, 07 Mar 2016 16:33:02 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
Cc: Gisle Vanem <gvanem <at> yahoo.no>, 22067-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Mon, 7 Mar 2016 08:31:42 -0800
pushed.




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Mon, 07 Mar 2016 17:21:01 GMT) Full text and rfc822 format available.

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

From: Gisle Vanem <gvanem <at> yahoo.no>
To: 22067 <at> debbugs.gnu.org
Subject: Re: bug#22067: closed (Re: [bug-diffutils] bug#22067: bug#22067:
 bug#20062: bug#20062: [PATCH] diff: add support for --color)
Date: Mon, 7 Mar 2016 18:20:27 +0100
> #22067: [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color
> 
> which was filed against the diffutils package, has been closed.

But still no support for colours on a plain Windows console
(except for Cygwin which has ANSI-colour support).

-- 
--gv




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Tue, 08 Mar 2016 22:02:02 GMT) Full text and rfc822 format available.

Message #55 received at 22067-done <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-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Tue, 08 Mar 2016 23:00:57 +0100
Jim Meyering <jim <at> meyering.net> writes:

> pushed.

thanks!

Is there going to be any release of diffutils with the new changes?

Regards,
Giuseppe




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Wed, 09 Mar 2016 02:34:01 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
Cc: Gisle Vanem <gvanem <at> yahoo.no>, 22067-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Tue, 8 Mar 2016 18:32:52 -0800
On Tue, Mar 8, 2016 at 2:00 PM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
> Jim Meyering <jim <at> meyering.net> writes:
>
>> pushed.
>
> thanks!
>
> Is there going to be any release of diffutils with the new changes?

Yes. It's in the queue, but first I'll make releases for grep and gzip.
sed is also overdue. Any help with triaging things in the bug
lists would be most welcome:

  http://debbugs.gnu.org/cgi/pkgreport.cgi?package=diffutils




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Thu, 10 Mar 2016 21:03:01 GMT) Full text and rfc822 format available.

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

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 22067-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Thu, 10 Mar 2016 22:02:38 +0100
Hi Jim,

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

> Yes. It's in the queue, but first I'll make releases for grep and gzip.
> sed is also overdue. Any help with triaging things in the bug
> lists would be most welcome:
>
>   http://debbugs.gnu.org/cgi/pkgreport.cgi?package=diffutils

I went through the open bugs (I skipped the ones already on wishlist)
and this is what I've found:

* 18641 [diffutils] [PATCH] diff: Add --only=REGEX option to parse only files that match REGEX
RFC

* 19508 [diffutils] [PATCH] diff: compare major, minor numbers of block/character special files
* 19509 [diffutils] [PATCH] diff: treat fifos as identical
not blocking issue, patch needs review

* 17185  diff --help doesn't mention short forms of some options
minor issue, not blocking

* 18655 Regression in 3.3 documentation versus 3.2
answered, bug must be closed

* 18973 diffutils-3.3 test-mbrtowc3.sh fails on Solaris without European localization
not triaged, no access to solaris to try it

* 19489 FYI: I've updated copyright dates -- and gnulib
not a bug, must be closed

* 19825 unified output format, number of consecutive unaffected lines, and POSIX
answered, can be closed

* 19835 RFC: diff: skip initial columns before comparing
RFC with patch not reviewed

* 19984 [diffutils] Diff and symlinks
commented, can be closed

* 20250 [diffutils] diff by size
RFC for size only comparison (I would close as NOTFIX)

* 20279 [diffutils] With -r, option to avoid duplicate differences due to relative symbolic links
RFC

* 20794 [diffutils] ignore subdirectory with diff
Went unanswered, looks like a RFC for a weird use case

* 20929 [diffutils] problem with diff -B and incomplete lines
Fixed by:

commit d2fd9d4683ef60c259a3b426f71cef1b89ff383d
Author: Paul Eggert <eggert <at> cs.ucla.edu>
Date:   Wed Sep 3 15:58:03 2014 -0700

    diff: fix bug with diff -B and incomplete lines


* 21034 [diffutils] [bug-diffutils] bug#21023: bug#21023: REQUEST: --no-dereference missing from manpage + please make a short option for it
RFC, should probably be closed and pointed out that --no-d will work as well

* 21579 [diffutils] diffutils 3.3 man page may need update

Fixed by:

commit 2cd4ff3a5ff52d89b6b992d158f389b757f4faf4
Author: Jim Meyering <meyering <at> fb.com>
Date:   Mon Aug 31 23:12:43 2015 -0700

    build: correct man-page generation rule


* 21665  [diffutils] Use of mmap for large files
already answered as WONTFIX, must be closed

* 21674 [diffutils] Fix for context-header
plain diff patch proposed, changes the format of the header

* 21715 [diffutils] feat req: an option to skip directory inode comparison
RFC

* 22108 [diffutils] diff wrapper script for very large files, low memory
duplicate of 21665

* 22245 [diffutils] [lamby <at> debian.org: Bug* 809007: diffutils: FTBFS: FAIL: test-update-copyright.sh]
I had that issue some time ago but it was fixed upstream.  I have not bisected

* 22507 [diffutils] diffutils releases
not a bug, working for the release :)

* 22529 [diffutils] cmp man page too sophisticated
not a bug

* 22535 [diffutils] REQ: regex dialect switching (for -F and -I)
RFC, I would close it as a WONTFIX

* 22816 [diffutils] cmp --verbose EOF message could be more verbose
RFC

* 16467 [diffutils] Diffutils 3.3 v. VMS (et al.)
could be fixed in gnulib, not a regression anyway

Regards,
Giuseppe




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Thu, 10 Mar 2016 21:24:02 GMT) Full text and rfc822 format available.

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

From: Jim Meyering <jim <at> meyering.net>
To: Giuseppe Scrivano <gscrivano <at> gnu.org>
Cc: 22067-done <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#20062: bug#20062:
 [PATCH] diff: add support for --color
Date: Thu, 10 Mar 2016 13:22:54 -0800
On Thu, Mar 10, 2016 at 1:02 PM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
> Hi Jim,
>
> Jim Meyering <jim <at> meyering.net> writes:
>
>> Yes. It's in the queue, but first I'll make releases for grep and gzip.
>> sed is also overdue. Any help with triaging things in the bug
>> lists would be most welcome:
>>
>>   http://debbugs.gnu.org/cgi/pkgreport.cgi?package=diffutils
>
> I went through the open bugs (I skipped the ones already on wishlist)
> and this is what I've found:

[... lots of triage! ...]

Wow!!!
Thank you for all that work. I have no excuse to delay, now :-)

For anything that is obviously not a bug, you are welcome (encouraged,
even) to mark them as such and to close them in the bug db.

I suppose you know how to do that? One way is to reply to the affected
message and to include something like this at the top of it:

Here's an example that merges two bugs, tags as NOTABUG, and closes.
    forcemerge 7354 7353
    tags 7353 notabug
    close 7353
    thanks

Or just send that text to control <at> debbugs.gnu.org. Anyone can do that.

If you just want to close a bug, it's easy to do that with a reply to
the original where you change the auto-provided DDDDD <at> debbugs.gnu.org
recipient address to be DDDDD-done <at> debbugs.gnu.org

Thanks again.
I will take time to go through any you do not close/resolve within the
next few days.

Thanks again,
Jim




Information forwarded to bug-diffutils <at> gnu.org:
bug#22067; Package diffutils. (Sat, 12 Mar 2016 11:59:01 GMT) Full text and rfc822 format available.

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

From: Ondřej Svoboda <ondrej <at> svobodasoft.cz>
To: bug-diffutils <at> gnu.org
Subject: Re: [bug-diffutils] bug#22067: bug#22067: bug#22067: bug#20062:
 bug#20062: [PATCH] diff: add support for --color
Date: Sat, 12 Mar 2016 12:57:51 +0100
Giuseppe, Jim, guys,

I am the author of "compare major, minor numbers of block/character 
special files" (19508) and "treat fifos as identical" (19509). I needed 
them to compare backups of my Maemo 5 system (Nokia N900) -- there are 
static device files in /dev and some fifos too.

The first patch hasn't received a code review yet -- may I ask you or 
the list to give it a look? I think the patch is in a better shape than 
"treat fifos as identical" which isn't finished although it received a 
suggestion by Eric Blake (on 12 Feb last year) that fstat could be used 
to compare the fifos.

I rebased the patches in my working copy and I will at least send new 
versions to the list.

I am finishing my studies now so I can dedicate only limited time to 
finishing the patches. Let me know if you would like the functionality 
in the next release and I'll see what I can do.

Cheers,
Ondra

On 10.3.2016 22:02, Giuseppe Scrivano wrote:
> Hi Jim,
>
> Jim Meyering <jim <at> meyering.net> writes:
>
>> Yes. It's in the queue, but first I'll make releases for grep and gzip.
>> sed is also overdue. Any help with triaging things in the bug
>> lists would be most welcome:
>>
>>    http://debbugs.gnu.org/cgi/pkgreport.cgi?package=diffutils
> I went through the open bugs (I skipped the ones already on wishlist)
> and this is what I've found:
>
> * 18641 [diffutils] [PATCH] diff: Add --only=REGEX option to parse only files that match REGEX
> RFC
>
> * 19508 [diffutils] [PATCH] diff: compare major, minor numbers of block/character special files
> * 19509 [diffutils] [PATCH] diff: treat fifos as identical
> not blocking issue, patch needs review
>
> * 17185  diff --help doesn't mention short forms of some options
> minor issue, not blocking
>
> * 18655 Regression in 3.3 documentation versus 3.2
> answered, bug must be closed
>
> * 18973 diffutils-3.3 test-mbrtowc3.sh fails on Solaris without European localization
> not triaged, no access to solaris to try it
>
> * 19489 FYI: I've updated copyright dates -- and gnulib
> not a bug, must be closed
>
> * 19825 unified output format, number of consecutive unaffected lines, and POSIX
> answered, can be closed
>
> * 19835 RFC: diff: skip initial columns before comparing
> RFC with patch not reviewed
>
> * 19984 [diffutils] Diff and symlinks
> commented, can be closed
>
> * 20250 [diffutils] diff by size
> RFC for size only comparison (I would close as NOTFIX)
>
> * 20279 [diffutils] With -r, option to avoid duplicate differences due to relative symbolic links
> RFC
>
> * 20794 [diffutils] ignore subdirectory with diff
> Went unanswered, looks like a RFC for a weird use case
>
> * 20929 [diffutils] problem with diff -B and incomplete lines
> Fixed by:
>
> commit d2fd9d4683ef60c259a3b426f71cef1b89ff383d
> Author: Paul Eggert <eggert <at> cs.ucla.edu>
> Date:   Wed Sep 3 15:58:03 2014 -0700
>
>      diff: fix bug with diff -B and incomplete lines
>
>
> * 21034 [diffutils] [bug-diffutils] bug#21023: bug#21023: REQUEST: --no-dereference missing from manpage + please make a short option for it
> RFC, should probably be closed and pointed out that --no-d will work as well
>
> * 21579 [diffutils] diffutils 3.3 man page may need update
>
> Fixed by:
>
> commit 2cd4ff3a5ff52d89b6b992d158f389b757f4faf4
> Author: Jim Meyering <meyering <at> fb.com>
> Date:   Mon Aug 31 23:12:43 2015 -0700
>
>      build: correct man-page generation rule
>
>
> * 21665  [diffutils] Use of mmap for large files
> already answered as WONTFIX, must be closed
>
> * 21674 [diffutils] Fix for context-header
> plain diff patch proposed, changes the format of the header
>
> * 21715 [diffutils] feat req: an option to skip directory inode comparison
> RFC
>
> * 22108 [diffutils] diff wrapper script for very large files, low memory
> duplicate of 21665
>
> * 22245 [diffutils] [lamby <at> debian.org: Bug* 809007: diffutils: FTBFS: FAIL: test-update-copyright.sh]
> I had that issue some time ago but it was fixed upstream.  I have not bisected
>
> * 22507 [diffutils] diffutils releases
> not a bug, working for the release :)
>
> * 22529 [diffutils] cmp man page too sophisticated
> not a bug
>
> * 22535 [diffutils] REQ: regex dialect switching (for -F and -I)
> RFC, I would close it as a WONTFIX
>
> * 22816 [diffutils] cmp --verbose EOF message could be more verbose
> RFC
>
> * 16467 [diffutils] Diffutils 3.3 v. VMS (et al.)
> could be fixed in gnulib, not a regression anyway
>
> Regards,
> Giuseppe
>




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 10 Apr 2016 11:24:03 GMT) Full text and rfc822 format available.

This bug report was last modified 8 years and 17 days ago.

Previous Next


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