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

[Jim Meyering]

On Mon, Nov 16, 2015 at 4:19 PM, Giuseppe Scrivano <address@hidden> wrote:
> Jim Meyering <address@hidden> writes:
>
>> On Tue, Nov 3, 2015 at 9:27 AM, Eric Blake <address@hidden> wrote:
>>> On 11/03/2015 10:05 AM, Giuseppe Scrivano wrote:
>>>
>>>> I have attached the patches that implement --palette, the missing tests
>>>> and update the NEWS file.
>>>>
>>>
>>>> +++ b/doc/diffutils.texi
>>>> @@ -3763,6 +3763,7 @@ Always use color.
>>>>  Specifying @option{--color} and no @var{when} is equivalent to
>>>>  @option{--color=auto}.
>>>>
>>>> +
>>>>  @item -C @var{lines}
>>>
>>> Spurious change?
>>>
>>>>  @itemx address@hidden@address@hidden
>>>>  Use the context output format, showing @var{lines} (an integer) lines of
>>>> @@ -3890,6 +3891,11 @@ if-then-else format.  @xref{Line Formats}.
>>>>  @itemx --show-c-function
>>>>  Show which C function each change is in.  @xref{C Function Headings}.
>>>>
>>>> address@hidden address@hidden
>>>> +It allows to specify what colors are used to colorize the output.  It
>>>
>>> Passive voice.  Would sound better as:
>>>
>>> Specify what color palette to use when colored output to use.
>>
>> Thanks for the quick review, Eric.
>> I'll wait for the next iteration.
>
> sorry for taking so long, I hope the attached version is fine.

I have begun reviewing carefully.
I adjusted NEWS.  Here is the modified paragraph:

** New features

   diff accepts two new options --color and --palette to generate
   and configure colored output.  --color takes an optional argument
   specifying when to colorize a line: --color=always, --color=auto,
   --color=never.  --palette is used to configure which colors are used.

I looked at the tests/colors script: we cannot/should not use sha1sum
for two reasons. 1) it is a short cut; better to include the precise expected
output in each case. Using this approach, if/when a test fails, there is
no record of what the expected output was. 2) the "sha1sum" command
is not universally available by that name. On BSD-based systems it is
called "sha1". Thus, I began the conversion, and in so doing, I found
some room for improvement: with the current patches I have, diff -u
emits a pair of identical color-changing escape sequences before each
"+"-prefixed line:

  $ diff -u --color=always a b|cat -A
  ^[[1;39m--- a^I1969-12-31 16:00:00.000000000 -0800$
  +++ b^I1969-12-31 16:00:00.000000000 -0800$
  ^[[0m^[[36m@@ -1 +1 @@^[[0m$
  ^[[31m-a$
  ^[[32m^[[32m+b$
  ^[[0m

Notice also how the final \e[0m is on the final line by itself,
with no following newline. Please adjust so that it appears at the
end of the final line instead. I confirmed that git-diff appears
to do the same thing, but noted that git uses \e[m instead (no
"0" part). Do you know of any pros/cons for one or the other?

I've attached the beginnings of the adjusted tests/colors
script that I used to discover these things. Can you finish the job
of converting it to use "compare" rather than sha1sum?

colors.sh
Description: Bourne shell script