On Tue, Mar 1, 2016 at 12:25 AM, Giuseppe Scrivano <gscriv...@gnu.org> wrote:
> Giuseppe Scrivano <gscriv...@gnu.org> writes:
>
>> Jim Meyering <j...@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 <gscriv...@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.
From 17e2698bcbee30a6cc282d61ad6242a64ba9c7cf Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscriv...@gnu.org>
Date: Mon, 1 Feb 2016 09:58:52 +0100
Subject: [PATCH] diff: --color: fix an infinite recursion bug
* src/diff.h (presume_output_tty): New extern variable.
* src/diff.c (PRESUME_OUTPUT_TTY_OPTION): New enum.
(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 | 9 +++++++++
4 files changed, 25 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..881c1b8 100755
--- a/tests/colors
+++ b/tests/colors
@@ -116,4 +116,13 @@ test $? = 1 || fail=1
gen_exp_u > exp || framework_failure_
compare exp out || fail=1
+# Before the fix in http://debbugs.gnu.org/22067,
+# this test would trigger an infinite loop bug.
+mkfifo fifo
+printf '%*s-a' 1000000 > a
+printf '%*s-b' 1000000 > b
+head -c 10 < fifo > /dev/null &
+diff --color=always ---presume-output-tty a b > fifo
+test $? = 141 || fail=1
+
Exit $fail
--
2.7.2
