Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>
Subject: Re: [PATCH 1/2] doc: add an option to have Asciidoctor build man pages directly
Date: Wed, 12 May 2021 02:59:34 -0400
Message-ID: <YJt81neO7zsGz2ah@coredump.intra.peff.net> (raw)
In-Reply-To: <YJt1/DO1cXNTRNxK@coredump.intra.peff.net>

On Wed, May 12, 2021 at 02:30:20AM -0400, Jeff King wrote:

> On Wed, May 12, 2021 at 02:22:06AM -0400, Jeff King wrote:
> 
> > With that change, plus a patch I'll send in a minute, it's easy to run
> > doc-diff on the result.
> 
> And here that is (note that if you don't apply the flags fix I showed,
> then doc-diff gets confused, because it expects back-to-back runs of
> "make" to handle the changed flags correctly).
> 
> Feel free to add it to your series if it helps (I had originally thought
> it would just be a one-off to look at the output, but there are enough
> changes, both correctness and style, that it may be useful to have this
> option around).

Adding in the "\e" to the extensions string fixes many problems. Just
skimming over the remaining changes (from asciidoctor+xmlto to
asciidoctor direct), here are some things I see:

Both of the xmlto pipelines seem to insert extra space before a literal.
The direct version fixes that. E.g.:

  -           Files to add content from. Fileglobs (e.g.  *.c) can be given to
  [...]
  +           Files to add content from. Fileglobs (e.g. *.c) can be given to add

So that's good. But what you can't see in the doc-diff rendered version
is that we've lost the bolding on literals (this is from the <pathspec>
option in git-add.1).

Another is that the direct version is less willing to break linkgit
targets across lines:

  -           explained for the configuration variable core.quotePath (see git-
  -           config(1)). See also --pathspec-file-nul and global
  +           explained for the configuration variable core.quotePath (see
  +           git-config(1)). See also --pathspec-file-nul and global

That seems fine to me, though it unfortunately does make the diff quite
noisy.

We seem to have a problem with some escape codes. E.g.:

  -           of nothing). The other file, git-add--interactive.perl, has 403
  -           lines added and 35 lines deleted if you commit what is in the
  -           index, but working tree file has further modifications (one
  +           of nothing). The other file, git-add&#x2d;&#x2d;interactive.perl,
  +           has 403 lines added and 35 lines deleted if you commit what is in
  +           the index, but working tree file has further modifications (one

and:

  -           Added content is represented by lines beginning with "+". You can
  -           prevent staging any addition lines by deleting them.
  +           Added content is represented by lines beginning with "&#43;". You
  +           can prevent staging any addition lines by deleting them.

which is a pretty bad regression.

The trailer misses the version field:

  -Git omitted                       1970-01-01                        GIT-ADD(1)
  +Git                               1970-01-01                        GIT-ADD(1)

The "omitted" is part of doc-diff's attempt to reduce noise in the
diff. But you can see that it's missing entirely in the direct version.

There are lots of whitespace changes for lists. They mostly seem fine
either way. It also formats numbered lists differently:

  -            1. Delete the remote-tracking branches "todo", "html" and
  +           (1) Delete the remote-tracking branches "todo", "html" and
                  "man". The next fetch or pull will create them again
                  unless you configure them not to. See git-fetch(1).
  -            2. Delete the "test" branch even if the "master" branch (or
  +           (2) Delete the "test" branch even if the "master" branch (or
                  whichever branch is currently checked out) does not have
                  all commits from the test branch.

I prefer the original, but could live with the latter (IIRC, this is
something that can be configured via asciidoctor, but I didn't dig).

This one is quite curious (from gitworkflows(7)):

  -       Example 1. Merge upwards
  +       Rule: Merge upwards

The source looks like this:

  .Merge upwards
  [caption="Rule: "]

So it looks like it takes the caption, rather than the phrase "example"
that I guess is coming from deep within the bowls of docbook. Both
asciidoc and asciidoctor produce the "Example" text when going through
xmlto, but both produce "Rule" when generating HTML. So I imagine the
latter was the intent, and this is a fix.

Links are a bit harder to read. E.g.:

   SEE ALSO
          git-check-ref-format(1), git-fetch(1), git-remote(1), “Understanding
  -       history: What is a branch?”[1] in the Git User’s Manual.
  +       history: What is <user-manual.html#what-is-a-branch> a branch?”" in the
  +       Git User’s Manual.

There may be more lurking, but it's hard to tell given how noisy the
diff is.

-Peff

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11 22:27 [PATCH] doc: use asciidoctor to " Felipe Contreras
2021-05-11 23:26 ` brian m. carlson
2021-05-12  0:58   ` Felipe Contreras
2021-05-12  2:11     ` [PATCH 1/2] doc: add an option to have Asciidoctor " brian m. carlson
2021-05-12  2:11       ` [PATCH 2/2] doc: remove GNU_ROFF option brian m. carlson
2021-05-12  2:18         ` Eric Sunshine
2021-05-12  2:28           ` brian m. carlson
2021-05-12  4:45         ` Felipe Contreras
2021-05-14  0:11           ` brian m. carlson
2021-05-15 13:30             ` Felipe Contreras
2021-05-13 13:11         ` Martin Ågren
2021-05-12  2:48       ` [PATCH 1/2] doc: add an option to have Asciidoctor build man pages directly Bagas Sanjaya
2021-05-12  5:03         ` Felipe Contreras
2021-05-13 23:24         ` brian m. carlson
2021-05-14 12:58           ` Felipe Contreras
2021-05-15 13:25           ` Felipe Contreras
2021-05-12  4:41       ` Felipe Contreras
2021-05-13 23:38         ` brian m. carlson
2021-05-14 19:02           ` Felipe Contreras
2021-05-12  4:43       ` Bagas Sanjaya
2021-05-13 23:54         ` brian m. carlson
2021-05-12  6:22       ` Jeff King
2021-05-12  6:30         ` Jeff King
2021-05-12  6:59           ` Jeff King [this message]
2021-05-12 19:29             ` Felipe Contreras
2021-05-13 17:30             ` Martin Ågren
2021-05-13 22:37               ` Felipe Contreras
2021-05-12 19:53           ` Eric Sunshine
2021-05-12 22:37             ` Jeff King
2021-05-14 15:34           ` Martin Ågren
2021-05-14  0:31     ` [PATCH v2 0/2] Asciidoctor native manpage builds brian m. carlson
2021-05-14  0:31       ` [PATCH v2 1/2] doc: add an option to have Asciidoctor build man pages directly brian m. carlson
2021-05-14  3:58         ` Junio C Hamano
2021-05-14  5:27           ` Jeff King
2021-05-14 20:00             ` Felipe Contreras
2021-05-14 19:55           ` brian m. carlson
2021-05-14 20:52             ` Felipe Contreras
2021-05-14 19:57           ` Felipe Contreras
2021-05-14 19:53         ` Felipe Contreras
2021-05-14 20:17           ` brian m. carlson
2021-05-14 23:31             ` Felipe Contreras
2021-05-14  0:31       ` [PATCH v2 2/2] doc: remove GNU_ROFF option brian m. carlson
2021-05-14 19:07       ` [PATCH v2 0/2] Asciidoctor native manpage builds Felipe Contreras
2021-05-14 20:00         ` brian m. carlson
2021-05-14 21:21           ` Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJt81neO7zsGz2ah@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=bagasdotme@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git