From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: got rm trim dirs
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Wed, 4 Mar 2020 16:20:06 -0700

On Wed, Mar 04, 2020 at 09:04:10PM +0100, Stefan Sperling wrote:
> But don't worry about any of that now. This race can be dealt with later.
> Your original worktree.c patch doesn't make anything worse, and it was a
> better starting position for further changes in this direction, I think.

Just one more before we revert to something like the first patch. Using
scandir, we can avoid the race as we iterate the directories. But,
perhaps this operation is too expensive on large dirs, since there would
be a lot of malloc and freeing for each file.

Personally, I think I would opt for a fixed up version of the first
patch to avoid all the memory shuffle, then work on a better total fix.

-- 

Tracey Emery

diff ee85c5e898e10f72841c918d9f453a6526ef7e2e /home/basepr1me/src/got
blob - 0e0bef70fa58ed856ec28833fae027d8744acaae
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1391,29 +1391,74 @@ 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;
+	struct dirent **sd_dent;
+	int dir_i, dir_cnt;
 
 	if (asprintf(&ondisk_path, "%s/%s", root_path, path) == -1)
 		return got_error_from_errno("asprintf");
 
-	if (unlink(ondisk_path) == -1) {
-		if (errno != ENOENT)
+	parent = dirname(ondisk_path);
+	if (parent == NULL) {
+		err = got_error_from_errno2("dirname", ondisk_path);
+		goto done;
+	}
+
+	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) {
+			goto done;
+		}
+	}
+
+	while(parent && strcmp(parent, root_path) != 0) {
+		if (dirfd != -1) {
+			dir_cnt = scandir(parent, &sd_dent, NULL, NULL);
+			if (dir_cnt == -1) {
+				err = got_error_from_errno2("scandir", parent);
+				goto done;
+			}
+			dir_cnt -= 2; /* account for self and parent */
+			if (dir_cnt != 0) {
+				for (dir_i = 0; dir_i < dir_cnt; dir_i++)
+					free(sd_dent[dir_i]);
+				free(sd_dent);
+				break;
+			}
+			if (dir_cnt == 0 && strcmp(parent, root_path) == 0) {
+				free(sd_dent);
+				break;
+			}
+			if (unlinkat(dirfd, parent, AT_REMOVEDIR) != 0) {
+				err = got_error_from_errno2("unlinkat", parent);
+				goto done;
+			}
+			free(sd_dent);
+		} else {
 			if (rmdir(parent) == -1) {
 				if (errno != ENOTEMPTY)
 					err = got_error_from_errno2("rmdir",
 					    parent);
 				break;
 			}
-			parent = dirname(parent);
 		}
+		parent = dirname(parent);
+		if (parent == NULL) {
+			err = got_error_from_errno2("dirname",
+			    ondisk_path);
+			goto done;
+		}
 	}
+done:
 	free(ondisk_path);
 	return err;
 }
@@ -1457,7 +1502,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 +2173,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 +3076,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,30 @@ 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
+
+	(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 +346,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