Merge pull request #5921 from dra27/fix-download-chars

Fix downloading URLs with invalid characters

master_changes.md

@@ -55,6 +55,7 @@ users)
 ## Lint
 
 ## Repository
+  * Fix download URLs containing invalid characters on Windows (e.g. the ? character in `?full_index=1`) [#5921 @dra27]
 
 ## Lock
 

src/core/opamStd.ml

@@ -1329,6 +1329,15 @@ module OpamSys = struct
       if !called then invalid_arg "Just what do you think you're doing, Dave?";
       called := true;
       console := printer
+
+  let is_valid_basename_char =
+    if Sys.win32 then
+      function
+      | '\000'..'\031'
+      | '<' | '>' | ':' | '"' | '/' | '\\' | '|' | '?' | '*' -> false
+      | _ -> true
+    else
+      fun c -> c <> '\000' && c <> '/'
 end
 
 

src/core/opamStd.mli

@@ -532,6 +532,10 @@ module Sys : sig
       OS) *)
   val path_sep: char
 
+  (** [true] if a character may be used in a filename, depending on the OS.
+      For example, on Windows, `:` and `?` can't be in the name. *)
+  val is_valid_basename_char: char -> bool
+
   (** Splits a PATH-like variable separated with [path_sep]. More involved than
       it seems, because there may be quoting on Windows. By default, it returns
       the path cleaned (remove trailing, leading, contiguous delimiters).

src/repository/opamDownload.ml

@@ -188,8 +188,18 @@ let download_as ?quiet ?validate ~overwrite ?compress ?checksum url dst =
       ()
 
 let download ?quiet ?validate ~overwrite ?compress ?checksum url dstdir =
+  let base =
+    let base = OpamUrl.basename url in
+    if Sys.win32 then
+      let f c =
+        if OpamStd.Sys.is_valid_basename_char c then c else '_'
+      in
+      String.map f base
+    else
+      base
+  in
   let dst =
-    OpamFilename.(create dstdir (Base.of_string (OpamUrl.basename url)))
+    OpamFilename.(create dstdir (Base.of_string base))
   in
   download_as ?quiet ?validate ~overwrite ?compress ?checksum url dst @@|
   fun () -> dst