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

[G. Branden Robinson]

Hi, Jean!

Thanks for the in-depth review of my monstrous patch sequence!

I'll reply inline for this one but mainly I wanted to ack your review
and let you know that I'll be getting back to these soon but probably
not immediately.

At 2018-06-26T19:00:48+0200, Jean Delvare wrote:
> > Add a hint to the formatter (man) to call the tbl preprocessor render
> > the table.
> 
> Missing "to" in above sentence?

Yup--thanks!

> > +'\\" t
> 
> A comment might be in order? Within a week I'll have forgotten the
> meaning of this line.
> 
> How portable is this construct?

Well, if we precede it with a comment, it probably gets less portable.
No fooling.  :-|

Various man(1) implementations use this as a hint to decide which
preprocessors to run.

groff_man(7) says:

       If a preprocessor like tbl or eqn is needed, it has become common
       to make the first line of the man page look like this:

              '\" word

       Note the single space character after the double quote.  word
       consists of letters for the needed preprocessors: ‘e’ for eqn,
       ‘r’ for refer, and ‘t’ for tbl.  Modern implementations of the
       man program read this first line and automatically call the right
       preprocessor(s).

So if the implementation is dumb, it looks _only_ at the first line and
preceding it with a comment line defeats the purpose.

> >  .TP
> >  .I QUILT_COLORS
> > -By default,
> > +is a sequence of definitions that direct
> 
> As mentioned before, I don't like this "structure".

Acknowledged.  I'll undo it here and in the other patches.

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

That omission was deliberate, though perhaps I could make it clearer
why.  You're passing through the escape sequences uninterpreted so
you're not limited to color changes.  You could stick a "1;" into one of
these variables to turn on boldface, for example.

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

Right.  And what I'm saying is that passing these escape sequences makes
more rendition changes than just color available.  QUILT_COLORS is
unfortunately named, unless you want to add code to filter out SGR
values that _aren't_ color changes.  And I would recommend against that.
I did some experimenting and things like bold ("1;") and "standout"
(reverse video, "7;") worked perfectly.  In fact I added them to the
in-page example, with explanatory comments.

Another advantage of thinking more expansively is that you achieve
broader portability and accessibility for people who are using
monochrome displays, have a defect in color vision, or simply prefer to
use alternative graphic renditions instead of or in addition to color
changes.

I doubt you're opposed to the goal; my challenge is to find wording that
communicates the traditional utility of this variable to your audience.
:)

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

Okay.  I'm noting that to fix it in the next cycle.  Thanks for the
catch!

> > +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".

I'll have to get creative; that table is already pushing the 80-column
limit.  :)

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

If people fiddle with that, they will find their display hideous, and
will probably kill the terminal window in disgust.  Honestly, I would
un-document this entirely.  I've never heard of a terminal or emulator
that supported ANSI SGR sequences that managed to screw up SGR 0.  I
could ask Thomas Dickey; as ncurses and xterm maintainer and a vigorous
monitor of portability issues, he would know better than I.  I pay
attention to this sort of thing but he's a true expert.

> > +.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.

Fair enough.  A lot of people don't know that *roff supports
syntactically transparent newline escapes, like C compilers always have.
(It shouldn't be a surprise, the same people at Bell Labs brought us
both.)

> > +See
> > +.BR console_codes (4)
> > +for a more convenient,
> > +if less canonical,
> > +resource.
> 
> Why split over 3 lines when it fits easily on one?

This is advice from the GNU Troff Manual:

$ info -n '(groff)Basics' | grep -A 2 'In keeping'
   * In keeping with this, it is helpful to begin a new line after every
     comma or phrase, since common corrections are to add or delete
     sentences or phrases.

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

Thank you!

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

I completely understand.  And like I said, I realized as I was breaking
it up as originally requested that the thing had become a beast.

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

This is probably going to be a significant challenge to do right.  It
would mean solving a problem that generations of Unix people have failed
at.

Depending on your definition of "right", of course.

Conceptually, what quilt is doing is great.  It's literate (well, only
"semi-literate" in the Knuthian sense, but Knuth has demanding standards
for literate programming).  It keeps the documentation of the interface
close to the interface.

It honors DRY (Don't Repeat Yourself).

Almost no man pages can make that claim of their topics.  Just this week
I took a pass over groff's own man pages and compared their documented
command synopses with the reality of their calls to getopt_long() and
similar.  I doubt you will be surprised when I tell you there was some
divergence.

So I figured there are three ways to solve this:

1.  Make the man page the source for the usage message.

    Pro:
    * We can use man page markup freely and correctly.
    * nroff knows how to cleanly and consistently degrade that into
      plain text.
    * Contributors to the command scripts don't have to understand
      anything about *roff (unless they need to document an interface
      change).

    Con:
    * All the command scripts would get a layer of indirection, becoming
      *.in files in the repository and having their real versions
      generated during the build.  Ugh.
    * It would move the usage message out of the command script
      entirely, to be replaced with some ugly text-expanded thing.
      This puts a lot of separation between the interface and its
      description.  Double ugh.  This is the #2 reason documentation
      gets out of date.  (#1 is, of course, "developers can't be
      bothered to write it in the first place".)

2.  Keep the command scripts the canonical source, but stuff the usage
    messages into strings complete with *roff markup, and then call
    nroff to write them out in normal operation.

    Pro:
    * Most similar to the current approach.
    * The documentation stays close to the implementation.
    * We can use man page markup freely and correctly.
    * nroff knows how to cleanly and consistently degrade that into
      plain text.

    Con:
    * Requires either adding special marker lines to the command scripts
      to that the marked-up usage messages can be sedded out, or adding
      a special flag to all command scripts that dumps the raw usage
      string (with markup) so it can be interpolated into the man page.
    * Unorthodox and, as far as I know, unprecedented.  Might scare
      and/or discourage developers and contributors.  Many shell
      programmers never learn a thing about *roff or its man macro
      package.  Patches will require extra-diligent review to ensure
      that they're not only correctly implemented, but following the
      fiddly details of correct markup.

3.  Like 2, but define and implement a limited, special-purpose, simple
    markup language that is easily stripped for text output and easily
    transformed into man macros for inclusion in the man page.

    Pro:
    * The documentation stays close to the implementation.
    * We only define the markup we actually need.

    Con:
    * It's a new thing for contributors to learn.
    * The support burden for our private markup language is completely
      ours.
    * We get the potential excitement and joy of mistakes in language
      design.
    * This wheel has already been invented by Perl hackers; it is called
      POD, and as a *roff journeyman I have to say I do not like the
      results.  (However, the POD people are slam-dunk winners over the
      monstrosity that is docbook-to-man, which is so horrible that it
      has been repeatedly orphaned by a series of maintainers over the
      years.  I don't mean in Linux distributions--I mean at its
      origin.  Somehow, docbook-to-man proved irresistibly attractive to
      the Git project. :| )

Which way do you lean, or would you rather I went out on a limb with a
recommendation?

Or maybe you have an option 4 that will slice this Gordian knot!

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

You're quite welcome!  Eventually, I'll get back to that
missing-node-from-quilt-graph problem that got me started on this trip.
:D

-- 
Regards,
Branden

signature.asc
Description: PGP signature