From: Omar Polo Subject: Re: "got patch" format? To: Stefan Sperling Cc: Christian Weisgerber , gameoftrees@openbsd.org Date: Sat, 19 Mar 2022 19:07:19 +0100 Stefan Sperling wrote: > On Sat, Mar 19, 2022 at 06:15:23PM +0100, Omar Polo wrote: > > Christian Weisgerber 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 < $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 < $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 < $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