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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 14, 2021, 23:31 UTC
Message-ID: <609f0840eb383_4312720868@natae.notmuch>
In-Reply-To: <YJ7avgek24eu+HHM@camp.crustytoothpaste.net>

brian m. carlson wrote:

Show 15 quoted lines

> On 2021-05-14 at 19:53:13, Felipe Contreras wrote:
> > brian m. carlson wrote:
> > > We generally require Asciidoctor 1.5, but versions before 1.5.3 didn't
> > > contain proper handling of the apostrophe, which is controlled normally
> > > by the GNU_ROFF option.  This option for the DocBook toolchain, as well
> > > as newer versions of Asciidoctor, makes groff output an ASCII apostrophe
> > > instead of a Unicode apostrophe in text, so as to make copy and pasting
> > > commands easier.  These newer versions of Asciidoctor (1.5.3 and above)
> > > detect groff and do the right thing in all cases, so the GNU_ROFF option
> > > is obsolete in this case.
> > 
> > I don't see what that paragraph has to do with the patch below.
> 
> It's relevant because it explains why it's acceptable to discount that
> feature that we're not supporting as part of the patch.

It's easier to first drop that feature, and then you don't have to explain anything.

Show 8 quoted lines

> > > We also need to update the code that tells Asciidoctor how to format our
> > > linkgit macros so that it can output proper code for man pages.
> > 
> > Yes, but why shove it in this patch? Now this is is doing *two*
> > logically-independent changes.
> 
> This is one logical change: implementing Asciidoctor native support for
> man pages.

By that logic all patch series can be a "logical change": "implement SHA-256 support".

Show 17 quoted lines

> > > Be careful to reset the font to the previous after the change.
> > 
> > This is a third change, since the current man pages already don't do
> > this:
> > 
> >   % zcat /usr/share/man/man1/git-add.1.gz | grep '\fB'
> >   you must use the \fBadd\fR command
> 
> As explained downthread, we don't know in the manual pages what font
> styling we're in.  troff has font-change commands, not nesting begin-end
> pairs, for italics and bold.  If the linkgit macro appears in the middle
> of a passage in italics, by not using \fP, we'll force the rest of the
> text which is to be italicized into roman.
> 
> The toolchain, whether Asciidoctor or the XSLT stylesheets, _does_ have
> this context and therefore can explicitly move between bold and roman,
> but our extensions do not.

Indeed but it's rare (there's probably zero instances), and it increases the delta. Yes, it's more correct, but it trades a hypothetical benefit for a real disadvantage.

Either way I see no point in arguing about this. If you feel strongly about this I can include it in my version.

Show 9 quoted lines

> > > Additionally, we don't want to use XML-style escapes for the litdd and
> > > plus macros, so let's only use the XML-style escapes in HTML and XML and
> > > use something different for our man pages.
> > 
> > That's a fourth change now, and one that complicates the Makefile even
> > more, when I've been trying to simplify it.
> 
> I'm sorry that this complicates work you'd like to do, but
> unfortunately, the other option is broken rendering.

Clean and maintainable code is a benefit to the project, not just me. And the Makefile is code too.

Forget about me, a clean and simple Makefile is better than a cluttered and complex Makefile. That is the point.

Yes, the rendering is "broken" without the change (that's loaded language, but OK), we want to know precisely how, and how it got fixed. We don't wan to sneak in all the fixes in the world in the first patch.

Moreover, there's no dicotomy here; we can fix the "broken" state in other ways that don't complicate the Makefile, as I did in my patch series [1].

But in fact, I have an even simpler version now:

 Asciidoctor::Extensions.register do
+  # Override attributes for man pages.
+  # https://github.com/asciidoctor/asciidoctor/issues/4059
+  if document.backend == 'manpage'
+    document.attributes.merge!({ 'litdd' => '\--', 'plus' => '+' })
+  end
+
   inline_macro Git::Documentation::LinkGitProcessor, :linkgit
   postprocessor Git::Documentation::DocumentPostProcessor
 end

Show 6 quoted lines

> > > If users are using a more modern toolchain or don't care
> > > about the rendering issues, they can enable the option.
> > 
> > What rendering issues?
> 
> They were mentioned upthread.

No. Pepole mentioned issues they *think* existed, nobody pointed out an actual reproducible issue.

As you experienced with the \e setback, the issue could have been with their build, or it could be present in v1.5.7, but not v1.5.8. It's hard to know if nobody spells out what the issue is.

So... What rendering issues?

Show 12 quoted lines

> > Also, the many should not suffer because of the few.
> > 
> > If a few people doing USE_ASCIIDOCTOR=YesPlease have issues (because of
> > ancient packages in their distribution, and their reluctance to type
> > `gem install`), then *they* can disable USE_ASCIIDOCTOR_MANPAGE (or just
> > disable USE_ASCIIDOCTOR altogether). Most people doing
> > USE_ASCIIDOCTOR=YesPlease should not suffer because of a
> > minority.
> 
> I don't believe we're going to agree on this.  I believe we should
> choose defaults that work with the most popular Linux distributions, and
> you don't.

Untrue.

`make USE_ASCIIDOCTOR= doc` works perfectly fine on Debian stable with my patches, and that's the default, nobody is changing that.

And so does this:

  gem install asciidoctor && make USE_ASCIIDOCTOR=1 doc

But in fact, so does `make USE_ASCIIDOCTOR=1 doc`, because asciidoctor 1.5.8 works just fine. I just did a doc-diff with v1.5.8, and I checked everything without finding any serious issue (not present in 2.0.15).

The only issue is that \\ was not handled correctly, but I now have a workaround for that:

  if document.basebackend?('manpage') and Asciidoctor::VERSION < '2.0.11'
    postprocessor do
      process do |document, output|
        output.gsub("\\(rs\\\\", "\\(rs\\(rs\\")
      end
    end
  end

> I think your approach is unnecessarily hostile to ordinary
> users and developers and understates the value that people derive from
> distributions.

Please tell me exactly how my patches are hostile to "ordinary developers" (who are not ordinary at all).

 1. Will they be able to build the documentation with default flags?
    Yes
 2. Will they be able to build with USE_ASCIIDOCTOR=1?
    Yes
 3. Will they be able to see a reasonable output?
    Yes
 4. Will they be forced to install a gem?
    No

So where exactly is the hostility?

Show 5 quoted lines

> > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > 
> > I most definitely do not sign off this.
> 
> This sign-off is not an approval of the patch.

Yes it is. According to the Developer Certificate of Origin [2] I must:

  agree that a record of the contribution (including my sign-off) is
  maintained indefinitely

Which I don't.

Feel free to disregard my lack of agreement, but I'm stating it for the record.

> I do want to be clear that I'm aware you don't approve of this patch and
> that's why I submitted a counterproposal: because I don't approve of
> your patch

That happens.

> and you seem unwilling to make changes to it.

Just because I disagree with your changes doesn't mean I'm unwilling to make changes to my patch, especially since I already agreed on making changes to my patch.

> I would love nothing more than to remove your name from it entirely,
> but unfortunately, that's not possible with the DCO.

It's not possible *if* 1) you use my code, and 2) you made changes I'm opposed to. But you are already in violation of the DCO anyway by disregarding my agreement, which is mandatory.

If you don't want to be in violation of the DCO you could try to fix either 1) or 2).

Cheers.

[1] https://lore.kernel.org/git/20210514121435.504423-8-felipe.contreras@gmail.com/T/#u [2] https://developercertificate.org/

-- 
Felipe Contreras