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

[Eric Blake]

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.

> +defaults to @samp{rs=0:hd=1:ad=32:de=31:ln=36} for red added lines,
> +green deleted lines, cyan line numbers, bold header.

That says the default, but doesn't say what conventions to apply to get
something other than the default.  What are 'rs', 'hd', 'ad', 'de', and
'ln'?  Can we link to the fact that the values of these variables are
generally applied inside a terminal '\e[Xm' sequence, for a given X?
Can we link to dircolors output for comparison?  Do we need parameters
for the '\e[' and 'm' prefix/suffix, in case the terminal understands
something different than ANSI escapes for colors?

> @@ -950,6 +956,7 @@ static char const * const option_help_msgid[] = {
>    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_("    --palette=SCHEME    specify the colors to use when --color is 
> active"),
>    "",
>    N_("    --help               display this help and exit"),
>    N_("-v, --version            output version information and exit"),

Where is SCHEME documented in --help?  Should it be?


> +
> +/* Parse a string as part of the DIFF_COLORS variable; this may involve

Stale reference to envvar instead of --palette.

> +
> +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 */
> +  };

This needs to be documented in more than just code comments.


> +  ext = NULL;
> +  strcpy (label, "??");
> +
> +  /* This is an overly conservative estimate, but any possible
> +     DIFF_COLORS string will *not* generate a color_buf longer than
> +     itself, so it is a safe way of allocating a buffer in
> +     advance.  */
> +  buf = color_buf = xstrdup (p);

Another stale ref to DIFF_COLORS.

> 0002-doc-mention-color-and-palette-in-NEWS.patch
> 

Looked okay to me.

> 0003-tests-Add-tests-for-color-and-palette.patch
> 

> +
> +# Compare with some known outputs
> +

> +diff -c --color=always a b | sha1sum \
> +     | grep 904a91f82474e3532459b759fdacbdb339070e14 || fail=1
> +
> +diff -N --color=always --palette="rs=0:hd=33:ad=34:de=35:ln=36" a b \
> +     | sha1sum | grep 7796a82c2e7bd1f4ee04cb44352d83e1db87c092 || fail=1

Nice; a test of a non-default palette.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature