| Message ID | 20230408001829.11031-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9a09ed322908b1a022a2948802f1ad4588223320 |
| Headers | show |
| Series | doc: simplify man version | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > diff --git a/Documentation/Makefile b/Documentation/Makefile > index a6ba5bd460..4721b000c1 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11 > ASCIIDOC_DOCBOOK = docbook > ASCIIDOC_CONF = -f asciidoc.conf > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ > - -amanversion=$(GIT_VERSION) \ > - -amanmanual='Git Manual' -amansource='Git' > + -amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK) Is this a complete patch, or will this leave us in an incomplete in-between place? We have some references to manversion in "git grep manversion Documentation/" in asciidoc.conf and asciidoctor-extensions.rb remaining after this ptach is applied, which presumably are no longer used. I would imagine that these leftover references end up substituting them with something benign, like an empty string, in the output, but it somehow makes me feel dirty [*]. Other than that, I like the simplification of requiring only two pieces of information to convey the same information that we are attempting to (and to some backends, failing to) give with three pieces of information. [Footnote] * If I am not guessing correctly how the result of applying this patch works in the above "I would imagine ..." that led to my possible misunderstanding of feeling "dirty", it would be a sign that the proposed log message is not explaining sufficiently and deserves an update. Even just saying "... and when they join the `source` and `version`, if `version` is left empty or unspecified, the resulting document would not show any extra whitespace. So it is safe to do the joining ourselves and stuff the result in the `source` field" or something would be sufficient, I would imagine, in order to help the future readers of "git log" that there is no need to "feel dirty" the same way I did.
On Sat, Apr 08, 2023 at 03:45:48PM -0700, Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > diff --git a/Documentation/Makefile b/Documentation/Makefile > > index a6ba5bd460..4721b000c1 100644 > > --- a/Documentation/Makefile > > +++ b/Documentation/Makefile > > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11 > > ASCIIDOC_DOCBOOK = docbook > > ASCIIDOC_CONF = -f asciidoc.conf > > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ > > - -amanversion=$(GIT_VERSION) \ > > - -amanmanual='Git Manual' -amansource='Git' > > + -amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' > > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS > > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) > > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK) > > Is this a complete patch, or will this leave us in an incomplete > in-between place? > > We have some references to manversion in "git grep manversion > Documentation/" in asciidoc.conf and asciidoctor-extensions.rb > remaining after this ptach is applied, which presumably are no > longer used. I would imagine that these leftover references end up > substituting them with something benign, like an empty string, in > the output, but it somehow makes me feel dirty [*]. I think we are OK with this patch on its own. Asciidoc seems to be smart enough to omit the empty XML element on its own. Asciidoctor isn't (and nor is ruby hack which adds it in), but docbook is essentially just concatenating them anyway. Either way, the generated roff looks like: .TH "GIT" "1" "2023\-04\-06" "Git 2\&.40\&.0\&.316\&.g67fafd" "Git Manual" (the first "GIT" is the command name, so this is from git.1). I do think we probably want to pair this with another patch removing the asciidoctor-extensions hack, but the reasoning there is separate (it was needed for some older versions that we can probably declare as "too old" now). -Peff
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > diff --git a/Documentation/Makefile b/Documentation/Makefile > > index a6ba5bd460..4721b000c1 100644 > > --- a/Documentation/Makefile > > +++ b/Documentation/Makefile > > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11 > > ASCIIDOC_DOCBOOK = docbook > > ASCIIDOC_CONF = -f asciidoc.conf > > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ > > - -amanversion=$(GIT_VERSION) \ > > - -amanmanual='Git Manual' -amansource='Git' > > + -amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' > > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS > > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) > > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK) > > Is this a complete patch, Yes it is complete. > or will this leave us in an incomplete in-between place? No. > We have some references to manversion in "git grep manversion > Documentation/" in asciidoc.conf and asciidoctor-extensions.rb > remaining after this ptach is applied, which presumably are no > longer used. I would imagine that these leftover references end up > substituting them with something benign, like an empty string, in > the output, but it somehow makes me feel dirty [*]. Passing an empty string has the same effect, because as it is explained in the commit message: DocBook Stylesheets simply join them *if* both are present (not empty). > Other than that, I like the simplification of requiring only two > pieces of information to convey the same information that we are > attempting to (and to some backends, failing to) give with three > pieces of information. Yes. > [Footnote] > > * If I am not guessing correctly how the result of applying this > patch works in the above "I would imagine ..." that led to my > possible misunderstanding of feeling "dirty", it would be a sign > that the proposed log message is not explaining sufficiently and > deserves an update. Even just saying "... and when they join the > `source` and `version`, if `version` is left empty or unspecified, > the resulting document would not show any extra whitespace. So it > is safe to do the joining ourselves and stuff the result in the > `source` field" or something would be sufficient, I would imagine, > in order to help the future readers of "git log" that there is no > need to "feel dirty" the same way I did. I don't know know what could give this impression, given that a link to the documentation and the link to the source code was given: if we have a Name and/or Version, use either or both of those, in the form "Name Version" or just "Name" or just "Version" https://github.com/docbook/xslt10-stylesheets/blob/master/xsl/common/refentry.xsl#L545 The code clearly tests for empty strings: test="not($Name = '') and not($Version = '') And it's not clear to me what else it would be checking for. asciidoc.py doesn't conditinally add this field: if manversion is not provided it just sets an empty field (if revnumber isn't provided either): <refmiscinfo class="version">{manversion={revnumber}}</refmiscinfo> If this works for programs that don't set manversion, why wouldn't it work for us?
Jeff King wrote: > I do think we probably want to pair this with another patch removing the > asciidoctor-extensions hack, but the reasoning there is separate (it was > needed for some older versions that we can probably declare as "too old" > now). Indeed. That patch requires deciding a minimum asciidoctor version for certain backends. This patch does not.
Jeff King <peff@peff.net> writes: >> Is this a complete patch, or will this leave us in an incomplete >> in-between place? >> >> We have some references to manversion in "git grep manversion >> Documentation/" in asciidoc.conf and asciidoctor-extensions.rb >> remaining after this ptach is applied, which presumably are no >> longer used. I would imagine that these leftover references end up >> substituting them with something benign, like an empty string, in >> the output, but it somehow makes me feel dirty [*]. > > I think we are OK with this patch on its own. Asciidoc seems to be smart > enough to omit the empty XML element on its own. OK, that kind of "why is this a safe thing to do" was what I expected to see explained in the proposed log message. Will queue. Thanks, both.
Felipe Contreras <felipe.contreras@gmail.com> writes: > Junio C Hamano wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > diff --git a/Documentation/Makefile b/Documentation/Makefile >> > index a6ba5bd460..4721b000c1 100644 >> > --- a/Documentation/Makefile >> > +++ b/Documentation/Makefile >> > @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11 >> > ASCIIDOC_DOCBOOK = docbook >> > ASCIIDOC_CONF = -f asciidoc.conf >> > ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ >> > - -amanversion=$(GIT_VERSION) \ >> > - -amanmanual='Git Manual' -amansource='Git' >> > + -amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' >> > ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS >> > TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) >> > TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK) >> >> Is this a complete patch, > > Yes it is complete. Good. > I don't know know what could give this impression, given that a link to > the documentation and the link to the source code was given: > ... > The code clearly tests for empty strings: > > test="not($Name = '') and not($Version = '') This part is exactly what I meant. The readers of "git log" shouldn't have to dig to external source material and find that line to convince themselves why this is safe thing to do. > And it's not clear to me what else it would be checking for. Good. The job of reviewers is not about nitpicking, but work with and help a patch author to polish the patch text (both proposed log message or diff) by pointing out what the author may have thought obvious to everybody, because it was obvious to the author, but may not be so obvious. The goal is not to convince reviewers how the patch text is correct in review discussion thread. The goal is to use reviewers' input to identify such parts of the patch text that needs clarifying and update the patch text. It is to avoid future readers of "git log -p" to ask the same question, because unlike reviewers, they will not have the original author readily available to answer their questions. Thanks.
Hi, I noticed this patch landed in "next", but I found a typo: Felipe Contreras wrote: > There is no need for us to demand that that they add support for the > version field when in reality all that is going to happen is that both > fields are going to be joined. s/that that/that/ Is there any procedure to fix this? Or is it a small enough issue to leave it as it is?
diff --git a/Documentation/Makefile b/Documentation/Makefile index a6ba5bd460..4721b000c1 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -150,8 +150,7 @@ ASCIIDOC_HTML = xhtml11 ASCIIDOC_DOCBOOK = docbook ASCIIDOC_CONF = -f asciidoc.conf ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ - -amanversion=$(GIT_VERSION) \ - -amanmanual='Git Manual' -amansource='Git' + -amanmanual='Git Manual' -amansource='Git $(GIT_VERSION)' ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)
The hacks to add version information to the man pages comes from 2007 7ef195ba3e (Documentation: Add version information to man pages, 2007-03-25). In that code we passed three fields to DocBook Stylesheets: `source`, `version`, and `manual`, however, all the stylesheets do is join the strings `source` and `version` [1]. Their own documentation explains that in pracice the source is just a combination of two fields [2]: In practice, there are many pages that simply have a version number in the "source" field. Splitting that information might have seemed more proper in 2007, but it not achieve anything in practice. Asciidoctor had support for this information in their manpage backend since day 1: v1.5.3 (2015), but it didn't include the version. In the docbook5 backend they did in v1.5.7 (2018), but again: no version. There is no need for us to demand that that they add support for the version field when in reality all that is going to happen is that both fields are going to be joined. Let's do that ourselves so we can forget about all our hacks for this and so it works for both asciidoc.py, and docbook5 and manpage backends of asciidoctor. [1] https://github.com/docbook/xslt10-stylesheets/blob/master/xsl/common/refentry.xsl#L545 [2] https://docbook.sourceforge.net/release/xsl/current/doc/common/template.get.refentry.source.html Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)