Hello,

currently 'got patch' is happy to do all sorts of things to all sorts of
files.  Well, it's not really ideal I think.  The following patch adds
some checks to the targeted files before patching 'em.

I think it's fine to allow patching modified files, but 'got patch'
should disallow patching non-existant or unknown files. Or add files
that are already there, or delete files there are not known.

There's a bit of churn because I just had to re-introduce in apply_patch
the pathlists i segregated to schedule_* in previous commit.  Also, at
this points those two helpers are useless.

This actually made the rename test to fail because in the last part
`eta' (the target of the rename) exists (untracked) and so it won't
proceed with the rename.

note that it needs the latest commit by stsp "fix 'got status' with an
obstructed file".

thoughts?

-----------------------------------------------
commit e80a119d2e3f8160906b6b15995c5f86dc7b4261 (patchcfs)
from: Omar Polo <op@omarpolo.com>
date: Sat Mar 12 22:18:43 2022 UTC
 
 check file status before applying the patch
 
diff c0c2b3d1e2c80fe9ee36670cff033f948cca9fd6 18430ad12bc18803acff0ab85239f15402877175
blob - 09c52c1e9bd95ceb4f5fa1f135b3fb570b7285b0
blob + 4bcea52af617043cda2fa50d87f21effce198199
--- got/got.c
+++ got/got.c
@@ -7245,7 +7245,7 @@ cmd_patch(int argc, char *argv[])
 #endif
 
 	error = got_patch(patchfd, worktree, repo, &print_remove_status,
-	    NULL, &add_progress, NULL);
+	    NULL, &add_progress, NULL, check_cancelled, NULL);
 
 done:
 	if (repo) {
blob - 04a23fc7f7da83078d23e8ec24820ce98aa43702
blob + 448bb17e65d75d8d5ba94bd577f5cde4fef566bf
--- include/got_patch.h
+++ include/got_patch.h
@@ -22,4 +22,5 @@
  */
 const struct got_error *
 got_patch(int, struct got_worktree *, struct got_repository *,
-    got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *);
+    got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *,
+    got_cancel_cb, void *);
blob - 8dc632ef6f52371e65674a21a73f8c9d2496afd5
blob + 5bb55ca2e3ebedd95e9143cb2d4735c4495d82e7
--- lib/patch.c
+++ lib/patch.c
@@ -381,44 +381,6 @@ apply_hunk(FILE *tmp, struct got_patch_hunk *h, long *
 }
 
 static const struct got_error *
-schedule_add(const char *path, struct got_worktree *worktree,
-    struct got_repository *repo, got_worktree_checkout_cb add_cb,
-    void *add_arg)
-{
-	static const struct got_error *err = NULL;
-	struct got_pathlist_head paths;
-	struct got_pathlist_entry *pe;
-
-	TAILQ_INIT(&paths);
-
-	err = got_pathlist_insert(&pe, &paths, path, NULL);
-	if (err == NULL)
-		err = got_worktree_schedule_add(worktree, &paths,
-		    add_cb, add_arg, repo, 1);
-	got_pathlist_free(&paths);
-	return err;
-}
-
-static const struct got_error *
-schedule_del(const char *path, struct got_worktree *worktree,
-    struct got_repository *repo, got_worktree_delete_cb delete_cb,
-    void *delete_arg)
-{
-	static const struct got_error *err = NULL;
-	struct got_pathlist_head paths;
-	struct got_pathlist_entry *pe;
-
-	TAILQ_INIT(&paths);
-
-	err = got_pathlist_insert(&pe, &paths, path, NULL);
-	if (err == NULL)
-		err = got_worktree_schedule_delete(worktree, &paths,
-		    0, NULL, delete_cb, delete_arg, repo, 0, 0);
-	got_pathlist_free(&paths);
-	return err;
-}
-
-static const struct got_error *
 patch_file(struct got_patch *p, const char *path, FILE *tmp)
 {
 	const struct got_error *err = NULL;
@@ -503,33 +465,128 @@ done:
 }
 
 static const struct got_error *
+build_pathlist(const char *p, char **path, struct got_pathlist_head *head,
+    struct got_worktree *worktree)
+{
+	const struct got_error *err;
+	struct got_pathlist_entry *pe;
+
+	err = got_worktree_resolve_path(path, worktree, p);
+	if (err == NULL)
+		err = got_pathlist_insert(&pe, head, *path, NULL);
+	return err;
+}
+
+static const struct got_error *
+can_rm(void *arg, unsigned char status, unsigned char staged_status,
+    const char *path, struct got_object_id *blob_id,
+    struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+    int dirfd, const char *de_name)
+{
+	if (status != GOT_STATUS_NO_CHANGE &&
+	    status != GOT_STATUS_ADD &&
+	    status != GOT_STATUS_MODIFY &&
+	    status != GOT_STATUS_MODE_CHANGE)
+		return got_error_path(path, GOT_ERR_FILE_STATUS);
+	if (staged_status == GOT_STATUS_DELETE)
+		return got_error_path(path, GOT_ERR_FILE_STATUS);
+	return NULL;
+}
+
+static const struct got_error *
+can_add(void *arg, unsigned char status, unsigned char staged_status,
+    const char *path, struct got_object_id *blob_id,
+    struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+    int dirfd, const char *de_name)
+{
+	if (status != GOT_STATUS_NONEXISTENT &&
+	    status != GOT_STATUS_MISSING)
+		return got_error_path(path, GOT_ERR_FILE_STATUS);
+	return NULL;
+}
+
+static const struct got_error *
+can_edit(void *arg, unsigned char status, unsigned char staged_status,
+    const char *path, struct got_object_id *blob_id,
+    struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+    int dirfd, const char *de_name)
+{
+	if (status != GOT_STATUS_NO_CHANGE &&
+	    status != GOT_STATUS_ADD &&
+	    status != GOT_STATUS_MODIFY)
+		return got_error_path(path, GOT_ERR_FILE_STATUS);
+	if (staged_status == GOT_STATUS_DELETE)
+		return got_error_path(path, GOT_ERR_FILE_STATUS);
+	return NULL;
+}
+
+static const struct got_error *
+check_file_status(struct got_patch *p, int file_renamed,
+    struct got_worktree *worktree, struct got_repository *repo,
+    struct got_pathlist_head *old, struct got_pathlist_head *new,
+    got_cancel_cb cancel_cb, void *cancel_arg)
+{
+	static const struct got_error *err;
+
+	if (p->old != NULL && p->new == NULL)
+		return got_worktree_status(worktree, old, repo, 0,
+		    can_rm, NULL, cancel_cb, cancel_arg);
+	else if (file_renamed) {
+		err = got_worktree_status(worktree, old, repo, 0,
+		    can_rm, NULL, cancel_cb, cancel_arg);
+		if (err)
+			return err;
+		return got_worktree_status(worktree, new, repo, 0,
+		    can_add, NULL, cancel_cb, cancel_arg);
+	} else if (p->old == NULL)
+		return got_worktree_status(worktree, new, repo, 0,
+		    can_add, NULL, cancel_cb, cancel_arg);
+	else
+		return got_worktree_status(worktree, new, repo, 0,
+		    can_edit, NULL, cancel_cb, cancel_arg);
+}
+
+static const struct got_error *
 apply_patch(struct got_worktree *worktree, struct got_repository *repo,
     struct got_patch *p, got_worktree_delete_cb delete_cb, void *delete_arg,
-    got_worktree_checkout_cb add_cb, void *add_arg)
+    got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
+    void *cancel_arg)
 {
 	const struct got_error *err = NULL;
+	struct got_pathlist_head oldpaths, newpaths;
 	int file_renamed = 0;
 	char *oldpath = NULL, *newpath = NULL;
 	char *tmppath = NULL, *template = NULL;
 	FILE *tmp = NULL;
 
-	err = got_worktree_resolve_path(&oldpath, worktree,
-	    p->old != NULL ? p->old : p->new);
+	TAILQ_INIT(&oldpaths);
+	TAILQ_INIT(&newpaths);
+
+	err = build_pathlist(p->old != NULL ? p->old : p->new, &oldpath,
+	    &oldpaths, worktree);
 	if (err)
 		goto done;
 
-	err = got_worktree_resolve_path(&newpath, worktree,
-	    p->new != NULL ? p->new : p->old);
+	err = build_pathlist(p->new != NULL ? p->new : p->old, &newpath,
+	    &newpaths, worktree);
 	if (err)
 		goto done;
 
+	if (p->old != NULL && p->new != NULL && strcmp(p->old, p->new))
+		file_renamed = 1;
+
+	err = check_file_status(p, file_renamed, worktree, repo, &oldpaths,
+	    &newpaths, cancel_cb, cancel_arg);
+	if (err)
+		goto done;
+
 	if (p->old != NULL && p->new == NULL) {
 		/*
 		 * special case: delete a file.  don't try to match
 		 * the lines but just schedule the removal.
 		 */
-		err = schedule_del(p->old, worktree, repo, delete_cb,
-		    delete_arg);
+		err = got_worktree_schedule_delete(worktree, &oldpaths,
+		    0, NULL, delete_cb, delete_arg, repo, 0, 0);
 		goto done;
 	}
 
@@ -551,26 +608,27 @@ apply_patch(struct got_worktree *worktree, struct got_
 		goto done;
 	}
 
-	file_renamed = p->old != NULL && strcmp(p->old, p->new);
 	if (file_renamed) {
-		err = schedule_del(oldpath, worktree, repo, delete_cb,
-		    delete_arg);
+		err = got_worktree_schedule_delete(worktree, &oldpaths,
+		    0, NULL, delete_cb, delete_arg, repo, 0, 0);
 		if (err == NULL)
-			err = schedule_add(newpath, worktree, repo,
-			    add_cb, add_arg);
+			err = got_worktree_schedule_add(worktree, &newpaths,
+			    add_cb, add_arg, repo, 1);
 	} else if (p->old == NULL)
-		err = schedule_add(newpath, worktree, repo, add_cb,
-		    add_arg);
+		err = got_worktree_schedule_add(worktree, &newpaths,
+		    add_cb, add_arg, repo, 1);
 	else
 		printf("M  %s\n", oldpath); /* XXX */
 
 done:
-	if (err != NULL && (file_renamed || p->old == NULL))
+	if (err != NULL && newpath != NULL && (file_renamed || p->old == NULL))
 		unlink(newpath);
 	free(template);
 	if (tmppath != NULL)
 		unlink(tmppath);
 	free(tmppath);
+	got_pathlist_free(&oldpaths);
+	got_pathlist_free(&newpaths);
 	free(oldpath);
 	free(newpath);
 	return err;
@@ -579,7 +637,8 @@ done:
 const struct got_error *
 got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo,
     got_worktree_delete_cb delete_cb, void *delete_arg,
-    got_worktree_checkout_cb add_cb, void *add_arg)
+    got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
+    void *cancel_arg)
 {
 	const struct got_error *err = NULL;
 	struct imsgbuf *ibuf;
@@ -628,7 +687,7 @@ got_patch(int fd, struct got_worktree *worktree, struc
 			break;
 
 		err = apply_patch(worktree, repo, &p, delete_cb, delete_arg,
-		    add_cb, add_arg);
+		    add_cb, add_arg, cancel_cb, cancel_arg);
 		patch_free(&p);
 		if (err)
 			break;
blob - 5916921f260f5a0eaefdbf6036b813fbf3570dd5
blob + 8fc24e71d8c31a7d54af30d9cfce4f5c598f3299
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -685,6 +685,7 @@ EOF
 		test_done $testroot $ret
 		return 1
 	fi
+	rm $testroot/wt/eta
 
 	cat <<EOF > $testroot/wt/patch
 --- alpha
@@ -730,6 +731,99 @@ EOF
 	test_done $testroot $ret
 }
 
+test_patch_illegal_status() {
+	local testroot=`test_init patch_illegal_status`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# edit an non-existant and unknown file
+	cat <<EOF > $testroot/wt/patch
+--- iota
++++ iota
+@@ -1 +1 @@
+- iota
++ IOTA
+EOF
+
+	(cd $testroot/wt && got patch patch) > /dev/null \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "edited a missing file" >&2
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo "got: iota: file has unexpected status" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# create iota and re-try
+	echo iota > $testroot/wt/iota
+
+	(cd $testroot/wt && got patch patch) > /dev/null \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "edited a missing file" >&2
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo "got: iota: file has unexpected status" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# edit obstructed file
+	rm $testroot/wt/alpha
+	mkdir $testroot/wt/alpha
+	cat <<EOF > $testroot/wt/patch
+--- alpha
++++ alpha
+@@ -1 +1,2 @@
+ alpha
++was edited
+EOF
+
+	(cd $testroot/wt && got patch patch) > /dev/null \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "edited a missing file" >&2
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo "got: alpha: file has unexpected status" \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 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_patch_simple_add_file
 run_test test_patch_simple_rm_file
@@ -743,3 +837,4 @@ run_test test_patch_malformed
 run_test test_patch_no_patch
 run_test test_patch_equals_for_context
 run_test test_patch_rename
+run_test test_patch_illegal_status