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

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

[Martin Ågren]

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

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

[Martin Ågren]

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

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

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

[Eric Sunshine]

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?

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

[Martin Ågren]

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:
> > 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

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

[Eric Sunshine]

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:
> > 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.)

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

[Martin Ågren]

On Mon, 25 Feb 2019 at 22:01, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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

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

[Eric Sunshine]

On Tue, Feb 26, 2019 at 12:59 AM Martin Ågren <martin.agren@gmail.com> wrote:
> 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.

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

[Martin Ågren]

On Tue, 26 Feb 2019 at 13:47, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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

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

[Martin Ågren]

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

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

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

[Martin Ågren]

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
-- 
2.21.0

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Mon, Feb 25, 2019 at 09:02:40PM +0100, Martin Ågren wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Mon, Feb 25, 2019 at 09:02:37PM +0100, Martin Ågren wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Martin Ågren]

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

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

[Martin Ågren]

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

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

[Martin Ågren]

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

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

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

[Martin Ågren]

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

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

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

[Martin Ågren]

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
-- 
2.21.0

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

"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. 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.

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

[Martin Ågren]

On Sun, 3 Mar 2019 at 02:25, Junio C Hamano <gitster@pobox.com> wrote:
>
> "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

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

[Martin Ågren]

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

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

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

[Martin Ågren]

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

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

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

[Jeff King]

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

-Peff

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

[Martin Ågren]

On Mon, 11 Mar 2019 at 03:59, Jeff King <peff@peff.net> wrote:
>
> 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

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

[Junio C Hamano]

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

> 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.

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

[Martin Ågren]

On Mon, 11 Mar 2019 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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

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

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

[Martin Ågren]

On Mon, 11 Mar 2019 at 10:24, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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