[PATCH 00/11] doc: asciidoctor: direct man page creation and fixes

[PATCH 00/11] doc: asciidoctor: direct man page creation and fixes

[Felipe Contreras]

This patch series enables direct man page creation with asciidoctor, but in addition tries to
minimize the difference with asciidoc+docbook.

I fixed as many issues as I could, and now the doc-diff looks very sensible. I could not find any
major issues, however, some minor issues still remain.

On the other hand the asciidoc method has issues as well, so it's not clear which method is superior
at this point; both have advantages and disadvantages.

At the very least the man pages with pure asciidoctor should be quite usable now.

This series builds on top of my cleanups [1]. It's probably not worth the effort to split the two.

[1] https://lore.kernel.org/git/20210514115631.503276-1-felipe.contreras@gmail.com

Felipe Contreras (11):
  doc: allow the user to provide ASCIIDOC_EXTRA
  doc: doc-diff: allow more than one flag
  doc: doc-diff: set docdate manually
  doc: use asciidoctor to build man pages directly
  doc: asciidoctor: add linkgit macros in man pages
  doc: join mansource and manversion
  doc: add man pages workaround for asciidoctor
  doc: asciidoctor: add hack for xrefs
  doc: asciidoctor: add hack to improve links
  doc: asciidoctor: add support for baseurl
  doc: asciidoctor: cleanup man page hack

 Documentation/Makefile                  | 24 ++++++++-----
 Documentation/asciidoctor-extensions.rb | 46 +++++++++++++++++++++++++
 Documentation/doc-diff                  |  8 ++---
 3 files changed, 65 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Felipe Contreras]

Without `override` all additions will be ignored by make.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/Makefile | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index f5605b7767..981e322f18 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -191,9 +191,9 @@ ASCIIDOC = asciidoctor
 ASCIIDOC_CONF =
 ASCIIDOC_HTML = xhtml5
 ASCIIDOC_DOCBOOK = docbook5
-ASCIIDOC_EXTRA += -acompat-mode -atabsize=8
-ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
-ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
+override ASCIIDOC_EXTRA += -acompat-mode -atabsize=8
+override ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
+override ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
 ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
 DBLATEX_COMMON =
 XMLTO_EXTRA += --skip-validation
@@ -206,12 +206,12 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 
 ifdef DEFAULT_PAGER
 DEFAULT_PAGER_SQ = $(subst ','\'',$(DEFAULT_PAGER))
-ASCIIDOC_EXTRA += -a 'git-default-pager=$(DEFAULT_PAGER_SQ)'
+override ASCIIDOC_EXTRA += -a 'git-default-pager=$(DEFAULT_PAGER_SQ)'
 endif
 
 ifdef DEFAULT_EDITOR
 DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
-ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
+override ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
 endif
 
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
@@ -375,7 +375,7 @@ technical/api-index.txt: technical/api-index-skel.txt \
 	technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))
 	$(QUIET_GEN)cd technical && '$(SHELL_PATH_SQ)' ./api-index.sh
 
-technical/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
+technical/%.html: override ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
 $(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html : %.txt \
 	asciidoc.conf GIT-ASCIIDOCFLAGS
 	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt
@@ -425,7 +425,7 @@ $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
 
 WEBDOC_DEST = /pub/software/scm/git/docs
 
-howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
+howto/%.html: override ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
 $(patsubst %.txt,%.html,$(HOWTO_TXT)): %.html : %.txt GIT-ASCIIDOCFLAGS
 	$(QUIET_ASCIIDOC) \
 	sed -e '1,/^$$/d' $< | \
-- 
2.31.1

[PATCH 02/11] doc: doc-diff: allow more than one flag

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/doc-diff | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index 1694300e50..ecd88b0524 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -146,7 +146,7 @@ render_tree () {
 	# through.
 	oid=$1 &&
 	dname=$2 &&
-	makemanflags=$3 &&
+	makemanflags="$3" &&
 	if ! test -d "$tmp/installed/$dname"
 	then
 		git -C "$tmp/worktree" checkout --detach "$oid" &&
@@ -181,6 +181,6 @@ render_tree () {
 	fi
 }
 
-render_tree $from_oid $from_dir $from_makemanflags &&
-render_tree $to_oid $to_dir $to_makemanflags &&
+render_tree $from_oid $from_dir "$from_makemanflags" &&
+render_tree $to_oid $to_dir "$to_makemanflags" &&
 git -C $tmp/rendered diff --no-index "$@" $from_dir $to_dir
-- 
2.31.1

[PATCH 03/11] doc: doc-diff: set docdate manually

[Felipe Contreras]

In order to minimize the differences in the footer.

Asciidoc automatically generates a date with format '%Y-%m-%d', while
asciidoctor '%F'.

I personally prefer the latter, so only modify it for diff purposes.

Fixes tons of these:

-Git omitted                       01/01/1970                        GIT-ADD(1)
+Git omitted                       1970-01-01                        GIT-ADD(1)

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/doc-diff | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index ecd88b0524..aae5fc1933 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -111,7 +111,7 @@ construct_makemanflags () {
 		echo USE_ASCIIDOCTOR=
 	elif test "$1" = "-asciidoctor"
 	then
-		echo USE_ASCIIDOCTOR=YesPlease
+		echo USE_ASCIIDOCTOR=YesPlease ASCIIDOC_EXTRA='-adocdate="01/01/1970"'
 	fi
 }
 
-- 
2.31.1

[PATCH 04/11] doc: use asciidoctor to build man pages directly

[Felipe Contreras]

There's no need to use xmlto to build the man pages when modern
asciidoctor can do it by itself.

This new mode will be active only when USE_ASCIIDOCTOR is set.

Suggested-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 981e322f18..ce9cea0817 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -198,6 +198,7 @@ ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
 DBLATEX_COMMON =
 XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
+TXT_TO_MAN = $(ASCIIDOC_COMMON) -b manpage
 endif
 
 SHELL_PATH ?= $(SHELL)
@@ -362,8 +363,13 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
+ifdef TXT_TO_MAN
+%.1 %.5 %.7 : %.txt $(ASCIIDOC_DEPS)
+	$(QUIET_ASCIIDOC)$(TXT_TO_MAN) -o $@ $<
+else
 %.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
 	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+endif
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<
-- 
2.31.1

[PATCH 05/11] doc: asciidoctor: add linkgit macros in man pages

[Felipe Contreras]

Fixes the doc-diff:

-       Please see git-commit(1) for alternative ways to add content to a
-       commit.
+       Please see  for alternative ways to add content to a commit.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index d906a00803..ad68f7b0bb 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'
+          "\e\\fB%s\e\\fR(%s)" % [target, attrs[1]]
         elsif parent.document.basebackend? 'docbook'
           "<citerefentry>\n" \
             "<refentrytitle>#{target}</refentrytitle>" \
-- 
2.31.1

[PATCH 06/11] doc: join mansource and manversion

[Felipe Contreras]

There's no real value in having two fields when one does the trick.

Also, this add the version for asciidoctor generated man pages.

Fixes the doc-diff:

  -Git omitted                       01/01/1970                        GIT-ADD(1)
  +Git                               01/01/1970                        GIT-ADD(1)

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 ce9cea0817..a514a4e72c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -137,8 +137,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.31.1

[PATCH 07/11] doc: add man pages workaround for asciidoctor

[Felipe Contreras]

Currently asciidoctor doesn't convert number character references
(&#xx;) correctly for man pages.

This hack fixes the issue with minimum changes elsewhere so it's easy to
remove when fixed.

Fixes doc-diffs like:

            so line count cannot be shown) and there is no difference between
            indexed copy and the working tree version (if the working tree
            version were also different, binary would have been shown in place
-           of nothing). The other file, git-add--interactive.perl, has 403
-           lines added and 35 lines deleted if you commit what is in the
-           index, but working tree file has further modifications (one
+           of nothing). The other file, git-add--interactive.perl,
+           has 403 lines added and 35 lines deleted if you commit what is in
+           the index, but working tree file has further modifications (one
            addition and one deletion).

https://github.com/asciidoctor/asciidoctor/issues/4059

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index ad68f7b0bb..11937c2c1d 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -45,6 +45,17 @@ module Git
 end
 
 Asciidoctor::Extensions.register do
+  # Override attributes for man pages.
+  # https://github.com/asciidoctor/asciidoctor/issues/4059
+  tree_processor do
+    process do |document|
+      if document.backend == 'manpage'
+        document.attributes.merge!({ 'litdd' => '\--', 'plus' => '+' })
+      end
+      document
+    end
+  end
+
   inline_macro Git::Documentation::LinkGitProcessor, :linkgit
   postprocessor Git::Documentation::DocumentPostProcessor
 end
-- 
2.31.1

[PATCH 08/11] doc: asciidoctor: add hack for xrefs

[Felipe Contreras]

The docbook manpage stylesheets convert cross-references with format the
'section called “%t”'. I personally prefer the asciidoctor version, but
for now add a hack to minimize the diff.

Thanks to the extensibility of Ruby we can override corresponding method
in the man page converter.

This fixes doc-diffs like:

          --worktree-attributes
              Look for attributes in .gitattributes files in the working tree as
  -           well (see the section called “ATTRIBUTES”).
  +           well (see ATTRIBUTES).

This can easily be removed later once we are confortable with the
asciidoctor version.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index 11937c2c1d..b2bbb318ad 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -1,5 +1,22 @@
 require 'asciidoctor'
 require 'asciidoctor/extensions'
+require 'asciidoctor/converter/manpage'
+
+module Asciidoctor
+  class Converter::ManPageConverter
+    alias orig_convert_inline_anchor convert_inline_anchor
+    def convert_inline_anchor(node)
+      case node.type
+      when :xref
+        return node.text if node.text
+        refid = node.attributes['refid']
+        'the section called &#8220;%s&#8221;' % refid.gsub('_', ' ')
+      else
+        orig_convert_inline_anchor(node)
+      end
+    end
+  end
+end
 
 module Git
   module Documentation
-- 
2.31.1

[PATCH 09/11] doc: asciidoctor: add hack to improve links

[Felipe Contreras]

The way asciidoctor handles links is very primitive compared to docbook.
Links are simply presented in the format "#{text} <#{target}>", which
may not be all that bad for the future, but pollutes the doc-diff.

By adding another modification to convert_inline_anchor() we can present
links in a form very similar to docbook, diminishing the doc-diff.

This significantly reduces the doc-diff:

From:

        abysmal performance). These safety and performance issues cannot be
        backward compatibly fixed and as such, its use is not recommended.
        Please use an alternative history filtering tool such as git
-       filter-repo[1]. If you still need to use git filter-branch, please
-       carefully read the section called “SAFETY” (and the section called
-       “PERFORMANCE”) to learn about the land mines of filter-branch, and then
-       vigilantly avoid as many of the hazards listed there as reasonably
-       possible.
+       <https://github.com/newren/git-filter-repo/> filter-repo" . If you
+       still need to use git filter-branch, please carefully read the section
+       called “SAFETY” (and the section called “PERFORMANCE”) to learn about
+       the land mines of filter-branch, and then vigilantly avoid as many of
+       the hazards listed there as reasonably possible.

-NOTES
-        1. git filter-repo
-           https://github.com/newren/git-filter-repo/
-
-        2. filter-lamely
-           https://github.com/newren/git-filter-repo/blob/master/contrib/filter-repo-demos/filter-lamely
-

To:

 NOTES
-        1. git filter-repo
+       [1]    git filter-repo
               https://github.com/newren/git-filter-repo/

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index b2bbb318ad..42133ee6c3 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -11,6 +11,19 @@ module Asciidoctor
         return node.text if node.text
         refid = node.attributes['refid']
         'the section called &#8220;%s&#8221;' % refid.gsub('_', ' ')
+      when :link
+        return node.target if node.text == node.target
+        doc = node.document
+
+        footnote = doc.footnotes.find { |e| e.id == node.target }
+        if !footnote
+          footnote_text = "%s\n\e.RS\n\e\\%%%s\n\e.RE" % [node.text, node.target]
+          index = doc.counter('footnote-number')
+          footnote = Document::Footnote.new(index, node.target, footnote_text)
+          doc.register(:footnotes, footnote)
+        end
+
+        "\e\\fB%s\e\\fR[%d]" % [node.text, footnote.index]
       else
         orig_convert_inline_anchor(node)
       end
-- 
2.31.1

[PATCH 10/11] doc: asciidoctor: add support for baseurl

[Felipe Contreras]

So that we can present relative links correctly.

Reduces the doc-diff:

   NOTES
  -        1. “Understanding history: What is a branch?”
  -           file:///$HOME/share/doc/git-doc/user-manual.html#what-is-a-branch
  +       [1]    “Understanding history: What is a branch?”
  +              user-manual.html#what-is-a-branch

   NOTES
  -        1. “Understanding history: What is a branch?”
  +       [1]    “Understanding history: What is a branch?”
                 file:///$HOME/share/doc/git-doc/user-manual.html#what-is-a-branch

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/Makefile                  | 1 +
 Documentation/asciidoctor-extensions.rb | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index a514a4e72c..cc5ff54d92 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -193,6 +193,7 @@ ASCIIDOC_DOCBOOK = docbook5
 override ASCIIDOC_EXTRA += -acompat-mode -atabsize=8
 override ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
 override ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
+override ASCIIDOC_EXTRA += -abaseurl='$(MAN_BASE_URL)'
 ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
 DBLATEX_COMMON =
 XMLTO_EXTRA += --skip-validation
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index 42133ee6c3..2ed92c3055 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -17,7 +17,9 @@ module Asciidoctor
 
         footnote = doc.footnotes.find { |e| e.id == node.target }
         if !footnote
-          footnote_text = "%s\n\e.RS\n\e\\%%%s\n\e.RE" % [node.text, node.target]
+          target = node.target
+          target = doc.attributes['baseurl'] + target unless target.include? ':'
+          footnote_text = "%s\n\e.RS\n\e\\%%%s\n\e.RE" % [node.text, target]
           index = doc.counter('footnote-number')
           footnote = Document::Footnote.new(index, node.target, footnote_text)
           doc.register(:footnotes, footnote)
-- 
2.31.1

[PATCH 11/11] doc: asciidoctor: cleanup man page hack

[Felipe Contreras]

There's basically nothing we need from the original
orig_convert_inline_anchor(), so let's remove calls to it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index 2ed92c3055..f23c5628a5 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -4,7 +4,6 @@ require 'asciidoctor/converter/manpage'
 
 module Asciidoctor
   class Converter::ManPageConverter
-    alias orig_convert_inline_anchor convert_inline_anchor
     def convert_inline_anchor(node)
       case node.type
       when :xref
@@ -26,8 +25,10 @@ module Asciidoctor
         end
 
         "\e\\fB%s\e\\fR[%d]" % [node.text, footnote.index]
+      when :ref, :bibref
+        ''
       else
-        orig_convert_inline_anchor(node)
+        nil
       end
     end
   end
-- 
2.31.1

Re: [PATCH 04/11] doc: use asciidoctor to build man pages directly

[Martin Ågren]

On Fri, 14 May 2021 at 14:14, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> There's no need to use xmlto to build the man pages when modern
> asciidoctor can do it by itself.
>
> This new mode will be active only when USE_ASCIIDOCTOR is set.

May I suggest incorporating something more like brian's patch here [1],
so that there's a separate knob for this thing?

The commit message is short on details and makes it sound like this is
it, we're done. But then there are several patches to fix up things.
Which is a good approach, so that this patch doesn't need to do several
things at once. This commit message could say something about it, e.g.,

  The doc-diff here [which doc-diff? see below] is a XYZ-line diff.
  Large parts of this difference will be addressed in the following
  patches.

About the use of doc-diff: If this commit introduces a new knob rather
than taking over USE_ASCIIDOCTOR=Yes, the next patch could be Peff's
patch to doc-diff that compares the two asciidoctor approaches [2], and
then the next few patches could diff between them. That would get the
asciidoc-vs-asciidoctor differences out of the way, so you can focus on
asciidoctor-vs-asciidoctor.

With a separate knob, it would feel like a lot easier decision to take
something like this. There are over 11000 lines in the doc-diff after
applying your series, and there's the missing boldness for literals.
Maybe those differences are all great (I would be missing the bold
literals, though). If this series doesn't affect someone using
"vanilla" USE_ASCIIDOCTOR=Yes, we could let this thing cook in master
and work incrementally on top.

[1] https://lore.kernel.org/git/20210514003104.94644-2-sandals@crustytoothpaste.net/

[2] https://lore.kernel.org/git/YJt81neO7zsGz2ah@coredump.intra.peff.net/


Martin

Re: [PATCH 03/11] doc: doc-diff: set docdate manually

[Martin Ågren]

On Fri, 14 May 2021 at 14:14, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
> Asciidoc automatically generates a date with format '%Y-%m-%d', while
> asciidoctor '%F'.

Is that really the format Asciidoc uses? Not that it matters much for
the purposes of this patch.

> I personally prefer the latter, so only modify it for diff purposes.

I agree completely.

> Fixes tons of these:
>
> -Git omitted                       01/01/1970                        GIT-ADD(1)
> +Git omitted                       1970-01-01                        GIT-ADD(1)

>         then
> -               echo USE_ASCIIDOCTOR=YesPlease
> +               echo USE_ASCIIDOCTOR=YesPlease ASCIIDOC_EXTRA='-adocdate="01/01/1970"'
>         fi

Nice.

If you introduce a separate Makefile flag and incorporate Peff's patch
to doc-diff "asciidoctor" and "asciidoctor-direct", you'd need to
duplicate this a bit, or maybe just emit the ASCIIDOC_EXTRA outside of
the whole if chain.

You could follow up with the patch below. If you'd rather keep it out of
your series to avoid it ballooning, fine. I can repost it later, once
the dust has settled. Don't let it hold up your work.

-- >8 --
Subject: [PATCH] doc-diff: drop --cut-footer switch

Now that our doc-diff convinces Asciidoctor to insert the exact same
formatted dummy date as AsciiDoc, we can drop the --cut-footer switch.
It has been useful to ignore this difference between the two tools, but
it's effectively a no-op now. Similar to when we repurposed this from
--cut-header-footer in 83b0b8953e ("doc-diff: replace
--cut-header-footer with --cut-footer", 2019-09-16), just drop it
without worrying about any kind of backwards compatibility or user-base.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/doc-diff | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index aae5fc1933..97671ca65d 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -21,7 +21,6 @@ asciidoc		use asciidoc with both commits
 to-asciidoc		use asciidoc with the 'to'-commit
 to-asciidoctor		use asciidoctor with the 'to'-commit
 asciidoctor		use asciidoctor with both commits
-cut-footer		cut away footer
 "
 SUBDIRECTORY_OK=1
 . "$(git --exec-path)/git-sh-setup"
@@ -31,7 +30,6 @@ force=
 clean=
 from_program=
 to_program=
-cut_footer=
 while test $# -gt 0
 do
 	case "$1" in
@@ -55,8 +53,6 @@ do
 	--asciidoc)
 		from_program=-asciidoc
 		to_program=-asciidoc ;;
-	--cut-footer)
-		cut_footer=-cut-footer ;;
 	--)
 		shift; break ;;
 	*)
@@ -118,8 +114,8 @@ construct_makemanflags () {
 from_makemanflags=$(construct_makemanflags "$from_program") &&
 to_makemanflags=$(construct_makemanflags "$to_program") &&
 
-from_dir=$from_oid$from_program$cut_footer &&
-to_dir=$to_oid$to_program$cut_footer &&
+from_dir=$from_oid$from_program &&
+to_dir=$to_oid$to_program &&
 
 # generate_render_makefile <srcdir> <dstdir>
 generate_render_makefile () {
@@ -168,16 +164,6 @@ render_tree () {
 			"$tmp/rendered/$dname+" |
 		make -j$parallel -f - &&
 		mv "$tmp/rendered/$dname+" "$tmp/rendered/$dname"
-
-		if test "$cut_footer" = "-cut-footer"
-		then
-			for f in $(find "$tmp/rendered/$dname" -type f)
-			do
-				head -n -2 "$f" | sed -e '${/^$/d}' >"$f+" &&
-				mv "$f+" "$f" ||
-				return 1
-			done
-		fi
 	fi
 }
 
-- 
2.31.1.751.gd2f1c929bd

Re: [PATCH 04/11] doc: use asciidoctor to build man pages directly

[Felipe Contreras]

Martin Ågren wrote:
> On Fri, 14 May 2021 at 14:14, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > There's no need to use xmlto to build the man pages when modern
> > asciidoctor can do it by itself.
> >
> > This new mode will be active only when USE_ASCIIDOCTOR is set.
> 
> May I suggest incorporating something more like brian's patch here [1],
> so that there's a separate knob for this thing?

Sure. Do you agree with the name? (USE_ASCIIDOCTOR_MANPAGE)

> The commit message is short on details and makes it sound like this is
> it, we're done. But then there are several patches to fix up things.
> Which is a good approach, so that this patch doesn't need to do several
> things at once. This commit message could say something about it, e.g.,
> 
>   The doc-diff here [which doc-diff? see below] is a XYZ-line diff.
>   Large parts of this difference will be addressed in the following
>   patches.

Right. I'll include that.

> About the use of doc-diff: If this commit introduces a new knob rather
> than taking over USE_ASCIIDOCTOR=Yes, the next patch could be Peff's
> patch to doc-diff that compares the two asciidoctor approaches [2], and
> then the next few patches could diff between them. That would get the
> asciidoc-vs-asciidoctor differences out of the way, so you can focus on
> asciidoctor-vs-asciidoctor.

You mean [1]. I think that belongs on the same patch. It's important
that if we do have a new switch, doc-diff is able to use it.

However, I personally don't need such switch, I want to compare
asciidoc-vs-asciidoctor-manpage wholesale.

I want to see *all* the diffs.

> With a separate knob, it would feel like a lot easier decision to take
> something like this. There are over 11000 lines in the doc-diff after
> applying your series, and there's the missing boldness for literals.
> Maybe those differences are all great (I would be missing the bold
> literals, though). If this series doesn't affect someone using
> "vanilla" USE_ASCIIDOCTOR=Yes, we could let this thing cook in master
> and work incrementally on top.

I did notice the missing boldness for literals, and I know how to fix
it. It's a small hack. I also noticed a few small rendering issues.

But from my point of view after my patches this is 98% done. Most of the
remaining diffs are fine, for example:

-GIT-CHECK-REF-FOR(1)              Git Manual              GIT-CHECK-REF-FOR(1)
+GIT-CHECK-REF-FORMAT(1)           Git Manual           GIT-CHECK-REF-FORMAT(1)

That's clearly a bug in asciidoc+docbook. Others are things asciidoctor
renders better, and most are whitespace noise.

Cheers.

[1] https://lore.kernel.org/git/YJt1%2FDO1cXNTRNxK@coredump.intra.peff.net/

-- 
Felipe Contreras

Re: [PATCH 03/11] doc: doc-diff: set docdate manually

[Felipe Contreras]

Martin Ågren wrote:
> On Fri, 14 May 2021 at 14:14, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >
> > Asciidoc automatically generates a date with format '%Y-%m-%d', while
> > asciidoctor '%F'.
> 
> Is that really the format Asciidoc uses? Not that it matters much for
> the purposes of this patch.

Yes:

https://github.com/asciidoc-py/asciidoc-py/blob/main/asciidoc/asciidoc.py#L1189

> > I personally prefer the latter, so only modify it for diff purposes.
> 
> I agree completely.
> 
> > Fixes tons of these:
> >
> > -Git omitted                       01/01/1970                        GIT-ADD(1)
> > +Git omitted                       1970-01-01                        GIT-ADD(1)
> 
> >         then
> > -               echo USE_ASCIIDOCTOR=YesPlease
> > +               echo USE_ASCIIDOCTOR=YesPlease ASCIIDOC_EXTRA='-adocdate="01/01/1970"'
> >         fi
> 
> Nice.
> 
> If you introduce a separate Makefile flag and incorporate Peff's patch
> to doc-diff "asciidoctor" and "asciidoctor-direct", you'd need to
> duplicate this a bit, or maybe just emit the ASCIIDOC_EXTRA outside of
> the whole if chain.

Indeed.

> You could follow up with the patch below. If you'd rather keep it out of
> your series to avoid it ballooning, fine. I can repost it later, once
> the dust has settled. Don't let it hold up your work.

I'll include it. Seems to fit the context.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Jeff King]

On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote:

> Without `override` all additions will be ignored by make.

That's true of all of our Makefile variables. Is there a particular
reason to give this one special treatment?

-Peff

Re: [PATCH 02/11] doc: doc-diff: allow more than one flag

[Jeff King]

On Fri, May 14, 2021 at 07:14:26AM -0500, Felipe Contreras wrote:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/doc-diff | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> index 1694300e50..ecd88b0524 100755
> --- a/Documentation/doc-diff
> +++ b/Documentation/doc-diff
> @@ -146,7 +146,7 @@ render_tree () {
>  	# through.
>  	oid=$1 &&
>  	dname=$2 &&
> -	makemanflags=$3 &&
> +	makemanflags="$3" &&

This line does nothing; the shell won't split whitespace here either
way.

> @@ -181,6 +181,6 @@ render_tree () {
>  	fi
>  }
>  
> -render_tree $from_oid $from_dir $from_makemanflags &&
> -render_tree $to_oid $to_dir $to_makemanflags &&
> +render_tree $from_oid $from_dir "$from_makemanflags" &&
> +render_tree $to_oid $to_dir "$to_makemanflags" &&

This part is necessary (and sufficient).

It would be nice to mention in the commit message why the use of
$makemanflags in render_tree must remain unquoted (as I did in mine when
I made the same change).

-Peff

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Jeff King]

On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote:

> On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote:
> 
> > Without `override` all additions will be ignored by make.
> 
> That's true of all of our Makefile variables. Is there a particular
> reason to give this one special treatment?

To go into further detail: usually we distinguish variables we use
internally from user-facing ones, and include the latter in the former.
I see a later patch wants to start passing ASCIIDOC_EXTRA on the
command-line, and we'd use two variables for that.

-Peff

Re: [PATCH 02/11] doc: doc-diff: allow more than one flag

[Felipe Contreras]

Jeff King wrote:
> On Fri, May 14, 2021 at 07:14:26AM -0500, Felipe Contreras wrote:
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/doc-diff | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> > index 1694300e50..ecd88b0524 100755
> > --- a/Documentation/doc-diff
> > +++ b/Documentation/doc-diff
> > @@ -146,7 +146,7 @@ render_tree () {
> >  	# through.
> >  	oid=$1 &&
> >  	dname=$2 &&
> > -	makemanflags=$3 &&
> > +	makemanflags="$3" &&
> 
> This line does nothing; the shell won't split whitespace here either
> way.

Right.

> > @@ -181,6 +181,6 @@ render_tree () {
> >  	fi
> >  }
> >  
> > -render_tree $from_oid $from_dir $from_makemanflags &&
> > -render_tree $to_oid $to_dir $to_makemanflags &&
> > +render_tree $from_oid $from_dir "$from_makemanflags" &&
> > +render_tree $to_oid $to_dir "$to_makemanflags" &&
> 
> This part is necessary (and sufficient).
> 
> It would be nice to mention in the commit message why the use of
> $makemanflags in render_tree must remain unquoted (as I did in mine when
> I made the same change).

Ahh, I didn't see exactly how you implemented it. Now I see it's very
similar.

-- 
Felipe Contreras

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Felipe Contreras]

Jeff King wrote:
> On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote:
> 
> > On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote:
> > 
> > > Without `override` all additions will be ignored by make.
> > 
> > That's true of all of our Makefile variables. Is there a particular
> > reason to give this one special treatment?
> 
> To go into further detail: usually we distinguish variables we use
> internally from user-facing ones, and include the latter in the former.
> I see a later patch wants to start passing ASCIIDOC_EXTRA on the
> command-line, and we'd use two variables for that.

Well, it's not exactly user-facing; it's only needed for doc-diff.

Would TEST_ASCIIDOC_EXTRA make sense?

-- 
Felipe Contreras

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Jeff King]

On Sat, May 15, 2021 at 07:13:48AM -0500, Felipe Contreras wrote:

> Jeff King wrote:
> > On Sat, May 15, 2021 at 05:32:08AM -0400, Jeff King wrote:
> > 
> > > On Fri, May 14, 2021 at 07:14:25AM -0500, Felipe Contreras wrote:
> > > 
> > > > Without `override` all additions will be ignored by make.
> > > 
> > > That's true of all of our Makefile variables. Is there a particular
> > > reason to give this one special treatment?
> > 
> > To go into further detail: usually we distinguish variables we use
> > internally from user-facing ones, and include the latter in the former.
> > I see a later patch wants to start passing ASCIIDOC_EXTRA on the
> > command-line, and we'd use two variables for that.
> 
> Well, it's not exactly user-facing; it's only needed for doc-diff.

It's meant for the caller of "make". Your proposed use is within
doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
different behavior.

> Would TEST_ASCIIDOC_EXTRA make sense?

I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
that are meant for users to inform us of extra flags they'd like
passed).

Of course that may not solve your problem in a sense; if you want
doc-diff to override it, then that might conflict with a theoretical
ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
the realm of hypothetical here).

-Peff

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Felipe Contreras]

Jeff King wrote:
> On Sat, May 15, 2021 at 07:13:48AM -0500, Felipe Contreras wrote:
> > Jeff King wrote:

> > > To go into further detail: usually we distinguish variables we use
> > > internally from user-facing ones, and include the latter in the former.
> > > I see a later patch wants to start passing ASCIIDOC_EXTRA on the
> > > command-line, and we'd use two variables for that.
> > 
> > Well, it's not exactly user-facing; it's only needed for doc-diff.
> 
> It's meant for the caller of "make". Your proposed use is within
> doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
> different behavior.

Yeah, they would, but I don't think it would be wrong behavior.

> > Would TEST_ASCIIDOC_EXTRA make sense?
> 
> I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
> that are meant for users to inform us of extra flags they'd like
> passed).

Right, but Makefiles do override those, like:

  override CFLAGS += -fPIC

Otherwise builds may fail.

> Of course that may not solve your problem in a sense; if you want
> doc-diff to override it, then that might conflict with a theoretical
> ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
> the realm of hypothetical here).

Setting ASCIIDOC_FLAGS in config.mk would not override the
user-supplied flags any more than setting them in the Makefile (they are
virtually the same thing as one includes the other).

It's only if the user has `override ASCIIDOC_FLAGS` in config.mk that
such a problem would arise. And that's really hypothetical.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Jeff King]

On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote:

> > It's meant for the caller of "make". Your proposed use is within
> > doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
> > different behavior.
> 
> Yeah, they would, but I don't think it would be wrong behavior.

It depends what they're trying to do. If they write:

  make ASCIIDOC_EXTRA=--one-extra-option

then they probably intend to to add to the options we set. If they
write:

  make ASCIIDOC_EXTRA='-acompat-mode -atabsize=4 ...etc...'

with the intent of replicating the flags but changing or removing some
elements, then it would no longer do what they want.

I don't mean to imply one is more right than the other (I'd suspect even
that the override behavior is more likely to be what somebody wants).
I'm mostly pointing out that this is unlike the rest of our Makefiles,
which do not ever use override (and that the effect is visible to the
caller, depending on what they want to do).

> > I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
> > that are meant for users to inform us of extra flags they'd like
> > passed).
> 
> Right, but Makefiles do override those, like:
> 
>   override CFLAGS += -fPIC
> 
> Otherwise builds may fail.

Some Makefiles do, but in this project we have not historically used
override. Instead, we provide defaults for things like CFLAGS, expect
the use to replace them if they like, and then aggregate them (along
with other internal variables) into things like ALL_CFLAGS.

> > Of course that may not solve your problem in a sense; if you want
> > doc-diff to override it, then that might conflict with a theoretical
> > ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
> > the realm of hypothetical here).
> 
> Setting ASCIIDOC_FLAGS in config.mk would not override the
> user-supplied flags any more than setting them in the Makefile (they are
> virtually the same thing as one includes the other).
> 
> It's only if the user has `override ASCIIDOC_FLAGS` in config.mk that
> such a problem would arise. And that's really hypothetical.

I mean that if your doc-diff runs:

  make USE_ASCIIDOCTOR=Yes ASCIIDOC_FLAGS=-adocdate=01/01/1970

then that will override anything the user put into config.mak. If they
had some option like:

  ASCIIDOC_FLAGS = --load-path=/some/special/directory

they need for asciidoctor to run correctly on their system, then things
would break for them. But we don't even have a user-facing
ASCIIDOC_FLAGS now, and nobody is asking for it, so it's pretty
hypothetical (I'd guess somebody in this situation would just set
ASCIIDOC="asciidoctor --load-path=...", and that already doesn't work
with doc-diff).

-Peff

Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA

[Felipe Contreras]

Jeff King wrote:
> On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote:
> 
> > > It's meant for the caller of "make". Your proposed use is within
> > > doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
> > > different behavior.
> > 
> > Yeah, they would, but I don't think it would be wrong behavior.
> 
> It depends what they're trying to do. If they write:
> 
>   make ASCIIDOC_EXTRA=--one-extra-option
> 
> then they probably intend to to add to the options we set. If they
> write:
> 
>   make ASCIIDOC_EXTRA='-acompat-mode -atabsize=4 ...etc...'
> 
> with the intent of replicating the flags but changing or removing some
> elements, then it would no longer do what they want.
> 
> I don't mean to imply one is more right than the other (I'd suspect even
> that the override behavior is more likely to be what somebody wants).

Yeah, but I am implying that one is more right than the other.

> I'm mostly pointing out that this is unlike the rest of our Makefiles,
> which do not ever use override (and that the effect is visible to the
> caller, depending on what they want to do).

It's used in the main Makefile, although in a different way.

I see how it is not consistent with the rest of the Makefiles, but I
wonder why it's not being used. It's rather useful.

> > > I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
> > > that are meant for users to inform us of extra flags they'd like
> > > passed).
> > 
> > Right, but Makefiles do override those, like:
> > 
> >   override CFLAGS += -fPIC
> > 
> > Otherwise builds may fail.
> 
> Some Makefiles do, but in this project we have not historically used
> override. Instead, we provide defaults for things like CFLAGS, expect
> the use to replace them if they like, and then aggregate them (along
> with other internal variables) into things like ALL_CFLAGS.

I know, but status quo is not an argument.

If we always did things the way we've always done things there would
never be progress.

I'm aruging there's no value in giving the user the opportunity to break
the build by doing `make BASIC_CFLAGS=`. Yes, it's more historically
consistent, that doesn't mean it's good.

> > > Of course that may not solve your problem in a sense; if you want
> > > doc-diff to override it, then that might conflict with a theoretical
> > > ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
> > > the realm of hypothetical here).
> > 
> > Setting ASCIIDOC_FLAGS in config.mk would not override the
> > user-supplied flags any more than setting them in the Makefile (they are
> > virtually the same thing as one includes the other).
> > 
> > It's only if the user has `override ASCIIDOC_FLAGS` in config.mk that
> > such a problem would arise. And that's really hypothetical.
> 
> I mean that if your doc-diff runs:
> 
>   make USE_ASCIIDOCTOR=Yes ASCIIDOC_FLAGS=-adocdate=01/01/1970
> 
> then that will override anything the user put into config.mak. If they
> had some option like:
> 
>   ASCIIDOC_FLAGS = --load-path=/some/special/directory
> 
> they need for asciidoctor to run correctly on their system, then things
> would break for them. But we don't even have a user-facing
> ASCIIDOC_FLAGS now, and nobody is asking for it, so it's pretty
> hypothetical (I'd guess somebody in this situation would just set
> ASCIIDOC="asciidoctor --load-path=...", and that already doesn't work
> with doc-diff).

Exactly, so it's unclear how much value we get by talking about these.

Either way, I don't feel very strongly about `override ASCIIDOC_EXTRA`.
I think it's superior but ASCIIDOC_FLAGS requires less changes, so I'm
fine with that.

Cheers.

-- 
Felipe Contreras