patchdoc: remove custom callouts format

The code to render callouts for manpages comes from 17 years ago: 776e994af5 (Properly render asciidoc "callouts" in git man pages., 2006-04-28), and it was needed back then, but DocBook Stylesheets added support for that in 2008 [1], since 1.74.0 it hasn't been necessary.

What's worse: the format of the upstream callouts is much nicer than our hacked version.

Compare this:

     $ git diff            (1)
     $ git diff --cached   (2)
     $ git diff HEAD       (3)

  1. Changes in the working tree not yet staged for the next
     commit.
  2. Changes between the index and your last commit; what you
     would be committing if you run git commit without -a
     option.
  3. Changes in the working tree since your last commit; what
     you would be committing if you run git commit -a

To this:

     $ git diff            (1)
     $ git diff --cached   (2)
     $ git diff HEAD       (3)

 1. Changes in the working tree not yet staged for the next commit.
 2. Changes between the index and your last commit; what you would
 be committing if you run git commit without -a option.
 3. Changes in the working tree since your last commit; what you
 would be committing if you run git commit -a

Let's drop our unnecessary inferior custom format and use the official one.

[1] https://sourceforge.net/p/docbook/code/7842/

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/manpage-normal.xsl | 15 ---------------
 1 file changed, 15 deletions(-)

On Mon, Apr 17, 2023 at 07:18:28PM -0600, Felipe Contreras wrote:

You don't say which is which, so I'm hoping that the top one is the new output. :) And running doc-diff shows that it is. Good.

It does look like at least one spot is made worse, though. In git-checkout, we have now:

          1. The following sequence checks out the master branch,
             reverts the Makefile to two revisions back, deletes hello.c
             by mistake, and gets it back from the index.
  
                 $ git checkout master             (1)
                 $ git checkout master~2 Makefile  (2)
                 $ rm -f hello.c
                 $ git checkout hello.c            (3)
  
             1. switch branch
             2. take a file out of another commit
             3. restore hello.c from the index
  
             If you want to check out all C source files out of the
             index, you can say
  
                 $ git checkout -- '*.c'
  
             Note the quotes around *.c. The file hello.c will also be
             checked out, even though it is no longer in the working
             tree, because the file globbing is used to match entries in
             the index (not in the working tree by the shell).

which is achieved through plus-continuation of each paragraph, to make it all part of the block under item "1." in the numbered list.

But after your patch, it's:

          1. The following sequence checks out the master branch,
             reverts the Makefile to two revisions back, deletes hello.c
             by mistake, and gets it back from the index.
  
                 $ git checkout master             (1)
                 $ git checkout master~2 Makefile  (2)
                 $ rm -f hello.c
                 $ git checkout hello.c            (3)
  
              1. switch branch
              2. take a file out of another commit
              3. restore hello.c from the index
  
                 If you want to check out all C source files out of
                 the index, you can say
  
                                $ git checkout -- '*.c'
  
                            Note the quotes around *.c. The file
                            hello.c will also be checked out, even
                            though it is no longer in the working
                            tree, because the file globbing is used
                            to match entries in the index (not in the
                            working tree by the shell).

The plus-continuation seems to get attached to the final item of the callout list, rather than to the surrounding block. I tried one or two things to disambiguate this (like opening a new block around the example+callout section), but couldn't get any reasonable output. It's also weird that it keeps indenting each block further (I'd expect "Note the quotes" to be in line with "If you want to").

This is perhaps a bug in asciidoc itself. Building with USE_ASCIIDOCTOR is mostly good, though it seems to drop the newline between the callout list and the next paragraph, which our custom one has:

          1. The following sequence checks out the master branch,
             reverts the Makefile to two revisions back, deletes hello.c
             by mistake, and gets it back from the index.
  
                 $ git checkout master             (1)
                 $ git checkout master~2 Makefile  (2)
                 $ rm -f hello.c
                 $ git checkout hello.c            (3)
  
              1. switch branch
              2. take a file out of another commit
              3. restore hello.c from the index
             If you want to check out all C source files out of the
             index, you can say
  
                 $ git checkout -- '*.c'
  
             Note the quotes around *.c. The file hello.c will also be
             checked out, even though it is no longer in the working
             tree, because the file globbing is used to match entries in
             the index (not in the working tree by the shell).

It's probably still worth moving forward with your patch, as I think it takes us in the direction we want long-term (and which builds with asciidoctor are already using). But we may want to pair it with a patch to work around the issue with git-checkout.1 using asciidoc to avoid regressing that section. It may require re-wording or re-organizing to work around the bug.

-Peff

On Tue, Apr 18, 2023 at 12:00:34AM -0400, Jeff King wrote:

Just to clarify my comment on asciidoctor: since our Makefile will put its output through docbook, too, it actually is using the xsl you're removing here (though I'm unclear on why its output looks good in general even before your patch).

But the problem with git-checkout.1 in particular is that the XML generated by asciidoc does not close the <calloutlist> at the right spot (it sticks the continuation onto the calloutlist), whereas asciidoctor does it right (it closes the calloutlist after item 3).

I had tried putting an open block around the whole example and callout list before, which asciidoc doesn't like. But doing it just around the callout list, like so:

Jeff King wrote:

It's probably a quirk of the git hack for callouts, but that's specific to manpages.

Have you looked at the HTML output generated by asciidoc-py? The badly indented output suggests this issue has nothing to do with my patch.

Jeff King wrote:

That's because git's usage of asciidoc leaves much to be desired.

The `+` character is used as a list continuation, how is the doc parser supposed to know that we want to continue the 1. list item, or the 3. callout?

That's why instead of:

  1. foo
  +
  bar
  +
  roo

It's better (and easier) to use an open block.

  1. foo
  +
  --
  bar

  roo
  --

This is what the documentation of asciidoctor recommends, and it even has an example to attack a block to a grandparent list item.

If we do this then the parser has no trouble understanding what we are trying to do:

  --- a/Documentation/git-checkout.txt
  +++ b/Documentation/git-checkout.txt
  @@ -523,36 +523,37 @@ EXAMPLES
     the `Makefile` to two revisions back, deletes `hello.c` by
     mistake, and gets it back from the index.
   +
  +--
   ------------
   $ git checkout master             <1>
   $ git checkout master~2 Makefile  <2>
   $ rm -f hello.c
   $ git checkout hello.c            <3>
   ------------
  -+
   <1> switch branch
   <2> take a file out of another commit
   <3> restore `hello.c` from the index
  -+
  +
   If you want to check out _all_ C source files out of the index,
   you can say
  -+
  +
   ------------
   $ git checkout -- '*.c'
   ------------
  -+
  +
   Note the quotes around `*.c`.  The file `hello.c` will also be
   checked out, even though it is no longer in the working tree,
   because the file globbing is used to match entries in the index
   (not in the working tree by the shell).
  -+
  +
   If you have an unfortunate branch that is named `hello.c`, this
   step would be confused as an instruction to switch to that branch.
   You should instead write:
  -+
  +
   ------------
   $ git checkout -- hello.c
   ------------
  +--
   
   . After working in the wrong branch, switching to the correct
     branch would be done using:

In my opinion it's a bug in our usage of asciidoc. It happened to look OK by accident.

That's a DocBook Stylesheets problem, if you apply the patch above to use an open block, then you get the same output with both asciidoc and asciidoctor.

I don't see why we insist on creating such complex list items though.

Creating a subsection is much clearer for everyone: the reader, the writer, and the parser:

On Mon, Apr 17, 2023 at 11:30:18PM -0600, Felipe Contreras wrote:

Ah, that makes sense. I tried something like this, but asciidoc was unhappy with the "+" continuation between the example and the callout inside the block (which makes sense as there is no "list" to continue within that block).

Just putting the example and its callouts in a block is sufficient, but I agree that putting all of the "The following sequence..." list item's content into a single block makes the source easier to read.

Unless the subsection has a meaningful title, the formatting of the result looks a bit odd to me:

  EXAMPLES
     1
         The following sequence checks out the master branch, reverts
         the Makefile to two revisions back, deletes hello.c by mistake,
         and gets it back from the index.

as opposed to:

          1. The following sequence checks out the master branch,
             reverts the Makefile to two revisions back, deletes hello.c
             by mistake, and gets it back from the index.

If each example had a short section title, it would make more sense.

At any rate, IMHO it is probably best to start with purely syntactic fixes that don't change the output, as that is uncontroversial and addresses the issue with your original patch (which is otherwise making most spots look nicer). And then any conversation about sections versus lists can proceed independently.

So I'd prefer the open block.

-Peff

Jeff King wrote:

To me it's the exact opposite: it's odd that the whole example is indented, when it's part of the main content.

I prefer the example to be indented at the same level as all the other text.

Also, because I use manpage color trick, the subsection header "1" is rendered in red, while the list number is not. It's odd that even the callout numbers inside the example are highlighted in red, but not the number of the example.

I don't particularly mind, but if you think that's a requirement, then why not add that?

I sent a patch series doing precisely t hat.

But the syntactic fixes changes the output.

Have you looked at the HTML output with asciidoc-py? It has the same indentation problem you spotted in the manpages.

I don't see it in git-scm.com, but I presume that documentation there is generated with asciidoctor.

What if I add a proper title?

    === 2. Merge

Another advantage is that we can link directly to the subsection in HTML:

    git-checkout.html#_2_merge

It's not something that's probably going to be used in practice, but to me it makes total semantic sense to have big chunks of prose in a section of its own.

Having a huge list item on the other hand does not make sense, it would be like having a list item that spans more than one page of a book.

Cheers.

On Tue, Apr 18, 2023 at 01:15:14AM -0600, Felipe Contreras wrote:

I hadn't looked at it, but yeah, I see it has the same issue. That makes sense, since the XML output from asciidoc was wrong (and our xslt was papering over it for the manpage build).

And yes, the pages no git-scm.com are built with asciidoctor.

From the perspective of somebody skimming through the examples, that doesn't seem to help much. But I'm not sure if there _is_ a succinct and descriptive title for each of those examples (or at least I could not easily come up with one, which is why I didn't offer a suggestion).

We may have to agree to disagree on that. But this is exactly why I suggested doing the syntactic fix first, rather than reorganizing. Once the fix is done, then there can be a separate discussion on reorganizing (which, frankly, I don't really have much interest in either way; I gave my opinion and I don't have anything else to say).

-Peff

Jeff King wrote:

Yeah, so the problem is already there and had nothing to do with my patch.

I sent a separate patch series to fix that _separate_ problem.

I think it helps that the examples are not indented to a level that no main prose in the manpage is indented at.

Do we though?

Do you actually think the output of this command with a huge list item makes semantic sense?

  (
    printf '1. '
    for x in $(seq 1 10000); do
      printf 'foo '
    done
    echo

    printf '2. bar\n'
  ) | asciidoctor - -o test.html

The syntactic fix is--first of all--orthogonal to my patch.

And secondly: causes another glitch. So it doesn't seem like much of a fix, more like a workaround in which we trade a big glitch for a small glitch.

I'm not sure it's a fix, but more relevantly: I'm not sure what that has to do with my patch.

The fix for how we use asciidoc in git-checkout.txt can be implemented in a totally separate patch series that has nothing to do with $subject.

Cheers.