Hi Jim,

Jim Meyering <jim@meyering.net> writes:

> Thank you for that patch and for your patience.

thank you very much for the review!

> I've skimmed through and so far have only a question and a request:
>
> Regarding this section:
>
> +void
> +set_header_color_context (void)
> +{
> +  process_signals ();
> +  if (colors_enabled)
> +    fprintf (outfile, "\x1B[1;39m");
> +}
> +
> +void
> +set_line_numbers_color_context (void)
> +{
> +  process_signals ();
> +  if (colors_enabled)
> +    fprintf (outfile, "\x1B[36m");
> +}
> +
> +void
> +set_add_color_context (void)
> +{
> +  process_signals ();
> +  if (colors_enabled)
> +    fprintf (outfile, "\x1B[32m");
> +  fflush (outfile);
> +}
> +
> +void
> +set_delete_color_context (void)
> +{
> +  process_signals ();
> +  if (colors_enabled)
> +    fprintf (outfile, "\x1B[31m");
> +}
> +
> +void
> +reset_color_context (void)
> +{
> +  static char const reset_sequence[] = "\x1b[0m";
> +  if (! colors_enabled)
> +    return;
> +
> +  fputs (reset_sequence, outfile);
> +}
>
> Why does set_add_color_context call fflush, yet the others do not?
> Please use fputs rather than fprintf for those literal strings.
> The former is often far more efficient.

Yes, it shouldn't as well.  I have changed it.


> Finally, should there be some way to specify different colors,
> e.g., for those who use different-background-colored terminals,
> or for the color blind?

I have took more code from coreutils ls and diff honors DIFF_COLORS
now.  I added it in a separate patch to facilitate the review.  Probably
all the shared code should end up in a gnulib module, but it probably
needs a better API before it can happen.

Changes in the first patch:

1) dropped fflush from set_add_color_context
2) use fputs instead of fprintf (the second patch replaces it)
3) change the code color of the header to the default color to match
the "git diff" output.

Regards,
Giuseppe