Rework logic of := and =: operators (ocaml#5926)

:= and =: now ensure that an empty directory empty is always introduced then all the append operators ensure that an empty directory entry is maintained (see env.test changes).
dra27 · May 15, 2024 · 9046905 · 9046905
1 parent 20a64c9
commit 9046905
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 32 deletions.
diff --git a/master_changes.md b/master_changes.md
@@ -68,6 +68,7 @@ users)
   * [BUG] Fix splitting environment variables [#5935 @dra27]
   * [BUG] When opam creates an empty variable then appends/prepends a value, ensure no additional separator is added [#5935 @dra27 - fix #5925]
   * [BUG] Fix `x-env-path-rewrite` splitting of values when reverting [#5935 @dra27 - fix #5838]
+  * [BUG] Rework the logic of := and =: so that an empty entry is correctly preserved on multiple updates [#5935 @dra27 - fix #5926]
 
 ## Opamfile
 

diff --git a/src/state/opamEnv.ml b/src/state/opamEnv.ml
@@ -309,13 +309,6 @@ let rezip_to_string ?insert z =
 let apply_op_zip ~sepfmt var op arg (rl1,l2 as zip) =
   let arg = transform_format ~sepfmt var arg in
   let empty_tr = { tr_entry = ""; tr_raw = ""; tr_sep = arg.tr_sep } in
-  let colon_eq ?(eqcol=false) = function (* prepend a, but keep ":"s *)
-    | [] | [{ tr_entry = ""; _}] -> [], [arg; empty_tr]
-    | { tr_entry = ""; _} :: l ->
-      (* keep surrounding colons *)
-      if eqcol then l@[empty_tr], [arg] else l, [empty_tr; arg]
-    | l -> l, [arg]
-  in
   let cygwin path =
     let contains_in {tr_entry = dir; _} item =
       Sys.file_exists (Filename.concat dir item)
@@ -367,10 +360,25 @@ let apply_op_zip ~sepfmt var op arg (rl1,l2 as zip) =
        without the rezip) *)
     rl1, arg::l2
   | ColonEq ->
-    let l, add = colon_eq (rezip zip) in [], add @ l
+    begin match rezip zip with
+      | [{ tr_entry = ""; _}] | [] -> (* empty or unset *)
+        [], [arg; empty_tr]
+      | ({ tr_entry = ""; _} as lead)::{ tr_entry = ""; _}::([] as zip) ->
+        (* VAR=':' *)
+        [], lead::arg::zip
+      | zip ->
+        [], arg::zip
+    end
   | EqColon ->
-    let l, add = colon_eq ~eqcol:true (List.rev_append l2 rl1) in
-    l, List.rev add
+    begin match List.rev_append l2 rl1 with
+      | [{ tr_entry = ""; _}] | [] -> (* empty or unset *)
+        [], [empty_tr; arg]
+      | ({ tr_entry = ""; _} as lead)::{ tr_entry = ""; _}::([] as zip) ->
+        (* VAR=':' *)
+        [], List.rev (lead::arg::zip)
+      | zip ->
+        [], List.rev (arg::zip)
+    end
 
 (** Undoes previous updates done by opam, useful for not duplicating already
     done updates; this is obviously not perfect, as all operators are not

diff --git a/tests/reftests/env.test b/tests/reftests/env.test
@@ -204,8 +204,8 @@ NV_VARS_5926_L_1=b::a
 NV_VARS_5926_L_2=b::a
 NV_VARS_5926_L_3=:a:b
 NV_VARS_5926_L_4=:a:b
-NV_VARS_5926_L_5=:b:a
-NV_VARS_5926_L_6=:b:a
+NV_VARS_5926_L_5=b::a
+NV_VARS_5926_L_6=b::a
 NV_VARS_5926_L_7=:a:b
 NV_VARS_5926_L_8=:a:b
 NV_VARS_5926_M_1=b:a1::a2
@@ -222,8 +222,8 @@ NV_VARS_5926_T_3=a::b
 NV_VARS_5926_T_4=a::b
 NV_VARS_5926_T_5=b:a:
 NV_VARS_5926_T_6=b:a:
-NV_VARS_5926_T_7=:a:b
-NV_VARS_5926_T_8=:a:b
+NV_VARS_5926_T_7=a::b
+NV_VARS_5926_T_8=a::b
 ### opam env | sort | grep "NV_VARS" | ';' -> ':'
 NV_VARS3='foo:': export NV_VARS3:
 NV_VARS4='': export NV_VARS4:
@@ -239,8 +239,8 @@ NV_VARS_5926_L_1='b::a': export NV_VARS_5926_L_1:
 NV_VARS_5926_L_2='b::a': export NV_VARS_5926_L_2:
 NV_VARS_5926_L_3=':a:b': export NV_VARS_5926_L_3:
 NV_VARS_5926_L_4=':a:b': export NV_VARS_5926_L_4:
-NV_VARS_5926_L_5=':b:a': export NV_VARS_5926_L_5:
-NV_VARS_5926_L_6=':b:a': export NV_VARS_5926_L_6:
+NV_VARS_5926_L_5='b::a': export NV_VARS_5926_L_5:
+NV_VARS_5926_L_6='b::a': export NV_VARS_5926_L_6:
 NV_VARS_5926_L_7=':a:b': export NV_VARS_5926_L_7:
 NV_VARS_5926_L_8=':a:b': export NV_VARS_5926_L_8:
 NV_VARS_5926_M_1='b:a1::a2': export NV_VARS_5926_M_1:
@@ -257,8 +257,8 @@ NV_VARS_5926_T_3='a::b': export NV_VARS_5926_T_3:
 NV_VARS_5926_T_4='a::b': export NV_VARS_5926_T_4:
 NV_VARS_5926_T_5='b:a:': export NV_VARS_5926_T_5:
 NV_VARS_5926_T_6='b:a:': export NV_VARS_5926_T_6:
-NV_VARS_5926_T_7=':a:b': export NV_VARS_5926_T_7:
-NV_VARS_5926_T_8=':a:b': export NV_VARS_5926_T_8:
+NV_VARS_5926_T_7='a::b': export NV_VARS_5926_T_7:
+NV_VARS_5926_T_8='a::b': export NV_VARS_5926_T_8:
 ### opam exec -- opam env --revert | grep "NV_VARS" | ';' -> ':'
 NV_VARS3='': export NV_VARS3:
 NV_VARS4='': export NV_VARS4:
@@ -293,7 +293,7 @@ NV_VARS_5926_T_4=':a': export NV_VARS_5926_T_4:
 NV_VARS_5926_T_5='': export NV_VARS_5926_T_5:
 NV_VARS_5926_T_6='a:': export NV_VARS_5926_T_6:
 NV_VARS_5926_T_7='': export NV_VARS_5926_T_7:
-NV_VARS_5926_T_8='a:': export NV_VARS_5926_T_8:
+NV_VARS_5926_T_8=':a': export NV_VARS_5926_T_8:
 ### NV_VARS=/another/path
 ### NV_VARS2=/another/different/path
 ### NV_VARS3=/yet/another/different/path
@@ -315,8 +315,8 @@ NV_VARS_5926_L_1=b::a
 NV_VARS_5926_L_2=b::a
 NV_VARS_5926_L_3=:a:b
 NV_VARS_5926_L_4=:a:b
-NV_VARS_5926_L_5=:b:a
-NV_VARS_5926_L_6=:b:a
+NV_VARS_5926_L_5=b::a
+NV_VARS_5926_L_6=b::a
 NV_VARS_5926_L_7=:a:b
 NV_VARS_5926_L_8=:a:b
 NV_VARS_5926_M_1=b:a1::a2
@@ -333,8 +333,8 @@ NV_VARS_5926_T_3=a::b
 NV_VARS_5926_T_4=a::b
 NV_VARS_5926_T_5=b:a:
 NV_VARS_5926_T_6=b:a:
-NV_VARS_5926_T_7=:a:b
-NV_VARS_5926_T_8=:a:b
+NV_VARS_5926_T_7=a::b
+NV_VARS_5926_T_8=a::b
 ### opam env | sort | grep "NV_VARS" | ';' -> ':'
 NV_VARS3='foo:/yet/another/different/path': export NV_VARS3:
 NV_VARS4='': export NV_VARS4:
@@ -350,8 +350,8 @@ NV_VARS_5926_L_1='b::a': export NV_VARS_5926_L_1:
 NV_VARS_5926_L_2='b::a': export NV_VARS_5926_L_2:
 NV_VARS_5926_L_3=':a:b': export NV_VARS_5926_L_3:
 NV_VARS_5926_L_4=':a:b': export NV_VARS_5926_L_4:
-NV_VARS_5926_L_5=':b:a': export NV_VARS_5926_L_5:
-NV_VARS_5926_L_6=':b:a': export NV_VARS_5926_L_6:
+NV_VARS_5926_L_5='b::a': export NV_VARS_5926_L_5:
+NV_VARS_5926_L_6='b::a': export NV_VARS_5926_L_6:
 NV_VARS_5926_L_7=':a:b': export NV_VARS_5926_L_7:
 NV_VARS_5926_L_8=':a:b': export NV_VARS_5926_L_8:
 NV_VARS_5926_M_1='b:a1::a2': export NV_VARS_5926_M_1:
@@ -368,8 +368,8 @@ NV_VARS_5926_T_3='a::b': export NV_VARS_5926_T_3:
 NV_VARS_5926_T_4='a::b': export NV_VARS_5926_T_4:
 NV_VARS_5926_T_5='b:a:': export NV_VARS_5926_T_5:
 NV_VARS_5926_T_6='b:a:': export NV_VARS_5926_T_6:
-NV_VARS_5926_T_7=':a:b': export NV_VARS_5926_T_7:
-NV_VARS_5926_T_8=':a:b': export NV_VARS_5926_T_8:
+NV_VARS_5926_T_7='a::b': export NV_VARS_5926_T_7:
+NV_VARS_5926_T_8='a::b': export NV_VARS_5926_T_8:
 ### opam exec -- opam env --revert | grep "NV_VARS" | ';' -> ':'
 NV_VARS3='/yet/another/different/path': export NV_VARS3:
 NV_VARS4='': export NV_VARS4:
@@ -404,7 +404,7 @@ NV_VARS_5926_T_4=':a': export NV_VARS_5926_T_4:
 NV_VARS_5926_T_5='': export NV_VARS_5926_T_5:
 NV_VARS_5926_T_6='a:': export NV_VARS_5926_T_6:
 NV_VARS_5926_T_7='': export NV_VARS_5926_T_7:
-NV_VARS_5926_T_8='a:': export NV_VARS_5926_T_8:
+NV_VARS_5926_T_8=':a': export NV_VARS_5926_T_8:
 ### : Full revert of uninstalled package with setenv :
 ### <pkg:foo.1>
 opam-version: "2.0"
@@ -472,8 +472,8 @@ NV_VARS_5926_L_1='b::a': export NV_VARS_5926_L_1:
 NV_VARS_5926_L_2='b::a': export NV_VARS_5926_L_2:
 NV_VARS_5926_L_3=':a:b': export NV_VARS_5926_L_3:
 NV_VARS_5926_L_4=':a:b': export NV_VARS_5926_L_4:
-NV_VARS_5926_L_5=':b:a': export NV_VARS_5926_L_5:
-NV_VARS_5926_L_6=':b:a': export NV_VARS_5926_L_6:
+NV_VARS_5926_L_5='b::a': export NV_VARS_5926_L_5:
+NV_VARS_5926_L_6='b::a': export NV_VARS_5926_L_6:
 NV_VARS_5926_L_7=':a:b': export NV_VARS_5926_L_7:
 NV_VARS_5926_L_8=':a:b': export NV_VARS_5926_L_8:
 NV_VARS_5926_M_1='b:a1::a2': export NV_VARS_5926_M_1:
@@ -490,8 +490,8 @@ NV_VARS_5926_T_3='a::b': export NV_VARS_5926_T_3:
 NV_VARS_5926_T_4='a::b': export NV_VARS_5926_T_4:
 NV_VARS_5926_T_5='b:a:': export NV_VARS_5926_T_5:
 NV_VARS_5926_T_6='b:a:': export NV_VARS_5926_T_6:
-NV_VARS_5926_T_7=':a:b': export NV_VARS_5926_T_7:
-NV_VARS_5926_T_8=':a:b': export NV_VARS_5926_T_8:
+NV_VARS_5926_T_7='a::b': export NV_VARS_5926_T_7:
+NV_VARS_5926_T_8='a::b': export NV_VARS_5926_T_8:
 ### OPAMNOENVNOTICE=1
 ### : Env hooks :
 ### <pkg:av.1>