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

Previous Next

Full log

View this message in rfc822 format

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: bug#20062: [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color
Date: Wed, 11 Mar 2015 16:18:33 -0700

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.

Previous Next

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