From: Stefan Sperling <stsp@stsp.name>
Subject: fix 'got remove' for paths missing on disk
To: gameoftrees@openbsd.org
Date: Mon, 24 Jan 2022 10:11:38 +0100

gonzalo found that trying to remove a non-existent directory results
in a "bad path" error:

	got: my/missing/dir: bad path

The error message is misleading. "bad path" implies the program somehow
ended up with non-path data where a path was expected, based on user
input or otherwise. Which is not the case here.

The patch below makes this case fail with ENOENT instead:

	got: my/missing/dir: No such file or directory

And also extends the -f option to avoid an error in this case, which
mirrors how unix rm(1) behaves.

This patch makes a small behaviour change for paths recorded in the file
index but missing on disk (status !): 'got rm' would previously succeed
on such files, but now requires -f to succeed. And, of course, no error
will be raised if the user asked for such files to be deleted with -S'!',
regardless of -f.

Behaviour for paths already in deleted status is unchanged: Deleting a
path already in status D still reports success.

With this out of the way, we can make 'got rm' report "unexpected status"
instead of "bad path" for paths which are unversioned (i.e. paths missing
from the file index). This is consistent with how other commands behave.

There remain more problems to be fixed. For example, recursive deletion of
a directory errors out if an unversioned path exists within, and adding
-f to the command does not override this error even though one might
reasonably expect that it would. This is not a new problem introduced
with this patch, so I am ignoring it for now.

ok?

diff refs/heads/main refs/heads/rm-f
blob - 1ba9101f0a27f2b37c7a01f8d02db40c5c95a9fa
blob + eaafde8411ebca559ac74b4970f3a9ee755e26b9
--- got/got.1
+++ got/got.1
@@ -1259,7 +1259,10 @@ The options for
 are as follows:
 .Bl -tag -width Ds
 .It Fl f
-Perform the operation even if a file contains local modifications.
+Perform the operation even if a file contains local modifications,
+and do not raise an error if a specified
+.Ar path
+does not exist on disk.
 .It Fl k
 Keep affected files on disk.
 .It Fl R
blob - f551f71ac97f147d51b0c32e92dce6513394be94
blob + 16b939dba60d2210e6c75b2ffbb51a4b1f8d608a
--- got/got.c
+++ got/got.c
@@ -6981,6 +6981,7 @@ cmd_remove(int argc, char *argv[])
 	struct got_pathlist_head paths;
 	struct got_pathlist_entry *pe;
 	int ch, delete_local_mods = 0, can_recurse = 0, keep_on_disk = 0, i;
+	int ignore_missing_paths = 0;
 
 	TAILQ_INIT(&paths);
 
@@ -6988,6 +6989,7 @@ cmd_remove(int argc, char *argv[])
 		switch (ch) {
 		case 'f':
 			delete_local_mods = 1;
+			ignore_missing_paths = 1;
 			break;
 		case 'k':
 			keep_on_disk = 1;
@@ -7002,6 +7004,7 @@ cmd_remove(int argc, char *argv[])
 					delete_local_mods = 1;
 					break;
 				case GOT_STATUS_MISSING:
+					ignore_missing_paths = 1;
 					break;
 				default:
 					errx(1, "invalid status code '%c'",
@@ -7084,7 +7087,7 @@ cmd_remove(int argc, char *argv[])
 
 	error = got_worktree_schedule_delete(worktree, &paths,
 	    delete_local_mods, status_codes, print_remove_status, NULL,
-	    repo, keep_on_disk);
+	    repo, keep_on_disk, ignore_missing_paths);
 done:
 	if (repo) {
 		const struct got_error *close_err = got_repo_close(repo);
blob - 2402196b13cc52e6f489e9fd9d985074fa7dcbfa
blob + 2a33834a903f96a6f0206be4636193177a5f443d
--- include/got_worktree.h
+++ include/got_worktree.h
@@ -194,7 +194,7 @@ const struct got_error *got_worktree_schedule_add(stru
 const struct got_error *
 got_worktree_schedule_delete(struct got_worktree *,
     struct got_pathlist_head *, int, const char *,
-    got_worktree_delete_cb, void *, struct got_repository *, int);
+    got_worktree_delete_cb, void *, struct got_repository *, int, int);
 
 /* A callback function which is used to select or reject a patch. */
 typedef const struct got_error *(*got_worktree_patch_cb)(int *, void *,
blob - 03ed920c296768d481852c8a28ea3acae1f379e1
blob + 02ace087b87f181bf1e2f38cb278ab69608602f7
--- lib/worktree.c
+++ lib/worktree.c
@@ -3954,6 +3954,7 @@ struct schedule_deletion_args {
 	struct got_repository *repo;
 	int delete_local_mods;
 	int keep_on_disk;
+	int ignore_missing_paths;
 	const char *status_codes;
 };
 
@@ -3969,9 +3970,15 @@ schedule_for_deletion(void *arg, unsigned char status,
 	struct stat sb;
 	char *ondisk_path;
 
+	if (status == GOT_STATUS_NONEXISTENT) {
+		if (a->ignore_missing_paths)
+			return NULL;
+		return got_error_set_errno(ENOENT, relpath);
+	}
+
 	ie = got_fileindex_entry_get(a->fileindex, relpath, strlen(relpath));
 	if (ie == NULL)
-		return got_error_path(relpath, GOT_ERR_BAD_PATH);
+		return got_error_path(relpath, GOT_ERR_FILE_STATUS);
 
 	staged_status = get_staged_status(ie);
 	if (staged_status != GOT_STATUS_NO_CHANGE) {
@@ -4018,6 +4025,10 @@ schedule_for_deletion(void *arg, unsigned char status,
 			err = got_error_path(relpath, GOT_ERR_FILE_MODIFIED);
 			goto done;
 		}
+		if (status == GOT_STATUS_MISSING && !a->ignore_missing_paths) {
+			err = got_error_set_errno(ENOENT, relpath);
+			goto done;
+		}
 		if (status != GOT_STATUS_MODIFY &&
 		    status != GOT_STATUS_MISSING) {
 			err = got_error_path(relpath, GOT_ERR_FILE_STATUS);
@@ -4073,7 +4084,7 @@ got_worktree_schedule_delete(struct got_worktree *work
     struct got_pathlist_head *paths, int delete_local_mods,
     const char *status_codes,
     got_worktree_delete_cb progress_cb, void *progress_arg,
-    struct got_repository *repo, int keep_on_disk)
+    struct got_repository *repo, int keep_on_disk, int ignore_missing_paths)
 {
 	struct got_fileindex *fileindex = NULL;
 	char *fileindex_path = NULL;
@@ -4096,6 +4107,7 @@ got_worktree_schedule_delete(struct got_worktree *work
 	sda.repo = repo;
 	sda.delete_local_mods = delete_local_mods;
 	sda.keep_on_disk = keep_on_disk;
+	sda.ignore_missing_paths = ignore_missing_paths;
 	sda.status_codes = status_codes;
 
 	TAILQ_FOREACH(pe, paths, entry) {
blob - cab753f3139225f2518ae82f6ba928625c46e9e6
blob + f82d2c0f4b893ae2e6c20f40ea0f892622559bec
--- regress/cmdline/rm.sh
+++ regress/cmdline/rm.sh
@@ -159,8 +159,51 @@ test_rm_and_add_elsewhere() {
 		return 1
 	fi
 
+	(cd $testroot/wt && got rm alpha > $testroot/stdout 2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got rm command succeeded unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	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
+
+	echo "got: alpha: No such file or directory" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got rm -f alpha > $testroot/stdout)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got rm command failed unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	echo 'D  alpha' > $testroot/stdout.expected
-	(cd $testroot/wt && got rm alpha > $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
 
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
@@ -170,6 +213,66 @@ test_rm_and_add_elsewhere() {
 		return 1
 	fi
 
+	# While here, test behaviour of rm on files in unversioned status.
+	(cd $testroot/wt && got rm epsilon/alpha > $testroot/stdout \
+		2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got rm command succeeded unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	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
+
+	echo "got: epsilon/alpha: file has unexpected status" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	# And test the same case with -f.
+	(cd $testroot/wt && got rm -f epsilon/alpha > $testroot/stdout \
+		2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got rm command succeeded unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	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
+
+	echo "got: epsilon/alpha: file has unexpected status" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	echo 'A  epsilon/alpha' > $testroot/stdout.expected
 	(cd $testroot/wt && got add epsilon/alpha > $testroot/stdout)
 
@@ -514,7 +617,79 @@ test_rm_status_code() {
 	test_done "$testroot" "$ret"
 }
 
+test_rm_nonexistent_directory() {
+	local testroot=`test_init rm_nonexistent_directory`
 
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	rm -r $testroot/wt/epsilon
+
+	(cd $testroot/wt && got rm epsilon > $testroot/stdout \
+		2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got rm command succeeded unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	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
+
+	echo "got: epsilon: No such file or directory" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got rm -f epsilon > $testroot/stdout \
+		2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "got rm command failed unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	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
+
+	echo -n '' > $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
+
 test_parseargs "$@"
 run_test test_rm_basic
 run_test test_rm_with_local_mods
@@ -525,3 +700,4 @@ run_test test_rm_directory_keep_files
 run_test test_rm_subtree
 run_test test_rm_symlink
 run_test test_rm_status_code
+run_test test_rm_nonexistent_directory