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

Previous Next

Full log

View this message in rfc822 format

From: Giuseppe Scrivano <gscrivano <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 20062 <at> debbugs.gnu.org
Subject: bug#20062: [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color
Date: Mon, 02 Nov 2015 20:17:07 +0100

Jim Meyering <jim <at> meyering.net> writes:

> On Sun, Nov 1, 2015 at 9:06 PM, Jim Meyering <jim <at> meyering.net> wrote:
>> On Tue, Oct 20, 2015 at 9:23 AM, Giuseppe Scrivano <gscrivano <at> gnu.org> wrote:
>>> Hi Jim,
>>>
>>> Jim Meyering <jim <at> 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:
>> ...
>>>> 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.
>
> Your first patch adds a useful new feature, but includes
> neither a NEWS addition nor any test. Would you please add those?

sure, I will do.

I forgot to update "option_help_msgid" in my previous patch which adds
--color-scheme to define the colors used by diff.

Anyway, I would like an opinion about the new name before I send te full
series again.  I avoided --colors as it is very similar to --color, but
I am not sure how --color-scheme sounds for a native speaker.

Regards,
Giuseppe

Previous Next

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