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

[Jim Meyering]

On Tue, Oct 20, 2015 at 9:23 AM, Giuseppe Scrivano <address@hidden> wrote:
> Hi Jim,
>
> Jim Meyering <address@hidden> 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:
...
>> 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.

Thanks for the quick work.
I'll try to be quicker, this time.

>> 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.

Those revisions to your first patch look fine, and I will look over that
one once more in the morning, then expect to push.

Regarding the support for configurable colors, as already mentioned
here, we really do want to avoid adding support for any more
environment variables.  Would you please adjust the second
patch to take the settings from a command line argument instead?
With that, it will be trivial to add an envvar-honoring wrapper script,
function or alias, for those who desire that convenience.

The most important reason not to provide this functionality via
an envvar is that it would then allow that envvar setting to alter
the behavior of any program that invokes diff, and that sort of
interaction is often surprising and undesirable.

Thanks again for contributing!