| Message ID | 20230418011828.47851-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 756991bc88a9c7a089cc7a8746c86159a7a155e8 |
| Headers | show |
| Series | doc: remove custom callouts format | expand |
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). 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: > 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. 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: diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index 6bb32ab460..bd83a6e5d2 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -530,9 +530,11 @@ $ 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 seems to improve things. The generated XML looks sensible, and the output looks like asciidoctor's, though unfortunately both omit the blank line between the end of the callout list and the next paragraph. That might be good enough to live with. -Peff
Jeff King wrote: > 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.
On Mon, Apr 17, 2023 at 11:30:18PM -0600, Felipe Contreras wrote: > 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 > -+ 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. > 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: 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. > > 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 So I'd prefer the open block. -Peff
Jeff King wrote: > On Mon, Apr 17, 2023 at 11:30:18PM -0600, Felipe Contreras wrote: > > > 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 > > -+ > > 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. > > > 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: > > 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. 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. > If each example had a short section title, it would make more sense. 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. > 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. 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. > > > 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 > > So I'd prefer the open block. 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.
Jeff King wrote: > On Tue, Apr 18, 2023 at 12:00:34AM -0400, Jeff King wrote: > > > 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. > > 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). 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.
On Tue, Apr 18, 2023 at 01:15:14AM -0600, Felipe Contreras wrote: > 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. 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. > > So I'd prefer the open block. > > What if I add a proper title? > > === 2. Merge 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). > 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. 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: > On Tue, Apr 18, 2023 at 01:15:14AM -0600, Felipe Contreras wrote: > > > 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. > > 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). 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. > > > So I'd prefer the open block. > > > > What if I add a proper title? > > > > === 2. Merge > > From the perspective of somebody skimming through the examples, that > doesn't seem to help much. I think it helps that the examples are not indented to a level that no main prose in the manpage is indented at. > > 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. > > We may have to agree to disagree on that. 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 > But this is exactly why I suggested doing the syntactic fix first, rather > than reorganizing. 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. > 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). 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.
diff --git a/Documentation/manpage-normal.xsl b/Documentation/manpage-normal.xsl index a9c7ec69f4..e4c5874ed3 100644 --- a/Documentation/manpage-normal.xsl +++ b/Documentation/manpage-normal.xsl @@ -8,19 +8,4 @@ <xsl:param name="man.output.quietly" select="1"/> <xsl:param name="refentry.meta.get.quietly" select="1"/> -<!-- convert asciidoc callouts to man page format --> -<xsl:template match="co"> - <xsl:value-of select="concat('\fB(',substring-after(@id,'-'),')\fR')"/> -</xsl:template> -<xsl:template match="calloutlist"> - <xsl:text>.sp </xsl:text> - <xsl:apply-templates/> - <xsl:text> </xsl:text> -</xsl:template> -<xsl:template match="callout"> - <xsl:value-of select="concat('\fB',substring-after(@arearefs,'-'),'. \fR')"/> - <xsl:apply-templates/> - <xsl:text>.br </xsl:text> -</xsl:template> - </xsl:stylesheet>
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(-)