[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add suppo
From: |
Paul Eggert |
Subject: |
[bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color |
Date: |
Thu, 12 Mar 2015 18:07:47 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
Giuseppe Scrivano wrote:
> +sigprocmask
This part of the change isn't needed any more.
> + case COLOR_OPTION:
> + specify_colors_style (optarg);
> + break;
Please be consistent about tabs vs spaces.
> + if (colors_style != NEVER && paginate)
> + error (EXIT_TROUBLE, 0, _("cannot specify both --color and --paginate"));
This part isn't needed any more, if I understand aright.
> +/* What kind of changes a hunk contains. */
> +enum colors_style
The comment doesn't match the code.
> +/* True if colors are printed. */
> +XTERN enum colors_style colors_style;
The comment doesn't match the code (it's not a boolean).
> - for (i = first0; i <= last0; i++)
> - print_1_line ("<", &files[0].linbuf[i]);
> + {
> + for (i = first0; i <= last0; i++)
> + {
> + set_color_context (DELETE, false);
> + print_1_line ("<", &files[0].linbuf[i]);
> + set_color_context (RESET, false);
> + }
> + }
> ...
> if (changes & NEW)
> - for (i = first1; i <= last1; i++)
> - print_1_line (">", &files[1].linbuf[i]);
> + {
> + for (i = first1; i <= last1; i++)
> + {
> + set_color_context (ADD, false);
> + print_1_line (">", &files[1].linbuf[i]);
> + set_color_context (RESET, false);
> + }
> + }
No need for the outer braces (twice).
> + /* Restore the default handler, and report the signal again. */
> + sigaction (signal, NULL, &act);
> + act.sa_handler = SIG_DFL;
> + sigaction (signal, &act, NULL);
> + raise (signal);
This assumes sigaction, but GNU diff is portable to hosts that lack sigaction.
Please see the code in sdiff.c, which uses sigaction only if available. You may
want to steal and/or librarize it, instead of using the ls.c-inspired code.
> + const char *const reset_sequence = "\x1b[0m";
This should be:
static char const reset_sequence[] = "\x1b[0m";
(I prefer putting the 'const' after the type, for consistency with "char *const
*".) That is, there's no need for the pointer variable here, and you can use
'sizeof reset_sequence - 1' instead of 'strlen (reset_sequence)'.
> +void
> +set_color_context (enum colors con, bool force)
This would be a bit clearer as separate functions for each combination of
arguments that are used. In particular, the separate function that can be
called from a signal handler should be commented as such, as it has more
constraints on its actions.
> + fprintf (outfile, "\x1B[31m");
> ...
> + fprintf (outfile, "\x1B[32m");
> ...
> + fprintf (outfile, "%s", reset_sequence);
Use fputs instead of fprintf.
> + if (write (fileno (outfile), reset_sequence,
> + strlen (reset_sequence)) < 0)
> + error (EXIT_TROUBLE, 0, "%s", _("write failed"));
We can't use 'fileno' or 'error' inside a signal handler; they're not
async-signal-safe. And we can't use 'outfile', as it's not volatile.
Instead, I suggest redoing begin_output so that 'outfile' is always stdout and
its file descriptor is 1; that way, 'write' can use STDOUT_FILENO instead of
fileno (stdout). (You can use dup2 to arrange for this after the pipe calls.)
Do not call 'error'; just return and let the signal handler re-raise the signal
and exit.
Also, don't assume that 'write' will write out the whole string; it might be a
partial write.
Also, what happens if a signal occurs when 'diff' is outputting an escape
sequence? E.g., suppose 'diff' is in the middle of outputting "\x1B[31m" and
gets interrupted after the '[', so that it outputs "\x1B[\x1b[0m". Is this
guaranteed to reset the terminal? If not, what sequence of bytes is guaranteed
to reset the terminal color even if the sequence is output immediately after a
truncated escape sequence?
> +static void
> +signal_handler (int signal)
> +{
> + struct sigaction act;
> +
> + if (output_is_tty)
> + set_color_context (RESET, true);
Don't have the signal handler inspect output_is_tty, as that would mean
output_is_tty would have to be volatile. Instead, install the signal handler
only if output_is_tty; that way, the handler itself can assume output_is_tty
without having to check it.
Isn't signal-handling fun?
By the way, are you running "./configure --enable-gcc-warnings"? If not, please
do that.
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, (continued)
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/10
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/10
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/10
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/11
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/11
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color,
Paul Eggert <=
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/12
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/13
- [bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/13
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Andreas Grünbacher, 2015/03/14
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Paul Eggert, 2015/03/14
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Andreas Grünbacher, 2015/03/14
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/15
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Norihiro Tanaka, 2015/03/15
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Norihiro Tanaka, 2015/03/15
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/03/15