Re: [Quilt-dev] [patch 26/26] Rewrite discussion of QUILT_COLORS configuration variable.

[Jean Delvare]

Hi Branden,

On Sat, 16 Jun 2018 12:22:58 -0400, address@hidden wrote:
> These are ANSI escape sequences as defined by ECMA-48; recast the entire
> discussion in light of that fact.
> 
> Condense the many tagged paragraphs with a templated discussion of
> defaults into a table.
> 
> Sort the QUILT_COLORS format names into alphabetical order.
> 
> Add a hint to the formatter (man) to call the tbl preprocessor render
> the table.

Missing "to" in above sentence?

> 
> Expand the example to be more demonstrative.
> 
> Add pointers to the ECMA-48 standard document and the console_codes
> section 4 man page (from Michael Kerrisk's man-pages project, widely
> available) to the See Also section.
> 
> Index: quilt/doc/quilt.1.in
> ===================================================================
> --- quilt.orig/doc/quilt.1.in
> +++ quilt/doc/quilt.1.in
> @@ -1,3 +1,4 @@
> +'\\" t

A comment might be in order? Within a week I'll have forgotten the
meaning of this line.

How portable is this construct?

>  .\\" Created by Martin Quinson from the tex documentation
>  .\\"
>  .TH quilt 1 "Dec 17, 2013" "quilt"
> @@ -403,103 +404,89 @@ If none of these variables is set, \\[lq
>  An empty value indicates that no pager should be used.
>  .TP
>  .I QUILT_COLORS
> -By default,
> +is a sequence of definitions that direct

As mentioned before, I don't like this "structure".

>  .I quilt
> -uses its predefined color set in order to be more
> -comprehensible when distiguishing various types of patches, e.g.\\&
> -applied/unapplied, failed, etc.
> +which ANSI escape sequences to associate with an output context,
> +overriding the defaults.

This description does not mention the purpose of the escape sequences.
Maybe "with ANSI color escape sequences" would make it clearer?

>  .IP
> -To override one or more color settings, set the
> +To override one or more settings, set

And here again your remove the word "color", while I think it was
valuable.

>  .I QUILT_COLORS
> -variable in following syntax - colon (:) separated list of elements,
> -each being of the form <format name>=<foreground color>[;<background
> -color>]  
> +to a colon-separated list of elements,
> +each of the form
> +.RI \\[lq] format-name\\c
> +.BI = digit-sequence\\c
> +.RB [ ; ...]\\[rq].
>  .IP
> -Format names with their respective default values are listed below,
> -along with their usage(s).
> -Color codes(values) are standard bash coloring escape codes.
> -See more at http://tldp.org/LDP/abs/html/colorizing.html#AEN20229
> -.RS 4
> -.TP
> -.B diff_hdr
> -Used in \\[lq]quilt diff\\[rq] to color the index line.
> -Defaults to 32 (green).
> -.TP
> -.B diff_add
> -Used in \\[lq]quilt diff\\[rq] to color added lines.
> -Defaults to 36 (azure).
> -.TP
> -.B diff_mod
> -Used in \\[lq]quilt diff\\[rq] to color modified lines.
> -Defaults to 35 (purple).
> -.TP
> -.B diff_rem
> -Used in \\[lq]quilt diff\\[rq] to color removed lines.
> -Defaults to 35 (purple).
> -.TP
> -.B diff_hunk
> -Used in \\[lq]quilt diff\\[rq] to color hunk header.
> -Defaults to 33 (brown/orange).
> -.TP
> -.B diff_ctx
> -Used in \\[lq]quilt diff\\[rq] to color the text after end of hunk
> -header (\\[lq]diff \\-\\-show\\-c\\-function\\[rq] generates this).
> -Defaults to 35 (purple).
> -.TP
> -.B diff_cctx
> -Used in \\[lq]quilt diff\\[rq] to color the 15-asterisk sequence before
> -or after a hunk.
> -Defaults to 33 (brown/orange).
> -.TP
> -.B patch_fuzz
> -Used in \\[lq]quilt push\\[rq] to color the patch fuzz information.
> -Defaults to 35 (purple).
> -.TP
> -.B patch_fail
> -Used in \\[lq]quilt push\\[rq] to color the fail message.
> -Defaults to 31 (red).
> -.TP
> -.B series_app
> -Used in \\[lq]quilt series\\[rq] and \\[lq]quilt patches\\[rq] to color
> -the applied patch names.
> -Defaults to 32 (green).
> -.TP
> -.B series_top
> -Used in \\[lq]quilt series\\[rq] and \\[lq]quilt patches\\[rq] to color
> -the top patch name.
> -Defaults to 33 (brown/orange).
> -.TP
> -.B series_una
> -Used in \\[lq]quilt series\\[rq] and \\[lq]quilt patches\\[rq] to color
> -unapplied patch names.
> -Defaults to 0 (no special color).
> -.RE
> -.RS 4
> -.PP
> -In addition, the
> -.B clear
> -format name is used to turn off special
> -coloring.
> -Its value is 0; it is not advised to modify it.
> -.PP
> -The content of
> -.I QUILT_COLORS
> -supersedes default values.
> -So the value \\[lq]diff_hdr=35;44\\[rq] will get you the
> -.I diff
> -headers in magenta over blue instead of the default green over unchanged
> -background.
> -For that, add the following content to
> +Each
> +.I format-name
> +with its respective default
> +.I digit-sequence
> +value is listed below,
> +along with an explanation of where it is used.
> +Each
> +.I digit-sequence
> +should be a SGR (Select Graphic Rendition) value supported by your
> +terminal.
> +The standardized SGR values were specified by ANSI and incorporated
> +into ISO-6429 and ECMA-48 (\\[sc]8.3.117).
> +The colors have standard names but their values were not defined within
> +a color space;
> +their precise appearance will vary and may be customizable in your
> +terminal (emulator).
> +.IP
> +Recognized
> +.IR format-name s,
> +along with the
> +.I quilt
> +commands that use them,
> +their use contexts,
> +and default values, follow.
> +.TS
> +box;
> +lI l l l.
> +format-name  command context default
> +_
> +.T&
> +lB l l l.
> +clear        -       -       0 (none)
> +diff_add     diff    added lines     36 (cyan)
> +diff_cctx    diff    asterisk sequences      33 (yellow)
> +diff_ctx     diff    text after hunk 35 (magenta)
> +diff_hdr     diff    index line      32 (green)
> +diff_hunk    diff    hunk header     33 (yellow)
> +diff_mod     diff    modified lines  35 (magenta)
> +diff_rem     diff    removed lines   35 (magenta)
> +patch_fail   push    failure message 31 (red)
> +patch_fuzz   push    fuzz information        35 (magenta)

patch_offs is missing from the list, but that's not your fault, it was
already missing from the manual page before.

> +series_app   series  applied patch names     32 (green)
> +series_top   series  top patch name  33 (yellow)
> +series_una   series  unapplied patch names   0 (none)

series_app, series_top and series_una are also used by "quilt patches".

> +.TE
> +.IP
> +The special
> +.I format-name
> +\\[lq]clear\\[rq] is used to turn off special graphics renditions and
> +return to the terminal defaults.
> +Changing its definition should
> +.I not

Is it really worth an emphasis?

> +be necessary for any terminal that claims to support ANSI escape
> +sequences.
> +If your terminal is corrupted despite your best efforts, try the command
> +\\[lq]tput sgr0\\[rq] to restore the default graphic rendition.
> +.IP
> +As an example, one can put the following in
>  .I \\[ti]/.quiltrc
>  (or
>  .IR /etc/quilt.quiltrc ):
> -.PP
> +.RS 12
>  .EX
> -.RS
>  QUILT_DIFF_ARGS="\\-\\-color"
> -QUILT_COLORS='diff_hdr=35;44'
> -.RE
> +# Render diff file headers in bold blue over yellow.
> +# Render diff hunk headers in "negative image" yellow.
> +# Render failed patches with a red background.
> +QUILT_COLORS="diff_hdr=1;34;43:diff_hunk=7;33:patch_fail=41"
>  .EE
> +.RE
>  .SH AUTHORS
>  .schar \\[:u] ue
>  .I Quilt
> @@ -532,6 +519,20 @@ and
>  .I patch
>  in detail.
>  .PP
> +.UR https://\\:www.ecma\\-international.org/\\:publications/\\
> +\\:standards/\\:Ecma\\-048.htm

Would seem more simple to keep it on a single line. There is no hard
limit to the line length.

> +.I Control Functions for Coded Character Sets
> +(ECMA-48)
> +.UE
> +specifies the ANSI escape sequences used by
> +.IR QUILT_COLORS ;
> +section 8.3.117 will be of the most interest.
> +See
> +.BR console_codes (4)
> +for a more convenient,
> +if less canonical,
> +resource.

Why split over 3 lines when it fits easily on one?

> +.PP
>  .BR diff (1),
>  .BR diffstat (1),
>  .BR guards (1),

Overall I like the spirit of the change, it makes things much clearer.

I would like to apologize for taking so long to review the whole
series. I often had to switch back to more urgent matters at work, that
didn't help.

Do you have any plans to also clean up the formatting issues in the
QUILT COMMANDS REFERENCE section? I have the feeling that it is not
what it should be. In particular there are too many blank lines in my
opinion. This section is generated automatically from the -h output of
each command, with an intermediate checkpoint as doc/reference. The
troff formatting is added in the Makefile with a sed command:

            $(SED) -e 's/^quilt \([^ ]*\)\(.*\)/.IP "\\fB\1\\fP\2 " 4/' \
                   -e $$'s/^  \\(-[^\t]*\\)\t\\?/.IP "    \\1" 8\\\n/'  \
                   -e $$'s/^  \t\\?//'                                  \
            < doc/reference                                             \

This is admittedly cryptic, but if you tell me how the output should be
modified, I can look into fixing the sed command to get there.

Thank you very much for all the time you spent to improve the
formatting and contents of the quilt manual page so far, it is very
appreciated.

-- 
Jean Delvare
SUSE L3 Support