threads / patch / 50588

patch, 3 partsasciidoctor-extensions: fix spurious space after linkgit

Subject: [PATCH 0/3] asciidoctor-extensions: fix spurious space after linkgit

## tl;dr

31 messages between Feb 25, 2019 and Mar 11, 2019. Diffs are folded; open one to read it.

replies: 30people: 5as markdown or json

Martin Ågren· Feb 25, 2019, 20:02 UTC · lore

Patch 3/3 gets rid of a spurious space which shows up immediately after Asciidoctor has expanded linkgit:foo[bar] if the macro is followed by punctuation such as a comma, period or closing parenthesis.

Patch 2/3 adds a missing dependency to the makefile, so that the docs will actually be rebuilt with that final patch. Patch 1/3 isn't needed for this series, but could help someone in the future, similar to how already having patch 2/3 would have helped me...

Martin Ågren (3):
  Documentation/Makefile: add missing xsl dependencies for manpages
  Documentation/Makefile: add missing dependency on
    asciidoctor-extensions
  asciidoctor-extensions: fix spurious space after linkgit
 Documentation/Makefile                  | 4 ++--
 Documentation/asciidoctor-extensions.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
-- 
2.21.0
Martin Ågren· Feb 25, 2019, 20:02 UTC · re: Martin Ågren · lore

[PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

These stylesheets very rarely change, but when they do, it really helps if the manpages depend on them. We're casting the net a bit too wide here, since we'll only ever use a subset of the stylesheets, but since these files change so rarely, that should be ok. It's better than missing a dependency.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 26a2342bea..0f8054a115 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -354,7 +354,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl
+%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-- 
2.21.0
Eric Sunshine· Feb 25, 2019, 20:08 UTC · re: Martin Ågren · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Mon, Feb 25, 2019 at 3:03 PM Martin Ågren <martin.agren@gmail.com> wrote:
Show 12 quoted lines
> These stylesheets very rarely change, but when they do, it really helps
> if the manpages depend on them. We're casting the net a bit too wide
> here, since we'll only ever use a subset of the stylesheets, but since
> these files change so rarely, that should be ok. It's better than
> missing a dependency.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> @@ -354,7 +354,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
> -%.1 %.5 %.7 : %.xml manpage-base-url.xsl
> +%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)

The wildcard expression also matches the manpage-base-url.xsl mentioned explicitly. Did you intentionally retain the explicit one or was that an oversight?

Martin Ågren· Feb 25, 2019, 20:28 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Mon, 25 Feb 2019 at 21:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 18 quoted lines
>
> On Mon, Feb 25, 2019 at 3:03 PM Martin Ågren <martin.agren@gmail.com> wrote:
> > These stylesheets very rarely change, but when they do, it really helps
> > if the manpages depend on them. We're casting the net a bit too wide
> > here, since we'll only ever use a subset of the stylesheets, but since
> > these files change so rarely, that should be ok. It's better than
> > missing a dependency.
> >
> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> > ---
> > diff --git a/Documentation/Makefile b/Documentation/Makefile
> > @@ -354,7 +354,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
> > -%.1 %.5 %.7 : %.xml manpage-base-url.xsl
> > +%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
>
> The wildcard expression also matches the manpage-base-url.xsl
> mentioned explicitly. Did you intentionally retain the explicit one or
> was that an oversight?
Hmm, I should have mentioned that, thanks for asking for clarification.

It's intentional. You can see in the context how manpage-base-url.xsl is actually generated, so the wildcard won't be able to expand to mention it. That's subtle enough that it warrants being mentioned upfront.

Martin
Eric Sunshine· Feb 25, 2019, 21:00 UTC · re: Martin Ågren · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Mon, Feb 25, 2019 at 3:29 PM Martin Ågren <martin.agren@gmail.com> wrote:
Show 14 quoted lines
> On Mon, 25 Feb 2019 at 21:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Mon, Feb 25, 2019 at 3:03 PM Martin Ågren <martin.agren@gmail.com> wrote:
> > > -%.1 %.5 %.7 : %.xml manpage-base-url.xsl
> > > +%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
> >
> > The wildcard expression also matches the manpage-base-url.xsl
> > mentioned explicitly. Did you intentionally retain the explicit one or
> > was that an oversight?
>
> Hmm, I should have mentioned that, thanks for asking for clarification.
>
> It's intentional. You can see in the context how manpage-base-url.xsl is
> actually generated, so the wildcard won't be able to expand to mention
> it. That's subtle enough that it warrants being mentioned upfront.

I guess I was further confused by the fact that manpage-base-url.xsl is still present even after "make distclean", which is not something I would have expected for a generated file. (Perhaps something to consider "fixing" if you re-roll, or not if it's out of scope.)

Martin Ågren· Feb 26, 2019, 05:59 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Mon, 25 Feb 2019 at 22:01, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
>
> On Mon, Feb 25, 2019 at 3:29 PM Martin Ågren <martin.agren@gmail.com> wrote:
> > On Mon, 25 Feb 2019 at 21:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > >
> > > The wildcard expression also matches the manpage-base-url.xsl
> > > mentioned explicitly. Did you intentionally retain the explicit one or
> > > was that an oversight?
> >
> > Hmm, I should have mentioned that, thanks for asking for clarification.
> >
> > It's intentional.
> I guess I was further confused by the fact that manpage-base-url.xsl
> is still present even after "make distclean", which is not something I
> would have expected for a generated file. (Perhaps something to
> consider "fixing" if you re-roll, or not if it's out of scope.)

Are you sure? `make distclean` in the top directory depends on the "clean" target, which calls out to `make -C Documentation/ clean`, which removes the file. A test-run works here:

Before:

$ ls | grep manpage-base manpage-base-url.xsl manpage-base-url.xsl.in manpage-base.xsl

After:

$ ls | grep manpage-base manpage-base-url.xsl.in manpage-base.xsl

Were you perhaps mislead by one of the similarly-named files?
Martin
Eric Sunshine· Feb 26, 2019, 12:47 UTC · re: Martin Ågren · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Tue, Feb 26, 2019 at 12:59 AM Martin Ågren <martin.agren@gmail.com> wrote:
Show 10 quoted lines
> On Mon, 25 Feb 2019 at 22:01, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > I guess I was further confused by the fact that manpage-base-url.xsl
> > is still present even after "make distclean", which is not something I
> > would have expected for a generated file. (Perhaps something to
> > consider "fixing" if you re-roll, or not if it's out of scope.)
>
> Are you sure? `make distclean` in the top directory depends on the
> "clean" target, which calls out to `make -C Documentation/ clean`, which
> removes the file. [...]
> Were you perhaps mislead by one of the similarly-named files?
Yes, I was misled by similar names. Sorry for the noise.
Martin Ågren· Feb 27, 2019, 18:19 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

On Tue, 26 Feb 2019 at 13:47, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 5 quoted lines
>
> On Tue, Feb 26, 2019 at 12:59 AM Martin Ågren <martin.agren@gmail.com> wrote:
> > Were you perhaps mislead by one of the similarly-named files?
>
> Yes, I was misled by similar names. Sorry for the noise.

Hey, no problem. And thanks for the subtle hint about "misled". ;-) I appreciate it.

Martin
Martin Ågren· Feb 25, 2019, 20:02 UTC · re: Martin Ågren · lore

[PATCH 2/3] Documentation/Makefile: add missing dependency on asciidoctor-extensions

asciidoctor-extensions.rb has never changed, but when it does -- such as in the next commit --, it helps if the xml-files depend on it. We're casting the net a bit too wide here, since we'll be rebuilding even with AsciiDoc, which won't look at this file. But since this file changes so rarely, that should be ok. It's better than missing a dependency.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 0f8054a115..a9697f5146 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -358,7 +358,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-%.xml : %.txt asciidoc.conf
+%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d manpage -o $@+ $< && \
 	mv $@+ $@
-- 
2.21.0
Martin Ågren· Feb 25, 2019, 20:02 UTC · re: Martin Ågren · lore

[PATCH 3/3] asciidoctor-extensions: fix spurious space after linkgit

When we render, e.g., "linkgit:gitglossary[7]." with Asciidoctor, we get "gitglossary(7) ." with a space between the linkgit macro expansion and the punctuation. We can fix this by dropping the trailing newline after we've turned `linkgit:foo[bar]` into `<citerefentry>..</citerefentry>`.

The diff produced by `USE_ASCIIDOCTOR=Yes ./doc-diff HEAD^ HEAD` is almost 6000 lines large and shows how this fixes "git-foo(x) ,", "(see git-bar(y) )" and so on. One might wonder whether this also turns, e.g., "see linkgit:foo[1] for more" into "see foo(1)for more", but no. We get "...</citerefentry> for more" in the XML, see, e.g., git-am.xml, so the space ends up in git-am.1 just fine.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/asciidoctor-extensions.rb +1 −1
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index ec83b4959e..f7a5982f8b 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -16,7 +16,7 @@ module Git
           "<citerefentry>\n" \
             "<refentrytitle>#{target}</refentrytitle>" \
             "<manvolnum>#{attrs[1]}</manvolnum>\n" \
-          "</citerefentry>\n"
+          "</citerefentry>"
         end
       end
     end
-- 
2.21.0
brian m. carlson· Feb 26, 2019, 03:20 UTC · re: Martin Ågren · lore

Re: [PATCH 3/3] asciidoctor-extensions: fix spurious space after linkgit

On Mon, Feb 25, 2019 at 09:02:40PM +0100, Martin Ågren wrote:
Show 30 quoted lines
> When we render, e.g., "linkgit:gitglossary[7]." with Asciidoctor, we get
> "gitglossary(7) ." with a space between the linkgit macro expansion and
> the punctuation. We can fix this by dropping the trailing newline after
> we've turned `linkgit:foo[bar]` into `<citerefentry>..</citerefentry>`.
> 
> The diff produced by `USE_ASCIIDOCTOR=Yes ./doc-diff HEAD^ HEAD` is
> almost 6000 lines large and shows how this fixes "git-foo(x) ,", "(see
> git-bar(y) )" and so on. One might wonder whether this also turns, e.g.,
> "see linkgit:foo[1] for more" into "see foo(1)for more", but no. We get
> "...</citerefentry> for more" in the XML, see, e.g., git-am.xml, so the
> space ends up in git-am.1 just fine.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Documentation/asciidoctor-extensions.rb | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
> index ec83b4959e..f7a5982f8b 100644
> --- a/Documentation/asciidoctor-extensions.rb
> +++ b/Documentation/asciidoctor-extensions.rb
> @@ -16,7 +16,7 @@ module Git
>            "<citerefentry>\n" \
>              "<refentrytitle>#{target}</refentrytitle>" \
>              "<manvolnum>#{attrs[1]}</manvolnum>\n" \
> -          "</citerefentry>\n"
> +          "</citerefentry>"
>          end
>        end
>      end

Yeah, I think this is clearly correct. I also like that you mentioned the concern about potentially not including the space, which might not be obvious to someone not intimately familiar with DocBook and the stylesheets.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson· Feb 26, 2019, 03:22 UTC · re: Martin Ågren · lore

Re: [PATCH 0/3] asciidoctor-extensions: fix spurious space after linkgit

On Mon, Feb 25, 2019 at 09:02:37PM +0100, Martin Ågren wrote:
Show 18 quoted lines
> Patch 3/3 gets rid of a spurious space which shows up immediately after
> Asciidoctor has expanded linkgit:foo[bar] if the macro is followed by
> punctuation such as a comma, period or closing parenthesis.
> 
> Patch 2/3 adds a missing dependency to the makefile, so that the docs
> will actually be rebuilt with that final patch. Patch 1/3 isn't needed
> for this series, but could help someone in the future, similar to how
> already having patch 2/3 would have helped me...
> 
> Martin Ågren (3):
>   Documentation/Makefile: add missing xsl dependencies for manpages
>   Documentation/Makefile: add missing dependency on
>     asciidoctor-extensions
>   asciidoctor-extensions: fix spurious space after linkgit
> 
>  Documentation/Makefile                  | 4 ++--
>  Documentation/asciidoctor-extensions.rb | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Thanks for putting this series together. I would suggest an update to the commit message in 1/3 explaining the edge case that Eric Sunshine mentioned, but otherwise, I think this looks good.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Martin Ågren· Feb 26, 2019, 06:03 UTC · re: brian m. carlson · lore

Re: [PATCH 0/3] asciidoctor-extensions: fix spurious space after linkgit

On Tue, 26 Feb 2019 at 04:22, brian m. carlson <sandals@crustytoothpaste.net> wrote:

>
> Thanks for putting this series together. I would suggest an update to
> the commit message in 1/3 explaining the edge case that Eric Sunshine
> mentioned, but otherwise, I think this looks good.

Thank you. I'll do that, after waiting for a while for any other comments, and to see what conclusion I and Eric can reach on the `make distclean` tangent.

Martin
Martin Ågren· Feb 27, 2019, 18:17 UTC · re: Martin Ågren · lore

[PATCH v2 0/3] asciidoctor-extensions: fix spurious space after linkgit

Just like v1 [1], this v2 removes a spurious space which shows up in a large number of places in our manpages when Asciidoctor expands the linkgit:foo[bar] macro. The only difference is a new paragraph in the commit message of the first patch to explain why we need to explicitly list a file we depend on.

Thanks Eric and brian for your comments on v1.
[1] https://public-inbox.org/git/cover.1551123979.git.martin.agren@gmail.com/
Martin Ågren (3):
  Documentation/Makefile: add missing xsl dependencies for manpages
  Documentation/Makefile: add missing dependency on
    asciidoctor-extensions
  asciidoctor-extensions: fix spurious space after linkgit
 Documentation/Makefile                  | 4 ++--
 Documentation/asciidoctor-extensions.rb | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
-- 
2.21.0
Martin Ågren· Feb 27, 2019, 18:17 UTC · re: Martin Ågren · lore

[PATCH v2 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

These stylesheets very rarely change, but when they do, it really helps if the manpages depend on them. We're casting the net a bit too wide here, since we'll only ever use a subset of the stylesheets, but since these files change so rarely, that should be ok. It's better than missing a dependency.

Observe that manpage-base-url.xsl is a generated file, so we need to list it explicitly, besides the `wildcard` expression we're adding here.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 26a2342bea..0f8054a115 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -354,7 +354,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl
+%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-- 
2.21.0
Martin Ågren· Feb 27, 2019, 18:17 UTC · re: Martin Ågren · lore

[PATCH v2 2/3] Documentation/Makefile: add missing dependency on asciidoctor-extensions

asciidoctor-extensions.rb has never changed, but when it does -- such as in the next commit --, it helps if the xml-files depend on it. We're casting the net a bit too wide here, since we'll be rebuilding even with AsciiDoc, which won't look at this file. But since this file changes so rarely, that should be ok. It's better than missing a dependency.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 0f8054a115..a9697f5146 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -358,7 +358,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-%.xml : %.txt asciidoc.conf
+%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d manpage -o $@+ $< && \
 	mv $@+ $@
-- 
2.21.0
Martin Ågren· Feb 27, 2019, 18:17 UTC · re: Martin Ågren · lore

[PATCH v2 3/3] asciidoctor-extensions: fix spurious space after linkgit

When we render, e.g., "linkgit:gitglossary[7]." with Asciidoctor, we get "gitglossary(7) ." with a space between the linkgit macro expansion and the punctuation. We can fix this by dropping the trailing newline after we've turned `linkgit:foo[bar]` into `<citerefentry>..</citerefentry>`.

The diff produced by `USE_ASCIIDOCTOR=Yes ./doc-diff HEAD^ HEAD` is almost 6000 lines large and shows how this fixes "git-foo(x) ,", "(see git-bar(y) )" and so on. One might wonder whether this also turns, e.g., "see linkgit:foo[1] for more" into "see foo(1)for more", but no. We get "...</citerefentry> for more" in the XML, see, e.g., git-am.xml, so the space ends up in git-am.1 just fine.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/asciidoctor-extensions.rb +1 −1
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index ec83b4959e..f7a5982f8b 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -16,7 +16,7 @@ module Git
           "<citerefentry>\n" \
             "<refentrytitle>#{target}</refentrytitle>" \
             "<manvolnum>#{attrs[1]}</manvolnum>\n" \
-          "</citerefentry>\n"
+          "</citerefentry>"
         end
       end
     end
-- 
2.21.0
brian m. carlson· Feb 28, 2019, 23:50 UTC · re: Martin Ågren · lore

Re: [PATCH v2 0/3] asciidoctor-extensions: fix spurious space after linkgit

On Wed, Feb 27, 2019 at 07:17:51PM +0100, Martin Ågren wrote:
Show 19 quoted lines
> Just like v1 [1], this v2 removes a spurious space which shows up in a
> large number of places in our manpages when Asciidoctor expands the
> linkgit:foo[bar] macro. The only difference is a new paragraph in the
> commit message of the first patch to explain why we need to explicitly
> list a file we depend on.
> 
> Thanks Eric and brian for your comments on v1.
> 
> [1] https://public-inbox.org/git/cover.1551123979.git.martin.agren@gmail.com/
> 
> Martin Ågren (3):
>   Documentation/Makefile: add missing xsl dependencies for manpages
>   Documentation/Makefile: add missing dependency on
>     asciidoctor-extensions
>   asciidoctor-extensions: fix spurious space after linkgit
> 
>  Documentation/Makefile                  | 4 ++--
>  Documentation/asciidoctor-extensions.rb | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
This version looks good to me. Thanks again for getting this cleaned up.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Junio C Hamano· Mar 3, 2019, 01:25 UTC · re: brian m. carlson · lore

Re: [PATCH v2 0/3] asciidoctor-extensions: fix spurious space after linkgit

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 22 quoted lines
> On Wed, Feb 27, 2019 at 07:17:51PM +0100, Martin Ågren wrote:
>> Just like v1 [1], this v2 removes a spurious space which shows up in a
>> large number of places in our manpages when Asciidoctor expands the
>> linkgit:foo[bar] macro. The only difference is a new paragraph in the
>> commit message of the first patch to explain why we need to explicitly
>> list a file we depend on.
>> 
>> Thanks Eric and brian for your comments on v1.
>> 
>> [1] https://public-inbox.org/git/cover.1551123979.git.martin.agren@gmail.com/
>> 
>> Martin Ågren (3):
>>   Documentation/Makefile: add missing xsl dependencies for manpages
>>   Documentation/Makefile: add missing dependency on
>>     asciidoctor-extensions
>>   asciidoctor-extensions: fix spurious space after linkgit
>> 
>>  Documentation/Makefile                  | 4 ++--
>>  Documentation/asciidoctor-extensions.rb | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> This version looks good to me. Thanks again for getting this cleaned up.
Thanks, all.  Will queue.
Martin Ågren· Mar 10, 2019, 15:14 UTC · re: Junio C Hamano · lore

[PATCH 0/2] fix spurious space after linkgit, now in *.html

On Sun, 3 Mar 2019 at 02:25, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > On Wed, Feb 27, 2019 at 07:17:51PM +0100, Martin Ågren wrote:
> >> Just like v1 [1], this v2 removes a spurious space which shows up in a
> >> large number of places in our manpages when Asciidoctor expands the
> >> linkgit:foo[bar] macro. [...]
> >
> > This version looks good to me. Thanks again for getting this cleaned up.
>
> Thanks, all.  Will queue.

Bleh. For some reason [1] I thought the html-files were exempt from this "extra space after linkgit" problem. They're not, as I just noticed. To add insult to injury, my original patch 2 which adds a missing dependency to the xml targets fails to add the exact same dependency for a few other targets. So of the three patches discussed above, at least two were incomplete.

Since this has hit "next", here are two patches on top to address this.
Sorry about this.
Martin

[1] I could have sworn I checked the html docs and saw that they didn't have this extra space. Looking at git-scm.com again reveals that it's not there. Huh. Seems like the site's html-rendering doesn't go through our Makefile at all and handles "linkgit:" on its own: https://github.com/git/git-scm.com/blob/master/script/doc_importer

Martin Ågren (2):
  Documentation/Makefile: add missing dependencies on
    asciidoctor-extensions
  asciidoctor-extensions: fix spurious space after linkgit in *.html
 Documentation/Makefile                  | 6 +++---
 Documentation/asciidoctor-extensions.rb | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
-- 
2.21.0
Martin Ågren· Mar 10, 2019, 15:14 UTC · re: Martin Ågren · lore

[PATCH 1/2] Documentation/Makefile: add missing dependencies on asciidoctor-extensions

I really should have caught this when I wrote 00c87bceaa ("Documentation/Makefile: add missing dependency on asciidoctor-extensions", 2019-02-27). That commit made sure that the xml-files depend on our Asciidoctor-specific extensions, but that just helps for the ".txt -> .xml -> .[157]" transformations.

Because we produce the html-files directly as ".txt -> .html" -- not as ".txt -> .xml -> .html" --, we need to make the html-files too depend on asciidoctor-extensions.rb.

There's one exception to the above paragraph. We do render user-manual.html by going through an intermediate user-manual.xml, which is an explicit, special-cased target. Add the missing dependency to user-manual.xml.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to Documentation/Makefile +3 −3
diff --git a/Documentation/Makefile b/Documentation/Makefile
index f63c775e88..f58904a929 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -334,12 +334,12 @@ clean:
 	$(RM) $(cmds_txt) $(mergetools_txt) *.made
 	$(RM) manpage-base-url.xsl
 
-$(MAN_HTML): %.html : %.txt asciidoc.conf
+$(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -d manpage -o $@+ $< && \
 	mv $@+ $@
 
-$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
+$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -o $@+ $< && \
 	mv $@+ $@
@@ -356,7 +356,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(TXT_TO_XML) -d manpage -o $@+ $< && \
 	mv $@+ $@
 
-user-manual.xml: user-manual.txt user-manual.conf
+user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d book -o $@+ $< && \
 	mv $@+ $@
-- 
2.21.0
Martin Ågren· Mar 10, 2019, 15:14 UTC · re: Martin Ågren · lore

[PATCH 2/2] asciidoctor-extensions: fix spurious space after linkgit in *.html

I really should have caught this when I wrote c3b4efa030 ("asciidoctor-extensions: fix spurious space after linkgit", 2019-02-27). Turns out that when we create html-files, we take a different path through this macro. So similar to c3b4efa030, we need to drop a "\n" which turns into a space before punctuation.

Just like with c3b4efa030, note that if what follows is a word, not punctuation, the white space renders just fine. So constructions such as "see linkgit:foo[1] for more" still render as intended.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/asciidoctor-extensions.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/asciidoctor-extensions.rb +1 −1
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index f7a5982f8b..0089e0cfb8 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -11,7 +11,7 @@ module Git
       def process(parent, target, attrs)
         if parent.document.basebackend? 'html'
           prefix = parent.document.attr('git-relative-html-prefix')
-          %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
+          %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>)
         elsif parent.document.basebackend? 'docbook'
           "<citerefentry>\n" \
             "<refentrytitle>#{target}</refentrytitle>" \
-- 
2.21.0
Jeff King· Mar 11, 2019, 02:59 UTC · re: Martin Ågren · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

On Sun, Mar 10, 2019 at 04:14:22PM +0100, Martin Ågren wrote:
Show 5 quoted lines
> [1] I could have sworn I checked the html docs and saw that they didn't
> have this extra space. Looking at git-scm.com again reveals that it's
> not there. Huh. Seems like the site's html-rendering doesn't go through our
> Makefile at all and handles "linkgit:" on its own:
> https://github.com/git/git-scm.com/blob/master/script/doc_importer

Correct. It significantly predates any support for asciidoctor in our Makefile, and needs to do some custom ruby-level tweaking (though possibly that could be cleaned up these days).

The doc_importer script you found, though, is not part of that process. I _think_ it's just leftover cruft. The actual import and conversion happens as part of the rake task in:

  https://github.com/git/git-scm.com/blob/master/lib/tasks/index.rake

(Not that any of this matters for your series; just sprinkling some fun facts into the conversation).

-Peff
Martin Ågren· Mar 11, 2019, 05:47 UTC · re: Jeff King · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

On Mon, 11 Mar 2019 at 03:59, Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
>
> On Sun, Mar 10, 2019 at 04:14:22PM +0100, Martin Ågren wrote:
>
> > [1] I could have sworn I checked the html docs and saw that they didn't
> > have this extra space. Looking at git-scm.com again reveals that it's
> > not there. Huh. Seems like the site's html-rendering doesn't go through our
> > Makefile at all and handles "linkgit:" on its own:
> > https://github.com/git/git-scm.com/blob/master/script/doc_importer
>
> Correct. It significantly predates any support for asciidoctor in our
> Makefile, and needs to do some custom ruby-level tweaking (though
> possibly that could be cleaned up these days).
>
> The doc_importer script you found, though, is not part of that process.
> I _think_ it's just leftover cruft. The actual import and conversion
> happens as part of the rake task in:
>
>   https://github.com/git/git-scm.com/blob/master/lib/tasks/index.rake
>
> (Not that any of this matters for your series; just sprinkling some fun
> facts into the conversation).
Thanks for this background info, and for the correct link. :-)
Martin
Junio C Hamano· Mar 11, 2019, 03:02 UTC · re: Martin Ågren · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

Martin Ågren <martin.agren@gmail.com> writes:
Show 10 quoted lines
> Bleh. For some reason [1] I thought the html-files were exempt from this
> "extra space after linkgit" problem. They're not, as I just noticed. To
> add insult to injury, my original patch 2 which adds a missing
> dependency to the xml targets fails to add the exact same dependency for
> a few other targets. So of the three patches discussed above, at least
> two were incomplete.
>
> Since this has hit "next", here are two patches on top to address this.
>
> Sorry about this.
Thanks.  

If it makes it easier, we can eject ma/asciidoctor-fixes out of 'next' when we rebuild 'next'. I actually am tempted to start 'next' from empty for this round after merging some obviously correct ones that are there to the 'master' branch.

Martin Ågren· Mar 11, 2019, 05:40 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

On Mon, 11 Mar 2019 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > Since this has hit "next", here are two patches on top to address this.
> >
> > Sorry about this.
>
> Thanks.
>
> If it makes it easier, we can eject ma/asciidoctor-fixes out of
> 'next' when we rebuild 'next'.  I actually am tempted to start
> 'next' from empty for this round after merging some obviously
> correct ones that are there to the 'master' branch.

Ok, then I'll take that offer. I'll squash these two patches into the original series. Thanks.

Martin
Junio C Hamano· Mar 11, 2019, 09:24 UTC · re: Martin Ågren · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

Martin Ågren <martin.agren@gmail.com> writes:
> Ok, then I'll take that offer. I'll squash these two patches into the
> original series. Thanks.

Our mails might cross, but just in case you haven't started, here are the locally squashed results with updated log messages.

Junio C Hamano· Mar 11, 2019, 09:26 UTC · re: Junio C Hamano · lore

[PATCH v3 1/3] Documentation/Makefile: add missing xsl dependencies for manpages

From: Martin Ågren <martin.agren@gmail.com>

These stylesheets very rarely change, but when they do, it really helps if the manpages depend on them. We're casting the net a bit too wide here, since we'll only ever use a subset of the stylesheets, but since these files change so rarely, that should be ok. It's better than missing a dependency.

Observe that manpage-base-url.xsl is a generated file, so we need to list it explicitly, besides the `wildcard` expression we're adding here.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/Makefile +1 −1
diff --git a/Documentation/Makefile b/Documentation/Makefile
index d5d936e6a7..ea6b8e5d72 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -347,7 +347,7 @@ $(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl
+%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-- 
2.21.0-155-ge902e9bcae
Junio C Hamano· Mar 11, 2019, 09:26 UTC · re: Junio C Hamano · lore

[PATCH v3 2/3] Documentation/Makefile: add missing dependency on asciidoctor-extensions

From: Martin Ågren <martin.agren@gmail.com>

asciidoctor-extensions.rb has never changed, but when it does -- such as in the next commit --, it helps if the xml-files depend on it. We're casting the net a bit too wide here, since we'll be rebuilding even with AsciiDoc, which won't look at this file. But since this file changes so rarely, that should be ok. It's better than missing a dependency.

Similarly, most of the html-files are produced directly from ".txt'; make the html-files too depend on asciidoctor-extensions.rb, both the HTMLified manual pages as well as the user-manual that does use .xml intermediary.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to Documentation/Makefile +4 −4
diff --git a/Documentation/Makefile b/Documentation/Makefile
index ea6b8e5d72..f58904a929 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -334,12 +334,12 @@ clean:
 	$(RM) $(cmds_txt) $(mergetools_txt) *.made
 	$(RM) manpage-base-url.xsl
 
-$(MAN_HTML): %.html : %.txt asciidoc.conf
+$(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -d manpage -o $@+ $< && \
 	mv $@+ $@
 
-$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf
+$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -o $@+ $< && \
 	mv $@+ $@
@@ -351,12 +351,12 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-%.xml : %.txt asciidoc.conf
+%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d manpage -o $@+ $< && \
 	mv $@+ $@
 
-user-manual.xml: user-manual.txt user-manual.conf
+user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d book -o $@+ $< && \
 	mv $@+ $@
-- 
2.21.0-155-ge902e9bcae
Junio C Hamano· Mar 11, 2019, 09:26 UTC · re: Junio C Hamano · lore

[PATCH v3 3/3] asciidoctor-extensions: fix spurious space after linkgit

From: Martin Ågren <martin.agren@gmail.com>

When we render, e.g., "linkgit:gitglossary[7]." with Asciidoctor, we get "gitglossary(7) ." with a space between the linkgit macro expansion and the punctuation. We can fix this by dropping the trailing newline after we've turned `linkgit:foo[bar]` into `<citerefentry>..</citerefentry>`.

The diff produced by `USE_ASCIIDOCTOR=Yes ./doc-diff HEAD^ HEAD` is almost 6000 lines large and shows how this fixes "git-foo(x) ,", "(see git-bar(y) )" and so on. One might wonder whether this also turns, e.g., "see linkgit:foo[1] for more" into "see foo(1)for more", but no. We get "...</citerefentry> for more" in the XML, see, e.g., git-am.xml, so the space ends up in git-am.1 just fine. The same is true for the HTML output.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/asciidoctor-extensions.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/asciidoctor-extensions.rb +2 −2
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index ec83b4959e..0089e0cfb8 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -11,12 +11,12 @@ module Git
       def process(parent, target, attrs)
         if parent.document.basebackend? 'html'
           prefix = parent.document.attr('git-relative-html-prefix')
-          %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
+          %(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>)
         elsif parent.document.basebackend? 'docbook'
           "<citerefentry>\n" \
             "<refentrytitle>#{target}</refentrytitle>" \
             "<manvolnum>#{attrs[1]}</manvolnum>\n" \
-          "</citerefentry>\n"
+          "</citerefentry>"
         end
       end
     end
-- 
2.21.0-155-ge902e9bcae
Martin Ågren· Mar 11, 2019, 11:02 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] fix spurious space after linkgit, now in *.html

On Mon, 11 Mar 2019 at 10:24, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > Ok, then I'll take that offer. I'll squash these two patches into the
> > original series. Thanks.
>
> Our mails might cross, but just in case you haven't started, here
> are the locally squashed results with updated log messages.
Thank you so much! This looks good to me.
Martin

← back to recent threads