Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename and slightly repurpose OpamClient.git_for_windows_check #5997

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 6, 2024

The diff for this PR is horrific in the GH interface, but with --ignore-space-change, it is just:

diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml
index 879b2a770..e768ca879 100644
--- a/src/client/opamClient.ml
+++ b/src/client/opamClient.ml
@@ -654,9 +654,7 @@ let is_git_for_windows git =
     end
   | _ -> false

-let git_for_windows_check =
-  if not Sys.win32 then fun ?git_location:_ () -> None else
-  fun ?git_location () ->
+let git_for_windows ?git_location () =
   let header () = OpamConsole.header_msg "Git" in
   let contains_git p =
     OpamSystem.resolve_command ~env:[||] (Filename.concat p "git.exe")
@@ -693,7 +691,8 @@ let git_for_windows_check =
            warning suggesting that the machine be reconfigured to enable them
            in PATH, but also gives the opportunity to use the git-location
            mechanism to select it for opam's internal use. *)
-          let test_for_installation ((gits, gfw_message, abort_action) as acc) (hive, key) =
+        let test_for_installation ((gits, gfw_message, abort_action) as acc)
+                                  (hive, key) =
           let process root =
             let git_location = Filename.concat root "cmd" in
             let git = Filename.concat git_location "git.exe" in
@@ -764,7 +763,9 @@ let git_for_windows_check =
       let options =
         (`Default, "Use default Cygwin Git")
         :: (List.filter_map (fun (git, bash) ->
-              if bash then None else
+            if bash then
+              None
+            else
               let bin = Filename.dirname git in
               Some (`Location bin, "Use found git in "^bin))
             gits)
@@ -851,7 +852,12 @@ let windows_checks ?cygwin_setup ?git_location config =
         (OpamFilename.Dir.to_string gl_cli) ;
       Some (Left gl_cli)
   in
-  let git_location = git_for_windows_check ?git_location () in
+  let git_location =
+    if Sys.win32 then
+      git_for_windows ?git_location ()
+    else
+      None
+  in
   OpamCoreConfig.update ?git_location ();
   let config =
     match git_location with

The idea is to make the diff of the large opam init PR which is coming a bit easier to handle. The semantic change is instead of having a no-op function is to make it the responsibility of the caller not to call this function on "not Windows".

@dra27 dra27 force-pushed the git-location-diff branch from 1bdb213 to 394d364 Compare June 7, 2024 08:55
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 933d4bc into ocaml:master Jun 7, 2024
29 checks passed
@dra27 dra27 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants