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

From: Paul Eggert <[email-masked]>
Date: Thu, 12 Mar 2015 14:46:28 -0700

On 03/12/2015 02:18 PM, Giuseppe Scrivano wrote:

 > +mbiter

Ouch.  I was hoping we didn't need to do that.  I expect the use of 
mbiter to hurt performance significantly.  I hope there's a better way, 
one that doesn't require counting individual multibyte characters.  It 
may be time to think about having a signal handler that resets the 
output tty.

 >            char const * const *line = &files[0].linbuf[i++];
 > +              set_color_context (DELETE);

Please use the same indenting (if the original uses tabs, do that), so 
that it's easier to read the diffs.

 > +    error (EXIT_TROUBLE, 0, _("Cannot specify both --color and 
--paginate."));

"cannot", not "Cannot", and no period at the end (it's not a sentence).

 > +              fwrite (mbi_cur_ptr (iter), mbi_len, 1, outfile);
 > +              written += mbi_len;

fwrite may write fewer than mbi_len characters.

 > -        putc (' ', out);
 > +                {
 > +                  written += putc (' ', out);
 > +                }

No need for {} here.  On the other hand, putc does not return 1-or-0 so 
this usage is problematic.

 > +          written += fprintf (out, flag_format, line_flag);

fprintf might return -1.

 > +        written += putc (c, out);
 > ...
 > +        written += putc (c, out);

Again, putc might return values other than 0 and 1.

 > +  if (output_is_tty && !set_initialized)
 > +    {
 > +      sigemptyset (&set);
 > +      for (j = 0; j < sizeof (sig) / sizeof (*sig); j++)
 > +        sigaddset (&set, sig[j]);
 > +      set_initialized = true;
 > +    }

This stuff can be done once, by 'main', with no need for a
set_initialized var.

 > +repeat:

Let's rephrase it without the goto; it'll be easier to understand that 
way, and just as efficient I expect.