Correct value-splitting w.r.t. x-env-path-rewrite

When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.

src/state/opamEnv.ml

@@ -236,7 +236,27 @@ let unzip_to ~sepfmt var elt current =
     | _ -> None
   in
   match (if String.equal elt "" then [("", "", separator_char_for ~sepfmt var)]
-         else split_var ~sepfmt var elt) with
+         else match sepfmt with
+              | `norewrite ->
+                (* Given FOO += "<value1><sep><value2>", then even with
+                   `norewrite it is necessary to split the value as FOO itself
+                   will have been split - i.e. if we don't split elt here then
+                   it cannot be reverted if it contains multiple directories
+                   (which would regression #4861) *)
+                let sepfmt = `rewrite_default (var :> string) in
+                split_var ~sepfmt var elt
+              | `rewrite_default _ ->
+                (* If no rewrite has been specified at all, then split elt as,
+                   again, #4861 would be regressed otherwise. *)
+                split_var ~sepfmt var elt
+              | `rewrite _ ->
+                (* If a rewrite rule _is_ in effect, then opam 2.2's limited
+                   (but compatible) semantics for setenv and build-env mean that
+                   we're _assuming_ that elt only contains a single path. This
+                   should be addressed in opam 3.0 by having a somewhat richer
+                   syntax for environment changes to make clear the "type" of
+                   the value in FOO += "bar". *)
+                [(elt, elt, separator_char_for ~sepfmt var)]) with
   | [] -> invalid_arg "OpamEnv.unzip_to"
   | (hd, _, _)::tl ->
     let rec aux acc = function
@@ -537,6 +557,14 @@ let env_expansion ?opam st upd =
   in
   { upd with envu_value = s }
 
+(* env_update_resolved_with_default creates an environment update with a fully
+   evaluated rewrite rule. It's used internally because the updates in question
+   are single directories only, which means that the update will then never be
+   subject to splitting in unzip_to *)
+let env_update_resolved_with_default ?comment var =
+  let rewrite = Some (SPF_Resolved (Some (default_sep_fmt_str var))) in
+  env_update_resolved ?comment ~rewrite var
+
 let compute_updates ?(force_path=false) st =
   (* Todo: put these back into their packages!
   let perl5 = OpamPackage.Name.of_string "perl5" in
@@ -547,7 +575,7 @@ let compute_updates ?(force_path=false) st =
     OpamPath.Switch.bin st.switch_global.root st.switch st.switch_config
   in
   let path =
-    env_update_resolved "PATH"
+    env_update_resolved_with_default "PATH"
       (if force_path then PlusEq else EqPlusEq)
       (OpamFilename.Dir.to_string bindir)
       ~comment:("Binary dir for opam switch "^OpamSwitch.to_string st.switch)
@@ -558,15 +586,15 @@ let compute_updates ?(force_path=false) st =
     | OpenBSD | NetBSD | FreeBSD | Darwin | DragonFly ->
       [] (* MANPATH is a global override on those, so disabled for now *)
     | _ ->
-      [ env_update_resolved "MANPATH" EqColon
+      [ env_update_resolved_with_default "MANPATH" EqColon
           (OpamFilename.Dir.to_string
              (OpamPath.Switch.man_dir st.switch_global.root
                 st.switch st.switch_config))
           ~comment:"Current opam switch man dir"
       ]
  in
  let switch_env =
-   (env_update_resolved "OPAM_SWITCH_PREFIX" Eq
+   (env_update_resolved_with_default "OPAM_SWITCH_PREFIX" Eq
       (OpamFilename.Dir.to_string
          (OpamPath.Switch.root st.switch_global.root st.switch))
       ~comment:"Prefix of the current opam switch")
@@ -589,14 +617,14 @@ let compute_updates ?(force_path=false) st =
 let updates_common ~set_opamroot ~set_opamswitch root switch =
   let root =
     if set_opamroot then
-      [ env_update_resolved "OPAMROOT" Eq
+      [ env_update_resolved_with_default "OPAMROOT" Eq
           (OpamFilename.Dir.to_string root)
           ~comment:"Opam root in use" ]
     else []
   in
   let switch =
     if set_opamswitch then
-      [ env_update_resolved "OPAMSWITCH" Eq
+      [ env_update_resolved_with_default "OPAMSWITCH" Eq
           (OpamSwitch.to_string switch) ]
     else [] in
   root @ switch
@@ -716,7 +744,7 @@ let switch_path_update ~force_path root switch =
       (OpamStateConfig.Switch.safe_load_t
          ~lock_kind:`Lock_read root switch)
   in
-  [ env_update_resolved "PATH"
+  [ env_update_resolved_with_default "PATH"
       (if force_path then PlusEq else EqPlusEq)
       (OpamFilename.Dir.to_string bindir)
       ~comment:"Current opam switch binary dir" ]

tests/reftests/env.win32.test

@@ -23,11 +23,11 @@ Done.
 set "MANPATH=:"${BASEDIR}/OPAM/rewriting/man""
 set "PATH=${BASEDIR}/OPAM/rewriting/bin;C:\Devel\bin1;C:\Devel\bin2;"C:\Devel\bin3;";C:\Devel\bin4;ZZZ:\;
 ### opam exec -- opam env --shell=cmd --revert | grep 'ZZZ:' | 'ZZZ:\\.*' -> 'ZZZ:\'
-set "PATH=C:\Devel\bin1;C:\Devel\bin2;"C:\Devel\bin3;";C:\Devel\bin4;ZZZ:\
 ### : Test for #5838
 ### opam env | grep MANPATH
 MANPATH=':"${BASEDIR}/OPAM/rewriting/man"'; export MANPATH;
 ### opam exec -- opam env --revert | grep MANPATH
+MANPATH=''; export MANPATH;
 ### : Tests forward and backslash rewriting on revert
 ### : This sequence of updates is what presently happens with the compiler
 ### <pkg:mixed-updates.1>