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:39:25 -0700

On Wed, Mar 04, 2020 at 04:20:06PM -0700, Tracey Emery wrote:
> 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.
> 

And, for expedience, here is the first diff fixed up, if it makes more
sense for now.

-- 

Tracey Emery

diff ee85c5e898e10f72841c918d9f453a6526ef7e2e /home/basepr1me/src/got
blob - 0e0bef70fa58ed856ec28833fae027d8744acaae
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -2992,7 +2992,7 @@ schedule_for_deletion(void *arg, unsigned char status,
 	const struct got_error *err = NULL;
 	struct got_fileindex_entry *ie = NULL;
 	struct stat sb;
-	char *ondisk_path;
+	char *ondisk_path, *parent = NULL;
 
 	ie = got_fileindex_entry_get(a->fileindex, relpath, strlen(relpath));
 	if (ie == NULL)
@@ -3038,6 +3038,26 @@ schedule_for_deletion(void *arg, unsigned char status,
 		} else if (unlink(ondisk_path) != 0) {
 			err = got_error_from_errno2("unlink", ondisk_path);
 			goto done;
+		}
+
+		parent = dirname(ondisk_path);
+
+		if (parent == NULL) {
+			err = got_error_from_errno2("dirname", ondisk_path);
+			goto done;
+		}
+		while (parent && strcmp(parent, a->worktree->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", parent);
+				goto done;
+			}
 		}
 	}
 
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