From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got rm trim dirs
To: Tracey Emery <tracey@traceyemery.net>
Cc: gameoftrees@openbsd.org
Date: Wed, 4 Mar 2020 09:59:08 +0100

On Tue, Mar 03, 2020 at 04:42:56PM -0700, Tracey Emery wrote:
> Hello,
> 
> Does it make sense to trim directories during a 'got rm -R .'
> opperation? I'm constantly forgetting to remove empty directories
> afterwards.
> 
> Ok?

Comments inline:

> diff ee85c5e898e10f72841c918d9f453a6526ef7e2e /home/basepr1me/src/got
> blob - 0e0bef70fa58ed856ec28833fae027d8744acaae
> file + lib/worktree.c
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -3039,6 +3039,16 @@ schedule_for_deletion(void *arg, unsigned char status,
>  			err = got_error_from_errno2("unlink", ondisk_path);
>  			goto done;
>  		}
> +		char *parent = dirname(ondisk_path);

Please don't declare new variables in the middle of a scope.

> +		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);

Couldn't we simply reuse remove_ondisk_file() instead of hand-rolled unlink +
loop over rmdir?

>  	}
>  
>  	got_fileindex_entry_mark_deleted_from_disk(ie);
> 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"
>  }
 
Test change looks good.

In addition, below is a patch with more elaborate tests with several levels
of removal. The new rm.sh test added here fails without your change, the
revert test passes either way. I was concerned whether revert would still
work with your patch, the answer is yes it does :-)


diff 770579228bbb2327c0b0268282af02979479a666 /home/stsp/src/got
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
@@ -322,9 +322,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