Merge pull request ocaml#9683 from Leonidas-from-XIV/remove-skip-update

Remove `--skip-update` and make it implied when possible

bin/pkg/lock.ml

@@ -8,7 +8,6 @@ let solve_lock_dir
   workspace
   ~local_packages
   version_preference
-  ~update_opam_repositories
   solver_env_from_current_system
   lock_dir_path
   =
@@ -27,9 +26,7 @@ let solve_lock_dir
   in
   let* repos =
     repositories_of_workspace workspace
-    |> get_repos
-         ~repositories:(repositories_of_lock_dir workspace ~lock_dir_path)
-         ~update_opam_repositories
+    |> get_repos ~repositories:(repositories_of_lock_dir workspace ~lock_dir_path)
   in
   Fiber.finalize
     ~finally:(fun () ->
@@ -66,13 +63,7 @@ let solve_lock_dir
     Ok (Lock_dir.Write_disk.prepare ~lock_dir_path ~files lock_dir, summary_message)
 ;;
 
-let solve
-  workspace
-  ~update_opam_repositories
-  ~solver_env_from_current_system
-  ~version_preference
-  ~lock_dirs_arg
-  =
+let solve workspace ~solver_env_from_current_system ~version_preference ~lock_dirs_arg =
   let open Fiber.O in
   (* a list of thunks that will perform all the file IO side
      effects after performing validation so that if materializing any
@@ -86,7 +77,6 @@ let solve
                workspace
                ~local_packages
                version_preference
-               ~update_opam_repositories
                solver_env_from_current_system)
      >>| List.partition_map ~f:Result.to_either
    in
@@ -108,39 +98,24 @@ let solve
     List.iter write_disk_list ~f:Lock_dir.Write_disk.commit
 ;;
 
-let lock ~version_preference ~update_opam_repositories ~lock_dirs_arg =
+let lock ~version_preference ~lock_dirs_arg =
   let open Fiber.O in
   let* workspace = Memo.run (Workspace.workspace ())
   and* solver_env_from_current_system =
     Dune_pkg.Sys_poll.make ~path:(Env_path.path Stdune.Env.initial)
     |> Dune_pkg.Sys_poll.solver_env_from_current_system
     >>| Option.some
   in
-  solve
-    workspace
-    ~update_opam_repositories
-    ~solver_env_from_current_system
-    ~version_preference
-    ~lock_dirs_arg
+  solve workspace ~solver_env_from_current_system ~version_preference ~lock_dirs_arg
 ;;
 
 let term =
   let+ builder = Common.Builder.term
   and+ version_preference = Version_preference.term
-  and+ skip_update =
-    Arg.(
-      value
-      & flag
-      & info
-          [ "skip-update" ]
-          ~doc:
-            "Do not fetch updates of opam repositories, will use the cached opam \
-             metadata. This allows offline use if the repositories are cached locally.")
   and+ lock_dirs_arg = Pkg_common.Lock_dirs_arg.term in
   let builder = Common.Builder.forbid_builds builder in
   let common, config = Common.init builder in
-  Scheduler.go ~common ~config (fun () ->
-    lock ~version_preference ~update_opam_repositories:(not skip_update) ~lock_dirs_arg)
+  Scheduler.go ~common ~config (fun () -> lock ~version_preference ~lock_dirs_arg)
 ;;
 
 let info =

bin/pkg/outdated.ml

@@ -14,7 +14,6 @@ let find_outdated_packages ~transitive ~lock_dirs_arg () =
         get_repos
           (repositories_of_workspace workspace)
           ~repositories:(repositories_of_lock_dir workspace ~lock_dir_path)
-          ~update_opam_repositories:true
       and+ local_packages = find_local_packages in
       let lock_dir = Lock_dir.read_disk lock_dir_path in
       let+ results = Dune_pkg_outdated.find ~repos ~local_packages lock_dir.packages in

bin/pkg/pkg_common.ml

@@ -86,7 +86,7 @@ let location_of_opam_url loc url =
       ]
 ;;
 
-let get_repos repos ~repositories ~update_opam_repositories =
+let get_repos repos ~repositories =
   let open Fiber.O in
   let module Repository = Dune_pkg.Pkg_workspace.Repository in
   repositories
@@ -104,7 +104,7 @@ let get_repos repos ~repositories ~update_opam_repositories =
       (match location_of_opam_url loc opam_url with
        | `Git ->
          let* source = Opam_repo.Source.of_opam_url loc opam_url in
-         Opam_repo.of_git_repo ~update:update_opam_repositories source
+         Opam_repo.of_git_repo source
        | `Path path -> Fiber.return @@ Opam_repo.of_opam_repo_dir_path loc path))
 ;;
 

bin/pkg/pkg_common.mli

@@ -41,7 +41,6 @@ val constraints_of_workspace
 val get_repos
   :  Dune_pkg.Pkg_workspace.Repository.t Dune_pkg.Pkg_workspace.Repository.Name.Map.t
   -> repositories:(Loc.t * Dune_pkg.Pkg_workspace.Repository.Name.t) list
-  -> update_opam_repositories:bool
   -> Dune_pkg.Opam_repo.t list Fiber.t
 
 val find_local_packages : Dune_pkg.Local_package.t Package_name.Map.t Fiber.t

src/dune_pkg/opam_repo.ml

@@ -58,25 +58,37 @@ module Source = struct
              we need to look up *)
           Rev_store.mem rev_store ~rev:ref
           >>= (function
-           | true -> Fiber.return @@ Some (Commitish.Commit ref)
+           | true ->
+             (* CR-Leonidas-from-XIV is this always a commit? *)
+             Fiber.return @@ Some (Commitish.Commit ref)
            | false ->
              Rev_store.ref_type rev_store ~source:url ~ref
-             >>| (function
-              | Some `Tag -> Some (Commitish.Tag ref)
-              | Some `Head -> Some (Commitish.Branch ref)
+             >>= (function
+              | Some `Tag -> Fiber.return @@ Some (Commitish.Tag ref)
+              | Some `Head -> Fiber.return @@ Some (Commitish.Branch ref)
               | None ->
-                User_error.raise
-                  ~loc
-                  ~hints:
-                    [ Pp.text
-                        "Make sure the URL is correct and the repository contains the \
-                         branch/tag"
-                    ]
-                  [ Pp.textf
-                      "Opam repository at '%s' does not have a reference '%s'"
-                      url
-                      ref
-                  ]))
+                (* we have to update the local repo as a side-effect and see if
+                   the commit exists *)
+                let* (_ : Rev_store.Remote.t) =
+                  Rev_store.add_repo rev_store ~source:url ~branch:None
+                  >>= Rev_store.Remote.update
+                in
+                Rev_store.mem rev_store ~rev:ref
+                >>= (function
+                 | true -> Fiber.return @@ Some (Commitish.Commit ref)
+                 | false ->
+                   User_error.raise
+                     ~loc
+                     ~hints:
+                       [ Pp.text
+                           "Make sure the URL is correct and the repository contains the \
+                            branch/tag"
+                       ]
+                     [ Pp.textf
+                         "Opam repository at '%s' does not have a reference '%s'"
+                         url
+                         ref
+                     ])))
       in
       { commit; url; loc }
     ;;
@@ -174,7 +186,7 @@ let of_opam_repo_dir_path loc opam_repo_dir_path =
   { source = Directory opam_repo_dir_path; serializable = None; loc }
 ;;
 
-let of_git_repo ~update (source : Source.t) =
+let of_git_repo (source : Source.t) =
   let+ at_rev =
     let* remote =
       let* repo = Rev_store.get in
@@ -184,9 +196,13 @@ let of_git_repo ~update (source : Source.t) =
         | _ -> None
       in
       let* remote = Rev_store.add_repo repo ~source:source.url ~branch in
-      match update with
-      | true -> Rev_store.Remote.update remote
-      | false -> Fiber.return @@ Rev_store.Remote.don't_update remote
+      match source.commit with
+      | Some (Commit rev) ->
+        let* exists = Rev_store.mem repo ~rev in
+        (match exists with
+         | true -> Fiber.return @@ Rev_store.Remote.don't_update remote
+         | false -> Rev_store.Remote.update remote)
+      | Some (Branch _) | Some (Tag _) | None -> Rev_store.Remote.update remote
     in
     match source.commit with
     | Some (Commit ref) -> Rev_store.Remote.rev_of_ref remote ~ref

src/dune_pkg/opam_repo.mli

@@ -38,13 +38,10 @@ end
     directory in the path given by [opam_repo_dir]. *)
 val of_opam_repo_dir_path : Loc.t -> Path.t -> t
 
-(** [of_git_repo git ~update source] loads the opam repository located
+(** [of_git_repo git source] loads the opam repository located
     at [source] from git. [source] can be any URL that [git remote add]
-    supports.
-
-    Set [update] to true to update the source to the newest revision, otherwise
-    it will use the latest data available in the cache (if any). *)
-val of_git_repo : update:bool -> Source.t -> t Fiber.t
+    supports. *)
+val of_git_repo : Source.t -> t Fiber.t
 
 val serializable : t -> Serializable.t option
 

src/dune_pkg/rev_store.ml

@@ -127,7 +127,7 @@ let mem { dir } ~rev =
   let failure_mode = Vcs.git_accept () in
   let stderr_to = make_stderr () in
   let stdout_to = make_stdout () in
-  let command = [ "rev-parse"; rev ] in
+  let command = [ "cat-file"; "-t"; rev ] in
   let+ res =
     Process.run ~dir ~display ~stdout_to ~stderr_to ~env failure_mode git command
   in

test/blackbox-tests/test-cases/pkg/opam-repository-download.t

@@ -121,6 +121,7 @@ A new package is released in the repo:
   $ cd mock-opam-repository
   $ git add -A
   $ git commit -m "bar.1.0.0" --quiet
+  $ NEWEST_REPO_HASH=$(git rev-parse HEAD)
   $ cd ..
 
 Since we have a working cached copy we get the old version of `bar` if we opt
@@ -130,7 +131,7 @@ out of the auto update.
   > (lang dune 3.10)
   > (repository
   >  (name mock)
-  >  (source "git+file://$(pwd)/mock-opam-repository"))
+  >  (source "git+file://$(pwd)/mock-opam-repository#${NEW_REPO_HASH}"))
   > (lock_dir
   >  (repositories mock))
   > EOF
@@ -142,14 +143,22 @@ To be safe it doesn't access the repo, we make sure to move the mock-repo away
 So now the test should work as it can't access the repo:
 
   $ rm -r dune.lock
-  $ dune pkg lock --skip-update
+  $ dune pkg lock
   Solution for dune.lock:
   - bar.0.0.1
   - foo.0.1.0
 
 But it will also get the new version of bar if we attempt to lock again (having
 restored the repo to where it was before)
 
+  $ cat > dune-workspace <<EOF
+  > (lang dune 3.10)
+  > (repository
+  >  (name mock)
+  >  (source "git+file://$(pwd)/mock-opam-repository#${NEWEST_REPO_HASH}"))
+  > (lock_dir
+  >  (repositories mock))
+  > EOF
   $ mv elsewhere mock-opam-repository
   $ rm -r dune.lock
   $ dune pkg lock