From 59201dd7a0ecc7582e93b1c1c90808ae92cc305d Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@gmail.com>
Date: Sat, 11 Dec 2010 00:29:04 -0800
Subject: [PATCH] $3 and stdout no longer refer to the same file.

This is slightly inelegant, as the old style
	echo foo
	echo blah
	chmod a+x $3

doesn't work anymore; the stuff you wrote to stdout didn't end up in $3.
You can rewrite it as:
	exec >$3
	echo foo
	echo blah
	chmod a+x $3

Anyway, it's better this way, because now we can tell the difference between
a zero-length $3 and a nonexistent one.  A .do script can thus produce
either one and we'll either delete the target or move the empty $3 to
replace it, whichever is right.

As a bonus, this simplifies our detection of whether you did something weird
with overlapping changes to stdout and $3.
---
 builder.py       | 49 ++++++++++++++++++++++++------------------------
 t/.gitignore     |  5 +++++
 t/CC.do          |  1 +
 t/LD.do          |  1 +
 t/clean.do       |  7 ++++---
 t/example/CC.do  |  1 +
 t/silencetest.do |  7 +++++++
 t/test.do        |  3 ++-
 t/touchtest.do   |  9 +++++++++
 9 files changed, 54 insertions(+), 29 deletions(-)
 create mode 100644 t/silencetest.do
 create mode 100644 t/touchtest.do

diff --git a/builder.py b/builder.py
index 8f34c59..eb5eb2f 100644
--- a/builder.py
+++ b/builder.py
@@ -56,7 +56,8 @@ class BuildJob:
     def __init__(self, t, sf, lock, shouldbuildfunc, donefunc):
         self.t = t  # original target name, not relative to vars.BASE
         self.sf = sf
-        self.tmpname = '%s.redo.tmp' % t
+        self.tmpname1 = '%s.redo1.tmp' % t
+        self.tmpname2 = '%s.redo2.tmp' % t
         self.lock = lock
         self.shouldbuildfunc = shouldbuildfunc
         self.donefunc = donefunc
@@ -66,7 +67,6 @@ class BuildJob:
         assert(self.lock.owned)
         t = self.t
         sf = self.sf
-        tmpname = self.tmpname
         try:
             if not self.shouldbuildfunc(t):
                 # target doesn't need to be built; skip the whole task
@@ -107,8 +107,9 @@ class BuildJob:
             else:
                 err('no rule to make %r\n' % t)
                 return self._after2(1)
-        unlink(tmpname)
-        ffd = os.open(tmpname, os.O_CREAT|os.O_RDWR|os.O_EXCL, 0666)
+        unlink(self.tmpname1)
+        unlink(self.tmpname2)
+        ffd = os.open(self.tmpname1, os.O_CREAT|os.O_RDWR|os.O_EXCL, 0666)
         close_on_exec(ffd, True)
         self.f = os.fdopen(ffd, 'w+')
         # this will run in the dofile's directory, so use only basenames here
@@ -116,7 +117,7 @@ class BuildJob:
                 os.path.basename(dofile),
                 os.path.basename(basename),  # target name (extension removed)
                 ext,  # extension (if any), including leading dot
-                os.path.basename(tmpname)  # randomized output file name
+                os.path.basename(self.tmpname2)  # randomized output file name
                 ]
         if vars.VERBOSE: argv[1] += 'v'
         if vars.XTRACE: argv[1] += 'x'
@@ -162,39 +163,37 @@ class BuildJob:
 
     def _after1(self, t, rv):
         f = self.f
-        tmpname = self.tmpname
         before_t = self.before_t
         after_t = _try_stat(t)
-        before_tmp = os.fstat(f.fileno())
-        after_tmp = _try_stat(tmpname)
-        after_where = os.lseek(f.fileno(), 0, os.SEEK_CUR)
+        st1 = os.fstat(f.fileno())
+        st2 = _try_stat(self.tmpname2)
         if after_t != before_t and not stat.S_ISDIR(after_t.st_mode):
-            err('%r modified %r directly!\n' % (self.argv[2], t))
-            err('...you should update $3 (a temp file) instead of $1.\n')
+            err('%s modified %s directly!\n' % (self.argv[2], t))
+            err('...you should update $3 (a temp file) or stdout, not $1.\n')
             rv = 206
-        elif after_tmp and before_tmp != after_tmp and before_tmp.st_size > 0:
-            err('%r wrote to stdout *and* replaced $3.\n' % self.argv[2])
+        elif st2 and st1.st_size > 0:
+            err('%s wrote to stdout *and* created $3.\n' % self.argv[2])
             err('...you should write status messages to stderr, not stdout.\n')
             rv = 207
-        elif after_where > 0 and after_tmp and after_tmp.st_size != after_where:
-            err('%r wrote differing data to stdout and $3.\n' % self.argv[2])
-            err('...you should write status messages to stderr, not stdout.\n')
-            rv = 208
         if rv==0:
-            if os.path.exists(tmpname) and os.stat(tmpname).st_size:
-                # there's a race condition here, but if the tmpfile disappears
-                # at *this* point you deserve to get an error, because you're
-                # doing something totally scary.
-                os.rename(tmpname, t)
-            else:
-                unlink(tmpname)
+            if st2:
+                os.rename(self.tmpname2, t)
+                os.unlink(self.tmpname1)
+            elif st1.st_size > 0:
+                os.rename(self.tmpname1, t)
+                if st2:
+                    os.unlink(self.tmpname2)
+            else: # no output generated at all; that's ok
+                unlink(self.tmpname1)
+                unlink(t)
             sf = self.sf
             sf.is_generated = True
             sf.update_stamp()
             sf.set_changed()
             sf.save()
         else:
-            unlink(tmpname)
+            unlink(self.tmpname1)
+            unlink(self.tmpname2)
             sf = self.sf
             sf.set_failed()
             sf.save()
diff --git a/t/.gitignore b/t/.gitignore
index a746a34..00dd4e8 100644
--- a/t/.gitignore
+++ b/t/.gitignore
@@ -11,3 +11,8 @@ test2.args
 /makedir
 /makedir.log
 /chdir1
+/silence
+/silence.do
+/touch1
+/touch1.do
+/deltest2
diff --git a/t/CC.do b/t/CC.do
index e015c46..3dbc6a2 100644
--- a/t/CC.do
+++ b/t/CC.do
@@ -1,3 +1,4 @@
+exec >$3
 cat <<-EOF
 	gcc -Wall -o /dev/fd/1 -c "\$1"
 EOF
diff --git a/t/LD.do b/t/LD.do
index 87a9314..e116a0b 100644
--- a/t/LD.do
+++ b/t/LD.do
@@ -1,3 +1,4 @@
+exec >$3
 cat <<-EOF
 	OUT="\$1"
 	shift
diff --git a/t/clean.do b/t/clean.do
index 8037f29..01d7920 100644
--- a/t/clean.do
+++ b/t/clean.do
@@ -1,4 +1,5 @@
 redo example/clean curse/clean deps/clean "space dir/clean"
-rm -f c c.c c.c.c c.c.c.b c.c.c.b.b d mode1 makedir.log chdir1
-rm -f hello [by]ellow *.o *~ .*~ CC LD passfail
-rm -rf makedir
\ No newline at end of file
+rm -f c c.c c.c.c c.c.c.b c.c.c.b.b d mode1 makedir.log chdir1 \
+	hello [by]ellow *.o *~ .*~ CC LD passfail silence silence.do \
+	touch1 touch1.do
+rm -rf makedir
diff --git a/t/example/CC.do b/t/example/CC.do
index a036046..abb528c 100644
--- a/t/example/CC.do
+++ b/t/example/CC.do
@@ -1,5 +1,6 @@
 redo-ifchange config.sh
 . ./config.sh
+exec >$3
 cat <<-EOF
 	redo-ifchange \$1.c
 	gcc -MD -MF \$3.deps.tmp -o \$3 -c \$1.c
diff --git a/t/silencetest.do b/t/silencetest.do
new file mode 100644
index 0000000..098272c
--- /dev/null
+++ b/t/silencetest.do
@@ -0,0 +1,7 @@
+echo 'echo hello' >silence.do
+redo silence
+[ -e silence ] || exit 55
+echo 'true' >silence.do
+redo silence
+[ ! -e silence ] || exit 66
+rm -f silence.do
diff --git a/t/test.do b/t/test.do
index 778e556..33b4c79 100644
--- a/t/test.do
+++ b/t/test.do
@@ -1,4 +1,5 @@
 redo-ifchange all
 ./hello >&2
 redo deltest deltest2 test.args test2.args passfailtest chdirtest \
-	curse/test deps/test "space dir/test" modetest makedir2
+	curse/test deps/test "space dir/test" modetest makedir2 \
+	silencetest touchtest
diff --git a/t/touchtest.do b/t/touchtest.do
new file mode 100644
index 0000000..95c6e64
--- /dev/null
+++ b/t/touchtest.do
@@ -0,0 +1,9 @@
+echo 'echo hello' >touch1.do
+redo touch1
+[ -e touch1 ] || exit 55
+rm -f touch1
+echo 'touch $3' >touch1.do
+redo touch1
+[ -e touch1 ] || exit 66
+[ -z "$(cat touch1)" ] || exit 77
+rm -f touch1.do