| Message ID | 20210514115631.503276-4-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | doc: asciidoc cleanups | expand |
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
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.
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
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.
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
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) $@+ $@ && \
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(-)