Replace PATH-splitting implementation in OpamEnv

Existing implementation was not correct w.r.t. separators which were themselves quoted. Use OpamStd.String.split_quoted instead. However, this causes superfluous quotes in variables to be removed. Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling is still broken.
dra27 · May 14, 2024 · 8bb1792 · 8bb1792
1 parent 6a02e09
commit 8bb1792
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 51 deletions.
diff --git a/src/state/opamEnv.ml b/src/state/opamEnv.ml
@@ -153,36 +153,7 @@ let split_var ~(sepfmt:sep_path_format) var value =
     | Target | Host ->
       OpamStd.String.split value sep
     | Target_quoted | Host_quoted ->
-      (* we suppose that it is in the form:
-         - "quoted":unquoted
-         - unquoted:"quoted"
-         - "quoted":unquoted:"quoted"
-         - unquoted:"quoted":unquoted
-         - "quoted"
-         - unquoted
-      *)
-      let rec aux remaining acc =
-        match String.get remaining 0 with
-        | '"' ->
-          (let remaining =
-             String.sub remaining 1 (String.length remaining - 1)
-           in
-           match OpamStd.String.cut_at remaining '"' with
-           | Some (quoted, rest) ->
-             aux rest (("\""^quoted^"\"")::acc)
-           | None -> remaining::acc)
-        | _ ->
-          let remaining =
-            if Char.equal (String.get remaining 0) sep then
-              String.sub remaining 1 (String.length remaining - 1)
-            else remaining in
-          (match OpamStd.String.cut_at remaining sep with
-           | Some (unquoted, rest) ->
-             aux rest (unquoted::acc)
-           | None -> remaining::acc)
-        | exception Invalid_argument _ -> acc
-      in
-      List.rev @@ aux value []
+      OpamStd.String.split_quoted value sep
 
 let join_var ~(sepfmt:sep_path_format) var values =
   let separator =

diff --git a/tests/reftests/env.unix.test b/tests/reftests/env.unix.test
@@ -115,7 +115,7 @@ Processing  2/3: [col-target-quoted: sh env | grep RCTQ_ENV | sort]
 + sh "-c" "env | grep RCTQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/col-target-quoted.1)
 - RCTQ_ENVBUILD=/a/given/path
 - RCTQ_ENVBUILD_ADD=/a/given/path:a/path/to
-- RCTQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path":/a/path/to:"t:/his/is/quoted"
+- RCTQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path":/a/path/to:t:/his/is/quoted
 - RCTQ_ENVBUILD_STR=something
 - RCTQ_ENVBUILD_WITH_COL="s:mething"
 - RCTQ_ENVSET_ADD=a/path/to
@@ -126,7 +126,7 @@ Done.
 ### opam env | sort | grep "RCTQ_ENV"
 RCTQ_ENVSET='/a/given/path'; export RCTQ_ENVSET;
 RCTQ_ENVSET_ADD='/a/given/path:a/path/to'; export RCTQ_ENVSET_ADD;
-RCTQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path":/a/path/to:"t:/his/is/quoted"'; export RCTQ_ENVSET_ADD_WITH_COL;
+RCTQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path":/a/path/to:t:/his/is/quoted'; export RCTQ_ENVSET_ADD_WITH_COL;
 RCTQ_ENVSET_STR='something'; export RCTQ_ENVSET_STR;
 RCTQ_ENVSET_WITH_COL='"s:mething"'; export RCTQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RCTQ_ENV
@@ -249,7 +249,7 @@ Processing  2/3: [col-host-quoted: sh env | grep RCHQ_ENV | sort]
 + sh "-c" "env | grep RCHQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/col-host-quoted.1)
 - RCHQ_ENVBUILD=/a/given/path
 - RCHQ_ENVBUILD_ADD=/a/given/path:a/path/to
-- RCHQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path":/a/path/to:"t:/his/is/quoted"
+- RCHQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path":/a/path/to:t:/his/is/quoted
 - RCHQ_ENVBUILD_STR=something
 - RCHQ_ENVBUILD_WITH_COL="s:mething"
 - RCHQ_ENVSET_ADD=a/path/to
@@ -260,7 +260,7 @@ Done.
 ### opam env | sort | grep "RCHQ_ENV"
 RCHQ_ENVSET='/a/given/path'; export RCHQ_ENVSET;
 RCHQ_ENVSET_ADD='/a/given/path:a/path/to'; export RCHQ_ENVSET_ADD;
-RCHQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path":/a/path/to:"t:/his/is/quoted"'; export RCHQ_ENVSET_ADD_WITH_COL;
+RCHQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path":/a/path/to:t:/his/is/quoted'; export RCHQ_ENVSET_ADD_WITH_COL;
 RCHQ_ENVSET_STR='something'; export RCHQ_ENVSET_STR;
 RCHQ_ENVSET_WITH_COL='"s:mething"'; export RCHQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RCHQ_ENV
@@ -380,7 +380,7 @@ Processing  2/3: [semicol-target-quoted: sh env | grep RSTQ_ENV | sort]
 + sh "-c" "env | grep RSTQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/semicol-target-quoted.1)
 - RSTQ_ENVBUILD=/a/given/path
 - RSTQ_ENVBUILD_ADD=/a/given/path;a/path/to
-- RSTQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"
+- RSTQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path";/a/path/to;t:/his/is/quoted
 - RSTQ_ENVBUILD_STR=something
 - RSTQ_ENVBUILD_WITH_COL=s:mething
 - RSTQ_ENVSET_ADD=a/path/to
@@ -391,7 +391,7 @@ Done.
 ### opam env | sort | grep "RSTQ_ENV"
 RSTQ_ENVSET='/a/given/path'; export RSTQ_ENVSET;
 RSTQ_ENVSET_ADD='/a/given/path;a/path/to'; export RSTQ_ENVSET_ADD;
-RSTQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"'; export RSTQ_ENVSET_ADD_WITH_COL;
+RSTQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path";/a/path/to;t:/his/is/quoted'; export RSTQ_ENVSET_ADD_WITH_COL;
 RSTQ_ENVSET_STR='something'; export RSTQ_ENVSET_STR;
 RSTQ_ENVSET_WITH_COL='s:mething'; export RSTQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RSTQ_ENV
@@ -514,7 +514,7 @@ Processing  2/3: [semicol-host-quoted: sh env | grep RSHQ_ENV | sort]
 + sh "-c" "env | grep RSHQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/semicol-host-quoted.1)
 - RSHQ_ENVBUILD=/a/given/path
 - RSHQ_ENVBUILD_ADD=/a/given/path;a/path/to
-- RSHQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"
+- RSHQ_ENVBUILD_ADD_WITH_COL="a:/nother/gi;ven/path";/a/path/to;t:/his/is/quoted
 - RSHQ_ENVBUILD_STR=something
 - RSHQ_ENVBUILD_WITH_COL=s:mething
 - RSHQ_ENVSET_ADD=a/path/to
@@ -525,7 +525,7 @@ Done.
 ### opam env | sort | grep "RSHQ_ENV"
 RSHQ_ENVSET='/a/given/path'; export RSHQ_ENVSET;
 RSHQ_ENVSET_ADD='/a/given/path;a/path/to'; export RSHQ_ENVSET_ADD;
-RSHQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"'; export RSHQ_ENVSET_ADD_WITH_COL;
+RSHQ_ENVSET_ADD_WITH_COL='"a:/nother/gi;ven/path";/a/path/to;t:/his/is/quoted'; export RSHQ_ENVSET_ADD_WITH_COL;
 RSHQ_ENVSET_STR='something'; export RSHQ_ENVSET_STR;
 RSHQ_ENVSET_WITH_COL='s:mething'; export RSHQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RSHQ_ENV
@@ -651,7 +651,7 @@ Processing  2/3: [rewrite: sh env | grep RO_ENV | sort]
 - RO_ENVBUILD=/another/given/path
 - RO_ENVBUILD_COL=s:mething
 - RO_ENVBUILD_COL_TARGET=a:/nother/gi;ven/path;a/path/to
-- RO_ENVBUILD_COL_TARGET_QUOTED="a:/nother/gi;ven/path":a/path/to:"this/i:s/quoted"
+- RO_ENVBUILD_COL_TARGET_QUOTED="a:/nother/gi;ven/path":a/path/to:this/i:s/quoted
 - RO_ENVBUILD_STR=something
 - RO_ENVSET_COL_TARGET=a/path/to
 - RO_ENVSET_COL_TARGET_QUOTED=a/path/to:"this/i:s/quoted"
@@ -662,7 +662,7 @@ Done.
 RO_ENVSET='/a/given/path'; export RO_ENVSET;
 RO_ENVSET_COL='"s:mething"'; export RO_ENVSET_COL;
 RO_ENVSET_COL_TARGET='a:/nother/gi;ven/path;a/path/to'; export RO_ENVSET_COL_TARGET;
-RO_ENVSET_COL_TARGET_QUOTED='"a:/nother/gi;ven/path":a/path/to:"this/i:s/quoted"'; export RO_ENVSET_COL_TARGET_QUOTED;
+RO_ENVSET_COL_TARGET_QUOTED='"a:/nother/gi;ven/path":a/path/to:this/i:s/quoted'; export RO_ENVSET_COL_TARGET_QUOTED;
 RO_ENVSET_STR='something'; export RO_ENVSET_STR;
 RO_ENVSET_STR_WS='something/else'; export RO_ENVSET_STR_WS;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RO_ENV

diff --git a/tests/reftests/env.win32.test b/tests/reftests/env.win32.test
@@ -26,11 +26,12 @@ 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 "P' -> 'set P' 
-set PATH=C:\Devel\bin1;C:\Devel\bin2;"C:\Devel\bin3;";C:\Devel\bin4;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>
@@ -171,7 +172,7 @@ Processing  2/3: [col-target-quoted: sh env | grep RCTQ_ENV | sort]
 + sh "-c" "env | grep RCTQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/col-target-quoted.1)
 - RCTQ_ENVBUILD=\a\given\path
 - RCTQ_ENVBUILD_ADD=\a\given\path:a/path/to
-- RCTQ_ENVBUILD_ADD_WITH_COL="a:\nother\gi;ven\path":/a/path/to:"t:/his/is/quoted"
+- RCTQ_ENVBUILD_ADD_WITH_COL="a:\nother\gi;ven\path":/a/path/to:t:/his/is/quoted
 - RCTQ_ENVBUILD_STR=something
 - RCTQ_ENVBUILD_WITH_COL="s:mething"
 - RCTQ_ENVSET_ADD=a/path/to
@@ -182,7 +183,7 @@ Done.
 ### opam env | sort | grep "RCTQ_ENV"
 RCTQ_ENVSET='\a\given\path'; export RCTQ_ENVSET;
 RCTQ_ENVSET_ADD='\a\given\path:a/path/to'; export RCTQ_ENVSET_ADD;
-RCTQ_ENVSET_ADD_WITH_COL='"a:\nother\gi;ven\path":/a/path/to:"t:/his/is/quoted"'; export RCTQ_ENVSET_ADD_WITH_COL;
+RCTQ_ENVSET_ADD_WITH_COL='"a:\nother\gi;ven\path":/a/path/to:t:/his/is/quoted'; export RCTQ_ENVSET_ADD_WITH_COL;
 RCTQ_ENVSET_STR='something'; export RCTQ_ENVSET_STR;
 RCTQ_ENVSET_WITH_COL='"s:mething"'; export RCTQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RCTQ_ENV
@@ -307,7 +308,7 @@ Processing  2/3: [col-host-quoted: sh env | grep RCHQ_ENV | sort]
 + sh "-c" "env | grep RCHQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/col-host-quoted.1)
 - RCHQ_ENVBUILD=/a/given/path
 - RCHQ_ENVBUILD_ADD=/a/given/path:a/path/to
-- RCHQ_ENVBUILD_ADD_WITH_COL=/cygdrive/a/nother/gi;ven/path:/a/path/to:"t:/his/is/quoted"
+- RCHQ_ENVBUILD_ADD_WITH_COL=/cygdrive/a/nother/gi;ven/path:/a/path/to:t:/his/is/quoted
 - RCHQ_ENVBUILD_STR=something
 - RCHQ_ENVBUILD_WITH_COL="s:mething"
 - RCHQ_ENVSET_ADD=a/path/to
@@ -318,7 +319,7 @@ Done.
 ### opam env | sort | grep "RCHQ_ENV"
 RCHQ_ENVSET='/a/given/path'; export RCHQ_ENVSET;
 RCHQ_ENVSET_ADD='/a/given/path:a/path/to'; export RCHQ_ENVSET_ADD;
-RCHQ_ENVSET_ADD_WITH_COL='/cygdrive/a/nother/gi;ven/path:/a/path/to:"t:/his/is/quoted"'; export RCHQ_ENVSET_ADD_WITH_COL;
+RCHQ_ENVSET_ADD_WITH_COL='/cygdrive/a/nother/gi;ven/path:/a/path/to:t:/his/is/quoted'; export RCHQ_ENVSET_ADD_WITH_COL;
 RCHQ_ENVSET_STR='something'; export RCHQ_ENVSET_STR;
 RCHQ_ENVSET_WITH_COL='"s:mething"'; export RCHQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RCHQ_ENV
@@ -443,7 +444,7 @@ Processing  2/3: [semicol-target-quoted: sh env | grep RSTQ_ENV | sort]
 + sh "-c" "env | grep RSTQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/semicol-target-quoted.1)
 - RSTQ_ENVBUILD=\a\given\path
 - RSTQ_ENVBUILD_ADD=\a\given\path;a/path/to
-- RSTQ_ENVBUILD_ADD_WITH_COL="a:\nother\gi;ven\path";/a/path/to;"t:/his/is/quoted"
+- RSTQ_ENVBUILD_ADD_WITH_COL="a:\nother\gi;ven\path";/a/path/to;t:/his/is/quoted
 - RSTQ_ENVBUILD_STR=something
 - RSTQ_ENVBUILD_WITH_COL=s:mething
 - RSTQ_ENVSET_ADD=a/path/to
@@ -454,7 +455,7 @@ Done.
 ### opam env | sort | grep "RSTQ_ENV"
 RSTQ_ENVSET='\a\given\path'; export RSTQ_ENVSET;
 RSTQ_ENVSET_ADD='\a\given\path;a/path/to'; export RSTQ_ENVSET_ADD;
-RSTQ_ENVSET_ADD_WITH_COL='"a:\nother\gi;ven\path";/a/path/to;"t:/his/is/quoted"'; export RSTQ_ENVSET_ADD_WITH_COL;
+RSTQ_ENVSET_ADD_WITH_COL='"a:\nother\gi;ven\path";/a/path/to;t:/his/is/quoted'; export RSTQ_ENVSET_ADD_WITH_COL;
 RSTQ_ENVSET_STR='something'; export RSTQ_ENVSET_STR;
 RSTQ_ENVSET_WITH_COL='s:mething'; export RSTQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RSTQ_ENV
@@ -579,7 +580,7 @@ Processing  2/3: [semicol-host-quoted: sh env | grep RSHQ_ENV | sort]
 + sh "-c" "env | grep RSHQ_ENV | sort" (CWD=${BASEDIR}/OPAM/rewriting/.opam-switch/build/semicol-host-quoted.1)
 - RSHQ_ENVBUILD=/a/given/path
 - RSHQ_ENVBUILD_ADD=/a/given/path;a/path/to
-- RSHQ_ENVBUILD_ADD_WITH_COL="/cygdrive/a/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"
+- RSHQ_ENVBUILD_ADD_WITH_COL="/cygdrive/a/nother/gi;ven/path";/a/path/to;t:/his/is/quoted
 - RSHQ_ENVBUILD_STR=something
 - RSHQ_ENVBUILD_WITH_COL=s:mething
 - RSHQ_ENVSET_ADD=a/path/to
@@ -590,7 +591,7 @@ Done.
 ### opam env | sort | grep "RSHQ_ENV"
 RSHQ_ENVSET='/a/given/path'; export RSHQ_ENVSET;
 RSHQ_ENVSET_ADD='/a/given/path;a/path/to'; export RSHQ_ENVSET_ADD;
-RSHQ_ENVSET_ADD_WITH_COL='"/cygdrive/a/nother/gi;ven/path";/a/path/to;"t:/his/is/quoted"'; export RSHQ_ENVSET_ADD_WITH_COL;
+RSHQ_ENVSET_ADD_WITH_COL='"/cygdrive/a/nother/gi;ven/path";/a/path/to;t:/his/is/quoted'; export RSHQ_ENVSET_ADD_WITH_COL;
 RSHQ_ENVSET_STR='something'; export RSHQ_ENVSET_STR;
 RSHQ_ENVSET_WITH_COL='s:mething'; export RSHQ_ENVSET_WITH_COL;
 ### cat OPAM/rewriting/.opam-switch/environment | grep RSHQ_ENV
@@ -721,7 +722,7 @@ Processing  2/3: [rewrite: sh env | grep RO_ENV | sort]
 - RO_ENVBUILD=\another\given\path
 - RO_ENVBUILD_COL=s:mething
 - RO_ENVBUILD_COL_TARGET=/cygdrive/a/nother/gi;ven/path;a/path/to
-- RO_ENVBUILD_COL_TARGET_QUOTED=/cygdrive/a/nother/gi;ven/path:a/path/to:"this/i:s/quoted"
+- RO_ENVBUILD_COL_TARGET_QUOTED=/cygdrive/a/nother/gi;ven/path:a/path/to:this/i:s/quoted
 - RO_ENVBUILD_STR=something
 - RO_ENVSET_COL_TARGET=a/path/to
 - RO_ENVSET_COL_TARGET_QUOTED=a/path/to:"this/i:s/quoted"
@@ -732,7 +733,7 @@ Done.
 RO_ENVSET='\a\given\path'; export RO_ENVSET;
 RO_ENVSET_COL='"s:mething"'; export RO_ENVSET_COL;
 RO_ENVSET_COL_TARGET='/cygdrive/a/nother/gi;ven/path;a/path/to'; export RO_ENVSET_COL_TARGET;
-RO_ENVSET_COL_TARGET_QUOTED='/cygdrive/a/nother/gi;ven/path:a/path/to:"this/i:s/quoted"'; export RO_ENVSET_COL_TARGET_QUOTED;
+RO_ENVSET_COL_TARGET_QUOTED='/cygdrive/a/nother/gi;ven/path:a/path/to:this/i:s/quoted'; export RO_ENVSET_COL_TARGET_QUOTED;
 RO_ENVSET_STR='something'; export RO_ENVSET_STR;
 RO_ENVSET_STR_WS2='something/else'; export RO_ENVSET_STR_WS2;
 RO_ENVSET_STR_WS='something/else'; export RO_ENVSET_STR_WS;