Re: [PATCH] doc: remove custom callouts format

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 18, 2023, 05:30 UTC
Message-ID: <643e2aeae0515_217cdb29473@chronos.notmuch>
In-Reply-To: <20230418040034.GC60552@coredump.intra.peff.net>

Jeff King wrote:

Show 89 quoted lines

> On Mon, Apr 17, 2023 at 07:18:28PM -0600, Felipe Contreras wrote:
> 
> > 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
> 
> 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).

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:

> This is perhaps a bug in asciidoc itself.

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

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

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:

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 6bb32ab460..2d16651d1f 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -519,84 +519,89 @@ to checkout these paths out of the index.
 EXAMPLES
 --------
 
-. 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.
-+
+=== 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).
-+
+
 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
 ------------
 
> 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.

I can add that patch depending what we want:

Open block:

  1. foo
  +
  --
  bar

  roo
  --

Or subsection:

  === 1

  foo

  bar

  roo

Cheers.

-- 
Felipe Contreras