[bug-diffutils] bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color

[Paul Eggert]

Giuseppe Scrivano wrote:

>   signal
> +sigprocmask
>   stat

Your mailer is inserting spaces at the start of lines, making the patch hard to 
read.  Perhaps attach the patch instead next time?

> +- Do not use color at all.  This is the default when no --color option
> +is present.

That leading "-" doesn't look right.  I'd remove it.  (Similarly elsewhere.)

+  if (! outfile)
+    return;
+
+  output_is_tty = isatty (fileno (outfile));
+
+  colors_enabled = (colors_style == ALWAYS)
+    || (colors_style == AUTO && output_is_tty);

The indenting and parentheses should be something like this:

   colors_enabled = (colors_style == ALWAYS
                     || (colors_style == AUTO && output_is_tty));

More important, don't call isatty unless it's needed, as isatty can be somewhat 
expensive on some hosts.  It's not needed if COLORS_STYLE == NEVER.

+            check_color_output ();
...
+        check_color_output ();
...
       outfile = stdout;
+      check_color_output ();

The first two calls to check_color_output do unnecessary work, since 'outfile' 
must be a pipe in that case, so there's no need to call isatty.  Only in the 
last case might isatty be needed.

+      size_t left = limit - base;
+      while (left)
+        {
+          size_t len = MIN (left, max_chunk);
+          fwrite (base, sizeof (char), len, outfile);
+          set_color_context (SAME);
+          left -= len;
+        }
+    }

I'm afraid this won't work in general, as set_color_context (SAME) sends bytes 
to stdout if stdout is a tty, whereas it shouldn't output anything in the normal 
case.  For example, it might try to change color in the middle of a multibyte 
character, and that's a no-no.

Also, I'm a bit dubious about all those calls to sigprocmask.  Can't we solve 
this without having to execute a sigmask-related system call for each buffer? 
How about using the method that 'ls' uses instead?  Install a signal handler 
that merely sets a static variable.  Perhaps the relevant 'ls' code should be 
Gnulib-ized, so that it can be shared between 'ls' and 'diff'.

+      fprintf (outfile, "\x1b[0m");
+      fflush (outfile);
+      if (output_is_tty)
+        sigprocmask (SIG_SETMASK, &old_sigproc_set, NULL);

No need to call fflush if output is not a tty.