From 40b6fb70e6f23459c135bc5336f24b5c211543a0 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Wed, 30 Apr 2025 19:30:19 +0200 Subject: [PATCH 01/10] Add some tests showing the behaviour of the internal patch implementation --- master_changes.md | 1 + tests/reftests/dune.inc | 21 ++++++ tests/reftests/repository-patchdiff.test | 88 ++++++++++++++++++++++++ 3 files changed, 110 insertions(+) create mode 100644 tests/reftests/repository-patchdiff.test diff --git a/master_changes.md b/master_changes.md index 9268015696e..c893811177c 100644 --- a/master_changes.md +++ b/master_changes.md @@ -111,6 +111,7 @@ users) ## Reftests ### Tests * Show the behaviour of `opam switch create` in presence of `avoid-version`/`deprecated` packages [#6494 @kit-ty-kate] + * Add some tests showing the behaviour of the internal patch implementation [#6490 @rjbou] ### Engine diff --git a/tests/reftests/dune.inc b/tests/reftests/dune.inc index a22d149786c..19d86fdc1fd 100644 --- a/tests/reftests/dune.inc +++ b/tests/reftests/dune.inc @@ -1595,6 +1595,27 @@ %{targets} (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:remove.test} %{read-lines:testing-env})))) +(rule + (alias reftest-repository-patchdiff) + (enabled_if (and (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1)))) + (action + (diff repository-patchdiff.test repository-patchdiff.out))) + +(alias + (name reftest) + (enabled_if (and (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1)))) + (deps (alias reftest-repository-patchdiff))) + +(rule + (targets repository-patchdiff.out) + (deps root-N0REP0) + (enabled_if (and (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1)))) + (package opam) + (action + (with-stdout-to + %{targets} + (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:repository-patchdiff.test} %{read-lines:testing-env})))) + (rule (alias reftest-repository) (enabled_if (and (or (<> %{env:TESTALL=1} 0) (= %{env:TESTN0REP0=0} 1)))) diff --git a/tests/reftests/repository-patchdiff.test b/tests/reftests/repository-patchdiff.test new file mode 100644 index 00000000000..5b15301edd8 --- /dev/null +++ b/tests/reftests/repository-patchdiff.test @@ -0,0 +1,88 @@ +N0REP0 +### +opam-version: "2.0" +### +opam-version: "2.0" +### +I'm some content +### +I'm some content +### :I: Git updates +### git -C ./REPO init -q +### git -C ./REPO config core.autocrlf false +### git -C ./REPO add repo +### git -C ./REPO add packages/first +### git -C ./REPO commit -qm "first" +### opam repository add repo git+file://${BASEDIR}/REPO --set-default +[repo] Initialised +### opam repository remove default --all +### OPAMDEBUG=-1 OPAMDEBUGSECTIONS="REPOSITORY REPO_BACKEND" +### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git +OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/fst +OPAM/repo/repo/packages/first/first.1/files/fst/a-file +OPAM/repo/repo/packages/first/first.1/files/fst/another-file +### :I:1: Add empty file +### touch REPO/packages/first/first.1/files/empty +### git -C ./REPO add packages/first/first.1/files/empty +### git -C ./REPO commit -qm "add empty file" +### opam update | "diff-[0-9a-f]+-[0-9a-f]+" -> "diff" + +<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> +REPOSITORY update repo from git+file://${BASEDIR}/REPO +[repo] synchronised from git+file://${BASEDIR}/REPO +REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff +### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git +OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/fst +OPAM/repo/repo/packages/first/first.1/files/fst/a-file +OPAM/repo/repo/packages/first/first.1/files/fst/another-file +### :I:2: Remove empty file +### git -C ./REPO rm packages/first/first.1/files/empty +rm 'packages/first/first.1/files/empty' +### git -C ./REPO commit -qm "remove empty file" +### opam update | "diff-[0-9a-f]+-[0-9a-f]+" -> "diff" + +<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> +REPOSITORY update repo from git+file://${BASEDIR}/REPO +REPOSITORY update empty, no validation performed +[repo] no changes from git+file://${BASEDIR}/REPO +REPOSITORY repo: applying empty update +### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git +OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/fst +OPAM/repo/repo/packages/first/first.1/files/fst/a-file +OPAM/repo/repo/packages/first/first.1/files/fst/another-file +### :I:3: Move a file into a new directory +### mkdir REPO/packages/first/first.1/files/snd +### git -C ./REPO mv packages/first/first.1/files/fst/another-file packages/first/first.1/files/snd/another-file +### git -C ./REPO commit -qm "move file into another directory" +### opam update | "diff-[0-9a-f]+-[0-9a-f]+" -> "diff" + +<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> +REPOSITORY update repo from git+file://${BASEDIR}/REPO +[repo] synchronised from git+file://${BASEDIR}/REPO +REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff +[ERROR] Could not update repository "repo": ${OPAM}: "rename" failed on ${BASEDIR}/OPAM/repo/repo/packages/first/first.1/files/fst/another-file: No such file or directory +# Return code 40 # +### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git +OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/fst +OPAM/repo/repo/packages/first/first.1/files/fst/a-file +OPAM/repo/repo/packages/first/first.1/files/fst/another-file +### :I:4: remove empty directories +### git -C ./REPO rm packages/first/first.1/files/fst/a-file +rm 'packages/first/first.1/files/fst/a-file' +### git -C ./REPO commit -qm "remove single file in directory" +### opam update | "diff-[0-9a-f]+-[0-9a-f]+" -> "diff" + +<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> +REPOSITORY update repo from git+file://${BASEDIR}/REPO +[repo] synchronised from git+file://${BASEDIR}/REPO +REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff +[ERROR] Could not update repository "repo": ${OPAM}: "rename" failed on ${BASEDIR}/OPAM/repo/repo/packages/first/first.1/files/fst/another-file: No such file or directory +# Return code 40 # +### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git +OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/fst +OPAM/repo/repo/packages/first/first.1/files/fst/another-file From 9088be9814e250079ae7c12816ee222f2e24a42c Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Wed, 30 Apr 2025 15:28:09 +0200 Subject: [PATCH 02/10] patchDiff: use record instead of tuple --- tests/lib/patchDiff.ml | 62 ++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/tests/lib/patchDiff.ml b/tests/lib/patchDiff.ml index 28fccb7d9d1..89d80a26041 100644 --- a/tests/lib/patchDiff.ml +++ b/tests/lib/patchDiff.ml @@ -241,10 +241,17 @@ type diff_patch = | DiffPatch | Patch of string +type setup = { + label: string; (* setup label *) + content: arborescence list; (* the content of directory, first and second one *) + kind: diff_patch; (* what test to run *) +} + let print_dirs dir = print "%s\n" (read_dir dir [ first; second ]) -let diff_patch dir content kind = +let diff_patch dir setup = + let { content; kind; _ } = setup in write_setup dir content; print "*** SETUP ***\n"; print_dirs dir; @@ -285,17 +292,42 @@ let diff_patch dir content kind = (** The tests *) let tests = [ - "normal", content_working_diff, DiffPatch; - "diff file/dir error", content_dir_file, DiffPatch; - "diff dir/file error", content_file_dir, DiffPatch; - "symlink fst", content_symlink_fst, DiffPatch; - "symlink snd", content_symlink_snd, DiffPatch; - "hardlink fst", content_hardlink_fst, DiffPatch; - "hardlink snd", content_hardlink_snd, DiffPatch; - "patch error garbage", content_patch_failure_garbage, - Patch diff_patch_failure_garbage; - "patch truncated", content_patch_failure_truncated, - Patch diff_patch_failure_truncated; + { label = "normal"; + content = content_working_diff; + kind = DiffPatch; + }; + { label = "diff file/dir error"; + content = content_dir_file; + kind = DiffPatch; + }; + { label = "diff dir/file error"; + content = content_file_dir; + kind = DiffPatch; + }; + { label = "symlink fst"; + content = content_symlink_fst; + kind = DiffPatch; + }; + { label = "symlink snd"; + content = content_symlink_snd; + kind = DiffPatch; + }; + { label = "hardlink fst"; + content = content_hardlink_fst; + kind = DiffPatch; + }; + { label = "hardlink snd"; + content = content_hardlink_snd; + kind = DiffPatch; + }; + { label = "patch error garbage"; + content = content_patch_failure_garbage; + kind = Patch diff_patch_failure_garbage; + }; + { label = "patch truncated"; + content = content_patch_failure_truncated; + kind = Patch diff_patch_failure_truncated; + }; ] let () = @@ -304,9 +336,9 @@ let () = set_binary_mode_out stdout true; Unix.dup2 Unix.stdout Unix.stderr; OpamFilename.with_tmp_dir @@ fun dir -> - List.iteri (fun i (label, content, kind) -> + List.iteri (fun i setup -> print "\n----------------------\n"; - print " Test %d: %s\n" (i+1) label; + print " Test %d: %s\n" (i+1) setup.label; print "----------------------\n\n"; - diff_patch dir content kind) + diff_patch dir setup) tests From 3d0a20f7c48e88c52edc3831a22d23165682adb6 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Wed, 30 Apr 2025 16:00:53 +0200 Subject: [PATCH 03/10] patchDiff: add the ability to create empty directories --- tests/lib/patchDiff.expected | 4 ++++ tests/lib/patchDiff.ml | 1 + 2 files changed, 5 insertions(+) diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index f24f4355d3a..31ab5d9bd25 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -24,6 +24,7 @@ > foo + first/file-only-fst > foo ++ first/same-dir + first/same-file > foo + second/ @@ -43,6 +44,7 @@ > bar + second/file-only-snd > foo ++ second/same-dir + second/same-file > foo @@ -100,6 +102,7 @@ + first/dir-only-fst + first/file-only-snd > foo ++ first/same-dir + first/same-file > foo + second/ @@ -119,6 +122,7 @@ > bar + second/file-only-snd > foo ++ second/same-dir + second/same-file > foo diff --git a/tests/lib/patchDiff.ml b/tests/lib/patchDiff.ml index 89d80a26041..5dd0879cc4b 100644 --- a/tests/lib/patchDiff.ml +++ b/tests/lib/patchDiff.ml @@ -222,6 +222,7 @@ let write_setup dir content = OpamFilename.write (inner_dir // name) content | Dir lst -> let inner_dir = inner_dir / name in + OpamFilename.mkdir inner_dir; List.iter (fun (n,c) -> OpamFilename.write (inner_dir // n) c) lst | Symlink -> OpamFilename.link ~relative:false ~target:(link_f ()) From f7216b99a16c31fd64602b6360f3ff8e2624eada Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Wed, 30 Apr 2025 16:01:37 +0200 Subject: [PATCH 04/10] patchDiff: add some tests showing the handling of empty files and new directories --- master_changes.md | 1 + tests/lib/patchDiff.expected | 116 +++++++++++++++++++++++++++++++++++ tests/lib/patchDiff.ml | 65 +++++++++++++++++++- 3 files changed, 181 insertions(+), 1 deletion(-) diff --git a/master_changes.md b/master_changes.md index c893811177c..294fb9166f9 100644 --- a/master_changes.md +++ b/master_changes.md @@ -105,6 +105,7 @@ users) ## Internal: Windows ## Test + * patchDiff: add some tests showing the handling of empty files and new directories [#6490 @rjbou] ## Benchmarks diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index 31ab5d9bd25..d5fe48305ea 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -341,3 +341,119 @@ patch format + second/same-file > foo + +---------------------- + Test 10: add empty file +---------------------- + +*** SETUP *** ++ first/ ++ first/same-file + > foo ++ second/ ++ second/im-empty ++ second/same-file + > foo + +*** DIFF *** +ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed + +---------------------- + Test 11: remove empty file +---------------------- + +*** SETUP *** ++ first/ ++ first/im-empty ++ first/same-file + > foo ++ second/ ++ second/same-file + > foo + +*** DIFF *** +ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed + +---------------------- + Test 12: move file into a new directory +---------------------- + +*** SETUP *** ++ first/ ++ first/move-me + > bar ++ first/same-file + > foo ++ second/ ++ second/inner ++ second/inner/move-me + > bar ++ second/same-file + > foo + +*** DIFF *** +--- /dev/null ++++ second/inner/move-me +@@ -0,0 +1,1 @@ ++bar +--- first/move-me ++++ /dev/null +@@ -1,1 +0,0 @@ +-bar + +*** PATCHED *** ++ first/ ++ first/inner ++ first/inner/move-me + > bar ++ first/same-file + > foo ++ second/ ++ second/inner ++ second/inner/move-me + > bar ++ second/same-file + > foo + + +---------------------- + Test 13: delete file that deletes the directory +---------------------- + +*** SETUP *** ++ first/ ++ first/im-here ++ first/im-here/delete-me + > bar ++ first/im-not-here ++ first/im-not-here/delete-me + > baz ++ first/same-file + > foo ++ second/ ++ second/im-here ++ second/same-file + > foo + +*** DIFF *** +--- first/im-here/delete-me ++++ /dev/null +@@ -1,1 +0,0 @@ +-bar +--- first/im-not-here/delete-me ++++ /dev/null +@@ -1,1 +0,0 @@ +-baz +\ No newline at end of file + +*** PATCHED *** ++ first/ ++ first/im-here ++ first/im-not-here ++ first/same-file + > foo ++ second/ ++ second/im-here ++ second/same-file + > foo + diff --git a/tests/lib/patchDiff.ml b/tests/lib/patchDiff.ml index 5dd0879cc4b..8575cd7ce3f 100644 --- a/tests/lib/patchDiff.ml +++ b/tests/lib/patchDiff.ml @@ -1,6 +1,7 @@ type content = | File of string (* file *) | Dir of (string * string) list (* directory with list filename * content *) + | NamedDir of string * (string * string) list (* directory with list filename * content *) | Symlink (* Soft Link *) | Hardlink (* Hard link *) | V (* void *) @@ -164,6 +165,43 @@ let _good_diff = "@@ -2,1 +2,0 @@\n" ^ "-bar\n" +let content_empty_file_snd = [ + same_file; + { name = "im-empty"; + first = V; + second = File ""; + }; +] + +let content_empty_file_fst = [ + same_file; + { name = "im-empty"; + first = File ""; + second = V; + }; +] + +let content_file_fst_to_file_in_dir_snd = [ + same_file; + let name = "move-me" in + { name; + first = File bar; + second = NamedDir ("inner", [name, bar]); + }; +] + +let content_single_file_in_dir_snd = [ + same_file; + { name = "im-here"; + first = Dir [ "delete-me", bar]; + second = Dir []; + }; + { name = "im-not-here"; + first = Dir [ "delete-me", "baz"]; + second = V; + }; +] + (** Utils *) let print = Printf.printf @@ -220,7 +258,13 @@ let write_setup dir content = let create inner_dir name = function | File content -> OpamFilename.write (inner_dir // name) content - | Dir lst -> + | (Dir _ | NamedDir _) as cdir -> + let name, lst = + match cdir with + | Dir lst -> name, lst + | NamedDir (name, lst) -> name, lst + | _ -> assert false + in let inner_dir = inner_dir / name in OpamFilename.mkdir inner_dir; List.iter (fun (n,c) -> OpamFilename.write (inner_dir // n) c) lst @@ -271,6 +315,9 @@ let diff_patch dir setup = (OpamFilename.Base.of_string second) with | exception Failure s -> print "ERROR: %s\n" s; None + | exception e -> + print "ERROR: %s\n" (Printexc.to_string e); + None | None -> print "No diff\n"; None | some -> some in @@ -329,6 +376,22 @@ let tests = [ content = content_patch_failure_truncated; kind = Patch diff_patch_failure_truncated; }; + { label = "add empty file"; + content = content_empty_file_snd; + kind = DiffPatch; + }; + { label = "remove empty file"; + content = content_empty_file_fst; + kind = DiffPatch; + }; + { label = "move file into a new directory"; + content = content_file_fst_to_file_in_dir_snd; + kind = DiffPatch; + }; + { label = "delete file that deletes the directory"; + content = content_single_file_in_dir_snd; + kind = DiffPatch; + }; ] let () = From e6399c55b318b971947841936ea7c037b7021075 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Wed, 30 Apr 2025 21:55:39 +0200 Subject: [PATCH 05/10] patchDiff: test the diff parser when generated using 'git diff' --- master_changes.md | 1 + tests/lib/dune | 2 +- tests/lib/patchDiff.expected | 127 +++++++++++++++++++++++++++++++++++ tests/lib/patchDiff.ml | 109 +++++++++++++++++++++++++----- 4 files changed, 222 insertions(+), 17 deletions(-) diff --git a/master_changes.md b/master_changes.md index 294fb9166f9..9a9f40be71a 100644 --- a/master_changes.md +++ b/master_changes.md @@ -106,6 +106,7 @@ users) ## Test * patchDiff: add some tests showing the handling of empty files and new directories [#6490 @rjbou] + * patchDiff: test the diff parser when generated using `git diff` [#6490 @rjbou] ## Benchmarks diff --git a/tests/lib/dune b/tests/lib/dune index 56108d82818..ddf5ae5fd46 100644 --- a/tests/lib/dune +++ b/tests/lib/dune @@ -12,4 +12,4 @@ (test (name patchDiff) (modules patchDiff) - (libraries opam-repository)) + (libraries str opam-repository)) diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index d5fe48305ea..f5b9de504ac 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -126,6 +126,97 @@ + second/same-file > foo +*** GIT DIFF *** +diff --git b/diff-dir-plus-fst/fst a/diff-dir-plus-fst/fst +index c0ffee..c0ffee c0ffee +--- b/diff-dir-plus-fst/fst ++++ a/diff-dir-plus-fst/fst +@@ -1 +1,2 @@ + foo ++bar +diff --git b/diff-dir-plus-snd/fst a/diff-dir-plus-snd/fst +index c0ffee..c0ffee c0ffee +--- b/diff-dir-plus-snd/fst ++++ a/diff-dir-plus-snd/fst +@@ -1,2 +1 @@ + foo +-bar +diff --git b/diff-file a/diff-file +index c0ffee..c0ffee c0ffee +--- b/diff-file ++++ a/diff-file +@@ -1 +1 @@ +-foo ++bar +diff --git b/diff-file-plus-fst a/diff-file-plus-fst +index c0ffee..c0ffee c0ffee +--- b/diff-file-plus-fst ++++ a/diff-file-plus-fst +@@ -1,2 +1 @@ + foo +-bar +diff --git b/diff-file-plus-snd a/diff-file-plus-snd +index c0ffee..c0ffee c0ffee +--- b/diff-file-plus-snd ++++ a/diff-file-plus-snd +@@ -1 +1,2 @@ + foo ++bar +diff --git b/file-only-fst a/file-only-fst +deleted file mode c0ffee +index c0ffee..c0ffee +--- b/file-only-fst ++++ /dev/null +@@ -1 +0,0 @@ +-foo +diff --git b/dir-only-fst/fst a/file-only-snd +similarity index 100% +rename from dir-only-fst/fst +rename to file-only-snd + +*** GIT PATCHED *** ++ first/ ++ first/diff-dir-plus-fst ++ first/diff-dir-plus-fst/fst + > foo + > bar ++ first/diff-dir-plus-snd ++ first/diff-dir-plus-snd/fst + > foo ++ first/diff-file + > bar ++ first/diff-file-plus-fst + > foo ++ first/diff-file-plus-snd + > foo + > bar ++ first/dir-only-fst ++ first/file-only-snd + > foo ++ first/same-dir ++ first/same-file + > foo ++ second/ ++ second/diff-dir-plus-fst ++ second/diff-dir-plus-fst/fst + > foo + > bar ++ second/diff-dir-plus-snd ++ second/diff-dir-plus-snd/fst + > foo ++ second/diff-file + > bar ++ second/diff-file-plus-fst + > foo ++ second/diff-file-plus-snd + > foo + > bar ++ second/file-only-snd + > foo ++ second/same-dir ++ second/same-file + > foo + ---------------------- Test 2: diff file/dir error @@ -415,6 +506,14 @@ ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed + second/same-file > foo +*** GIT DIFF *** +diff --git b/move-me a/inner/move-me +similarity index 100% +rename from move-me +rename to inner/move-me + +*** GIT PATCH ERROR *** +ERROR: Unix.Unix_error(Unix.ENOENT, "rename", "/tmp/build_c0ffee_dune/opam-c0ffee-c0ffee/first/move-me") ---------------------- Test 13: delete file that deletes the directory @@ -457,3 +556,31 @@ ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed + second/same-file > foo +*** GIT DIFF *** +diff --git b/im-here/delete-me a/im-here/delete-me +deleted file mode c0ffee +index c0ffee..c0ffee +--- b/im-here/delete-me ++++ /dev/null +@@ -1 +0,0 @@ +-bar +diff --git b/im-not-here/delete-me a/im-not-here/delete-me +deleted file mode c0ffee +index c0ffee..c0ffee +--- b/im-not-here/delete-me ++++ /dev/null +@@ -1 +0,0 @@ +-baz +\ No newline at end of file + +*** GIT PATCHED *** ++ first/ ++ first/im-here ++ first/im-not-here ++ first/same-file + > foo ++ second/ ++ second/im-here ++ second/same-file + > foo + diff --git a/tests/lib/patchDiff.ml b/tests/lib/patchDiff.ml index 8575cd7ce3f..4a1d0b0e96c 100644 --- a/tests/lib/patchDiff.ml +++ b/tests/lib/patchDiff.ml @@ -205,6 +205,12 @@ let content_single_file_in_dir_snd = [ (** Utils *) let print = Printf.printf +let rm_hex = + let re = + Str.regexp {|[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]?|} + in + let by = "c0ffee" in + fun s -> Str.global_replace re by s open OpamFilename.Op let read_dir root names = @@ -221,6 +227,8 @@ let read_dir root names = (OpamFilename.rec_dirs dir)) names |> List.flatten + |> List.filter (fun (name, _) -> + not (OpamStd.String.contains ~sub:".git" name)) |> List.map (fun (file, content) -> (OpamSystem.back_to_forward file, content)) in @@ -239,13 +247,13 @@ let read_dir root names = let first = "first" let second = "second" -let write_setup dir content = +let write_setup ?(only_fst=false) dir content = let first_root = dir / first in let second_root = dir / second in List.iter (fun d -> OpamFilename.cleandir d; OpamFilename.mkdir d) - [ first_root; second_root; ] ; + (if only_fst then [first_root] else [ first_root; second_root; ]); let link_f = let link = lazy ( let f = dir // "linked_file" in @@ -279,9 +287,53 @@ let write_setup dir content = in List.iter (fun {name; first; second} -> create first_root name first; - create second_root name second) + if not only_fst then create second_root name second) content +(* --Git-- *) +let git_cmds repo_root commands error_msg = + let commands = + List.map (fun args -> + OpamSystem.make_command "git" + ("-C"::(OpamFilename.Dir.to_string repo_root)::args)) + commands + in + try + List.iter (fun command -> + match OpamProcess.run command with + | {OpamProcess.r_code = 0; _ } -> () + | _ -> failwith (OpamProcess.string_of_command command)) + commands + with Failure e -> + print "ERROR:%s: %s\n" error_msg (rm_hex e) + +let make_git_repo dir = + let first_root = dir / first in + let commands = [ + [ "init"]; + [ "add"; "--all" ]; + [ "commit"; "-qm"; "first" ]; + ] in + git_cmds first_root commands "Git init" + +let generate_git_diff dir = + let first_root = dir / first in + let name = dir // "diff-git" in + OpamFilename.remove name; + OpamFilename.touch name; + let commands = [ + [ "add"; "--all" ]; + [ "commit"; "-qm"; "second" ]; + [ "status" ]; + [ "-c"; "diff.noprefix=false"; "diff"; "--text"; "--no-ext-diff"; "-R"; "-p"; + "HEAD..HEAD^"; "--output="^(OpamFilename.to_string name) ] + ] in + git_cmds first_root commands "Git generate diff"; + print "*** GIT DIFF ***\n"; + print "%s\n" (rm_hex @@ OpamFilename.read name); + name +(* --Git-- *) + type diff_patch = | DiffPatch | Patch of string @@ -290,13 +342,14 @@ type setup = { label: string; (* setup label *) content: arborescence list; (* the content of directory, first and second one *) kind: diff_patch; (* what test to run *) + git: bool; (* add a test where the first directory is a git directory or not *) } let print_dirs dir = print "%s\n" (read_dir dir [ first; second ]) let diff_patch dir setup = - let { content; kind; _ } = setup in + let { content; kind; git; _ } = setup in write_setup dir content; print "*** SETUP ***\n"; print_dirs dir; @@ -314,9 +367,9 @@ let diff_patch dir setup = (OpamFilename.Base.of_string first) (OpamFilename.Base.of_string second) with - | exception Failure s -> print "ERROR: %s\n" s; None + | exception Failure s -> print "ERROR: %s\n" (rm_hex s); None | exception e -> - print "ERROR: %s\n" (Printexc.to_string e); + print "ERROR: %s\n" (rm_hex @@ Printexc.to_string e); None | None -> print "No diff\n"; None | some -> some @@ -324,18 +377,29 @@ let diff_patch dir setup = match diff with | None -> () | Some diff -> + if git then make_git_repo dir; print "%s\n" (OpamFilename.read diff); - let result = - OpamFilename.patch ~allow_unclean:false diff - (dir / first) + let apply ~git diff = + let git = if git then "GIT " else "" in + let result = + OpamFilename.patch ~allow_unclean:false diff + (dir / first) + in + match result with + | None -> + print "*** %sPATCHED ***\n" git; + print_dirs dir; + true + | Some exn -> + print "*** %sPATCH ERROR ***\n" git; + print "ERROR: %s\n" (rm_hex @@ Printexc.to_string exn); + false in - match result with - | None -> - print "*** PATCHED ***\n"; - print_dirs dir - | Some exn -> - print "*** PATCH ERROR ***\n"; - print "ERROR: %s\n" (Printexc.to_string exn) + let patched = apply ~git:false diff in + if patched && git then + (let diff = generate_git_diff dir in + write_setup ~only_fst:true dir content; + let _ : bool = apply ~git:true diff in ()) (** The tests *) @@ -343,54 +407,67 @@ let tests = [ { label = "normal"; content = content_working_diff; kind = DiffPatch; + git = true; }; { label = "diff file/dir error"; content = content_dir_file; kind = DiffPatch; + git = true; }; { label = "diff dir/file error"; content = content_file_dir; kind = DiffPatch; + git = true; }; { label = "symlink fst"; content = content_symlink_fst; kind = DiffPatch; + git = false; }; { label = "symlink snd"; content = content_symlink_snd; kind = DiffPatch; + git = false; }; { label = "hardlink fst"; content = content_hardlink_fst; kind = DiffPatch; + git = false; }; { label = "hardlink snd"; content = content_hardlink_snd; kind = DiffPatch; + git = false; }; { label = "patch error garbage"; content = content_patch_failure_garbage; kind = Patch diff_patch_failure_garbage; + git = false; }; { label = "patch truncated"; content = content_patch_failure_truncated; kind = Patch diff_patch_failure_truncated; + git = false; }; { label = "add empty file"; content = content_empty_file_snd; kind = DiffPatch; + git = true; }; { label = "remove empty file"; content = content_empty_file_fst; kind = DiffPatch; + git = true; }; { label = "move file into a new directory"; content = content_file_fst_to_file_in_dir_snd; kind = DiffPatch; + git = true; }; { label = "delete file that deletes the directory"; content = content_single_file_in_dir_snd; kind = DiffPatch; + git = true; }; ] From 6ff5d0be8f5d1283a64e318c0af926dbffeffef7 Mon Sep 17 00:00:00 2001 From: Kate Date: Fri, 2 May 2025 16:40:24 +0100 Subject: [PATCH 06/10] Upgrade to patch 3.0.0~alpha2 --- .github/workflows/ci.ml | 4 +- .github/workflows/depexts.yml | 2 +- .github/workflows/main.yml | 4 +- master_changes.md | 4 ++ opam-core.opam | 2 +- src/core/opamSystem.ml | 6 +-- src/repository/opamRepositoryBackend.ml | 32 ++++++-------- src_ext/Makefile.sources | 4 +- tests/lib/patchDiff.expected | 54 +++++++++++++++++++++++- tests/reftests/repository-patchdiff.test | 6 +-- 10 files changed, 83 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.ml b/.github/workflows/ci.ml index 6c4064484a4..85461b47fc3 100644 --- a/.github/workflows/ci.ml +++ b/.github/workflows/ci.ml @@ -517,8 +517,8 @@ let main oc : unit = ("OPAM12CACHE", "~/.cache/opam1.2/cache"); (* These should be identical to the values in appveyor.yml *) ("OPAM_REPO", "https://github.com/ocaml/opam-repository.git"); - ("OPAM_TEST_REPO_SHA", "0c42e982f4cf97fc698132fb2a16b49524a26ab3"); - ("OPAM_REPO_SHA", "0c42e982f4cf97fc698132fb2a16b49524a26ab3"); + ("OPAM_TEST_REPO_SHA", "35eb2f107a989a2d623b0bbe170696398fcb9b1e"); + ("OPAM_REPO_SHA", "35eb2f107a989a2d623b0bbe170696398fcb9b1e"); ("SOLVER", ""); (* Cygwin configuration *) ("CYGWIN_MIRROR", "http://mirrors.kernel.org/sourceware/cygwin/"); diff --git a/.github/workflows/depexts.yml b/.github/workflows/depexts.yml index 180f6c59715..6ca11733b22 100644 --- a/.github/workflows/depexts.yml +++ b/.github/workflows/depexts.yml @@ -19,7 +19,7 @@ defaults: env: OPAMVERSION: 2.4.0-alpha1 OPAM_REPO: https://github.com/ocaml/opam-repository.git - OPAM_REPO_SHA: 335860ac6bdab55d9d7376c827d68c361a4a30f9 + OPAM_REPO_SHA: 35eb2f107a989a2d623b0bbe170696398fcb9b1e jobs: opam-cache: diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index cc916e2d34a..55c8a24929d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -26,8 +26,8 @@ env: OPAMBSROOT: ~/.cache/.opam.cached OPAM12CACHE: ~/.cache/opam1.2/cache OPAM_REPO: https://github.com/ocaml/opam-repository.git - OPAM_TEST_REPO_SHA: 0c42e982f4cf97fc698132fb2a16b49524a26ab3 - OPAM_REPO_SHA: 0c42e982f4cf97fc698132fb2a16b49524a26ab3 + OPAM_TEST_REPO_SHA: 35eb2f107a989a2d623b0bbe170696398fcb9b1e + OPAM_REPO_SHA: 35eb2f107a989a2d623b0bbe170696398fcb9b1e SOLVER: CYGWIN_MIRROR: http://mirrors.kernel.org/sourceware/cygwin/ CYGWIN_ROOT: D:\cygwin diff --git a/master_changes.md b/master_changes.md index 9a9f40be71a..0a7ebfe0b7a 100644 --- a/master_changes.md +++ b/master_changes.md @@ -46,6 +46,8 @@ users) ## Var/Option ## Update / Upgrade + * Fix a crash when updating a repository that is deleting or adding empty files [#6490 @kit-ty-kate] + * Fix an extreme performance issue (takes several hours) when applying a large repository update [#6490 @kit-ty-kate] ## Tree @@ -75,6 +77,8 @@ users) ## VCS ## Build + * Update the requirement for the `patch` library to `3.0.0~alpha2` [#6490 @kit-ty-kate] + * Upgrade the downloaded-if-missing `patch` to `3.0.0~alpha2` [#6490 @kit-ty-kate] ## Infrastructure diff --git a/opam-core.opam b/opam-core.opam index caa4b881246..7df832e02ed 100644 --- a/opam-core.opam +++ b/opam-core.opam @@ -30,7 +30,7 @@ depends: [ "sha" {>= "1.13"} "jsonm" "swhid_core" - "patch" {>= "3.0.0~alpha1"} + "patch" {>= "3.0.0~alpha2"} "uutf" (("host-system-mingw" {os = "win32" & os-distribution != "cygwinports"} & "conf-mingw-w64-gcc-i686" {os = "win32" & os-distribution != "cygwinports"} & diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index e240847f8a5..4feab33cbbe 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -1630,14 +1630,14 @@ let internal_patch ~allow_unclean ~patch_filename ~dir diffs = let content = read file in let content = patch ~file:file (Some content) diff in write file content; - | Patch.Delete file -> + | Patch.Delete file | Patch.Git_ext (file, _, Patch.Delete_only) -> let file = get_path file in remove_file_t ~with_log:false file - | Patch.Create file -> + | Patch.Create file | Patch.Git_ext (_, file, Patch.Create_only) -> let file = get_path file in let content = patch ~file None diff in write file content - | Patch.Rename_only (src, dst) -> + | Patch.Git_ext (_, _, Patch.Rename_only (src, dst)) -> let src = get_path src in let dst = get_path dst in (* we use rename as we have all guarantee *) diff --git a/src/repository/opamRepositoryBackend.ml b/src/repository/opamRepositoryBackend.ml index ec3e4d8461b..aec9119cbda 100644 --- a/src/repository/opamRepositoryBackend.ml +++ b/src/repository/opamRepositoryBackend.ml @@ -79,10 +79,10 @@ let get_files_for_diff parent_dir dir1 dir2 = match dir1, dir2 with | None, None -> assert false | Some dir, None -> - List.map (fun file -> Patch.Delete (dir^"/"^file)) + List.map (fun file -> (Some (dir^"/"^file), None)) (getfiles parent_dir dir) | None, Some dir -> - List.map (fun file -> Patch.Create (dir^"/"^file)) + List.map (fun file -> (None, Some (dir^"/"^file))) (getfiles parent_dir dir) | Some dir1, Some dir2 -> let files1 = List.fast_sort String.compare (getfiles parent_dir dir1) in @@ -91,18 +91,16 @@ let get_files_for_diff parent_dir dir1 dir2 = | (file1::files1 as orig1), (file2::files2 as orig2) -> let cmp = String.compare file1 file2 in if cmp = 0 then - aux (Patch.Edit - (dir1^"/"^file1, dir2^"/"^file2) - :: acc) + aux ((Some (dir1^"/"^file1), Some (dir2^"/"^file2)) :: acc) files1 files2 else if cmp < 0 then - aux (Patch.Delete (dir1^"/"^file1) :: acc) files1 orig2 + aux ((Some (dir1^"/"^file1), None) :: acc) files1 orig2 else - aux (Patch.Create (dir2^"/"^file2) :: acc) orig1 files2 + aux ((None, Some (dir2^"/"^file2)) :: acc) orig1 files2 | file1::files1, [] -> - aux (Patch.Delete (dir1^"/"^file1) :: acc) files1 [] + aux ((Some (dir1^"/"^file1), None) :: acc) files1 [] | [], file2::files2 -> - aux (Patch.Create (dir2^"/"^file2) :: acc) [] files2 + aux ((None, Some (dir2^"/"^file2)) :: acc) [] files2 | [], [] -> acc in @@ -115,8 +113,10 @@ let get_diff parent_dir dir1 dir2 = (slog OpamFilename.Base.to_string) dir1 (slog OpamFilename.Base.to_string) dir2; let readfile parent_dir file = - let file = Filename.concat (OpamFilename.Dir.to_string parent_dir) file in - OpamSystem.read file + let real_file = + Filename.concat (OpamFilename.Dir.to_string parent_dir) file + in + (file, OpamSystem.read real_file) in let lstat_opt parent_dir = function | None -> None @@ -127,15 +127,9 @@ let get_diff parent_dir dir1 dir2 = let rec aux diffs dir1 dir2 = let files = get_files_for_diff parent_dir dir1 dir2 in let diffs = - List.fold_left (fun diffs operation -> - let file1, file2 = match operation with - | Patch.Delete filename -> (Some filename, None) - | Patch.Create filename -> (None, Some filename) - | Patch.Edit (file1, file2) - | Patch.Rename_only (file1, file2) -> (Some file1, Some file2) - in + List.fold_left (fun diffs (file1, file2) -> let add_to_diffs content1 content2 diffs = - match Patch.diff operation content1 content2 with + match Patch.diff content1 content2 with | None -> diffs | Some diff -> diff :: diffs in diff --git a/src_ext/Makefile.sources b/src_ext/Makefile.sources index 6bf1bf8fc51..f19f5e76e0b 100644 --- a/src_ext/Makefile.sources +++ b/src_ext/Makefile.sources @@ -60,5 +60,5 @@ MD5_swhid_core = 77d88d4b1d96261c866f140c64d89af8 URL_menhir = https://gitlab.inria.fr/fpottier/menhir/-/archive/20240715/archive.tar.gz MD5_menhir = d39a8943fe1be28199e5ec1f4133504c -URL_patch = https://github.com/hannesm/patch/releases/download/v3.0.0-alpha1/patch-3.0.0-alpha1.tar.gz -MD5_patch = 03aa87f8500c9caf4a73b2299c19b514 +URL_patch = https://github.com/hannesm/patch/releases/download/v3.0.0-alpha2/patch-3.0.0-alpha2.tar.gz +MD5_patch = 7f11023c7231b916cfe3dd28ff6ce948 diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index f5b9de504ac..f6e63902d95 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -447,7 +447,34 @@ patch format > foo *** DIFF *** -ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed +diff --git second/im-empty second/im-empty +new file mode 100644 + +*** PATCHED *** ++ first/ ++ first/im-empty ++ first/same-file + > foo ++ second/ ++ second/im-empty ++ second/same-file + > foo + +*** GIT DIFF *** +diff --git b/im-empty a/im-empty +new file mode c0ffee +index c0ffee..c0ffee + +*** GIT PATCHED *** ++ first/ ++ first/im-empty ++ first/same-file + > foo ++ second/ ++ second/im-empty ++ second/same-file + > foo + ---------------------- Test 11: remove empty file @@ -463,7 +490,30 @@ ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed > foo *** DIFF *** -ERROR: File "src/patch.ml", line 423, characters 58-64: Assertion failed +diff --git first/im-empty first/im-empty +deleted file mode 100644 + +*** PATCHED *** ++ first/ ++ first/same-file + > foo ++ second/ ++ second/same-file + > foo + +*** GIT DIFF *** +diff --git b/im-empty a/im-empty +deleted file mode c0ffee +index c0ffee..c0ffee + +*** GIT PATCHED *** ++ first/ ++ first/same-file + > foo ++ second/ ++ second/same-file + > foo + ---------------------- Test 12: move file into a new directory diff --git a/tests/reftests/repository-patchdiff.test b/tests/reftests/repository-patchdiff.test index 5b15301edd8..338aaeb0e2b 100644 --- a/tests/reftests/repository-patchdiff.test +++ b/tests/reftests/repository-patchdiff.test @@ -34,6 +34,7 @@ REPOSITORY update repo from git+file://${BASEDIR}/REPO REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff ### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git OPAM/repo/repo/packages/first/first.1/files +OPAM/repo/repo/packages/first/first.1/files/empty OPAM/repo/repo/packages/first/first.1/files/fst OPAM/repo/repo/packages/first/first.1/files/fst/a-file OPAM/repo/repo/packages/first/first.1/files/fst/another-file @@ -45,9 +46,8 @@ rm 'packages/first/first.1/files/empty' <><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> REPOSITORY update repo from git+file://${BASEDIR}/REPO -REPOSITORY update empty, no validation performed -[repo] no changes from git+file://${BASEDIR}/REPO -REPOSITORY repo: applying empty update +[repo] synchronised from git+file://${BASEDIR}/REPO +REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff ### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git OPAM/repo/repo/packages/first/first.1/files OPAM/repo/repo/packages/first/first.1/files/fst From 11612897eb21673ae9fd3b23227e61ce33fa1ad3 Mon Sep 17 00:00:00 2001 From: Kate Date: Fri, 2 May 2025 16:59:01 +0100 Subject: [PATCH 07/10] Speedup OpamSystem.dir_is_empty and change its type to handle unreachable directories better --- master_changes.md | 1 + src/client/opamAuxCommands.ml | 16 ++++++++++------ src/client/opamClient.ml | 4 +--- src/core/opamDirTrack.ml | 2 +- src/core/opamFilename.ml | 2 +- src/core/opamFilename.mli | 4 +++- src/core/opamSystem.ml | 13 +++++++++++-- src/core/opamSystem.mli | 4 +++- src/repository/opamHTTP.ml | 3 +-- src/repository/opamLocal.ml | 5 ++--- src/state/opamRepositoryState.ml | 2 +- 11 files changed, 35 insertions(+), 21 deletions(-) diff --git a/master_changes.md b/master_changes.md index 0a7ebfe0b7a..2c309321ab8 100644 --- a/master_changes.md +++ b/master_changes.md @@ -145,3 +145,4 @@ users) ## opam-core * `OpamSystem`: remove `print_stats` function [#6485 @hannesm] + * `OpamSystem.dir_is_empty`: Speedup and change its type to handle unreachable directories better [#6490 @kit-ty-kate] diff --git a/src/client/opamAuxCommands.ml b/src/client/opamAuxCommands.ml index 187d90dc603..4f6ebf73220 100644 --- a/src/client/opamAuxCommands.ml +++ b/src/client/opamAuxCommands.ml @@ -64,14 +64,18 @@ let remove_files_from_destdir st pfx packages = (OpamConsole.colorise `bold (OpamFilename.to_string f)); if not OpamStateConfig.(!r.dryrun) then OpamFilename.remove f) else if OpamFilename.exists_dir d then - if OpamFilename.dir_is_empty d then - (if OpamConsole.verbose () then - OpamConsole.msg "Removing %s\n" - (OpamConsole.colorise `bold (OpamFilename.Dir.to_string d)); - if not OpamStateConfig.(!r.dryrun) then OpamFilename.rmdir d) - else + begin match OpamFilename.dir_is_empty d with + | Some true -> + if OpamConsole.verbose () then + OpamConsole.msg "Removing %s\n" + (OpamConsole.colorise `bold (OpamFilename.Dir.to_string d)); + if not OpamStateConfig.(!r.dryrun) then + OpamFilename.rmdir d + | Some false -> OpamConsole.note "Not removing non-empty directory %s" (OpamConsole.colorise `bold (OpamFilename.Dir.to_string d)) + | None -> () + end | _ -> () let name_from_project_dirname d = diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml index da2da2bdc33..566677c1f81 100644 --- a/src/client/opamClient.ml +++ b/src/client/opamClient.ml @@ -1805,9 +1805,7 @@ let init log "INIT %a" (slog @@ OpamStd.Option.to_string OpamRepositoryBackend.to_string) repo; let original_root = OpamStateConfig.(!r.original_root_dir) in - let root_empty = - not (OpamFilename.exists_dir original_root) - || OpamFilename.dir_is_empty original_root in + let root_empty = OpamFilename.dir_is_empty original_root <> Some false in let root = OpamStateConfig.(!r.root_dir) in let root, remove_root = let ignore_non_fatal f x = diff --git a/src/core/opamDirTrack.ml b/src/core/opamDirTrack.ml index 74f5c7fcb07..a20c487bcb3 100644 --- a/src/core/opamDirTrack.ml +++ b/src/core/opamDirTrack.ml @@ -257,7 +257,7 @@ let revert ?title ?(verbose=OpamConsole.verbose()) ?(force=false) (already, fname::modified, nonempty, cannot) else if cur_item_ct = Some Dir then let d = OpamFilename.Dir.of_string f in - if OpamFilename.dir_is_empty d then + if OpamFilename.dir_is_empty d = Some true then (rmdir d; acc) else let nonempty = diff --git a/src/core/opamFilename.ml b/src/core/opamFilename.ml index bd438f89cc8..7f1081c1629 100644 --- a/src/core/opamFilename.ml +++ b/src/core/opamFilename.ml @@ -74,7 +74,7 @@ let rmdir dirname = let rec rmdir_cleanup dirname = let sd = Dir.to_string dirname in - if OpamSystem.dir_is_empty sd then ( + if OpamSystem.dir_is_empty sd = Some true then ( rmdir dirname; let parent = Filename.dirname sd in if parent <> sd then rmdir_cleanup parent diff --git a/src/core/opamFilename.mli b/src/core/opamFilename.mli index 1b571223967..5c82a7234c3 100644 --- a/src/core/opamFilename.mli +++ b/src/core/opamFilename.mli @@ -50,7 +50,9 @@ val mkdir: Dir.t -> unit (** List the sub-directory recursively *) val rec_dirs: Dir.t -> Dir.t list -val dir_is_empty: Dir.t -> bool +(** Returns whether a directory is empty. + Returns [None] if the directory could not be found. *) +val dir_is_empty: Dir.t -> bool option (** List the sub-directory (do not recurse) *) val dirs: Dir.t -> Dir.t list diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 4feab33cbbe..3d336054f93 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -389,8 +389,17 @@ let dirs dir = directories_with_links dir let dir_is_empty dir = - try in_dir dir (fun () -> Sys.readdir (Sys.getcwd ()) = [||]) - with File_not_found _ -> false + try + let dir = Unix.opendir dir in + Fun.protect ~finally:(fun () -> Unix.closedir dir) @@ fun () -> + let rec loop () = + match Unix.readdir dir with + | "." | ".." -> loop () + | _ -> false + | exception End_of_file -> true + in + Some (loop ()) + with Unix.Unix_error(Unix.ENOENT, _, _) -> None let with_tmp_dir fn = let dir = mk_temp_dir () in diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index 2668ccf2c59..caa7058c629 100644 --- a/src/core/opamSystem.mli +++ b/src/core/opamSystem.mli @@ -174,7 +174,9 @@ val rec_dirs: string -> string list (** Return the list of directories in the current directory. *) val dirs: string -> string list -val dir_is_empty: string -> bool +(** Returns whether a directory is empty. + Returns [None] if the directory could not be found. *) +val dir_is_empty: string -> bool option (** [directories_with_links dir] returns the directories in the directory [dir]. Links pointing to directory are also returned. *) diff --git a/src/repository/opamHTTP.ml b/src/repository/opamHTTP.ml index fd4c4ac44e3..a95cdf23997 100644 --- a/src/repository/opamHTTP.ml +++ b/src/repository/opamHTTP.ml @@ -52,8 +52,7 @@ module B = struct @@ fun () -> OpamRepositoryBackend.job_text repo_name "sync" (sync_state repo_name quarantine url) @@+ fun () -> - if not (OpamFilename.exists_dir repo_root) || - OpamFilename.dir_is_empty repo_root then + if OpamFilename.dir_is_empty repo_root <> Some false then Done (OpamRepositoryBackend.Update_full quarantine) else OpamStd.Exn.finally finalise @@ fun () -> diff --git a/src/repository/opamLocal.ml b/src/repository/opamLocal.ml index 4320b36cc0f..0da5a4c12c7 100644 --- a/src/repository/opamLocal.ml +++ b/src/repository/opamLocal.ml @@ -87,7 +87,7 @@ let rsync ?(args=[]) ?(exclude_vcdirs=true) src dst = else ( OpamSystem.mkdir dst; let convert_path = Lazy.force convert_path in - call_rsync (fun () -> not (OpamSystem.dir_is_empty dst)) + call_rsync (fun () -> OpamSystem.dir_is_empty dst = Some false) ( rsync_arg :: args @ exclude_args @ [ "--delete"; "--delete-excluded"; convert_path src; convert_path dst; ]) @@| function @@ -174,8 +174,7 @@ module B = struct | Up_to_date _ -> finalise (); Done OpamRepositoryBackend.Update_empty | Result _ -> - if not (OpamFilename.exists_dir repo_root) || - OpamFilename.dir_is_empty repo_root then + if OpamFilename.dir_is_empty repo_root <> Some false then Done (OpamRepositoryBackend.Update_full quarantine) else OpamStd.Exn.finally finalise @@ fun () -> diff --git a/src/state/opamRepositoryState.ml b/src/state/opamRepositoryState.ml index ce1fb2dd140..0678b608ef3 100644 --- a/src/state/opamRepositoryState.ml +++ b/src/state/opamRepositoryState.ml @@ -130,7 +130,7 @@ let clean_repo_tmp tmp_dir = (let dir = Lazy.force tmp_dir in OpamFilename.rmdir dir; let parent = OpamFilename.dirname_dir dir in - if OpamFilename.dir_is_empty parent then + if OpamFilename.dir_is_empty parent = Some true then OpamFilename.rmdir parent) let remove_from_repos_tmp rt name = From ba9b450d5d4d160982529f6d2bb81cfeedc4c840 Mon Sep 17 00:00:00 2001 From: Kate Date: Fri, 2 May 2025 17:05:31 +0100 Subject: [PATCH 08/10] Move the implementation of OpamFilename.rmdir_cleanup to OpamSystem.rmdir_cleanup --- master_changes.md | 1 + src/core/opamFilename.ml | 9 ++------- src/core/opamSystem.ml | 8 ++++++++ src/core/opamSystem.mli | 5 +++++ 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/master_changes.md b/master_changes.md index 2c309321ab8..85b2878479c 100644 --- a/master_changes.md +++ b/master_changes.md @@ -145,4 +145,5 @@ users) ## opam-core * `OpamSystem`: remove `print_stats` function [#6485 @hannesm] + * `OpamSystem`: add the `rmdir_cleanup` function [#6490 @kit-ty-kate] * `OpamSystem.dir_is_empty`: Speedup and change its type to handle unreachable directories better [#6490 @kit-ty-kate] diff --git a/src/core/opamFilename.ml b/src/core/opamFilename.ml index 7f1081c1629..a64a0aba739 100644 --- a/src/core/opamFilename.ml +++ b/src/core/opamFilename.ml @@ -72,13 +72,8 @@ let with_tmp_dir_job fjob = let rmdir dirname = OpamSystem.remove_dir (Dir.to_string dirname) -let rec rmdir_cleanup dirname = - let sd = Dir.to_string dirname in - if OpamSystem.dir_is_empty sd = Some true then ( - rmdir dirname; - let parent = Filename.dirname sd in - if parent <> sd then rmdir_cleanup parent - ) +let rmdir_cleanup dirname = + OpamSystem.rmdir_cleanup (Dir.to_string dirname) let cwd () = Dir.of_string (Unix.getcwd ()) diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 3d336054f93..b066142f719 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -401,6 +401,14 @@ let dir_is_empty dir = Some (loop ()) with Unix.Unix_error(Unix.ENOENT, _, _) -> None +let rec rmdir_cleanup dirname = + if dir_is_empty dirname = Some true then ( + remove_dir dirname; + let parent = Filename.dirname dirname in + if parent <> (dirname : string) then + rmdir_cleanup parent + ) + let with_tmp_dir fn = let dir = mk_temp_dir () in try diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index caa7058c629..4ae39f1c3ab 100644 --- a/src/core/opamSystem.mli +++ b/src/core/opamSystem.mli @@ -141,6 +141,11 @@ val remove_file: string -> unit directory (not for symlinks or other files). *) val remove_dir: string -> unit +(** Removes an empty directory, as well as any empty leading path components. + Must be called only on a directory that is known to not have empty parents, + only internal opam directory (and not tmp dir). *) +val rmdir_cleanup : string -> unit + (** Change the current working directory *) val chdir: string -> unit From 62abb42a0647b75016d5bf5dac8d8fffac54d61e Mon Sep 17 00:00:00 2001 From: Kate Date: Fri, 2 May 2025 17:09:20 +0100 Subject: [PATCH 09/10] OpamSystem.internal_patch: remove parent directories when all of their content has been moved somewhere else --- master_changes.md | 1 + src/core/opamSystem.ml | 21 ++++++++++++--------- tests/lib/patchDiff.expected | 6 ------ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/master_changes.md b/master_changes.md index 85b2878479c..590799c4842 100644 --- a/master_changes.md +++ b/master_changes.md @@ -147,3 +147,4 @@ users) * `OpamSystem`: remove `print_stats` function [#6485 @hannesm] * `OpamSystem`: add the `rmdir_cleanup` function [#6490 @kit-ty-kate] * `OpamSystem.dir_is_empty`: Speedup and change its type to handle unreachable directories better [#6490 @kit-ty-kate] + * `OpamSystem.internal_patch`: remove parent directories when all of their content has been moved somewhere else [#6490 @kit-ty-kate] diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index b066142f719..d35760eeb2e 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -1636,20 +1636,20 @@ let internal_patch ~allow_unclean ~patch_filename ~dir diffs = in let apply diff = match diff.Patch.operation with | Patch.Edit (file1, file2) -> + let file1 = get_path file1 in + let file2 = get_path file2 in + let file1_exists = Sys.file_exists file1 in (* That seems to be the GNU patch behaviour *) - let file = - let file1 = get_path file1 in - if Sys.file_exists file1 then - file1 - else - get_path file2 - in + let file = if file1_exists then file1 else file2 in let content = read file in let content = patch ~file:file (Some content) diff in write file content; + if file1_exists && file1 <> (file2 : string) then + rmdir_cleanup (Filename.dirname file1) | Patch.Delete file | Patch.Git_ext (file, _, Patch.Delete_only) -> let file = get_path file in - remove_file_t ~with_log:false file + remove_file_t ~with_log:false file; + rmdir_cleanup (Filename.dirname file) | Patch.Create file | Patch.Git_ext (_, file, Patch.Create_only) -> let file = get_path file in let content = patch ~file None diff in @@ -1658,7 +1658,10 @@ let internal_patch ~allow_unclean ~patch_filename ~dir diffs = let src = get_path src in let dst = get_path dst in (* we use rename as we have all guarantee *) - Unix.rename src dst + Unix.rename src dst; + let dirname_src = Filename.dirname src in + if dirname_src <> (Filename.dirname dst : string) then + rmdir_cleanup dirname_src in List.iter apply diffs diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index f6e63902d95..02610effa15 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -99,7 +99,6 @@ + first/diff-file-plus-snd > foo > bar -+ first/dir-only-fst + first/file-only-snd > foo + first/same-dir @@ -190,7 +189,6 @@ rename to file-only-snd + first/diff-file-plus-snd > foo > bar -+ first/dir-only-fst + first/file-only-snd > foo + first/same-dir @@ -597,8 +595,6 @@ ERROR: Unix.Unix_error(Unix.ENOENT, "rename", "/tmp/build_c0ffee_dune/opam-c0ffe *** PATCHED *** + first/ -+ first/im-here -+ first/im-not-here + first/same-file > foo + second/ @@ -625,8 +621,6 @@ index c0ffee..c0ffee *** GIT PATCHED *** + first/ -+ first/im-here -+ first/im-not-here + first/same-file > foo + second/ From 4cffeec376e67d05d39a8296f7934aa791e37f9b Mon Sep 17 00:00:00 2001 From: Kate Date: Fri, 2 May 2025 17:12:49 +0100 Subject: [PATCH 10/10] OpamSystem.internal_patch: fix moving files to new directories when receiving a git diff --- master_changes.md | 2 ++ src/core/opamSystem.ml | 3 +-- tests/lib/patchDiff.expected | 16 ++++++++++++++-- tests/reftests/repository-patchdiff.test | 11 ++++------- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/master_changes.md b/master_changes.md index 590799c4842..820211c0faa 100644 --- a/master_changes.md +++ b/master_changes.md @@ -48,6 +48,7 @@ users) ## Update / Upgrade * Fix a crash when updating a repository that is deleting or adding empty files [#6490 @kit-ty-kate] * Fix an extreme performance issue (takes several hours) when applying a large repository update [#6490 @kit-ty-kate] + * Fix a crash when updating a git repository that moved a file to a new directory [#6490 @kit-ty-kate] ## Tree @@ -148,3 +149,4 @@ users) * `OpamSystem`: add the `rmdir_cleanup` function [#6490 @kit-ty-kate] * `OpamSystem.dir_is_empty`: Speedup and change its type to handle unreachable directories better [#6490 @kit-ty-kate] * `OpamSystem.internal_patch`: remove parent directories when all of their content has been moved somewhere else [#6490 @kit-ty-kate] + * `OpamSystem.internal_patch`: fix moving files to new directories when receiving a git diff [#6490 @kit-ty-kate] diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index d35760eeb2e..8f8a14b6597 100644 --- a/src/core/opamSystem.ml +++ b/src/core/opamSystem.ml @@ -1657,8 +1657,7 @@ let internal_patch ~allow_unclean ~patch_filename ~dir diffs = | Patch.Git_ext (_, _, Patch.Rename_only (src, dst)) -> let src = get_path src in let dst = get_path dst in - (* we use rename as we have all guarantee *) - Unix.rename src dst; + mv src dst; let dirname_src = Filename.dirname src in if dirname_src <> (Filename.dirname dst : string) then rmdir_cleanup dirname_src diff --git a/tests/lib/patchDiff.expected b/tests/lib/patchDiff.expected index 02610effa15..9d6c1bbd100 100644 --- a/tests/lib/patchDiff.expected +++ b/tests/lib/patchDiff.expected @@ -560,8 +560,20 @@ similarity index 100% rename from move-me rename to inner/move-me -*** GIT PATCH ERROR *** -ERROR: Unix.Unix_error(Unix.ENOENT, "rename", "/tmp/build_c0ffee_dune/opam-c0ffee-c0ffee/first/move-me") +*** GIT PATCHED *** ++ first/ ++ first/inner ++ first/inner/move-me + > bar ++ first/same-file + > foo ++ second/ ++ second/inner ++ second/inner/move-me + > bar ++ second/same-file + > foo + ---------------------- Test 13: delete file that deletes the directory diff --git a/tests/reftests/repository-patchdiff.test b/tests/reftests/repository-patchdiff.test index 338aaeb0e2b..b1db1dc4c34 100644 --- a/tests/reftests/repository-patchdiff.test +++ b/tests/reftests/repository-patchdiff.test @@ -63,13 +63,12 @@ OPAM/repo/repo/packages/first/first.1/files/fst/another-file REPOSITORY update repo from git+file://${BASEDIR}/REPO [repo] synchronised from git+file://${BASEDIR}/REPO REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff -[ERROR] Could not update repository "repo": ${OPAM}: "rename" failed on ${BASEDIR}/OPAM/repo/repo/packages/first/first.1/files/fst/another-file: No such file or directory -# Return code 40 # ### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git OPAM/repo/repo/packages/first/first.1/files OPAM/repo/repo/packages/first/first.1/files/fst OPAM/repo/repo/packages/first/first.1/files/fst/a-file -OPAM/repo/repo/packages/first/first.1/files/fst/another-file +OPAM/repo/repo/packages/first/first.1/files/snd +OPAM/repo/repo/packages/first/first.1/files/snd/another-file ### :I:4: remove empty directories ### git -C ./REPO rm packages/first/first.1/files/fst/a-file rm 'packages/first/first.1/files/fst/a-file' @@ -80,9 +79,7 @@ rm 'packages/first/first.1/files/fst/a-file' REPOSITORY update repo from git+file://${BASEDIR}/REPO [repo] synchronised from git+file://${BASEDIR}/REPO REPOSITORY repo: applying patch update at ${BASEDIR}/OPAM/log/git-diff -[ERROR] Could not update repository "repo": ${OPAM}: "rename" failed on ${BASEDIR}/OPAM/repo/repo/packages/first/first.1/files/fst/another-file: No such file or directory -# Return code 40 # ### find OPAM/repo/repo/packages/first/first.1/files | sort | grep -v .git OPAM/repo/repo/packages/first/first.1/files -OPAM/repo/repo/packages/first/first.1/files/fst -OPAM/repo/repo/packages/first/first.1/files/fst/another-file +OPAM/repo/repo/packages/first/first.1/files/snd +OPAM/repo/repo/packages/first/first.1/files/snd/another-file