Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Mar 12, 2022 at 10:56:32AM +0100, Stefan Sperling wrote:
> > You do not yet have to parse any of these headers. You could just treat
> > them as noise, for now.
> 
> I think my suggestion boils down to this diff.
> The patch tests keep passing.
> 
> What do you think could go wrong if we did this?
> I don't see a problem.

well, this is the first step yes, but it doesn't really work because it
doesn't properly register the rename.  Also, apply_patch set `path' to
p->new (if not NULL) so then it'll fail when trying to open the "orig"
file.

The tests are still passing because none of them has an old path
different from the new path.  (/dev/null is an exception, p->old or
p->new are set to NULL if they were /dev/null in the patch.)

Here's another attempt.  There are two small commits initially that are
just small issue I found while working on the rename.

I took this as a chance to refactor apply_patch, it was trying to do too
much stuff and I started to loosing keeping track of things.  The
proposed diff below moves the guts of apply_patch into a new function
patch_file and adds two small helpers (schedule_add, schedule_del) so
that apply_patch remains manegeable.  It also adds a test for the rename
case that you can try against your original diff if you want.

-----------------------------------------------
commit e40006c4c42aa8463ce352e955afc88483925817
from: Omar Polo <op@omarpolo.com>
date: Sat Mar 12 10:37:46 2022 UTC
 
 apply_patch: move sanity check early in recv_patch
 
diff f95923a581dde77a840de5e9f56060e84118b413 55e3d2a66e53d4aa787181c36d052b2545b2d8b5
blob - 6cc85331d4129d6f7dfec267d04e16173c9608a6
blob + 957d639a49780d6a41491cd6e9117b1783a6b61f
--- lib/patch.c
+++ lib/patch.c
@@ -172,6 +172,10 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
 		err = got_error_from_errno("strdup");
 		goto done;
 	}
+	if (p->old == NULL && p->new == NULL) {
+		err = got_error(GOT_ERR_PATCH_MALFORMED);
+		goto done;
+	}
 
 	imsg_free(&imsg);
 
@@ -396,9 +400,6 @@ apply_patch(struct got_worktree *worktree, struct got_
 
 	TAILQ_INIT(&paths);
 
-	if (p->old == NULL && p->new == NULL)
-		return got_error(GOT_ERR_PATCH_MALFORMED);
-
 	err = got_worktree_resolve_path(&path, worktree,
 	    p->new != NULL ? p->new : p->old);
 	if (err)

-----------------------------------------------
commit da7797a7a741039c3e78c795ab778720be562f22
from: Omar Polo <op@omarpolo.com>
date: Sat Mar 12 10:38:58 2022 UTC
 
 got-read-patch: plug memory leak
 
diff 55e3d2a66e53d4aa787181c36d052b2545b2d8b5 1dfeba738a1f61dacb8d52ae4a08c6c7958e6326
blob - ed5eb50b17c3c73f369b043bdf1ea72f54f5ff88
blob + 0f19ba3a58a9ae3c8acd25da10ed6237d9fe334b
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -171,9 +171,6 @@ find_patch(FILE *fp)
 			else
 				err = send_patch(old, new);
 
-			free(old);
-			free(new);
-
 			if (err)
 				break;
 
@@ -184,6 +181,8 @@ find_patch(FILE *fp)
 		}
 	}
 
+	free(old);
+	free(new);
 	free(line);
 	if (ferror(fp) && err == NULL)
 		err = got_error_from_errno("getline");

-----------------------------------------------
commit 776ed7fd54533342fb377eb87d448d828e59c91e
from: Omar Polo <op@omarpolo.com>
date: Sat Mar 12 10:44:20 2022 UTC
 
 patch: drop redundant unlink
 
diff 1dfeba738a1f61dacb8d52ae4a08c6c7958e6326 1c535f311e564b2dcf583685b54eea36bf947b84
blob - 957d639a49780d6a41491cd6e9117b1783a6b61f
blob + 20354401aafe8d6c09947cd371d13b8809803b73
--- lib/patch.c
+++ lib/patch.c
@@ -520,11 +520,8 @@ done:
 	if (tmppath != NULL)
 		unlink(tmppath);
 	free(tmppath);
-	if (orig != NULL) {
-		if (p->old == NULL && err != NULL)
-			unlink(path);
+	if (orig != NULL)
 		fclose(orig);
-	}
 	free(path);
 	free(line);
 	got_pathlist_free(&paths);

-----------------------------------------------
commit e1708f63b37b7f1d66b58d674c00a90dd5e9d04e (patchrename)
from: Omar Polo <op@omarpolo.com>
date: Sat Mar 12 12:04:19 2022 UTC
 
 refactor apply_patch to support renaming files
 
 add two helper functions (schedule_add, schedule_del) and move the guts
 of apply_patch into a new function `patch_file'.  This simplifies
 apply_patch and makes easier to figure out what happens.
 
diff 1c535f311e564b2dcf583685b54eea36bf947b84 3d55bad94d59dbc12a9c12d838b80dd720060307
blob - 20354401aafe8d6c09947cd371d13b8809803b73
blob + 8dc632ef6f52371e65674a21a73f8c9d2496afd5
--- lib/patch.c
+++ lib/patch.c
@@ -381,69 +381,65 @@ apply_hunk(FILE *tmp, struct got_patch_hunk *h, long *
 }
 
 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)
+schedule_add(const char *path, struct got_worktree *worktree,
+    struct got_repository *repo, got_worktree_checkout_cb add_cb,
+    void *add_arg)
 {
-	const struct got_error *err = NULL;
+	static const struct got_error *err = NULL;
 	struct got_pathlist_head paths;
 	struct got_pathlist_entry *pe;
-	char *path = NULL, *tmppath = NULL, *template = NULL;
-	FILE *orig = NULL, *tmp = NULL;
-	struct got_patch_hunk *h;
-	size_t i;
-	long lineno = 0;
-	off_t copypos, pos;
-	char *line = NULL;
-	size_t linesize = 0;
-	ssize_t linelen;
 
 	TAILQ_INIT(&paths);
 
-	err = got_worktree_resolve_path(&path, worktree,
-	    p->new != NULL ? p->new : p->old);
-	if (err)
-		return err;
 	err = got_pathlist_insert(&pe, &paths, path, NULL);
-	if (err)
-		goto done;
+	if (err == NULL)
+		err = got_worktree_schedule_add(worktree, &paths,
+		    add_cb, add_arg, repo, 1);
+	got_pathlist_free(&paths);
+	return err;
+}
 
-	if (p->old != NULL && p->new == NULL) {
-		/*
-		 * special case: delete a file.  don't try to match
-		 * the lines but just schedule the removal.
-		 */
+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);
-		goto done;
-	} else if (p->old != NULL && strcmp(p->old, p->new)) {
-		err = got_error(GOT_ERR_PATCH_PATHS_DIFFER);
-		goto done;
-	}
+	got_pathlist_free(&paths);
+	return err;
+}
 
-	if (asprintf(&template, "%s/got-patch",
-	    got_worktree_get_root_path(worktree)) == -1) {
-		err = got_error_from_errno(template);
-		goto done;
-	}
+static const struct got_error *
+patch_file(struct got_patch *p, const char *path, FILE *tmp)
+{
+	const struct got_error *err = NULL;
+	struct got_patch_hunk *h;
+	size_t i;
+	long lineno = 0;
+	FILE *orig;
+	off_t copypos, pos;
+	char *line = NULL;
+	size_t linesize = 0;
+	ssize_t linelen;
 
-	err = got_opentemp_named(&tmppath, &tmp, template);
-	if (err)
-		goto done;
-
 	if (p->old == NULL) {				/* create */
 		h = STAILQ_FIRST(&p->head);
-		if (h == NULL || STAILQ_NEXT(h, entries) != NULL) {
-			err = got_error(GOT_ERR_PATCH_MALFORMED);
-			goto done;
-		}
+		if (h == NULL || STAILQ_NEXT(h, entries) != NULL)
+			return got_error(GOT_ERR_PATCH_MALFORMED);
 		for (i = 0; i < h->len; ++i) {
-			if (fprintf(tmp, "%s", h->lines[i]+1) < 0) {
-				err = got_error_from_errno("fprintf");
-				goto done;
-			}
+			if (fprintf(tmp, "%s", h->lines[i]+1) < 0)
+				return got_error_from_errno("fprintf");
 		}
-		goto rename;
+		return err;
 	}
 
 	if ((orig = fopen(path, "r")) == NULL) {
@@ -453,6 +449,9 @@ apply_patch(struct got_worktree *worktree, struct got_
 
 	copypos = 0;
 	STAILQ_FOREACH(h, &p->head, entries) {
+		if (h->lines == NULL)
+			break;
+
 	tryagain:
 		err = locate_hunk(orig, h, &pos, &lineno);
 		if (err != NULL)
@@ -494,37 +493,86 @@ apply_patch(struct got_worktree *worktree, struct got_
 		}
 	}
 
-	if (!feof(orig)) {
+	if (!feof(orig))
 		err = copy(tmp, orig, copypos, -1);
-		if (err)
-			goto done;
+
+done:
+	if (orig != NULL)
+		fclose(orig);
+	return err;
+}
+
+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)
+{
+	const struct got_error *err = NULL;
+	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);
+	if (err)
+		goto done;
+
+	err = got_worktree_resolve_path(&newpath, worktree,
+	    p->new != NULL ? p->new : p->old);
+	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);
+		goto done;
 	}
 
-rename:
-	if (rename(tmppath, path) == -1) {
-		err = got_error_from_errno3("rename", tmppath, path);
+	if (asprintf(&template, "%s/got-patch",
+	    got_worktree_get_root_path(worktree)) == -1) {
+		err = got_error_from_errno(template);
 		goto done;
 	}
 
-	if (p->old == NULL)
-		err = got_worktree_schedule_add(worktree, &paths,
-		    add_cb, add_arg, repo, 1);
+	err = got_opentemp_named(&tmppath, &tmp, template);
+	if (err)
+		goto done;
+	err = patch_file(p, oldpath, tmp);
+	if (err)
+		goto done;
+
+	if (rename(tmppath, newpath) == -1) {
+		err = got_error_from_errno3("rename", tmppath, newpath);
+		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);
+		if (err == NULL)
+			err = schedule_add(newpath, worktree, repo,
+			    add_cb, add_arg);
+	} else if (p->old == NULL)
+		err = schedule_add(newpath, worktree, repo, add_cb,
+		    add_arg);
 	else
-		printf("M  %s\n", path); /* XXX */
+		printf("M  %s\n", oldpath); /* XXX */
+
 done:
+	if (err != NULL && (file_renamed || p->old == NULL))
+		unlink(newpath);
 	free(template);
-	if (err != NULL && p->old == NULL && path != NULL)
-		unlink(path);
-	if (tmp != NULL)
-		fclose(tmp);
 	if (tmppath != NULL)
 		unlink(tmppath);
 	free(tmppath);
-	if (orig != NULL)
-		fclose(orig);
-	free(path);
-	free(line);
-	got_pathlist_free(&paths);
+	free(oldpath);
+	free(newpath);
 	return err;
 }
 
blob - 7a5ad3671fa62d02f74a21cc4f0edf7be252462a
blob + 5916921f260f5a0eaefdbf6036b813fbf3570dd5
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -624,6 +624,112 @@ EOF
 	test_done $testroot $ret
 }
 
+test_patch_rename() {
+	local testroot=`test_init patch_rename`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cat <<EOF > $testroot/wt/patch
+--- alpha
++++ eta
+@@ -0,0 +0,0 @@
+EOF
+
+	echo 'D  alpha' > $testroot/stdout.expected
+	echo 'A  eta'  >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got patch patch) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
+	fi
+
+	if [ -f $testroot/wt/alpha ]; then
+		echo "alpha was not removed" >&2
+		test_done $testroot 1
+		return 1
+	fi
+	if [ ! -f $testroot/wt/eta ]; then
+		echo "eta was not created" >&2
+		test_done $testroot 1
+		return 1
+	fi
+
+	echo alpha > $testroot/wt/eta.expected
+	cmp -s $testroot/wt/eta.expected $testroot/wt/eta
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/wt/eta.expected $testroot/wt/eta
+		test_done $testroot $ret
+		return 1
+	fi
+
+	# revert the changes and try again with a rename + edit
+	(cd $testroot/wt && got revert alpha eta) > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cat <<EOF > $testroot/wt/patch
+--- alpha
++++ eta
+@@ -1 +1,2 @@
+ alpha
++but now is eta
+EOF
+
+	(cd $testroot/wt && got patch patch) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
+	fi
+
+	if [ -f $testroot/wt/alpha ]; then
+		echo "alpha was not removed" >&2
+		test_done $testroot 1
+		return 1
+	fi
+	if [ ! -f $testroot/wt/eta ]; then
+		echo "eta was not created" >&2
+		test_done $testroot 1
+		return 1
+	fi
+
+	echo alpha > $testroot/wt/eta.expected
+	echo 'but now is eta' >> $testroot/wt/eta.expected
+	cmp -s $testroot/wt/eta.expected $testroot/wt/eta
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/wt/eta.expected $testroot/wt/eta
+	fi
+	test_done $testroot $ret
+}
+
 test_parseargs "$@"
 run_test test_patch_simple_add_file
 run_test test_patch_simple_rm_file
@@ -636,3 +742,4 @@ run_test test_patch_dont_apply
 run_test test_patch_malformed
 run_test test_patch_no_patch
 run_test test_patch_equals_for_context
+run_test test_patch_rename