threads / patch / 4290

patchDon't write directly to a make target ($@).

Subject: [PATCH] Don't write directly to a make target ($@).

## tl;dr

6 messages between May 25, 2006 and May 25, 2006. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jim Meyering· May 25, 2006, 13:32 UTC · lore

Otherwise, if make is suspended, or killed with prejudice, or if the system crashes, you could be left with an up-to-date, yet corrupt, generated file.

---
 Makefile |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)
59fd5cb51824364100cacd92f1ca4674853a13a8
Show changes to Makefile +20 −14
diff --git a/Makefile b/Makefile
index dbf19c6..3af3187 100644
--- a/Makefile
+++ b/Makefile
@@ -496,37 +496,43 @@ builtin-help.o: common-cmds.h
 	rm -f $@ && ln git$X $@
 
 common-cmds.h: Documentation/git-*.txt
-	./generate-cmdlist.sh > $@
+	./generate-cmdlist.sh > t$@
+	mv t$@ $@
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	rm -f $@
+	rm -f $@ t$@
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
 	    -e 's/@@NO_PYTHON@@/$(NO_PYTHON)/g' \
-	    $@.sh >$@
-	chmod +x $@
+	    $@.sh >t$@
+	chmod +x t$@
+	mv t$@ $@
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)) : % : %.perl
-	rm -f $@
+	rm -f $@ t$@
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    $@.perl >$@
-	chmod +x $@
+	    $@.perl >t$@
+	chmod +x t$@
+	mv t$@ $@
 
 $(patsubst %.py,%,$(SCRIPT_PYTHON)) : % : %.py
-	rm -f $@
+	rm -f $@ t$@
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    -e 's|@@GIT_PYTHON_PATH@@|$(GIT_PYTHON_DIR_SQ)|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    $@.py >$@
-	chmod +x $@
+	    $@.py >t$@
+	chmod +x t$@
+	mv t$@ $@
 
 git-cherry-pick: git-revert
-	cp $< $@
+	cp $< t$@
+	mv t$@ $@
 
 git-status: git-commit
-	cp $< $@
+	cp $< t$@
+	mv t$@ $@
 
 # These can record GIT_VERSION
 git$X git.spec \
@@ -653,7 +659,8 @@ install-doc:
 ### Maintainer's dist rules
 
 git.spec: git.spec.in
-	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@
+	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > t$@
+	mv t$@ $@
 
 GIT_TARNAME=git-$(GIT_VERSION)
 dist: git.spec git-tar-tree
@@ -724,4 +731,3 @@ check-docs::
 		*) echo "no link: $$v";; \
 		esac ; \
 	done | sort
-

-- 
1.3.2
Junio C Hamano· May 25, 2006, 16:28 UTC · re: Jim Meyering · lore

Re: [PATCH] Don't write directly to a make target ($@).

Jim Meyering <jim@meyering.net> writes:
> Otherwise, if make is suspended, or killed with prejudice, or if the
> system crashes, you could be left with an up-to-date, yet corrupt,
> generated file.

Thanks. Maybe you would want a "make clean" target for them too if you do this. I often use $@+ instead of t$@ so that I can say "rm -f *+" there.

Show 8 quoted lines
> @@ -496,37 +496,43 @@ builtin-help.o: common-cmds.h
>  	rm -f $@ && ln git$X $@
>  
>  common-cmds.h: Documentation/git-*.txt
> -	./generate-cmdlist.sh > $@
> +	./generate-cmdlist.sh > t$@
> +	mv t$@ $@
>  
IOW, like this:
common-cmds.h: Documentation/git-*.txt
	rm -f $@+ $@
        ./generate-cmdlist.sh > $@+
        mv $@+ $@
clean::
	rm -f *+
Timo Hirvonen· May 25, 2006, 16:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] Don't write directly to a make target ($@).

Junio C Hamano <junkio@cox.net> wrote:
Show 28 quoted lines
> Jim Meyering <jim@meyering.net> writes:
> 
> > Otherwise, if make is suspended, or killed with prejudice, or if the
> > system crashes, you could be left with an up-to-date, yet corrupt,
> > generated file.
> 
> Thanks.  Maybe you would want a "make clean" target for them too
> if you do this.  I often use $@+ instead of t$@ so that I can
> say "rm -f *+" there.
> 
> > @@ -496,37 +496,43 @@ builtin-help.o: common-cmds.h
> >  	rm -f $@ && ln git$X $@
> >  
> >  common-cmds.h: Documentation/git-*.txt
> > -	./generate-cmdlist.sh > $@
> > +	./generate-cmdlist.sh > t$@
> > +	mv t$@ $@
> >  
> 
> IOW, like this:
> 
> common-cmds.h: Documentation/git-*.txt
> 	rm -f $@+ $@
>         ./generate-cmdlist.sh > $@+
>         mv $@+ $@
> 
> clean::
> 	rm -f *+
Or just use one tmp file, i.e. ".tmp" instead of t$@.
-- 
http://onion.dynserv.net/~timo/
Jim Meyering· May 25, 2006, 16:42 UTC · re: Timo Hirvonen · lore

Re: [PATCH] Don't write directly to a make target ($@).

Timo Hirvonen <tihirvon@gmail.com> wrote:
> Or just use one tmp file, i.e. ".tmp" instead of t$@.

Argh, no. That'd fail big time with parallel builds.

Jim Meyering· May 25, 2006, 16:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Don't write directly to a make target ($@).

Junio C Hamano <junkio@cox.net> wrote:
Show 9 quoted lines
> Jim Meyering <jim@meyering.net> writes:
>
>> Otherwise, if make is suspended, or killed with prejudice, or if the
>> system crashes, you could be left with an up-to-date, yet corrupt,
>> generated file.
>
> Thanks.  Maybe you would want a "make clean" target for them too
> if you do this.  I often use $@+ instead of t$@ so that I can
> say "rm -f *+" there.

I chose a prefix rather than a suffix, so that if git is built on a file system with unreasonable file name length limitations, the prefix will distinguish the temporary from the target; in that situation, a suffixed temporary name could map to the target name.

However, assuming reasonable file name length limits, using a suffix is generally better, since it works even when the target is an absolute name. Adding a prefix obviously won't work with an absolute name.

I'm happy to ignore 14-byte(and 8.3)-limited file systems and go with a suffix, if you still prefer that. New patch coming up.

Jim Meyering· May 25, 2006, 16:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] Don't write directly to a make target ($@).

Junio C Hamano <junkio@cox.net> wrote:
Show 28 quoted lines
> Jim Meyering <jim@meyering.net> writes:
>
>> Otherwise, if make is suspended, or killed with prejudice, or if the
>> system crashes, you could be left with an up-to-date, yet corrupt,
>> generated file.
>
> Thanks.  Maybe you would want a "make clean" target for them too
> if you do this.  I often use $@+ instead of t$@ so that I can
> say "rm -f *+" there.
>
>> @@ -496,37 +496,43 @@ builtin-help.o: common-cmds.h
>>  	rm -f $@ && ln git$X $@
>>
>>  common-cmds.h: Documentation/git-*.txt
>> -	./generate-cmdlist.sh > $@
>> +	./generate-cmdlist.sh > t$@
>> +	mv t$@ $@
>>
>
> IOW, like this:
>
> common-cmds.h: Documentation/git-*.txt
> 	rm -f $@+ $@
>         ./generate-cmdlist.sh > $@+
>         mv $@+ $@
>
> clean::
> 	rm -f *+
I've included a revised patch below.

I left off the `clean' addition, because I believe "make clean" should not remove wildcard patterns like "*+", on the off-chance that someone uses names like that for files they care about. Besides, in practice, those temporary files are left behind so rarely that they're not a bother, and they're removed again as part of the next build.

---------
Subject: [PATCH] Don't write directly to a make target ($@).  Update atomically.

Otherwise, if make is killed with prejudice, or if the system crashes, you could be left with an up-to-date, yet corrupt generated file.

---
 Makefile |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)
620173525c5075f2056af107a85881d0d50d3a89
Show changes to Makefile +20 −14
diff --git a/Makefile b/Makefile
index dbf19c6..04536f6 100644
--- a/Makefile
+++ b/Makefile
@@ -496,37 +496,43 @@ builtin-help.o: common-cmds.h
 	rm -f $@ && ln git$X $@
 
 common-cmds.h: Documentation/git-*.txt
-	./generate-cmdlist.sh > $@
+	./generate-cmdlist.sh > $@+
+	mv $@+ $@
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	rm -f $@
+	rm -f $@ $@+
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
 	    -e 's/@@NO_PYTHON@@/$(NO_PYTHON)/g' \
-	    $@.sh >$@
-	chmod +x $@
+	    $@.sh >$@+
+	chmod +x $@+
+	mv $@+ $@
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)) : % : %.perl
-	rm -f $@
+	rm -f $@ $@+
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    $@.perl >$@
-	chmod +x $@
+	    $@.perl >$@+
+	chmod +x $@+
+	mv $@+ $@
 
 $(patsubst %.py,%,$(SCRIPT_PYTHON)) : % : %.py
-	rm -f $@
+	rm -f $@ $@+
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    -e 's|@@GIT_PYTHON_PATH@@|$(GIT_PYTHON_DIR_SQ)|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    $@.py >$@
-	chmod +x $@
+	    $@.py >$@+
+	chmod +x $@+
+	mv $@+ $@
 
 git-cherry-pick: git-revert
-	cp $< $@
+	cp $< $@+
+	mv $@+ $@
 
 git-status: git-commit
-	cp $< $@
+	cp $< $@+
+	mv $@+ $@
 
 # These can record GIT_VERSION
 git$X git.spec \
@@ -653,7 +659,8 @@ install-doc:
 ### Maintainer's dist rules
 
 git.spec: git.spec.in
-	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@
+	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@+
+	mv $@+ $@
 
 GIT_TARNAME=git-$(GIT_VERSION)
 dist: git.spec git-tar-tree
@@ -724,4 +731,3 @@ check-docs::
 		*) echo "no link: $$v";; \
 		esac ; \
 	done | sort
-

-- 
1.3.2

← back to recent threads