[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: b
From: |
Giuseppe Scrivano |
Subject: |
[bug-diffutils] bug#20062: 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 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Jim Meyering <address@hidden> writes:
> On Sun, Nov 1, 2015 at 9:06 PM, Jim Meyering <address@hidden> wrote:
>> 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.
>
> 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
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Jim Meyering, 2015/11/02
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/11/02
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color, Jim Meyering, 2015/11/02
- [bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color,
Giuseppe Scrivano <=
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/11/03
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Eric Blake, 2015/11/03
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Jim Meyering, 2015/11/03
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/11/16
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Jim Meyering, 2015/11/26
- [bug-diffutils] bug#20062: bug#20062: [PATCH] diff: add support for --color, Giuseppe Scrivano, 2015/11/27