From 39e017869d0e5aa4b42df2f78bcdf92c1b7f6189 Mon Sep 17 00:00:00 2001
From: Avery Pennarun <apenwarr@gmail.com>
Date: Thu, 13 Dec 2018 12:58:56 +0000
Subject: [PATCH] Ensure correct operation with read-only target dirs and .do
 file dirs.

Although I expect this is rather rare, some people may want to build in
a read-write subdir of a read-only tree.  Other than some confusing
error reporting, this works fine in redo after the recent changes to
temp file handling, but let's add a test to make sure it stays that
way.  The test found a bug in minimal/do, so let's fix that.

Reported-by: Jeff Stearns <jeff.stearns@gmail.com>
---
 minimal/do                |  7 ++++---
 redo/builder.py           | 23 ++++++++++-------------
 t/204-readonly/.gitignore |  1 +
 t/204-readonly/all.do     | 25 +++++++++++++++++++++++++
 t/204-readonly/clean.do   |  4 ++++
 5 files changed, 44 insertions(+), 16 deletions(-)
 create mode 100644 t/204-readonly/.gitignore
 create mode 100644 t/204-readonly/all.do
 create mode 100644 t/204-readonly/clean.do

diff --git a/minimal/do b/minimal/do
index 61c05d6..6a70880 100755
--- a/minimal/do
+++ b/minimal/do
@@ -36,7 +36,7 @@ _dirsplit()
 }
 
 # Like /usr/bin/dirname, but avoids a fork and uses _dirsplit semantics.
-dirname()
+qdirname()
 (
 	_dirsplit "$1"
 	dir=${_dirsplit_dir%/}
@@ -283,7 +283,7 @@ _run_dofile()
 # done.
 _do()
 {
-	local dir="$1" target="$1$2" tmp="$1$2.redo.tmp"
+	local dir="$1" target="$1$2" tmp="$1$2.redo.tmp" tdir=
 	local dopath= dodir= dofile= ext=
 	if [ "$_cmd" = "redo" ] ||
 	    ( [ ! -e "$target" -o -d "$target" ] &&
@@ -309,7 +309,8 @@ _do()
 		target=$(_relpath "$target" "$PWD") || return 98
 		tmp=$(_relpath "$tmp" "$PWD") || return 97
 		base=${target%$ext}
-		[ ! -e "$DO_BUILT" ] || [ ! -d "$(dirname "$target")" ] ||
+		tdir=$(qdirname "$target")
+		[ ! -e "$DO_BUILT" ] || [ ! -w "$tdir/." ] ||
 		: >>"$target.did.tmp"
 		# $qtmp is a temporary file used to capture stdout.
 		# Since it might be accidentally deleted as a .do file
diff --git a/redo/builder.py b/redo/builder.py
index 8cd66c5..c029354 100644
--- a/redo/builder.py
+++ b/redo/builder.py
@@ -374,23 +374,22 @@ class _BuildJob(object):
             # FIXME: race condition here between updating stamp/is_generated
             # and actually renaming the files into place.  There needs to
             # be some kind of two-stage commit, I guess.
-            if st1.st_size > 0:
+            if st1.st_size > 0 and not st2:
                 # script wrote to stdout.  Copy its contents to the tmpfile.
                 unlink(self.tmpname)
                 try:
                     newf = open(self.tmpname, 'w')
-                except OSError, e:
-                    dnt = os.path.dirname(t)
+                except IOError, e:
+                    dnt = os.path.dirname(os.path.abspath(t))
                     if not os.path.exists(dnt):
                         # This could happen, so report a simple error message
                         # that gives a hint for how to fix your .do script.
                         err('%s: target dir %r does not exist!\n' % (t, dnt))
                     else:
-                        # I don't know why this would happen, so raise the
-                        # full exception if it ever does.
-                        err('%s: save stdout to %s: %s\n'
-                            % (t, self.tmpname, e))
-                        raise
+                        # This could happen for, eg. a permissions error on
+                        # the target directory.
+                        err('%s: copy stdout: %s\n' % (t, e))
+                    rv = 209
                 else:
                     self.outfile.seek(0)
                     while 1:
@@ -407,12 +406,10 @@ class _BuildJob(object):
                     # Atomically replace the target file
                     os.rename(self.tmpname, t)
                 except OSError, e:
-                    # Nowadays self.tmpname is in the same directory as
-                    # t, so there's no very good reason for this to
-                    # fail. Thus, raise the full exception if it ever
-                    # does.
+                    # This could happen for, eg. a permissions error on
+                    # the target directory.
                     err('%s: rename %s: %s\n' % (t, self.tmpname, e))
-                    raise
+                    rv = 209
             else: # no output generated at all; that's ok
                 unlink(t)
             sf = self.sf
diff --git a/t/204-readonly/.gitignore b/t/204-readonly/.gitignore
new file mode 100644
index 0000000..eb60835
--- /dev/null
+++ b/t/204-readonly/.gitignore
@@ -0,0 +1 @@
+/rodir
diff --git a/t/204-readonly/all.do b/t/204-readonly/all.do
new file mode 100644
index 0000000..612a6ec
--- /dev/null
+++ b/t/204-readonly/all.do
@@ -0,0 +1,25 @@
+[ -e rodir ] && chmod u+w rodir
+[ -e rodir/rwdir ] && chmod u+w rodir/rwdir
+rm -rf rodir
+mkdir rodir rodir/rwdir
+
+cd rodir
+cat >default.ro1.do <<-EOF
+	chmod u+w "\$(dirname "\$1")"
+	echo 'redir' >\$3
+EOF
+cat >default.ro2.do <<-EOF
+	chmod u+w "\$(dirname "\$1")"
+	echo 'stdout'
+EOF
+
+# Check that:
+#  - redo works when the .do file is in a read-only directory.
+#  - redo works when the target is in a read-only directory that becomes
+#    writable only *after* launching the .do script. (For example, the .do
+#    might mount a new read-write filesystem in an otherwise read-only
+#    tree.)
+chmod a-w . rwdir
+redo rwdir/a.ro1
+chmod a-w . rwdir
+redo rwdir/a.ro2
diff --git a/t/204-readonly/clean.do b/t/204-readonly/clean.do
new file mode 100644
index 0000000..63f30f3
--- /dev/null
+++ b/t/204-readonly/clean.do
@@ -0,0 +1,4 @@
+[ -e rodir ] && chmod u+w rodir
+[ -e rodir/rwdir ] && chmod u+w rodir/rwdir
+rm -rf rodir
+rm -f *~ .*~