Re: [PATCH 1/2] doc: add an option to have Asciidoctor build man pages directly

brian m. carlson wrote:

On 2021-05-12 at 04:41:41, Felipe Contreras wrote:

quoted

brian m. carlson wrote:

quoted

since some common distros are > still on 1.5.

Are "some common distros" namely Debian stable *exclusively*?

If so, I would consider flipping the default the other way around,
espececially since it's only te default shipped by the Debian stable
packages (easily fixed by `gem install asciidoctor`).

CentOS 7 and Ubuntu 18.04 also ship 1.5.  CentOS 8 does not appear to
ship Asciidoctor at all.

I does not matter what version of asciidoctor they *ship* with.... At all.

I happen to have accesss to an Ubuntu 18.04 machine...

  time gem install --user-install asciidoctor
  real	0m3.179s

It took me 3 seconds to get asciidoctor-2.0.15.

Ubuntu 18.04 ships with ruby 2.5.1p57 (2018-03-29 revision 63029). And
that's all you need.

quoted

quoted

If users are using a more modern toolchain or don't care
about the rendering issues, they can enable the option.

The other way around: if users are using an ancient distribution they
can disable the option.

Debian stable is not ancient.  It is less than two years old, which for
a Linux distro or any operating system distribution, is not at all
considered even reasonably old.

I guess your definition of what's "old" and mine are *very* different.

But the problem doesn't come from when the distribution was released,
but when the packages *inside* the distribution were released.

I am not going to propose or give my approval to a change that causes
problems building Git with the distro packages on Debian stable or the
latest Ubuntu LTS, in any way, shape, or form.  People should be able to
use the distro packages if that's most convenient.

My proposed changes do not cause any problems building Git with any
distro package on Debian stable or the latest Ubuntu LTS in any way,
shape or form.

quoted

quoted

Suggested-by: Bagas Sanjaya <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: brian m. carlson <redacted>

Commit-message-by: brian m. carlson [off-list ref]

I certainly would not want to pretend to have written the text above.

I'll reroll with the author reset.

Thats is not what I just requested.

quoted

quoted

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index d906a00803..40fa87b121 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb

@@ -15,6 +15,8 @@ module Git
           "#{target}(#{attrs[1]})</ulink>"
         elsif parent.document.basebackend? 'html'
           %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>)
+        elsif parent.document.basebackend? 'manpage'
+          %(\\fB#{target}\\fP\\fR(#{attrs[1]})\\fP)

I still prefer my original version, especially since:

 1. I suspect most git developers are familiar with printf directives:
    %s.
 2. Where is that \\fP coming from? I don't see that with xmlto, nor the
    publicly genrated man pages[1].

That's coming from my knowledge of troff, having used it extensively
over the years, and my general hesitance to affect global state.

Good. Send it as a *separate* patch.

Most people in the mailing list are trying to **minimize** the diff
between asciidoc+docbook and asciidoctor, not piggyback improvements.

If you want to flex your troff knowledge do it as a separate patch,
without my authorship, or s-o-b line.

quoted

 3. Doesn't work on my machine without my original \e; I see
    "\fBgittutorial\fR(7)".

Works just fine on my system using Asciidoctor 2.0.12.  The \e would
insert an additional escape character and shouldn't be needed unless
we're in copy mode (which we should not be here).

I fail to see how that could work since asciidoctor replaces '\'
with '\(rs'.

This is a simple test that reproduces the issue:

  ruby -r asciidoctor <<\EOF | groff -Tascii -man | less
  puts Asciidoctor.convert("This is a \\fBtest\\fR.", backend: :manpage)
  EOF

Those that work in your machine?

This happens both with v2.0.12 and the latest master (2.0.16.dev).

Cheers.

-- 
Felipe Contreras