From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got rm trim dirs
To: Tracey Emery <tracey@traceyemery.net>
Cc: gameoftrees@openbsd.org
Date: Thu, 5 Mar 2020 10:00:11 +0100

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

Yes, let's go with this for now, I would say. It's easy to see that this
is still one of the racy spots, which can be fixed up later.

> 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
>