opatch fails to apply patch produced by diff, causing 'opam update' to fail

[edwintorok]

See ocaml/opam#6937

Steps to reproduce:

$ git clone https://github.com/ocaml/opam-repository && cd opam-repository
$ git checkout 4e7f3a62e3e8654e6c3b146b76136959efbcb3f5
$ git clean -xfd
$ opatch -p 1 ../checkseum.patch
Fatal error: Patch "../checkseum.patch" does not apply cleanly

$ git reset --hard HEAD
$ git clean -xfd
$ patch -p 1 <../checkseum.patch
patching file packages/checkseum/checkseum.0.5.2/opam
patching file packages/checkseum/checkseum.0.5.3/opam (copied from packages/checkseum/checkseum.0.5.2/opam)

checkseum.patch

Looks like checkseum.0.5.2 is first patched, then copied, then attempted to get patched again.

[hannesm]

Thanks for the report, I'm not sure when I'll find time to look into this.

[hannesm]

Thanks for your report. I have a fix for this in #44, but it doesn't feel completely right.

So, there are two issues at hand:

• (a) the "Edit" operation with file a and file b is morally a "move file a to b, and then edit file b"
• (b) in the specific diff you posted, there was first and edit of checkseum.0.5.2/opam, and thereafter a copy to 0.5.3/opam and edit of that file -- and the latter diff hunk included the former edit as well, so: 1. I made the apply a no-op and return ok if the existing data already includes the patch, 2. I introduced a "Copy" git_ext to actually copy a file around (next to rename etc.).

As mentioned, this doesn't feel principled yet, and may need some further work (and for sure more testing).

[edwintorok]

Might be useful to see whether we could run part of git or patch's testsuite using opatch.
These look interesting: https://github.com/mirror/patch/blob/master/tests/concat-git-diff and https://github.com/mirror/patch/blob/master/tests/copy-rename and https://github.com/mirror/patch/blob/master/tests/criss-cross

[edwintorok]

There is some more documentation here: https://git-scm.com/docs/diff-generate-patch
In particular point 4 is of interest:

All the file1 files in the output refer to files before the commit, and all the file2 files refer to files after the commit. It is incorrect to apply each change to each file sequentially. For example, this patch will swap a and b:
diff --git a/a b/b
rename from a
rename to b
diff --git a/b b/a
rename from b
rename to a

This is indeed not compatible with POSIX patch which says

If the patch file contains more than one patch, patch shall attempt to apply each of them as if they came from separate patch files

So when diff --git is used then opatch needs to implement the specification from the above doc. That means it probably either needs to retain the original file(s) in some way, perhaps create a temporary place to store all the modified files, and moved them all at once at the end?

[edwintorok]

Another option might be to try to produce more compatible output in the first place, use git diff --no-renames, although that'd be a change in opam. OTOH there are changes in git that cannot be represented in a (POSIX) patch compatible way, e.g. binary file diffs, but I don't think opatch would support those, and I wouldn't expect to see binary files in an opam repository, unless something went wrong with encoding.