{"thread":{"id":"59565","subject":"[PATCH] doc: simplify man version","startedAt":"2023-04-08T00:18:35Z","lastAt":"2023-04-16T03:45:34Z","messageCount":8,"participants":["Felipe Contreras","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475028","messageId":"20230408001829.11031-1-felipe.contreras@gmail.com","threadId":"59565","inReplyTo":null,"subject":"[PATCH] doc: simplify man version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-08T00:18:29Z","receivedAt":"2023-04-08T00:18:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The hacks to add version information to the man pages comes from 2007\n7ef195ba3e (Documentation: Add version information to man pages,\n2007-03-25). In that code we passed three fields to DocBook Stylesheets:\n`source`, `version`, and `manual`, however, all the stylesheets do is\njoin the strings `source` and `version` [1].\n\nTheir own documentation explains that in pracice the source is just a\ncombination of two fields [2]:\n\n In practice, there are many pages that simply have a version number in\n the \"source\" field.\n\nSplitting that information might have seemed more proper in 2007, but it\nnot achieve anything in practice.\n\nAsciidoctor had support for this information in their manpage backend\nsince day 1: v1.5.3 (2015), but it didn't include the version. In the\ndocbook5 backend they did in v1.5.7 (2018), but again: no version.\n\nThere is no need for us to demand that that they add support for the\nversion field when in reality all that is going to happen is that both\nfields are going to be joined.\n\nLet's do that ourselves so we can forget about all our hacks for this\nand so it works for both asciidoc.py, and docbook5 and manpage backends\nof asciidoctor.\n\n[1] https://github.com/docbook/xslt10-stylesheets/blob/master/xsl/common/refentry.xsl#L545\n[2] https://docbook.sourceforge.net/release/xsl/current/doc/common/template.get.refentry.source.html\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/Makefile | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex a6ba5bd460..4721b000c1 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11\n ASCIIDOC_DOCBOOK = docbook\n ASCIIDOC_CONF = -f asciidoc.conf\n ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n-\t\t-amanversion=$(GIT_VERSION) \\\n-\t\t-amanmanual='Git Manual' -amansource='Git'\n+\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n-- \n2.40.0+fc1\n\n"},{"id":"475041","messageId":"xmqqr0suf0wz.fsf@gitster.g","threadId":"59565","inReplyTo":"20230408001829.11031-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] doc: simplify man version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-08T22:45:48Z","receivedAt":"2023-04-08T22:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index a6ba5bd460..4721b000c1 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11\n> ASCIIDOC_DOCBOOK = docbook\n> ASCIIDOC_CONF = -f asciidoc.conf\n> ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n> -\t\t-amanversion=$(GIT_VERSION) \\\n> -\t\t-amanmanual='Git Manual' -amansource='Git'\n> +\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n> ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n> TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n> TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n\nIs this a complete patch, or will this leave us in an incomplete\nin-between place?\n\nWe have some references to manversion in \"git grep manversion\nDocumentation/\" in asciidoc.conf and asciidoctor-extensions.rb\nremaining after this ptach is applied, which presumably are no\nlonger used. I would imagine that these leftover references end up\nsubstituting them with something benign, like an empty string, in\nthe output, but it somehow makes me feel dirty [*].\n\nOther than that, I like the simplification of requiring only two\npieces of information to convey the same information that we are\nattempting to (and to some backends, failing to) give with three\npieces of information.\n\n\n[Footnote]\n\n* If I am not guessing correctly how the result of applying this\n patch works in the above \"I would imagine ...\" that led to my\n possible misunderstanding of feeling \"dirty\", it would be a sign\n that the proposed log message is not explaining sufficiently and\n deserves an update. Even just saying \"... and when they join the\n `source` and `version`, if `version` is left empty or unspecified,\n the resulting document would not show any extra whitespace. So it\n is safe to do the joining ourselves and stuff the result in the\n `source` field\" or something would be sufficient, I would imagine,\n in order to help the future readers of \"git log\" that there is no\n need to \"feel dirty\" the same way I did.\n"},{"id":"475053","messageId":"20230409190805.GA77061@coredump.intra.peff.net","threadId":"59565","inReplyTo":"xmqqr0suf0wz.fsf@gitster.g","subject":"Re: [PATCH] doc: simplify man version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-09T19:08:05Z","receivedAt":"2023-04-09T19:08:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2023 at 03:45:48PM -0700, Junio C Hamano wrote:\n\n> Felipe Contreras writes:\n> \n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > index a6ba5bd460..4721b000c1 100644\n> > --- a/Documentation/Makefile\n> > +++ b/Documentation/Makefile\n> > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11\n> > ASCIIDOC_DOCBOOK = docbook\n> > ASCIIDOC_CONF = -f asciidoc.conf\n> > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n> > -\t\t-amanversion=$(GIT_VERSION) \\\n> > -\t\t-amanmanual='Git Manual' -amansource='Git'\n> > +\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n> > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n> > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n> > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n> \n> Is this a complete patch, or will this leave us in an incomplete\n> in-between place?\n> \n> We have some references to manversion in \"git grep manversion\n> Documentation/\" in asciidoc.conf and asciidoctor-extensions.rb\n> remaining after this ptach is applied, which presumably are no\n> longer used. I would imagine that these leftover references end up\n> substituting them with something benign, like an empty string, in\n> the output, but it somehow makes me feel dirty [*].\n\nI think we are OK with this patch on its own. Asciidoc seems to be smart\nenough to omit the empty XML element on its own. Asciidoctor isn't (and\nnor is ruby hack which adds it in), but docbook is essentially just\nconcatenating them anyway. Either way, the generated roff looks like:\n\n .TH \"GIT\" \"1\" \"2023\\-04\\-06\" \"Git 2\\&.40\\&.0\\&.316\\&.g67fafd\" \"Git Manual\"\n\n(the first \"GIT\" is the command name, so this is from git.1).\n\nI do think we probably want to pair this with another patch removing the\nasciidoctor-extensions hack, but the reasoning there is separate (it was\nneeded for some older versions that we can probably declare as \"too old\"\nnow).\n\n-Peff\n"},{"id":"475102","messageId":"64349c7dc4f49_7452943c@chronos.notmuch","threadId":"59565","inReplyTo":"xmqqr0suf0wz.fsf@gitster.g","subject":"Re: [PATCH] doc: simplify man version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-10T23:32:13Z","receivedAt":"2023-04-10T23:32:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > index a6ba5bd460..4721b000c1 100644\n> > --- a/Documentation/Makefile\n> > +++ b/Documentation/Makefile\n> > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11\n> > ASCIIDOC_DOCBOOK = docbook\n> > ASCIIDOC_CONF = -f asciidoc.conf\n> > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n> > -\t\t-amanversion=$(GIT_VERSION) \\\n> > -\t\t-amanmanual='Git Manual' -amansource='Git'\n> > +\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n> > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n> > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n> > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n> \n> Is this a complete patch,\n\nYes it is complete.\n\n> or will this leave us in an incomplete in-between place?\n\nNo.\n\n> We have some references to manversion in \"git grep manversion\n> Documentation/\" in asciidoc.conf and asciidoctor-extensions.rb\n> remaining after this ptach is applied, which presumably are no\n> longer used. I would imagine that these leftover references end up\n> substituting them with something benign, like an empty string, in\n> the output, but it somehow makes me feel dirty [*].\n\nPassing an empty string has the same effect, because as it is explained\nin the commit message: DocBook Stylesheets simply join them *if* both\nare present (not empty).\n\n> Other than that, I like the simplification of requiring only two\n> pieces of information to convey the same information that we are\n> attempting to (and to some backends, failing to) give with three\n> pieces of information.\n\nYes.\n\n> [Footnote]\n> \n> * If I am not guessing correctly how the result of applying this\n> patch works in the above \"I would imagine ...\" that led to my\n> possible misunderstanding of feeling \"dirty\", it would be a sign\n> that the proposed log message is not explaining sufficiently and\n> deserves an update. Even just saying \"... and when they join the\n> `source` and `version`, if `version` is left empty or unspecified,\n> the resulting document would not show any extra whitespace. So it\n> is safe to do the joining ourselves and stuff the result in the\n> `source` field\" or something would be sufficient, I would imagine,\n> in order to help the future readers of \"git log\" that there is no\n> need to \"feel dirty\" the same way I did.\n\nI don't know know what could give this impression, given that a link to\nthe documentation and the link to the source code was given:\n\n if we have a Name and/or Version, use either or both of those, in the\n form \"Name Version\" or just \"Name\" or just \"Version\"\n\nhttps://github.com/docbook/xslt10-stylesheets/blob/master/xsl/common/refentry.xsl#L545\n\nThe code clearly tests for empty strings:\n\n test=\"not($Name = '') and not($Version = '')\n\nAnd it's not clear to me what else it would be checking for.\n\nasciidoc.py doesn't conditinally add this field: if manversion is not\nprovided it just sets an empty field (if revnumber isn't provided\neither):\n\n {manversion={revnumber}}\n\nIf this works for programs that don't set manversion, why wouldn't it\nwork for us?\n\n-- \nFelipe Contreras\n"},{"id":"475103","messageId":"64349e979419a_745294e8@chronos.notmuch","threadId":"59565","inReplyTo":"20230409190805.GA77061@coredump.intra.peff.net","subject":"Re: [PATCH] doc: simplify man version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-10T23:41:11Z","receivedAt":"2023-04-10T23:41:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n\n> I do think we probably want to pair this with another patch removing the\n> asciidoctor-extensions hack, but the reasoning there is separate (it was\n> needed for some older versions that we can probably declare as \"too old\"\n> now).\n\nIndeed. That patch requires deciding a minimum asciidoctor version for\ncertain backends.\n\nThis patch does not.\n\n-- \nFelipe Contreras\n"},{"id":"475104","messageId":"xmqqa5zfcngg.fsf@gitster.g","threadId":"59565","inReplyTo":"20230409190805.GA77061@coredump.intra.peff.net","subject":"Re: [PATCH] doc: simplify man version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-10T23:43:59Z","receivedAt":"2023-04-10T23:44:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Is this a complete patch, or will this leave us in an incomplete\n>> in-between place?\n>> \n>> We have some references to manversion in \"git grep manversion\n>> Documentation/\" in asciidoc.conf and asciidoctor-extensions.rb\n>> remaining after this ptach is applied, which presumably are no\n>> longer used. I would imagine that these leftover references end up\n>> substituting them with something benign, like an empty string, in\n>> the output, but it somehow makes me feel dirty [*].\n>\n> I think we are OK with this patch on its own. Asciidoc seems to be smart\n> enough to omit the empty XML element on its own.\n\nOK, that kind of \"why is this a safe thing to do\" was what I\nexpected to see explained in the proposed log message.\n\nWill queue.\n\nThanks, both.\n"},{"id":"475151","messageId":"xmqqwn2i9ybb.fsf@gitster.g","threadId":"59565","inReplyTo":"64349c7dc4f49_7452943c@chronos.notmuch","subject":"Re: [PATCH] doc: simplify man version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T16:30:00Z","receivedAt":"2023-04-11T16:30:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>> \n>> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n>> > index a6ba5bd460..4721b000c1 100644\n>> > --- a/Documentation/Makefile\n>> > +++ b/Documentation/Makefile\n>> > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11\n>> > ASCIIDOC_DOCBOOK = docbook\n>> > ASCIIDOC_CONF = -f asciidoc.conf\n>> > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n>> > -\t\t-amanversion=$(GIT_VERSION) \\\n>> > -\t\t-amanmanual='Git Manual' -amansource='Git'\n>> > +\t\t-amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)'\n>> > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n>> > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n>> > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n>> \n>> Is this a complete patch,\n>\n> Yes it is complete.\n\nGood.\n\n> I don't know know what could give this impression, given that a link to\n> the documentation and the link to the source code was given:\n> ...\n> The code clearly tests for empty strings:\n>\n> test=\"not($Name = '') and not($Version = '')\n\nThis part is exactly what I meant. The readers of \"git log\"\nshouldn't have to dig to external source material and find that\nline to convince themselves why this is safe thing to do.\n\n> And it's not clear to me what else it would be checking for.\n\nGood. The job of reviewers is not about nitpicking, but work with\nand help a patch author to polish the patch text (both proposed log\nmessage or diff) by pointing out what the author may have thought\nobvious to everybody, because it was obvious to the author, but may\nnot be so obvious. The goal is not to convince reviewers how the\npatch text is correct in review discussion thread. The goal is to\nuse reviewers' input to identify such parts of the patch text that\nneeds clarifying and update the patch text. It is to avoid future\nreaders of \"git log -p\" to ask the same question, because unlike\nreviewers, they will not have the original author readily available\nto answer their questions.\n\nThanks.\n"},{"id":"475477","messageId":"643b6f503ad8d_3bb4294bc@chronos.notmuch","threadId":"59565","inReplyTo":"20230408001829.11031-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] doc: simplify man version","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-16T03:45:20Z","receivedAt":"2023-04-16T03:45:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nI noticed this patch landed in \"next\", but I found a typo:\n\nFelipe Contreras wrote:\n> There is no need for us to demand that that they add support for the\n> version field when in reality all that is going to happen is that both\n> fields are going to be joined.\n\ns/that that/that/\n\nIs there any procedure to fix this? Or is it a small enough issue to leave it\nas it is?\n\n-- \nFelipe Contreras\n"}]}