Fix reverting additions to PATH-like variables

master_changes.md

@@ -114,6 +114,7 @@ users)
 ## State
   * Handle empty environment variable updates - missed cherry-pick from 2.0 [#4840 @dra27]
   * Repository state: stop scanning directory once opam file is found [#4847 @rgrinberg]
+  * Fix reverting environment additions to PATH-like variables when several dirs added at once [#4861 @dra27]
 
 ## Opam file format
   *

src/state/opamEnv.ml

@@ -29,14 +29,30 @@ let join_var l =
 (* To allow in-place updates, we store intermediate values of path-like as a
    pair of list [(rl1, l2)] such that the value is [List.rev_append rl1 l2] and
    the place where the new value should be inserted is in front of [l2] *)
-let unzip_to elt =
-  let rec aux acc = function
-    | [] -> None
-    | x::r ->
-      if x = elt then Some (acc, r)
-      else aux (x::acc) r
+
+
+let unzip_to elt current =
+  (* If [r = l @ rs] then [remove_prefix l r] is [Some rs], otherwise [None] *)
+  let rec remove_prefix l r =
+    match l, r with
+    | (l::ls, r::rs) when l = r ->
+      remove_prefix ls rs
+    | ([], rs) -> Some rs
+    | _ -> None
   in
-  aux []
+    match split_var elt with
+    | [] -> invalid_arg "OpamEnv.unzip_to"
+    | hd::tl ->
+      let rec aux acc = function
+      | [] -> None
+      | x::r ->
+        if x = hd then
+          match remove_prefix tl r with
+          | Some r -> Some (acc, r)
+          | None -> aux (x::acc) r
+        else aux (x::acc) r
+      in
+        aux [] current
 
 let rezip ?insert (l1, l2) =
   List.rev_append l1 (match insert with None -> l2 | Some i -> i::l2)

tests/reftests/dune.inc

@@ -135,6 +135,23 @@
    %{targets}
    (run ./run.exe %{bin:opam} %{dep:dot-install.test} %{read-lines:testing-env}))))
 
+(rule
+ (alias reftest-env)
+ (action
+  (diff env.test env.out)))
+
+(alias
+ (name reftest)
+ (deps (alias reftest-env)))
+
+(rule
+ (targets env.out)
+ (deps root-N0REP0)
+ (action
+  (with-stdout-to
+   %{targets}
+   (run ./run.exe %{bin:opam} %{dep:env.test} %{read-lines:testing-env}))))
+
 (rule
  (alias reftest-init)
  (action

tests/reftests/env.test

@@ -0,0 +1,27 @@
+N0REP0
+### <pkg:nv.1>
+opam-version: "2.0"
+setenv: [ NV_VARS += "%{_:doc}%:%{_:share}%" ]
+flags: compiler
+### opam update
+
+<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><>
+[default] synchronised from file://${BASEDIR}/REPO
+Now run 'opam upgrade' to apply any package updates.
+### opam switch create setenv nv
+
+<><> Installing new switch packages <><><><><><><><><><><><><><><><><><><><><><>
+Switch invariant: ["nv"]
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> installed nv.1
+Done.
+### opam env | grep "NV_VARS" | '[:;]' -> '-' | '[:;]' -> '-' | '[:;]' -> '-'
+NV_VARS='${BASEDIR}/OPAM/setenv/doc/nv-${OPAMTMP}/OPAM/setenv/share/nv'- export NV_VARS-
+### opam exec -- opam env --revert | grep "NV_VARS" | '[:;]' -> '-' | '[:;]' -> '-' | '[:;]' -> '-'
+NV_VARS=''- export NV_VARS-
+### NV_VARS=/another/path
+### opam env | grep "NV_VARS" | '[:;]' -> '-' | '[:;]' -> '-' | '[:;]' -> '-'
+NV_VARS='${BASEDIR}/OPAM/setenv/doc/nv-${OPAMTMP}/OPAM/setenv/share/nv-/another/path'- export NV_VARS-
+### opam exec -- opam env --revert | grep "NV_VARS" | '[:;]' -> '-'
+NV_VARS='/another/path'- export NV_VARS-