From 6d767e2a655869d0b28e0b358bc6c0c3947cae2d Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@gmail.com>
Date: Mon, 22 Nov 2010 04:40:54 -0800
Subject: [PATCH] user-friendliness sanity checks: catch common mistakes
 regarding $1/$2/$3.

.do files should never modify $1, and should write to *either* $3 or stdout,
but not both.  If they write to both, it's probably because they forgot to
redirect stdout to stderr, a very easy mistake to make but a hard one to
detect.

Now redo detects it for you and prints an informative message.
---
 builder.py           | 33 +++++++++++++++++++++++++++++++--
 t/deps/.gitignore    |  2 ++
 t/deps/clean.do      |  3 ++-
 t/deps/overwrite.do  |  4 ++++
 t/deps/overwrite1.do |  2 ++
 t/deps/overwrite2.do |  5 +++++
 t/deps/overwrite3.do |  8 ++++++++
 t/deps/test.do       |  2 +-
 8 files changed, 55 insertions(+), 4 deletions(-)
 create mode 100644 t/deps/overwrite.do
 create mode 100644 t/deps/overwrite1.do
 create mode 100644 t/deps/overwrite2.do
 create mode 100644 t/deps/overwrite3.do

diff --git a/builder.py b/builder.py
index ef6319a..e21478a 100644
--- a/builder.py
+++ b/builder.py
@@ -1,4 +1,4 @@
-import sys, os, random
+import sys, os, random, errno
 import vars, jwack, state
 from helpers import log, log_, debug2, err, unlink, close_on_exec
 
@@ -31,6 +31,16 @@ def _nice(t):
     return os.path.normpath(os.path.join(vars.PWD, t))
 
 
+def _try_stat(filename):
+    try:
+        return os.stat(filename)
+    except OSError, e:
+        if e.errno == errno.ENOENT:
+            return None
+        else:
+            raise
+
+
 class BuildJob:
     def __init__(self, t, lock, shouldbuildfunc, donefunc):
         self.t = t
@@ -38,6 +48,7 @@ class BuildJob:
         self.lock = lock
         self.shouldbuildfunc = shouldbuildfunc
         self.donefunc = donefunc
+        self.before_t = _try_stat(self.t)
 
     def start(self):
         assert(self.lock.owned)
@@ -96,13 +107,30 @@ class BuildJob:
 
     def _after(self, t, rv):
         try:
-            self._after1(t, rv)
+            rv = self._after1(t, rv)
         finally:
             self._after2(rv)
 
     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)
+        if after_t != before_t:
+            err('%r modified %r directly!\n' % (self.argv[2], t))
+            err('...you should update $3 (a temp file) instead of $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])
+            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
@@ -121,6 +149,7 @@ class BuildJob:
         else:
             if vars.VERBOSE or vars.XTRACE:
                 log('%s (done)\n\n' % _nice(t))
+        return rv
 
     def _after2(self, rv):
         try:
diff --git a/t/deps/.gitignore b/t/deps/.gitignore
index 9463dda..efb409b 100644
--- a/t/deps/.gitignore
+++ b/t/deps/.gitignore
@@ -1,2 +1,4 @@
 t1a
 t2.count
+overwrite
+overwrite[123]
diff --git a/t/deps/clean.do b/t/deps/clean.do
index 7898c96..b18296c 100644
--- a/t/deps/clean.do
+++ b/t/deps/clean.do
@@ -1 +1,2 @@
-rm -f *~ .*~ *.count t1a
+rm -f *~ .*~ *.count t1a overwrite overwrite[123]
+
diff --git a/t/deps/overwrite.do b/t/deps/overwrite.do
new file mode 100644
index 0000000..8ae3639
--- /dev/null
+++ b/t/deps/overwrite.do
@@ -0,0 +1,4 @@
+redo overwrite1 2>&1 && exit 55
+redo overwrite2 2>&1 && exit 56
+redo overwrite3 2>&1 && exit 57
+exit 0
diff --git a/t/deps/overwrite1.do b/t/deps/overwrite1.do
new file mode 100644
index 0000000..392f38f
--- /dev/null
+++ b/t/deps/overwrite1.do
@@ -0,0 +1,2 @@
+# this shouldn't be allowed; we're supposed to write to $3, not $1
+echo >$1
diff --git a/t/deps/overwrite2.do b/t/deps/overwrite2.do
new file mode 100644
index 0000000..dca62e5
--- /dev/null
+++ b/t/deps/overwrite2.do
@@ -0,0 +1,5 @@
+# this shouldn't be allowed; stdout is connected to $3 already, so if we
+# replace it *and* write to stdout, we're probably confused.
+echo hello world
+rm -f $3
+echo goodbye world >$3
diff --git a/t/deps/overwrite3.do b/t/deps/overwrite3.do
new file mode 100644
index 0000000..e5643fd
--- /dev/null
+++ b/t/deps/overwrite3.do
@@ -0,0 +1,8 @@
+# we don't delete $3 here, we just truncate and overwrite it.  But redo
+# can detect this by checking the current file position of our stdout when
+# we exit, and making sure it equals either 0 or the file size.
+#
+# If it doesn't, then we accidentally wrote to *both* stdout and a separate
+# file, and we should get warned about it.
+echo hello world
+echo goodbye world >$3
diff --git a/t/deps/test.do b/t/deps/test.do
index e0df597..6a30599 100644
--- a/t/deps/test.do
+++ b/t/deps/test.do
@@ -1,3 +1,3 @@
-redo test1 test2 ifchange-fail
+redo test1 test2 ifchange-fail overwrite