v3, 5 partsdoc: asciidoc cleanups

This patch series is an attempt to cleanup the Makefile of the documentation.

Since v2 only certain $(RM) commands are moved from one patch to another.

Felipe Contreras (5):
  doc: refactor common asciidoc dependencies
  doc: improve asciidoc dependencies
  doc: remove unnecessary rm instances
  doc: simplify Makefile using .DELETE_ON_ERROR
  doc: avoid using rm directly

 Documentation/Makefile | 77 ++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

Range-diff against v2:
1:  55b188c8ad = 1:  cbcaefed3e doc: refactor common asciidoc dependencies
2:  e69d0a5b89 = 2:  1c8b0b9123 doc: improve asciidoc dependencies
3:  4f18675ce9 < -:  ---------- doc: remove unnecessary rm instances
-:  ---------- > 3:  3da461a55d doc: remove unnecessary rm instances
4:  935675e070 ! 4:  a8c4c4ae26 doc: simplify Makefile using .DELETE_ON_ERROR
    @@ Documentation/Makefile: docdep_prereqs = \
      	cmd-list.made $(cmds_txt)
      
      doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl
    --	$(QUIET_GEN)$(RM) $@+ $@ && \
    --	$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
    +-	$(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
     -	mv $@+ $@
     +	$(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)
      
    @@ Documentation/Makefile: clean:
      	$(RM) GIT-ASCIIDOCFLAGS
      
      $(MAN_HTML): %.html : %.txt $(ASCIIDOC_DEPS)
    --	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
    --	$(TXT_TO_HTML) -d manpage -o $@+ $< && \
    +-	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -d manpage -o $@+ $< && \
     -	mv $@+ $@
     +	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -d manpage -o $@ $<
      
      $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
    --	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
    --	$(TXT_TO_HTML) -o $@+ $< && \
    +-	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -o $@+ $< && \
     -	mv $@+ $@
     +	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -o $@ $<
      
    @@ Documentation/Makefile: manpage-base-url.xsl: manpage-base-url.xsl.in
      	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
      
      %.xml : %.txt $(ASCIIDOC_DEPS)
    --	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
    --	$(TXT_TO_XML) -d manpage -o $@+ $< && \
    +-	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@+ $< && \
     -	mv $@+ $@
     +	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<
      
      user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
    --	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
    --	$(TXT_TO_XML) -d book -o $@+ $< && \
    +-	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d book -o $@+ $< && \
     -	mv $@+ $@
     +	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d book -o $@ $<
      
    @@ Documentation/Makefile: XSLTOPTS += --stringparam html.stylesheet docbook-xsl.cs
      XSLTOPTS += --param generate.consistent.ids 1
      
      user-manual.html: user-manual.xml $(XSLT)
    --	$(QUIET_XSLTPROC)$(RM) $@+ $@ && \
    --	xsltproc $(XSLTOPTS) -o $@+ $(XSLT) $< && \
    +-	$(QUIET_XSLTPROC)xsltproc $(XSLTOPTS) -o $@+ $(XSLT) $< && \
     -	mv $@+ $@
     +	$(QUIET_XSLTPROC)xsltproc $(XSLTOPTS) -o $@ $(XSLT) $<
      
    @@ Documentation/Makefile: XSLTOPTS += --stringparam html.stylesheet docbook-xsl.cs
      	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split -o $@ user-manual.texi
      
      user-manual.texi: user-manual.xml
    --	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
    --	$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@++ && \
    +-	$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@++ && \
     -	$(PERL_PATH) fix-texi.perl <$@++ >$@+ && \
     -	rm $@++ && \
     -	mv $@+ $@
    @@ Documentation/Makefile: XSLTOPTS += --stringparam html.stylesheet docbook-xsl.cs
     +	rm $@+
      
      user-manual.pdf: user-manual.xml
    --	$(QUIET_DBLATEX)$(RM) $@+ $@ && \
    --	$(DBLATEX) -o $@+ $(DBLATEX_COMMON) $< && \
    +-	$(QUIET_DBLATEX)$(DBLATEX) -o $@+ $(DBLATEX_COMMON) $< && \
     -	mv $@+ $@
     +	$(QUIET_DBLATEX)$(DBLATEX) -o $@ $(DBLATEX_COMMON) $<
      
      gitman.texi: $(MAN_XML) cat-texi.perl texi.xsl
    --	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
    -+	$(QUIET_DB2TEXI) \
    + 	$(QUIET_DB2TEXI) \
      	($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \
      		$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \
     -		rm $(xml)+ &&) true) > $@++ && \
    @@ Documentation/Makefile: XSLTOPTS += --stringparam html.stylesheet docbook-xsl.cs
      	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi
      
      $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
    --	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
    --	$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@+ && \
    +-	$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@+ && \
     -	mv $@+ $@
     +	$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@
      
      howto-index.txt: howto-index.sh $(HOWTO_TXT)
    --	$(QUIET_GEN)$(RM) $@+ $@ && \
    --	'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@+ && \
    +-	$(QUIET_GEN)'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@+ && \
     -	mv $@+ $@
     +	$(QUIET_GEN)'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@
      
      $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
      	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt
    -@@ Documentation/Makefile: WEBDOC_DEST = /pub/software/scm/git/docs
    - 
    - howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
    +@@ Documentation/Makefile: howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
      $(patsubst %.txt,%.html,$(HOWTO_TXT)): %.html : %.txt GIT-ASCIIDOCFLAGS
    --	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
    -+	$(QUIET_ASCIIDOC) \
    + 	$(QUIET_ASCIIDOC) \
      	sed -e '1,/^$$/d' $< | \
     -	$(TXT_TO_HTML) - >$@+ && \
     -	mv $@+ $@
5:  b621f3b8e9 = 5:  2c330987b5 doc: avoid using rm directly

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

asciidoc needs asciidoc.conf, asciidoctor asciidoctor-extensions.rb.

Neither needs the other.

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

Commits 50cff52f1a (When generating manpages, delete outdated targets first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove cmd-list.made before redirecting to it., 2007-08-06) created these rm instances for a very rare corner-case: building as root by mistake.

It's odd to have workarounds here, but nowhere else in the Makefile-- which already fails in this stuation, starting from Documentation/technical/.

We gain nothing but complexity, so let's remove them.

Comments-by: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/Makefile | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

Felipe Contreras <felipe.contreras@gmail.com> writes:

This is curious. I understand what other kinds of trailers like Helped-by and Suggested-by people sometimes use mean, but this one does not tell anything other than that this person had some comments on an earlier rounds, does not tell us what kind of comments they were and if something was done to address those comments or the comments were totally ignored or what.

The patch text itself, and the justification given in the second paragraph, both make perfect sense to me.

Junio C Hamano wrote:

If they were ignored they would not be part of the commit. It is understood the comments were incorporated.

When you see 'Tested-by' do you wonder what was the result of the test? I presume most people would understand that the test succeeded, and that's why the trailer line was added.

I happen to have at hand a script that ranks the most common trailer lines:

 * acked-by: 25% (1945)

Doesn't apply.

 * reviewed-by: 22% (1729)

This does apply, but I'm not confident Jeff would like me to add that.

 * helped-by: 17% (1336)
 * reported-by: 12% (960)
 * mentored-by: 5% (379)
 * suggested-by: 4% (281)
 * cc: 3% (222)
 * noticed-by: 2% (165)
 * tested-by: 2% (153)
 * improved-by: 1% (88)
 * thanks-to: 1% (65)
 * signed-off-by: 1% (50)
 * based-on-patch-by: 1% (50)
 * contributions-by: 1% (43)
 * co-authored-by: 1% (41)

I don't think any of these apply.

Cheers.

Felipe Contreras wrote:

Oh, and there are some instances already in the history:

7a9b0b802e (git-clean: add colors to interactive git-clean, 2013-06-25) 1b8fd46732 (git-clean: show items of del_list in columns, 2013-06-25) 52fd972d59 (git-completion: fix regression in zsh support, 2011-05-10)

Although one of those is mine.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

I actually think people use this one to say "person X gave a valuable input in the review discussion", which is exactly the case here, and that was why I wondered you needed to invent a completely new one.

Junio C Hamano wrote:

I think the opposite: "helped-by" encompasses virtually anything. A person that reviews, reports, mentors, suggests, tests, provides a patch, contributes, and co-authors... is helping.

The question is *how* he helped in this particular patch... By providing comments.

Felipe Contreras <felipe.contreras@gmail.com> writes:

But your own stats disagrees with your opinion, so don't invent a new thing, period.

Junio C Hamano wrote:

Stats don't have opinions.

You and I have had this discussion before, between the status quo:

	die ("could not find author in commit %s",
	     oid_to_hex(&commit->object.oid));

And:

	die("could not find author in commit %s",
		oid_to_hex(&commit->object.oid));

Eventually you yourself updated the documentation to explicitly state that it's fine to not align the subsequent lines to the opening parenthesis: f26443da04 (CodingGuidelines: on splitting a long line, 2014-05-02).

It is wrong to demand something that is not in the guidelines, especially if later on the guidelines might include the very thing supposedly frowned upon [*].

In this particular case the guideline is not missing, it actually sides with me:

  You can also create your own tag or use one that's in common usage
  such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".

If you want to forbid certain commit trailers--or limit the allowed trailers to a sanctioned list--then update the guidelines first to reflect that.

But I don't think it makes sense to do that, because commit trailers--just like words, and hundreds of other things--follow a Zipf's law, where the 10th most common word appears around 1/10th of the time.

If you graph the frequency of commit trailers vs. a Zipf distrubtion with a 80:20 rule, it follows it almost perfectly [1]. That means 80% of the commit trailers appear 20% of the time.

My script can calculate any quantile and for example found out that just two lines--Acked-by and Reviewed-by--acound for 47% of all the lines. They are the top 1%.

The top 10% is constituted by 16 lines, and the top 25% are 33.

The median is 1 appearance. That means of the 155 unique lines, half of them have just 1 appearance, in fact, more than half: 100 of them.

For more details see the mail I just sent [2].

Unsurprisingly commit trailer lines follow the same pattern as income distribution and musical bands popularity; the rich get richer.

As much as you might despise the poorest among commit trailers, they are the majority, and they will keep being the majority.

Cheers.

[1] https://i.imgur.com/1wQgSlP.png [2] https://lore.kernel.org/git/60ad75ac7ffca_2ae08208b@natae.notmuch/

[Footnote]

 * Granted, my patch back then did not match the new guideline
   perfectly, but also it wasn't wrong for reasons stated then.

Felipe Contreras wrote:

Plus, here are some instances of commit trailer lines used only *once* by big names of git.git (they are the authors).

2014-02-18: brought-to-attention-by: Junio C Hamano 2012-03-16: discussion-triggered-by: Junio C Hamano 2011-04-26: heavylifting-by: Junio C Hamano 2010-06-23: initial-fix-by: Junio C Hamano 2008-10-23: tested-on-freebsd-by: Junio C Hamano 2008-10-23: tested-on-aix-by: Junio C Hamano 2008-08-19: tested-on-mingw-by: Junio C Hamano 2008-03-23: reference: Junio C Hamano

2020-12-11: initial-patch-by: Johannes Schindelin 2020-11-19: pointed-out-by: Johannes Schindelin 2020-08-12: simplified-testcase-and-bisection-by: Elijah Newren 2020-04-27: documentation-by: Elijah Newren 2019-10-10: inspired-by-a-patch-by: Johannes Schindelin 2018-07-16: explanation-by: Stefan Beller 2018-01-04: also-reported-by: Johannes Schindelin 2017-09-23: reported-at: Jeff King 2017-01-06: test-provided-by: Stefan Beller 2016-10-07: issue-found-by: Jakub Narębski 2016-09-04: benchmarked-by: Christian Couder 2016-05-11: initial-test-by: Johannes Schindelin 2016-05-03: fix-proposed-by: Stefan Beller 2016-01-13: assisted-by: Johannes Schindelin 2016-01-12: derived-from-code-by: Johannes Schindelin 2015-09-24: history-dug-by: Jeff King 2015-09-09: reported-and-suggested-by: Johannes Schindelin 2015-06-30: clever-idea-by: Jeff King 2015-05-20: test-adapted-from: Jeff King 2015-03-03: edited-by: Stefan Beller 2014-12-12: tweaked-by: Michael Haggerty 2014-12-08: analyzed-by: Nguyễn Thái Ngọc Duy 2014-11-02: adapted-by: Nguyễn Thái Ngọc Duy 2014-06-08: reminded-by: Nguyễn Thái Ngọc Duy 2013-12-28: clarified-by: Jeff King 2013-08-21: revised-by: Shawn O. Pearce 2013-08-16: reported-and-analyzed-by: Nguyễn Thái Ngọc Duy 2013-07-09: more-spots-found-by: Jeff King 2013-02-23: brown-paper-bag-fixes-by: Jeff King 2012-10-12: initial-work-by: Nguyễn Thái Ngọc Duy 2012-07-22: reported-tested-and-explained-by: Jonathan Nieder 2011-09-22: tracked-down-by: Nguyễn Thái Ngọc Duy 2011-09-16: originally-by: Jakub Narębski 2011-04-28: inspired-by-patch-by: Jakub Narębski 2011-04-28: inspired-by-code-by: Jakub Narębski 2011-04-28: based-on-code-by: Jakub Narębski 2011-01-06: works-around: Jonathan Nieder 2011-01-02: helpd-by: Jonathan Nieder 2009-12-07: looks-fine-to-me-by: Jeff King 2009-11-11: originally-submitted-by: Jonathan Nieder 2009-05-15: looks-right-to-me-by: Johannes Schindelin 2008-12-26: patch-by: Shawn O. Pearce 2008-12-07: kind-of-noticed-by: Jakub Narębski 2008-07-03: backtraced-by: Shawn O. Pearce 2008-06-13: deemed-obviously-correct-by: Shawn O. Pearce 2008-03-29: patch-submitted-by: Eric Wong 2007-12-07: test-proposed-by: Jakub Narębski 2007-11-17: completely-acked-by: Jeff King 2007-09-12: proposed-by: Shawn O. Pearce 2007-06-03: hopefully-signed-off-by: Johannes Schindelin 2007-05-19: confirmed-by: Eric Wong

Currently GNU make already removes files when catching an interruption signal, however, in order to deal with other kinds of errors a workaround is in place to store target output to a temporary file, and only move it to its right place on success.

By enabling the built-in .DELETE_ON_ERROR we let make do this task, so we don't have to.

This way the rules can be simplified a lot.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/Makefile | 47 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

That's what we have $(RM) for.

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

The above description is correct. I can see that the end result is identical to the previous one.

Is this satisfactory to everybody (I haven't taken a look beyond comparing the end results)?

It does not seem to play well with what brian is already doing in 'seen'; it overlaps with its changes in interesting ways. Is this series meant to be a superset of the topic and supersedes it? How are you two coordinating to avoid duplicated work and stepping on each other's toes?

Thanks.

On Tue, May 25, 2021 at 02:51:54AM +0900, Junio C Hamano wrote:

Yes, all five patches look good to me.

The "comments-by" sub-thread looked unproductive to me, so I mostly skipped it. ;) But as the person whose name was in the trailer, I will say that I do not care either way if it is included. It did not seem to add anything to me. Some trailers are good for pointing later readers to folks who may be able to help understand or debug an old commit. Some are good for just giving credit to people who helped. "Comments-by" didn't really seem to do either to me (and I don't feel like I contributed much worthy of credit anyway).

-Peff

Jeff King wrote:

So... Acked-by?

But you did provide comments, and those comments resulted in changes, so the comments-by line is accurate. And for what it's worth that commit trailer line has been used 3 times already, which places it in the 3rd top decile.

Perhaps some people would prefer feedback-by, but that that line has been used *zero* times. Even brown-paper-bag-fixes-by has been used more.

Cheers.

On Thu, May 27, 2021 at 11:59:24AM -0500, Felipe Contreras wrote:

If you want. I have never found that trailer to be particularly helpful.

I didn't say it was inaccurate. I just think it is not useful.

-Peff

Jeff King wrote:

I understood. I'm not saying the tag must be there because it's useful (I agree it's usefulness is marginal at best), I'm saying including it does not hurt.

Jeff King <peff@peff.net> writes:

The issue that it interacts in a funny way with what brian already has in 'seen' aside, the patches look good to me, too, so let's start merging it down.

Exactly the same feeling. It didn't add any value that was why I saw no point in using something almost nobody uses (instead of the more commonly used Helped-by, for example).

Thanks.

Junio C Hamano wrote:

As I demonstrated with probabilistic certainty; everyone uses uncommon lines (including you). The fact that a line is uncommon doesn't necessarily mean it's bad, in fact, I do see value in going outside the box (like one of my favorites: Brown-paper-bag-fixes-by [4981fe750b]).

And of course if you find Comments-by so egregious, you can amend my commit message (as you have done many times in the past). But this might be a good opportunity to exercise the advice Jeff gave in another thread, and even if you are not 100% happy with something, let it slide.

Cheers.