From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: got rm trim dirs
To: gameoftrees@openbsd.org
Date: Wed, 4 Mar 2020 08:43:24 -0700

On Wed, Mar 04, 2020 at 04:12:40PM +0100, Stefan Sperling wrote:
> 
> Indeed, this make it hard to reuse remove_ondisk_file() as it is.
> We could move the unlinkat() into remove_ondisk_file() and pass dirfd
> in as a new parameter, and check dirfd for -1 inside remove_ondisk_file().
> 
> The point of unlinkat is that the correct files get reported by 'got status'
> and removed by 'got rm', if the directory we're operating within happens to
> be deleted and recreated concurrently to the status crawl.
> 
> With unlink() via a path computed from parentdir + filename, we cannot be 100%
> sure that the file is really a child of the directory we opened at parentdir's
> path. Instead of a path, unlinkat() uses a file descriptor which represents
> the parent directory. This descriptor remains valid even if the directory's
> path gets deleted and replaced in the meantime.

How does this look, then. Did I approach this correctly? There is also
some added error checking in remove_ondisk_file.

-- 

Tracey Emery

diff ee85c5e898e10f72841c918d9f453a6526ef7e2e /home/basepr1me/src/got
blob - 0e0bef70fa58ed856ec28833fae027d8744acaae
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1391,29 +1391,47 @@ done:
 }
 
 static const struct got_error *
-remove_ondisk_file(const char *root_path, const char *path)
+remove_ondisk_file(const char *root_path, const char *path, int dirfd,
+    const char *de_name)
 {
 	const struct got_error *err = NULL;
-	char *ondisk_path = NULL;
+	char *ondisk_path = NULL, *parent = NULL;
 
 	if (asprintf(&ondisk_path, "%s/%s", root_path, path) == -1)
 		return got_error_from_errno("asprintf");
 
-	if (unlink(ondisk_path) == -1) {
-		if (errno != ENOENT)
+	if (dirfd != -1) {
+		if (unlinkat(dirfd, de_name, 0) != 0) {
+			err = got_error_from_errno2("unlinkat", ondisk_path);
+			goto done;
+		}
+	} else if (unlink(ondisk_path) == -1) {
+		if (errno != ENOENT) {
 			err = got_error_from_errno2("unlink", ondisk_path);
-	} else {
-		char *parent = dirname(ondisk_path);
-		while (parent && strcmp(parent, root_path) != 0) {
-			if (rmdir(parent) == -1) {
-				if (errno != ENOTEMPTY)
-					err = got_error_from_errno2("rmdir",
-					    parent);
-				break;
-			}
-			parent = dirname(parent);
+			goto done;
 		}
 	}
+
+	parent = dirname(ondisk_path);
+	if (parent == NULL) {
+		err = got_error_from_errno2("dirname", ondisk_path);
+		goto done;
+	}
+
+	while (parent && strcmp(parent, root_path) != 0) {
+		if (rmdir(parent) == -1) {
+			if (errno != ENOTEMPTY)
+				err = got_error_from_errno2("rmdir",
+				    parent);
+			break;
+		}
+		parent = dirname(parent);
+		if (parent == NULL) {
+			err = got_error_from_errno2("dirname", ondisk_path);
+			goto done;
+		}
+	}
+done:
 	free(ondisk_path);
 	return err;
 }
@@ -1457,7 +1475,8 @@ delete_blob(struct got_worktree *worktree, struct got_
 		if (err)
 			return err;
 		if (status == GOT_STATUS_NO_CHANGE) {
-			err = remove_ondisk_file(worktree->root_path, ie->path);
+			err = remove_ondisk_file(worktree->root_path, ie->path,
+			    -1, NULL);
 			if (err)
 				return err;
 		}
@@ -2127,7 +2146,8 @@ merge_file_cb(void *arg, struct got_blob_object *blob1
 			    GOT_STATUS_DELETE, path1);
 			if (err)
 				goto done;
-			err = remove_ondisk_file(a->worktree->root_path, path1);
+			err = remove_ondisk_file(a->worktree->root_path, path1,
+			    -1, NULL);
 			if (err)
 				goto done;
 			if (ie)
@@ -3029,16 +3049,10 @@ schedule_for_deletion(void *arg, unsigned char status,
 	}
 
 	if (!a->keep_on_disk && status != GOT_STATUS_MISSING) {
-		if (dirfd != -1) {
-			if (unlinkat(dirfd, de_name, 0) != 0) {
-				err = got_error_from_errno2("unlinkat",
-				    ondisk_path);
-				goto done;
-			}
-		} else if (unlink(ondisk_path) != 0) {
-			err = got_error_from_errno2("unlink", ondisk_path);
+		err = remove_ondisk_file(a->worktree->root_path, relpath, dirfd,
+		    de_name);
+		if (err)
 			goto done;
-		}
 	}
 
 	got_fileindex_entry_mark_deleted_from_disk(ie);
blob - fdeae68f86183c12278162f1e566e5eccf587652
file + regress/cmdline/revert.sh
--- regress/cmdline/revert.sh
+++ regress/cmdline/revert.sh
@@ -990,6 +990,69 @@ function test_revert_added_subtree {
 	test_done "$testroot" "$ret"
 }
 
+function test_revert_deleted_subtree {
+	local testroot=`test_init revert_deleted_subtree`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	mkdir -p $testroot/wt/epsilon/foo/bar/baz
+	mkdir -p $testroot/wt/epsilon/foo/bar/bax
+	echo "new file" > $testroot/wt/epsilon/foo/a.o
+	echo "new file" > $testroot/wt/epsilon/foo/a.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/b.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/b.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/f.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/f.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/c.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/c.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/e.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/e.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/x.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/x.d
+	(cd $testroot/wt && got add -R epsilon >/dev/null)
+	(cd $testroot/wt && got commit -m "add subtree" >/dev/null)
+
+	# now delete and revert the entire subtree
+	(cd $testroot/wt && got rm -R epsilon/foo >/dev/null)
+
+	echo "R  epsilon/foo/a.o" > $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/b.d" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/b.o" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/bax/e.d" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/bax/e.o" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/bax/x.d" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/bax/x.o" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/baz/c.d" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/baz/c.o" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/baz/f.d" >> $testroot/stdout.expected
+	echo "R  epsilon/foo/bar/baz/f.o" >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got revert -R . > $testroot/stdout)
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo -n > $testroot/stdout.expected
+	(cd $testroot/wt && got status > $testroot/stdout)
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 run_test test_revert_basic
 run_test test_revert_rm
 run_test test_revert_add
@@ -1003,3 +1066,4 @@ run_test test_revert_patch_added
 run_test test_revert_patch_removed
 run_test test_revert_patch_one_change
 run_test test_revert_added_subtree
+run_test test_revert_deleted_subtree
blob - 9ea8be8b1e091a6c1974b3a81b2f05a550bcd266
file + regress/cmdline/rm.sh
--- regress/cmdline/rm.sh
+++ regress/cmdline/rm.sh
@@ -239,6 +239,18 @@ function test_rm_directory {
 		return 1
 	fi
 
+	(cd $testroot/wt && ls -l > $testroot/stdout)
+
+	echo -n '' > $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	test_done "$testroot" "$ret"
 }
 
@@ -322,9 +334,68 @@ function test_rm_directory_keep_files {
 	test_done "$testroot" "$ret"
 }
 
+function test_rm_subtree {
+	local testroot=`test_init rm_subtree`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	mkdir -p $testroot/wt/epsilon/foo/bar/baz
+	mkdir -p $testroot/wt/epsilon/foo/bar/bax
+	echo "new file" > $testroot/wt/epsilon/foo/a.o
+	echo "new file" > $testroot/wt/epsilon/foo/a.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/b.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/b.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/f.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/f.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/c.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/baz/c.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/e.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/e.d
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/x.o
+	echo "new file" > $testroot/wt/epsilon/foo/bar/bax/x.d
+	(cd $testroot/wt && got add -R epsilon >/dev/null)
+	(cd $testroot/wt && got commit -m "add subtree" >/dev/null)
+
+	# now delete and revert the entire subtree
+	(cd $testroot/wt && got rm -R epsilon/foo >/dev/null)
+
+	if [ -d $testroot/wt/epsilon/foo ]; then
+		echo "removed dir epsilon/foo still exists on disk" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	echo "D  epsilon/foo/a.o" > $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/b.d" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/b.o" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/bax/e.d" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/bax/e.o" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/bax/x.d" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/bax/x.o" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/baz/c.d" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/baz/c.o" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/baz/f.d" >> $testroot/stdout.expected
+	echo "D  epsilon/foo/bar/baz/f.o" >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got status > $testroot/stdout)
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 run_test test_rm_basic
 run_test test_rm_with_local_mods
 run_test test_double_rm
 run_test test_rm_and_add_elsewhere
 run_test test_rm_directory
 run_test test_rm_directory_keep_files
+run_test test_rm_subtree