git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/5] doc: asciidoc cleanups
@ 2021-05-14 11:56 Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 1/5] doc: refactor common asciidoc dependencies Felipe Contreras
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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

It's quie different from v1 mainly because now the .DELETE_ON_ERROR approach is being used as Aevar
suggested, however it tries to do the same thing.

It does not enable asciidoctor direct man page creation, that's in a separate patch.

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 v1:
1:  62d76c126a < -:  ---------- doc: standardize asciidoc calls
2:  0677725926 < -:  ---------- doc: add an asciidoc helper
3:  ca69c75596 < -:  ---------- doc: disable asciidoc-helper for asciidoctor
4:  f379515577 < -:  ---------- doc: simplify the handling of interruptions
5:  d2d10b34f3 < -:  ---------- doc: remove redundant rm
6:  d78e08aa2a < -:  ---------- doc: refactor common dependencies
7:  450a79d36f < -:  ---------- doc: improve asciidoc dependencies
8:  5be9efaa11 < -:  ---------- doc: join xml and man rules
-:  ---------- > 1:  55b188c8ad doc: refactor common asciidoc dependencies
-:  ---------- > 2:  e69d0a5b89 doc: improve asciidoc dependencies
-:  ---------- > 3:  4f18675ce9 doc: remove unnecessary rm instances
-:  ---------- > 4:  935675e070 doc: simplify Makefile using .DELETE_ON_ERROR
-:  ---------- > 5:  b621f3b8e9 doc: avoid using rm directly
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/5] doc: refactor common asciidoc dependencies
  2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
@ 2021-05-14 11:56 ` Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 2/5] doc: improve " Felipe Contreras
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 2aae4c9cbb..46d9b98dac 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -139,6 +139,7 @@ ASCIIDOC_CONF = -f asciidoc.conf
 ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \
 		-amanversion=$(GIT_VERSION) \
 		-amanmanual='Git Manual' -amansource='Git'
+ASCIIDOC_DEPS = asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
 TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)
 TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)
 MANPAGE_XSL = manpage-normal.xsl
@@ -354,12 +355,12 @@ clean:
 	$(RM) manpage-base-url.xsl
 	$(RM) GIT-ASCIIDOCFLAGS
 
-$(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
+$(MAN_HTML): %.html : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -d manpage -o $@+ $< && \
 	mv $@+ $@
 
-$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
+$(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_HTML) -o $@+ $< && \
 	mv $@+ $@
@@ -371,7 +372,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_XMLTO)$(RM) $@ && \
 	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
-%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
+%.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
 	$(TXT_TO_XML) -d manpage -o $@+ $< && \
 	mv $@+ $@
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/5] doc: improve asciidoc dependencies
  2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 1/5] doc: refactor common asciidoc dependencies Felipe Contreras
@ 2021-05-14 11:56 ` Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 3/5] doc: remove unnecessary rm instances Felipe Contreras
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 46d9b98dac..0f59cc0853 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -139,7 +139,7 @@ ASCIIDOC_CONF = -f asciidoc.conf
 ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \
 		-amanversion=$(GIT_VERSION) \
 		-amanmanual='Git Manual' -amansource='Git'
-ASCIIDOC_DEPS = asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
+ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS
 TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)
 TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)
 MANPAGE_XSL = manpage-normal.xsl
@@ -194,6 +194,7 @@ ASCIIDOC_DOCBOOK = docbook5
 ASCIIDOC_EXTRA += -acompat-mode -atabsize=8
 ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
 ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
+ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
 DBLATEX_COMMON =
 XMLTO_EXTRA += --skip-validation
 XMLTO_EXTRA += -x manpage.xsl
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 1/5] doc: refactor common asciidoc dependencies Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 2/5] doc: improve " Felipe Contreras
@ 2021-05-14 11:56 ` Felipe Contreras
  2021-05-15  9:24   ` Jeff King
  2021-05-14 11:56 ` [PATCH v2 4/5] doc: simplify Makefile using .DELETE_ON_ERROR Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 5/5] doc: avoid using rm directly Felipe Contreras
  4 siblings, 1 reply; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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.

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

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 0f59cc0853..aae8803e4c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -318,8 +318,7 @@ cmds_txt = cmds-ancillaryinterrogators.txt \
 $(cmds_txt): cmd-list.made
 
 cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)
-	$(QUIET_GEN)$(RM) $@ && \
-	$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(cmds_txt) $(QUIET_STDERR) && \
+	$(QUIET_GEN)$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(cmds_txt) $(QUIET_STDERR) && \
 	date >$@
 
 mergetools_txt = mergetools-diff.txt mergetools-merge.txt
@@ -327,7 +326,7 @@ mergetools_txt = mergetools-diff.txt mergetools-merge.txt
 $(mergetools_txt): mergetools-list.made
 
 mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
-	$(QUIET_GEN)$(RM) $@ && \
+	$(QUIET_GEN) \
 	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
 		. ../git-mergetool--lib.sh && \
 		show_tool_names can_diff "* " || :' >mergetools-diff.txt && \
@@ -370,8 +369,7 @@ 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 $(wildcard manpage*.xsl)
-	$(QUIET_XMLTO)$(RM) $@ && \
-	$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 4/5] doc: simplify Makefile using .DELETE_ON_ERROR
  2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
                   ` (2 preceding siblings ...)
  2021-05-14 11:56 ` [PATCH v2 3/5] doc: remove unnecessary rm instances Felipe Contreras
@ 2021-05-14 11:56 ` Felipe Contreras
  2021-05-14 11:56 ` [PATCH v2 5/5] doc: avoid using rm directly Felipe Contreras
  4 siblings, 0 replies; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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 | 61 +++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index aae8803e4c..eaff97dcb8 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -296,9 +296,7 @@ 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) && \
-	mv $@+ $@
+	$(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)
 
 ifneq ($(MAKECMDGOALS),clean)
 -include doc.dep
@@ -356,14 +354,10 @@ clean:
 	$(RM) GIT-ASCIIDOCFLAGS
 
 $(MAN_HTML): %.html : %.txt $(ASCIIDOC_DEPS)
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
-	$(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 $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -o $@ $<
 
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
@@ -372,14 +366,10 @@ 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 $@+ $< && \
-	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 $@+ $< && \
-	mv $@+ $@
+	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d book -o $@ $<
 
 technical/api-index.txt: technical/api-index-skel.txt \
 	technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))
@@ -400,46 +390,35 @@ XSLTOPTS += --stringparam html.stylesheet docbook-xsl.css
 XSLTOPTS += --param generate.consistent.ids 1
 
 user-manual.html: user-manual.xml $(XSLT)
-	$(QUIET_XSLTPROC)$(RM) $@+ $@ && \
-	xsltproc $(XSLTOPTS) -o $@+ $(XSLT) $< && \
-	mv $@+ $@
+	$(QUIET_XSLTPROC)xsltproc $(XSLTOPTS) -o $@ $(XSLT) $<
 
 git.info: user-manual.texi
 	$(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 >$@++ && \
-	$(PERL_PATH) fix-texi.perl <$@++ >$@+ && \
-	rm $@++ && \
-	mv $@+ $@
+	$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@+ && \
+	$(PERL_PATH) fix-texi.perl <$@+ >$@ && \
+	rm $@+
 
 user-manual.pdf: user-manual.xml
-	$(QUIET_DBLATEX)$(RM) $@+ $@ && \
-	$(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) \
 	($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \
 		$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \
-		rm $(xml)+ &&) true) > $@++ && \
-	$(PERL_PATH) cat-texi.perl $@ <$@++ >$@+ && \
-	rm $@++ && \
-	mv $@+ $@
+		rm $(xml)+ &&) true) > $@+ && \
+	$(PERL_PATH) cat-texi.perl $@ <$@+ >$@ && \
+	rm $@+
 
 gitman.info: gitman.texi
 	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi
 
 $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
-	$(QUIET_DB2TEXI)$(RM) $@+ $@ && \
-	$(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)) >$@+ && \
-	mv $@+ $@
+	$(QUIET_GEN)'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@
 
 $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
 	$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt
@@ -448,10 +427,9 @@ WEBDOC_DEST = /pub/software/scm/git/docs
 
 howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
 $(patsubst %.txt,%.html,$(HOWTO_TXT)): %.html : %.txt GIT-ASCIIDOCFLAGS
-	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
+	$(QUIET_ASCIIDOC) \
 	sed -e '1,/^$$/d' $< | \
-	$(TXT_TO_HTML) - >$@+ && \
-	mv $@+ $@
+	$(TXT_TO_HTML) - >$@
 
 install-webdoc : html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)
@@ -492,4 +470,7 @@ doc-l10n install-l10n::
 	$(MAKE) -C po $@
 endif
 
+# Delete the target file on error
+.DELETE_ON_ERROR:
+
 .PHONY: FORCE
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 5/5] doc: avoid using rm directly
  2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
                   ` (3 preceding siblings ...)
  2021-05-14 11:56 ` [PATCH v2 4/5] doc: simplify Makefile using .DELETE_ON_ERROR Felipe Contreras
@ 2021-05-14 11:56 ` Felipe Contreras
  4 siblings, 0 replies; 11+ messages in thread
From: Felipe Contreras @ 2021-05-14 11:56 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, brian m . carlson, Martin Ågren, Jeff King,
	Ævar Arnfjörð Bjarmason, Felipe Contreras

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

diff --git a/Documentation/Makefile b/Documentation/Makefile
index eaff97dcb8..f5605b7767 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -398,7 +398,7 @@ git.info: user-manual.texi
 user-manual.texi: user-manual.xml
 	$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@+ && \
 	$(PERL_PATH) fix-texi.perl <$@+ >$@ && \
-	rm $@+
+	$(RM) $@+
 
 user-manual.pdf: user-manual.xml
 	$(QUIET_DBLATEX)$(DBLATEX) -o $@ $(DBLATEX_COMMON) $<
@@ -407,9 +407,9 @@ gitman.texi: $(MAN_XML) cat-texi.perl texi.xsl
 	$(QUIET_DB2TEXI) \
 	($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \
 		$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \
-		rm $(xml)+ &&) true) > $@+ && \
+		$(RM) $(xml)+ &&) true) > $@+ && \
 	$(PERL_PATH) cat-texi.perl $@ <$@+ >$@ && \
-	rm $@+
+	$(RM) $@+
 
 gitman.info: gitman.texi
 	$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi
-- 
2.31.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-14 11:56 ` [PATCH v2 3/5] doc: remove unnecessary rm instances Felipe Contreras
@ 2021-05-15  9:24   ` Jeff King
  2021-05-15 12:04     ` Felipe Contreras
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2021-05-15  9:24 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, brian m . carlson, Martin Ågren,
	Ævar Arnfjörð Bjarmason

On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:

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

Aren't there tons more that you end up removing in the next patch? E.g.:

   doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl
  -       $(QUIET_GEN)$(RM) $@+ $@ && \
  -       $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
  -       mv $@+ $@
  +       $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)

That does differ in that it removes $@+, too, but the premise is the
same (we know that $@+ could not be a problem, as we're about to
clobber it anyway).

I'm OK with getting rid of all of them, but it seems like it ought to be
happening all in this patch.

(And in general the rest of the series looks OK to me).

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-15  9:24   ` Jeff King
@ 2021-05-15 12:04     ` Felipe Contreras
  2021-05-17  8:53       ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Contreras @ 2021-05-15 12:04 UTC (permalink / raw)
  To: Jeff King, Felipe Contreras
  Cc: git, Junio C Hamano, brian m . carlson, Martin Ågren,
	Ævar Arnfjörð Bjarmason

Jeff King wrote:
> On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:
> 
> > 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/.
> 
> Aren't there tons more that you end up removing in the next patch? E.g.:
> 
>    doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl
>   -       $(QUIET_GEN)$(RM) $@+ $@ && \
>   -       $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
>   -       mv $@+ $@
>   +       $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)
> 
> That does differ in that it removes $@+, too, but the premise is the
> same (we know that $@+ could not be a problem, as we're about to
> clobber it anyway).
> 
> I'm OK with getting rid of all of them, but it seems like it ought to be
> happening all in this patch.

Yeah, but the rationale is different.

  1. $(RM) $@: these remove the target file because of permissions
     (i.e. root owned)
  2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds

To get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,
to get rid of #1 we don't, we can just do it.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-15 12:04     ` Felipe Contreras
@ 2021-05-17  8:53       ` Jeff King
  2021-05-17 10:42         ` Felipe Contreras
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2021-05-17  8:53 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, brian m . carlson, Martin Ågren,
	Ævar Arnfjörð Bjarmason

On Sat, May 15, 2021 at 07:04:05AM -0500, Felipe Contreras wrote:

> Jeff King wrote:
> > On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:
> > 
> > > 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/.
> > 
> > Aren't there tons more that you end up removing in the next patch? E.g.:
> > 
> >    doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl
> >   -       $(QUIET_GEN)$(RM) $@+ $@ && \
> >   -       $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
> >   -       mv $@+ $@
> >   +       $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)
> > 
> > That does differ in that it removes $@+, too, but the premise is the
> > same (we know that $@+ could not be a problem, as we're about to
> > clobber it anyway).
> > 
> > I'm OK with getting rid of all of them, but it seems like it ought to be
> > happening all in this patch.
> 
> Yeah, but the rationale is different.
> 
>   1. $(RM) $@: these remove the target file because of permissions
>      (i.e. root owned)
>   2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds
> 
> To get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,
> to get rid of #1 we don't, we can just do it.

To get rid of the "mv", you need something like .DELETE_ON_ERROR. But
the "rm" in the second case has nothing to do with interrupted builds.
It is is doing the same nothing that the ones you are getting rid of
here are.

I.e., I was suggesting to get rid of the "rm" call in the hunk I showed
above, but leave the "mv" for the follow-on patch.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-17  8:53       ` Jeff King
@ 2021-05-17 10:42         ` Felipe Contreras
  2021-05-17 11:40           ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Contreras @ 2021-05-17 10:42 UTC (permalink / raw)
  To: Jeff King, Felipe Contreras
  Cc: git, Junio C Hamano, brian m . carlson, Martin Ågren,
	Ævar Arnfjörð Bjarmason

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

> > > That does differ in that it removes $@+, too, but the premise is the
> > > same (we know that $@+ could not be a problem, as we're about to
> > > clobber it anyway).
> > > 
> > > I'm OK with getting rid of all of them, but it seems like it ought to be
> > > happening all in this patch.
> > 
> > Yeah, but the rationale is different.
> > 
> >   1. $(RM) $@: these remove the target file because of permissions
> >      (i.e. root owned)
> >   2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds
> > 
> > To get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,
> > to get rid of #1 we don't, we can just do it.
> 
> To get rid of the "mv", you need something like .DELETE_ON_ERROR. But
> the "rm" in the second case has nothing to do with interrupted builds.
> It is is doing the same nothing that the ones you are getting rid of
> here are.
> 
> I.e., I was suggesting to get rid of the "rm" call in the hunk I showed
> above, but leave the "mv" for the follow-on patch.

Ahh, I see. It's quite a bit more work, but sure, I can do that.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/5] doc: remove unnecessary rm instances
  2021-05-17 10:42         ` Felipe Contreras
@ 2021-05-17 11:40           ` Jeff King
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2021-05-17 11:40 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, brian m . carlson, Martin Ågren,
	Ævar Arnfjörð Bjarmason

On Mon, May 17, 2021 at 05:42:31AM -0500, Felipe Contreras wrote:

> > I.e., I was suggesting to get rid of the "rm" call in the hunk I showed
> > above, but leave the "mv" for the follow-on patch.
> 
> Ahh, I see. It's quite a bit more work, but sure, I can do that.

If it's a lot more work, I can live with the split as you have here. I
didn't think it would be a lot, though.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-05-17 11:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 11:56 [PATCH v2 0/5] doc: asciidoc cleanups Felipe Contreras
2021-05-14 11:56 ` [PATCH v2 1/5] doc: refactor common asciidoc dependencies Felipe Contreras
2021-05-14 11:56 ` [PATCH v2 2/5] doc: improve " Felipe Contreras
2021-05-14 11:56 ` [PATCH v2 3/5] doc: remove unnecessary rm instances Felipe Contreras
2021-05-15  9:24   ` Jeff King
2021-05-15 12:04     ` Felipe Contreras
2021-05-17  8:53       ` Jeff King
2021-05-17 10:42         ` Felipe Contreras
2021-05-17 11:40           ` Jeff King
2021-05-14 11:56 ` [PATCH v2 4/5] doc: simplify Makefile using .DELETE_ON_ERROR Felipe Contreras
2021-05-14 11:56 ` [PATCH v2 5/5] doc: avoid using rm directly Felipe Contreras

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git