{"thread":{"id":"55689","subject":"[PATCH v2 0/5] doc: asciidoc cleanups","startedAt":"2021-05-14T11:56:38Z","lastAt":"2021-05-17T11:40:07Z","messageCount":11,"participants":["Felipe Contreras","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"424558","messageId":"20210514115631.503276-1-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":null,"subject":"[PATCH v2 0/5] doc: asciidoc cleanups","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:26Z","receivedAt":"2021-05-14T11:56:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This patch series is an attempt to cleanup the Makefile of the documentation.\n\nIt's quie different from v1 mainly because now the .DELETE_ON_ERROR approach is being used as Aevar\nsuggested, however it tries to do the same thing.\n\nIt does not enable asciidoctor direct man page creation, that's in a separate patch.\n\nFelipe Contreras (5):\n doc: refactor common asciidoc dependencies\n doc: improve asciidoc dependencies\n doc: remove unnecessary rm instances\n doc: simplify Makefile using .DELETE_ON_ERROR\n doc: avoid using rm directly\n\n Documentation/Makefile | 77 ++++++++++++++++--------------------------\n 1 file changed, 29 insertions(+), 48 deletions(-)\n\nRange-diff against v1:\n1: 62d76c126a < -: ---------- doc: standardize asciidoc calls\n2: 0677725926 < -: ---------- doc: add an asciidoc helper\n3: ca69c75596 < -: ---------- doc: disable asciidoc-helper for asciidoctor\n4: f379515577 < -: ---------- doc: simplify the handling of interruptions\n5: d2d10b34f3 < -: ---------- doc: remove redundant rm\n6: d78e08aa2a < -: ---------- doc: refactor common dependencies\n7: 450a79d36f < -: ---------- doc: improve asciidoc dependencies\n8: 5be9efaa11 < -: ---------- doc: join xml and man rules\n-: ---------- > 1: 55b188c8ad doc: refactor common asciidoc dependencies\n-: ---------- > 2: e69d0a5b89 doc: improve asciidoc dependencies\n-: ---------- > 3: 4f18675ce9 doc: remove unnecessary rm instances\n-: ---------- > 4: 935675e070 doc: simplify Makefile using .DELETE_ON_ERROR\n-: ---------- > 5: b621f3b8e9 doc: avoid using rm directly\n-- \n2.31.1\n\n"},{"id":"424559","messageId":"20210514115631.503276-2-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":"20210514115631.503276-1-felipe.contreras@gmail.com","subject":"[PATCH v2 1/5] doc: refactor common asciidoc dependencies","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:27Z","receivedAt":"2021-05-14T11:56:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n Documentation/Makefile | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 2aae4c9cbb..46d9b98dac 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -139,6 +139,7 @@ ASCIIDOC_CONF = -f asciidoc.conf\n ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n \t\t-amanversion=$(GIT_VERSION) \\\n \t\t-amanmanual='Git Manual' -amansource='Git'\n+ASCIIDOC_DEPS = asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n MANPAGE_XSL = manpage-normal.xsl\n@@ -354,12 +355,12 @@ clean:\n \t$(RM) manpage-base-url.xsl\n \t$(RM) GIT-ASCIIDOCFLAGS\n \n-$(MAN_HTML): %.html : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n+$(MAN_HTML): %.html : %.txt $(ASCIIDOC_DEPS)\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n \t$(TXT_TO_HTML) -d manpage -o $@+ $< && \\\n \tmv $@+ $@\n \n-$(OBSOLETE_HTML): %.html : %.txto asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n+$(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n \t$(TXT_TO_HTML) -o $@+ $< && \\\n \tmv $@+ $@\n@@ -371,7 +372,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in\n \t$(QUIET_XMLTO)$(RM) $@ && \\\n \t$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<\n \n-%.xml : %.txt asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n+%.xml : %.txt $(ASCIIDOC_DEPS)\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n \t$(TXT_TO_XML) -d manpage -o $@+ $< && \\\n \tmv $@+ $@\n-- \n2.31.1\n\n"},{"id":"424560","messageId":"20210514115631.503276-3-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":"20210514115631.503276-1-felipe.contreras@gmail.com","subject":"[PATCH v2 2/5] doc: improve asciidoc dependencies","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:28Z","receivedAt":"2021-05-14T11:56:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"asciidoc needs asciidoc.conf, asciidoctor asciidoctor-extensions.rb.\n\nNeither needs the other.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/Makefile | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 46d9b98dac..0f59cc0853 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -139,7 +139,7 @@ ASCIIDOC_CONF = -f asciidoc.conf\n ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \\\n \t\t-amanversion=$(GIT_VERSION) \\\n \t\t-amanmanual='Git Manual' -amansource='Git'\n-ASCIIDOC_DEPS = asciidoc.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n+ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS\n TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML)\n TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK)\n MANPAGE_XSL = manpage-normal.xsl\n@@ -194,6 +194,7 @@ ASCIIDOC_DOCBOOK = docbook5\n ASCIIDOC_EXTRA += -acompat-mode -atabsize=8\n ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions\n ASCIIDOC_EXTRA += -alitdd='&\\#x2d;&\\#x2d;'\n+ASCIIDOC_DEPS = asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n DBLATEX_COMMON =\n XMLTO_EXTRA += --skip-validation\n XMLTO_EXTRA += -x manpage.xsl\n-- \n2.31.1\n\n"},{"id":"424561","messageId":"20210514115631.503276-4-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":"20210514115631.503276-1-felipe.contreras@gmail.com","subject":"[PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:29Z","receivedAt":"2021-05-14T11:56:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Commits 50cff52f1a (When generating manpages, delete outdated targets\nfirst., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove\ncmd-list.made before redirecting to it., 2007-08-06) created these rm\ninstances for a very rare corner-case: building as root by mistake.\n\nIt's odd to have workarounds here, but nowhere else in the Makefile--\nwhich already fails in this stuation, starting from\nDocumentation/technical/.\n\nWe gain nothing but complexity, so let's remove them.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/Makefile | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 0f59cc0853..aae8803e4c 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -318,8 +318,7 @@ cmds_txt = cmds-ancillaryinterrogators.txt \\\n $(cmds_txt): cmd-list.made\n \n cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)\n-\t$(QUIET_GEN)$(RM) $@ && \\\n-\t$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(cmds_txt) $(QUIET_STDERR) && \\\n+\t$(QUIET_GEN)$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(cmds_txt) $(QUIET_STDERR) && \\\n \tdate >$@\n \n mergetools_txt = mergetools-diff.txt mergetools-merge.txt\n@@ -327,7 +326,7 @@ mergetools_txt = mergetools-diff.txt mergetools-merge.txt\n $(mergetools_txt): mergetools-list.made\n \n mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)\n-\t$(QUIET_GEN)$(RM) $@ && \\\n+\t$(QUIET_GEN) \\\n \t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n \t\t. ../git-mergetool--lib.sh && \\\n \t\tshow_tool_names can_diff \"* \" || :' >mergetools-diff.txt && \\\n@@ -370,8 +369,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in\n \t$(QUIET_GEN)sed \"s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|\" $< > $@\n \n %.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)\n-\t$(QUIET_XMLTO)$(RM) $@ && \\\n-\t$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<\n+\t$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<\n \n %.xml : %.txt $(ASCIIDOC_DEPS)\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-- \n2.31.1\n\n"},{"id":"424562","messageId":"20210514115631.503276-5-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":"20210514115631.503276-1-felipe.contreras@gmail.com","subject":"[PATCH v2 4/5] doc: simplify Makefile using .DELETE_ON_ERROR","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:30Z","receivedAt":"2021-05-14T11:56:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently GNU make already removes files when catching an interruption\nsignal, however, in order to deal with other kinds of errors a\nworkaround is in place to store target output to a temporary file, and\nonly move it to its right place on success.\n\nBy enabling the built-in .DELETE_ON_ERROR we let make do this task, so\nwe don't have to.\n\nThis way the rules can be simplified a lot.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Felipe Contreras \n---\n Documentation/Makefile | 61 +++++++++++++++---------------------------\n 1 file changed, 21 insertions(+), 40 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex aae8803e4c..eaff97dcb8 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -296,9 +296,7 @@ docdep_prereqs = \\\n \tcmd-list.made $(cmds_txt)\n \n doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl\n-\t$(QUIET_GEN)$(RM) $@+ $@ && \\\n-\t$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n-\tmv $@+ $@\n+\t$(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)\n \n ifneq ($(MAKECMDGOALS),clean)\n -include doc.dep\n@@ -356,14 +354,10 @@ clean:\n \t$(RM) GIT-ASCIIDOCFLAGS\n \n $(MAN_HTML): %.html : %.txt $(ASCIIDOC_DEPS)\n-\t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-\t$(TXT_TO_HTML) -d manpage -o $@+ $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -d manpage -o $@ $<\n \n $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)\n-\t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-\t$(TXT_TO_HTML) -o $@+ $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_ASCIIDOC)$(TXT_TO_HTML) -o $@ $<\n \n manpage-base-url.xsl: manpage-base-url.xsl.in\n \t$(QUIET_GEN)sed \"s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|\" $< > $@\n@@ -372,14 +366,10 @@ manpage-base-url.xsl: manpage-base-url.xsl.in\n \t$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<\n \n %.xml : %.txt $(ASCIIDOC_DEPS)\n-\t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-\t$(TXT_TO_XML) -d manpage -o $@+ $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<\n \n user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS\n-\t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-\t$(TXT_TO_XML) -d book -o $@+ $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d book -o $@ $<\n \n technical/api-index.txt: technical/api-index-skel.txt \\\n \ttechnical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))\n@@ -400,46 +390,35 @@ XSLTOPTS += --stringparam html.stylesheet docbook-xsl.css\n XSLTOPTS += --param generate.consistent.ids 1\n \n user-manual.html: user-manual.xml $(XSLT)\n-\t$(QUIET_XSLTPROC)$(RM) $@+ $@ && \\\n-\txsltproc $(XSLTOPTS) -o $@+ $(XSLT) $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_XSLTPROC)xsltproc $(XSLTOPTS) -o $@ $(XSLT) $<\n \n git.info: user-manual.texi\n \t$(QUIET_MAKEINFO)$(MAKEINFO) --no-split -o $@ user-manual.texi\n \n user-manual.texi: user-manual.xml\n-\t$(QUIET_DB2TEXI)$(RM) $@+ $@ && \\\n-\t$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@++ && \\\n-\t$(PERL_PATH) fix-texi.perl <$@++ >$@+ && \\\n-\trm $@++ && \\\n-\tmv $@+ $@\n+\t$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@+ && \\\n+\t$(PERL_PATH) fix-texi.perl <$@+ >$@ && \\\n+\trm $@+\n \n user-manual.pdf: user-manual.xml\n-\t$(QUIET_DBLATEX)$(RM) $@+ $@ && \\\n-\t$(DBLATEX) -o $@+ $(DBLATEX_COMMON) $< && \\\n-\tmv $@+ $@\n+\t$(QUIET_DBLATEX)$(DBLATEX) -o $@ $(DBLATEX_COMMON) $<\n \n gitman.texi: $(MAN_XML) cat-texi.perl texi.xsl\n-\t$(QUIET_DB2TEXI)$(RM) $@+ $@ && \\\n+\t$(QUIET_DB2TEXI) \\\n \t($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \\\n \t\t$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \\\n-\t\trm $(xml)+ &&) true) > $@++ && \\\n-\t$(PERL_PATH) cat-texi.perl $@ <$@++ >$@+ && \\\n-\trm $@++ && \\\n-\tmv $@+ $@\n+\t\trm $(xml)+ &&) true) > $@+ && \\\n+\t$(PERL_PATH) cat-texi.perl $@ <$@+ >$@ && \\\n+\trm $@+\n \n gitman.info: gitman.texi\n \t$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi\n \n $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml\n-\t$(QUIET_DB2TEXI)$(RM) $@+ $@ && \\\n-\t$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@+ && \\\n-\tmv $@+ $@\n+\t$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) --to-stdout $*.xml >$@\n \n howto-index.txt: howto-index.sh $(HOWTO_TXT)\n-\t$(QUIET_GEN)$(RM) $@+ $@ && \\\n-\t'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@+ && \\\n-\tmv $@+ $@\n+\t$(QUIET_GEN)'$(SHELL_PATH_SQ)' ./howto-index.sh $(sort $(HOWTO_TXT)) >$@\n \n $(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt\n \t$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt\n@@ -448,10 +427,9 @@ WEBDOC_DEST = /pub/software/scm/git/docs\n \n howto/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../\n $(patsubst %.txt,%.html,$(HOWTO_TXT)): %.html : %.txt GIT-ASCIIDOCFLAGS\n-\t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n+\t$(QUIET_ASCIIDOC) \\\n \tsed -e '1,/^$$/d' $< | \\\n-\t$(TXT_TO_HTML) - >$@+ && \\\n-\tmv $@+ $@\n+\t$(TXT_TO_HTML) - >$@\n \n install-webdoc : html\n \t'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)\n@@ -492,4 +470,7 @@ doc-l10n install-l10n::\n \t$(MAKE) -C po $@\n endif\n \n+# Delete the target file on error\n+.DELETE_ON_ERROR:\n+\n .PHONY: FORCE\n-- \n2.31.1\n\n"},{"id":"424563","messageId":"20210514115631.503276-6-felipe.contreras@gmail.com","threadId":"55689","inReplyTo":"20210514115631.503276-1-felipe.contreras@gmail.com","subject":"[PATCH v2 5/5] doc: avoid using rm directly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-14T11:56:31Z","receivedAt":"2021-05-14T11:56:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"That's what we have $(RM) for.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/Makefile | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex eaff97dcb8..f5605b7767 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -398,7 +398,7 @@ git.info: user-manual.texi\n user-manual.texi: user-manual.xml\n \t$(QUIET_DB2TEXI)$(DOCBOOK2X_TEXI) user-manual.xml --encoding=UTF-8 --to-stdout >$@+ && \\\n \t$(PERL_PATH) fix-texi.perl <$@+ >$@ && \\\n-\trm $@+\n+\t$(RM) $@+\n \n user-manual.pdf: user-manual.xml\n \t$(QUIET_DBLATEX)$(DBLATEX) -o $@ $(DBLATEX_COMMON) $<\n@@ -407,9 +407,9 @@ gitman.texi: $(MAN_XML) cat-texi.perl texi.xsl\n \t$(QUIET_DB2TEXI) \\\n \t($(foreach xml,$(sort $(MAN_XML)),xsltproc -o $(xml)+ texi.xsl $(xml) && \\\n \t\t$(DOCBOOK2X_TEXI) --encoding=UTF-8 --to-stdout $(xml)+ && \\\n-\t\trm $(xml)+ &&) true) > $@+ && \\\n+\t\t$(RM) $(xml)+ &&) true) > $@+ && \\\n \t$(PERL_PATH) cat-texi.perl $@ <$@+ >$@ && \\\n-\trm $@+\n+\t$(RM) $@+\n \n gitman.info: gitman.texi\n \t$(QUIET_MAKEINFO)$(MAKEINFO) --no-split --no-validate $*.texi\n-- \n2.31.1\n\n"},{"id":"424634","messageId":"YJ+TQiYVgocUBQRV@coredump.intra.peff.net","threadId":"55689","inReplyTo":"20210514115631.503276-4-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-15T09:24:18Z","receivedAt":"2021-05-15T09:24:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:\n\n> Commits 50cff52f1a (When generating manpages, delete outdated targets\n> first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove\n> cmd-list.made before redirecting to it., 2007-08-06) created these rm\n> instances for a very rare corner-case: building as root by mistake.\n> \n> It's odd to have workarounds here, but nowhere else in the Makefile--\n> which already fails in this stuation, starting from\n> Documentation/technical/.\n\nAren't there tons more that you end up removing in the next patch? E.g.:\n\n doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl\n - $(QUIET_GEN)$(RM) $@+ $@ && \\\n - $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n - mv $@+ $@\n + $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)\n\nThat does differ in that it removes $@+, too, but the premise is the\nsame (we know that $@+ could not be a problem, as we're about to\nclobber it anyway).\n\nI'm OK with getting rid of all of them, but it seems like it ought to be\nhappening all in this patch.\n\n(And in general the rest of the series looks OK to me).\n\n-Peff\n"},{"id":"424653","messageId":"609fb8b5bf799_e173a20897@natae.notmuch","threadId":"55689","inReplyTo":"YJ+TQiYVgocUBQRV@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-15T12:04:05Z","receivedAt":"2021-05-15T12:04:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:\n> \n> > Commits 50cff52f1a (When generating manpages, delete outdated targets\n> > first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove\n> > cmd-list.made before redirecting to it., 2007-08-06) created these rm\n> > instances for a very rare corner-case: building as root by mistake.\n> > \n> > It's odd to have workarounds here, but nowhere else in the Makefile--\n> > which already fails in this stuation, starting from\n> > Documentation/technical/.\n> \n> Aren't there tons more that you end up removing in the next patch? E.g.:\n> \n> doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl\n> - $(QUIET_GEN)$(RM) $@+ $@ && \\\n> - $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n> - mv $@+ $@\n> + $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)\n> \n> That does differ in that it removes $@+, too, but the premise is the\n> same (we know that $@+ could not be a problem, as we're about to\n> clobber it anyway).\n> \n> I'm OK with getting rid of all of them, but it seems like it ought to be\n> happening all in this patch.\n\nYeah, but the rationale is different.\n\n 1. $(RM) $@: these remove the target file because of permissions\n (i.e. root owned)\n 2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds\n\nTo get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,\nto get rid of #1 we don't, we can just do it.\n\n-- \nFelipe Contreras\n"},{"id":"424717","messageId":"YKIu/DUfmU8JbDY2@coredump.intra.peff.net","threadId":"55689","inReplyTo":"609fb8b5bf799_e173a20897@natae.notmuch","subject":"Re: [PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-17T08:53:16Z","receivedAt":"2021-05-17T08:53:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 15, 2021 at 07:04:05AM -0500, Felipe Contreras wrote:\n\n> Jeff King wrote:\n> > On Fri, May 14, 2021 at 06:56:29AM -0500, Felipe Contreras wrote:\n> > \n> > > Commits 50cff52f1a (When generating manpages, delete outdated targets\n> > > first., 2007-08-02) and f9286765b2 (Documentation/Makefile: remove\n> > > cmd-list.made before redirecting to it., 2007-08-06) created these rm\n> > > instances for a very rare corner-case: building as root by mistake.\n> > > \n> > > It's odd to have workarounds here, but nowhere else in the Makefile--\n> > > which already fails in this stuation, starting from\n> > > Documentation/technical/.\n> > \n> > Aren't there tons more that you end up removing in the next patch? E.g.:\n> > \n> > doc.dep : $(docdep_prereqs) $(DOC_DEP_TXT) build-docdep.perl\n> > - $(QUIET_GEN)$(RM) $@+ $@ && \\\n> > - $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n> > - mv $@+ $@\n> > + $(QUIET_GEN)$(PERL_PATH) ./build-docdep.perl >$@ $(QUIET_STDERR)\n> > \n> > That does differ in that it removes $@+, too, but the premise is the\n> > same (we know that $@+ could not be a problem, as we're about to\n> > clobber it anyway).\n> > \n> > I'm OK with getting rid of all of them, but it seems like it ought to be\n> > happening all in this patch.\n> \n> Yeah, but the rationale is different.\n> \n> 1. $(RM) $@: these remove the target file because of permissions\n> (i.e. root owned)\n> 2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds\n> \n> To get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,\n> to get rid of #1 we don't, we can just do it.\n\nTo get rid of the \"mv\", you need something like .DELETE_ON_ERROR. But\nthe \"rm\" in the second case has nothing to do with interrupted builds.\nIt is is doing the same nothing that the ones you are getting rid of\nhere are.\n\nI.e., I was suggesting to get rid of the \"rm\" call in the hunk I showed\nabove, but leave the \"mv\" for the follow-on patch.\n\n-Peff\n"},{"id":"424722","messageId":"60a24897afda0_126a0520834@natae.notmuch","threadId":"55689","inReplyTo":"YKIu/DUfmU8JbDY2@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-05-17T10:42:31Z","receivedAt":"2021-05-17T10:42:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Sat, May 15, 2021 at 07:04:05AM -0500, Felipe Contreras wrote:\n> > Jeff King wrote:\n\n> > > That does differ in that it removes $@+, too, but the premise is the\n> > > same (we know that $@+ could not be a problem, as we're about to\n> > > clobber it anyway).\n> > > \n> > > I'm OK with getting rid of all of them, but it seems like it ought to be\n> > > happening all in this patch.\n> > \n> > Yeah, but the rationale is different.\n> > \n> > 1. $(RM) $@: these remove the target file because of permissions\n> > (i.e. root owned)\n> > 2. $(RM) $@+ $@ && $(CODE) && mv $@+ $@: these are for interrupted builds\n> > \n> > To get rid of #2 we need an alternative solution, like .DELETE_ON_ERROR,\n> > to get rid of #1 we don't, we can just do it.\n> \n> To get rid of the \"mv\", you need something like .DELETE_ON_ERROR. But\n> the \"rm\" in the second case has nothing to do with interrupted builds.\n> It is is doing the same nothing that the ones you are getting rid of\n> here are.\n> \n> I.e., I was suggesting to get rid of the \"rm\" call in the hunk I showed\n> above, but leave the \"mv\" for the follow-on patch.\n\nAhh, I see. It's quite a bit more work, but sure, I can do that.\n\n-- \nFelipe Contreras\n"},{"id":"424725","messageId":"YKJWFFTUjaQ8Eivm@coredump.intra.peff.net","threadId":"55689","inReplyTo":"60a24897afda0_126a0520834@natae.notmuch","subject":"Re: [PATCH v2 3/5] doc: remove unnecessary rm instances","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-17T11:40:04Z","receivedAt":"2021-05-17T11:40:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 17, 2021 at 05:42:31AM -0500, Felipe Contreras wrote:\n\n> > I.e., I was suggesting to get rid of the \"rm\" call in the hunk I showed\n> > above, but leave the \"mv\" for the follow-on patch.\n> \n> Ahh, I see. It's quite a bit more work, but sure, I can do that.\n\nIf it's a lot more work, I can live with the split as you have here. I\ndidn't think it would be a lot, though.\n\n-Peff\n"}]}