Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Mar 19, 2022 at 06:15:23PM +0100, Omar Polo wrote:
> > Christian Weisgerber <naddy@mips.inka.de> wrote:
> > > I just tried to use "got patch" and promptly failed because I had
> > > generated the patch file with "diff -u foo.orig foo", and "got patch"
> > > choked because foo.orig didn't exist in the worktree.  Is this a bug,
> > > work in progress, ...?
> > 
> > patch(1) (and in most cases 'git apply' too) prefer the new path over
> > the old path, so in your case both pick up 'foo' as path and it works.
> > 
> > in 'got patch' i tried to handle the rename case too, and in fact at the
> > moment it handles "git-style rename diffs" well.  This means however
> > that both the "old" path and the "new" path must exists in the worktree.
> > (see test_patch_rename)
> > 
> > (The only case where 'git diff' produces a diff for a rename that's not
> > currently handled by 'got patch' is when a file is renamed without any
> > change.  in that case the diff has only the header and no hunks in it.)
> > 
> > 'git apply' solves this by requiring some git-specific "annotations" to
> > the diff for it to interpret the paths as rename.  ("headers" like
> > similarity index, rename from/to and the leading "diff --git".)  In
> > practice thought seems that "diff --git" and paths wit a/ and b/ are
> > enough for it to do the rename.  I haven't checked the sources and the
> > git-apply manpage doesn't mention anything (judging from a quick grep.)
> > 
> > We have multiple way to fix this:
> > 
> >  - don't support patches that rename files (can always be achieved by a
> >    remove folled by the addition of the file like 'got diff' already
> >    does.)
> 
> Given these three options, I would prefer the above, for now.

i agree but...

> >  - add some heuristics in got_worktree_patch_check_path.  We could, for
> >    instance, ignore the old path and just use the new when the old file
> >    is nonexistent or unversioned.  Diff below is an example of this.
> > 
> >  - invent our own annotations?  "diff --got" anyone? :D
> 
> That would be bad for interop. Everyone else would have to parse our
> custom annotation format.

(i was joking)

> So I think 'got patch' should expect git-compatible annotations,
> and only support renames if they are present. We can leave this
> for later, it doesn't have to happen right now.
> 
> 'got diff' might eventually produce git-compatible annotations
> instead of full delete + full add.
> 
> SVN did this, too. There is 'svn diff --git', and 'svn patch' can
> parse Git's annotations. Everyone wins.

...turns out that keeping the rename only when the git annotations are
present is really easy.  Just tweak send_patch to prefer the newname if
it's not NULL and it's not a git-style diff.

ok?

diff 78f5ac2436c8d17d1dd687d69e51354707275988 /home/op/w/got
blob - deb0e8192f09bb269bb784019dc776e0602e2925
file + libexec/got-read-patch/got-read-patch.c
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -60,14 +60,21 @@
 struct imsgbuf ibuf;
 
 static const struct got_error *
-send_patch(const char *oldname, const char *newname)
+send_patch(const char *oldname, const char *newname, int git)
 {
 	struct got_imsg_patch p;
 
 	memset(&p, 0, sizeof(p));
 
-	if (oldname != NULL)
+	/*
+	 * Prefer the new name if it's not /dev/null and it's not
+	 * a git-style diff.
+	 */
+	if (!git && newname != NULL && oldname != NULL)
+		strlcpy(p.old, newname, sizeof(p.old));
+	else if (oldname != NULL)
 		strlcpy(p.old, oldname, sizeof(p.old));
+
 	if (newname != NULL)
 		strlcpy(p.new, newname, sizeof(p.new));
 
@@ -169,7 +176,7 @@ find_patch(FILE *fp)
 			    (!create && old == NULL))
 				err = got_error(GOT_ERR_PATCH_MALFORMED);
 			else
-				err = send_patch(old, new);
+				err = send_patch(old, new, git);
 
 			if (err)
 				break;
blob - 32c852932a5cc8f51426acc413933a80b95f41de
file + regress/cmdline/patch.sh
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -647,8 +647,9 @@ test_patch_rename() {
 	fi
 
 	cat <<EOF > $testroot/wt/patch
---- alpha
-+++ eta
+diff --git a/alpha b/eta
+--- a/alpha
++++ b/eta
 @@ -0,0 +0,0 @@
 EOF
 
@@ -700,8 +701,9 @@ EOF
 	rm $testroot/wt/eta
 
 	cat <<EOF > $testroot/wt/patch
---- alpha
-+++ eta
+diff --git a/alpha b/eta
+--- a/alpha
++++ b/eta
 @@ -1 +1,2 @@
  alpha
 +but now is eta
@@ -863,6 +865,7 @@ test_patch_nop() {
 +++ /dev/null
 @@ -1 +0,0 @@
 -beta
+diff --git a/gamma/delta b/gamma/delta.new
 --- gamma/delta
 +++ gamma/delta.new
 @@ -1 +1 @@
@@ -1048,6 +1051,40 @@ EOF
 	test_done $testroot $ret
 }
 
+test_patch_orig() {
+	local testroot=`test_init patch_orig`
+
+	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.orig
++++ alpha
+@@ -1 +1,2 @@
+ alpha
++was edited
+EOF
+
+	(cd $testroot/wt && got patch patch) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo 'M  alpha' > $testroot/stdout.expected	
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done $testroot $ret
+}
+
 test_parseargs "$@"
 run_test test_patch_simple_add_file
 run_test test_patch_simple_rm_file
@@ -1066,3 +1103,4 @@ run_test test_patch_nop
 run_test test_patch_preserve_perm
 run_test test_patch_create_dirs
 run_test test_patch_with_offset
+run_test test_patch_orig