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

Previous Next

Full log

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: Eric Blake <eblake <at> redhat.com>, 20062 <at> debbugs.gnu.org
Subject: Re: [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support
 for --color
Date: Thu, 12 Mar 2015 18:07:47 -0700

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.

Previous Next

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