diff mbox series

[v2,3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@"

Message ID patch-3.6-96e2338ed8e-20210329T161723Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series Makefile: don't die on AIX with open ./git | expand

Commit Message

Ævar Arnfjörð Bjarmason March 29, 2021, 4:20 p.m. UTC 
Now that preceding commits have moved the generation of objects and
scripts in the Makefile to use the "[...] -o $@+ && mv $@+ $@" pattern
we can stop removing "$@" and "$@+" before the rule is run.

I suppose that we could leave this at removing "$@" before we start
out, per the "age old convention" comment in[1]. I.e. instead of:

    rm -f thing thing+
    prepare contents for thing >thing+
    mv thing+ thing

Do:

    rm -f thing
    prepare contents for thing >thing+
    mv thing+ thing

Since the removal of "thing+" is redundant as we're about to clobber
it anyway, but we might be so paranoid as to be guarding against mv(1)
leaving behind a corrupt "thing".

But I think guarding against "mv" failing is a step too far in
paranoia, let's just rely on the "[...] -o $@+ && mv $@+ $@" pattern
working instead.

Especially as we'll see in a follow-up commit, we're just about to use
the GNU make ".DELETE_ON_ERROR" feature, which will reliably do this
for us.

1. http://lore.kernel.org/git/xmqqpn097e9o.fsf@gitster.c.googlers.com

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Junio C Hamano March 29, 2021, 6:46 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>     rm -f thing thing+
>     prepare contents for thing >thing+
>     mv thing+ thing
> ...
> But I think guarding against "mv" failing is a step too far in
> paranoia,...

If mv fails the $(MAKE) rule would fail, so it is OK.  It may leave
thing+ behind, but there is no reason to expect why you would be
able to rm it the next time, so from that point of view, it is OK to
drop the first "rm -f thing+", I would think.  The only case I can
thing of that would help is when you are sharing the working tree
with your team member, the directories are writable to both of you,
but somehow the other person creates thing+ with 0644 or 0755 mode
bits.  You cannot redirect into thing+ the other person left behind,
but you can "rm -f thing+ && cmd >$thing+" (or "cmd -o $thing+") in
such a situation, and that is probably where the pattern comes from
---i.e. simple hygiene.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index ef2d4a9973b..f08635067b3 100644
--- a/Makefile
+++ b/Makefile
@@ -2210,7 +2210,6 @@  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
 	$(perllibdir_SQ)
 define cmd_munge_script
-$(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
@@ -2278,8 +2277,7 @@  endif
 PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
 
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1{' \
+	$(QUIET_GEN)sed -e '1{' \
 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e '	r GIT-PERL-HEADER' \
 	    -e '	G' \
@@ -2299,8 +2297,7 @@  GIT-PERL-DEFINES: FORCE
 	    fi
 
 GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
-	$(QUIET_GEN)$(RM) $@ && \
-	INSTLIBDIR='$(perllibdir_SQ)' && \
+	$(QUIET_GEN)INSTLIBDIR='$(perllibdir_SQ)' && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
 	sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
@@ -2325,8 +2322,7 @@  git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	mv $@+ $@
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
@@ -2339,15 +2335,13 @@  $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
 ifndef NO_PYTHON
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
+	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \