`patch --dry-run` is not POSIX

dbuenzli commented 8 years ago

Here's the POSIX spec and a problematic tool:

> patch --dry-run 
patch: unrecognized option: dry-run
BusyBox v1.24.2 (2016-05-31 09:38:03 GMT) multi-call binary.

Usage: patch [OPTIONS] [ORIGFILE [PATCHFILE]]

    -p,--strip N        Strip N leading components from file names
    -i,--input DIFF     Read DIFF instead of stdin
    -R,--reverse        Reverse patch
    -N,--forward        Ignore already applied patches
    -E,--remove-empty-files Remove output files if they become empty

The current behaviour also leads to misleading error message:

These patches didn't apply at /root/.opam/system/build/camomile.0.8.5:
  - cmx.patch
  - no-camlp4.patch
  - cmxs.patch

Debug mode:

00:33.485  ACTION                  camomile: applying cmxs.patch.

00:33.541  SYSTEM                  [log-1602-ffb3fd] (in 0.020s) patch -p0 -i /root/.opam/system/build/camomile.0.8.5/cmxs.patch --dry-run
00:33.576  SYSTEM                  [log-1602-1872ec] (in 0.010s) patch -p1 -i /root/.opam/system/build/camomile.0.8.5/cmxs.patch --dry-run
00:33.611  SYSTEM                  [log-1602-d2c37b] (in 0.010s) patch -p2 -i /root/.opam/system/build/camomile.0.8.5/cmxs.patch --dry-run
00:33.646  SYSTEM                  [log-1602-6d07ae] (in 0.010s) patch -p3 -i /root/.opam/system/build/camomile.0.8.5/cmxs.patch --dry-run
00:33.682  SYSTEM                  [log-1602-e7966f] (in 0.010s) patch -p4 -i /root/.opam/system/build/camomile.0.8.5/cmxs.patch --dry-run
00:33.682  SYSTEM                  error: Patch /root/.opam/system/build/camomile.0.8.5/cmxs.patch does not apply.

AltGr commented 8 years ago

As shown by the debug mode, what opam currently does -- which seems quite sub-optimal, is:

attempt to patch successively with --dry-run, and a -p parameter ranging from -p0 to -p4, until one succeeds
when one succeeds, re-run it without --dry-run
otherwise, fail with the message you have seen

The reason to do this was, I believe, that it's hard to know what -p parameter the patch expects, and that trying without --dry-run could lead to a partial application; but besides being ineffective, it also means we had to discard error messages from patch in order not to be overly verbose.

I can see a few ways out:

keep as is, but replace --dry-run with something like -o /dev/null, which might be functionally equivalent.
run without --dry-run, but reset the source tree each time to be sure. Even less efficient...
be more strict on opam repository about the -p parameter, and enforce 1, which is the standard for patch files generated from VC tools. Patches in the repository appear to use either 1, or be based on /tmp/opam-xxxxx-xxxxx/, which we could fix mechanically.
implement (or link ?) some OCaml code for handling patch files

I agree the current situation is not very nice, esp. w.r.t. the lack of error reporting. What do you think ?

dbuenzli commented 8 years ago

be more strict on opam repository about the -p parameter, and enforce 1, which is the standard for patch files generated from VC tools. Patches in the repository appear to use either 1, or be based on /tmp/opam-xxxxx-xxxxx/, which we could fix mechanically.

I'd say this (and mention in in the docs) or maybe you can keep it as is now and simulate a --dry-run with -o.

ocaml / opam

`patch --dry-run` is not POSIX #2576