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

[Jim Meyering]

On Fri, Nov 27, 2015 at 7:01 AM, Giuseppe Scrivano <address@hidden> wrote:
> Hi Jim,
>
> thanks for the great advices.
>
> Jim Meyering <address@hidden> 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.

Thank you for the quick and nice work.
Please adjust the test to use "out" rather than (my fault) "k" as the
output file name.

Your fixup diff highlights the fact that there is too much
duplication in the definitions of the reset_color_context and
the four set_*_color_context functions. Any reason not to use
a single function named e.g., set_color_context, and pass it
the C_* enum constant?

This comment was clearly intended for a different enum:

  +/* What kind of changes a hunk contains.  */
  +enum colors_style

Two nits in the .texi:

  +Do not use color at all.  This is the default when no --color option
  +is present.
  address@hidden auto
  address@hidden auto @r{color option}
  address@hidden terminal, using color iff
  +Only use color if standard output is a terminal.

s/present/specified/
s/Only use color/Use color only/

In the --help output addition, the spacing is slightly off:

     N_("    --speed-large-files  assume large files and many
scattered small changes"),
  +  N_("    --color[=WHEN]         colorize the output; WHEN can be
'never', 'always',"),
  +  N_("                             or 'auto' (the default)"),
     "",
     N_("    --help               display this help and exit"),
     N_("-v, --version            output version information and exit"),

The "c" in the added description, "colorize the ..." is indented
two spaces too much. Same for the continuation line.

In your palette option description, the continuation line must be
indented by two spaces, so that help2man knows to format it
as such:

  +  N_("    --palette=PALETTE    specify the colors to use when
--color is active"),
  +  N_("                         PALETTE is a colon-separated list
terminfo capabilities"),

Nit: s/const char/char const/, e.g., here:

  +extern void set_color_palette (const char *palette);

For any pair of arrays whose lengths must be aligned, e.g.,

  +static struct bin_str color_indicator[] =
  +  {
  +    { LEN_STR_PAIR ("\033[") },        /* lc: Left of color sequence */
  +    { LEN_STR_PAIR ("m") },        /* rc: Right of color sequence */
  +    { 0, NULL },            /* ec: End color (replaces lc+rs+rc) */
  +    { LEN_STR_PAIR ("0") },        /* rs: Reset to ordinary colors */
  +    { LEN_STR_PAIR ("1") },        /* hd: Header */
  +    { LEN_STR_PAIR ("32") },        /* ad: Add line */
  +    { LEN_STR_PAIR ("31") },        /* de: Delete line */
  +    { LEN_STR_PAIR ("36") },        /* ln: Line number */
  +  };
  +
  +static const char *const indicator_name[]=
  +  {
  +    "lc", "rc", "ec", "rs", "hd", "ad", "de", "ln", NULL
  +  };

Please add the following to ensure that no one adds
an element to one without also adding the matching
element in the other:

  ARGMATCH_VERIFY (indicator_name, color_indicator);

But that would be the first use of argmatch.h and the
argmatch module, so you'll actually have to make two
more changes (include the .h and add the argmatch
module to bootstrap.conf). I've attached a tiny patch.

With that, I think we'll be ready to go.
Thank you for your patience.

argmatch.patch
Description: Text Data