Hi Jim,

thanks for the great advices.

Jim Meyering <jim@meyering.net> writes:

> 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 don't know if there is any difference in practice between \e[m and
\e0[m. I took the implementation in ls as reference which uses \e0[m.


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

Sure.  The new tests helped me to spot two issues in the "diff: add
support for --color" patch.  I also added some extra check to avoid to
enter the same colors context twice.  These fixes are in
0004-fixup-diff-add-support-for-color.patch.

To generate sequences on the same line they belong, I have created a new
patch to facilitate the review.  Probably the patch should get squashed
into 0001-diff-add-support-for-color.patch and 0005-tests-Add-tests-for-color-and-palette.patch
once reviewed.

I have not changed the first two patches of the series, I am including
them just for completeness.

Regards,
Giuseppe