[PATCH] doc: simplify man version

[PATCH] doc: simplify man version

[Felipe Contreras]

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(-)

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)
-- 
2.40.0+fc1

Re: [PATCH] doc: simplify man version

[Junio C Hamano]

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.

Re: [PATCH] doc: simplify man version

[Jeff King]

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

Re: [PATCH] doc: simplify man version

[Felipe Contreras]

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?

-- 
Felipe Contreras

Re: [PATCH] doc: simplify man version

[Felipe Contreras]

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.

-- 
Felipe Contreras

Re: [PATCH] doc: simplify man version

[Junio C Hamano]

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.

Re: [PATCH] doc: simplify man version

[Junio C Hamano]

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.

Re: [PATCH] doc: simplify man version

[Felipe Contreras]

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?

-- 
Felipe Contreras