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

Check that the repositories given to "opam repository remove" actually exist #5014

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

kit-ty-kate
Copy link
Member

Fixes #5012

@rjbou rjbou added the PR: WIP Not for merge at this stage label Jan 20, 2022
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@rjbou rjbou self-assigned this May 18, 2022
@rjbou rjbou force-pushed the check-repos-remove-exist branch from d0cde13 to 6a0b06e Compare May 18, 2022 12:26
@rjbou rjbou self-requested a review January 23, 2023 13:47
@kit-ty-kate kit-ty-kate added PR: NEEDS UPDATE and removed PR: WIP Not for merge at this stage labels Jul 10, 2024
@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 10, 2024
@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch from 6a0b06e to 935929f Compare August 12, 2024 21:23
@kit-ty-kate
Copy link
Member Author

I've put back my original changes together with fixes to what needed fixing together with a couple of new tests.

Your original wip commit was 6a0b06e, i've also tried to improve the previous attempt in check-repos-remove-exist-old to no avail.
Given the increase of complexity of solutions that do not use ref i don't think it's worth demonizing references for such a local and non-performance-cricical use, and I think this work is ready to be reviewed and merged.

@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch 2 times, most recently from ddad6d5 to dc29f65 Compare August 16, 2024 16:56
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.

You don't want to load repository state to avoid having to load repositories to remove some ones ?

tests/reftests/repository.test Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch from bd79d97 to fd2a4ea Compare December 17, 2024 00:09
@kit-ty-kate kit-ty-kate requested a review from rjbou December 17, 2024 00:09
@kit-ty-kate
Copy link
Member Author

updated

@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch from fd2a4ea to 1ec90be Compare December 17, 2024 00:10
@kit-ty-kate
Copy link
Member Author

rebased

src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
src/client/opamCommands.ml Outdated Show resolved Hide resolved
@@ -2424,27 +2429,49 @@ let repository cli =
let rm = List.filter (fun n -> not (List.mem n names)) in
let full_wipe = List.mem `All scope in
let global = global || full_wipe in
let gt =
let update_gt () =
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of

      OpamRepositoryState.with_ `Lock_write gt @@ fun rt ->
      if full_wipe then
        let repos =
          OpamRepositoryName.Map.keys rt.OpamStateTypes.repositories
        in
        check_for_repos repos names
          (OpamConsole.warning
             "No configured repositories by these names found: %s");
        OpamRepositoryState.drop @@
        List.fold_left OpamRepositoryCommand.remove rt names
      else begin
        let has_known_repos =
          List.fold_left (fun has_known_repos switch ->
              let repos = OpamRepositoryCommand.switch_repos rt switch in
              check_for_repos' repos names
                (OpamConsole.warning
                   "No configured repositories by these names found in \
                    the selection of switch '%s': %s"
                   (OpamSwitch.to_string switch))
              || has_known_repos)
            false switches
        in
        if scope = [`Current_switch] && has_known_repos then
          OpamConsole.msg
            "Repositories removed from the selections of switch %s. \
             Use '--all' to forget about them altogether.\n"
            (OpamSwitch.to_string (OpamStateConfig.get_switch ()))
      end;
      let rm =
        List.filter (fun n ->
            not (List.exists (OpamRepositoryName.equal n) names))
      in
      ignore @@ OpamRepositoryCommand.update_selection gt
        ~global ~switches:switches rm;
      `Ok ()

diff
The idea is to factorise some code and make it more step by step, more "readable":

  • first loading repostate
  • then check availability, display comments, and remove in global case (take advantage of the if)
  • then for all, update switch selection (no need to drop as it is in an with_ function
  • end

Copy link
Member Author

Choose a reason for hiding this comment

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

I incorporated this diff except for the factorization of the repo-state loading as it would load it in write mode regardless of need (non full_wipe do not need it to be in write mode)

Copy link
Collaborator

Choose a reason for hiding this comment

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

bien vu!

src/client/opamRepositoryCommand.mli Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch from 1ec90be to 0c43661 Compare December 17, 2024 18:46
@kit-ty-kate
Copy link
Member Author

updated

@kit-ty-kate kit-ty-kate force-pushed the check-repos-remove-exist branch from 0c43661 to 567cd03 Compare December 17, 2024 18:47
@kit-ty-kate kit-ty-kate requested a review from rjbou December 17, 2024 18:49
@rjbou rjbou removed their assignment Dec 18, 2024
@rjbou rjbou merged commit 26290a4 into ocaml:master Dec 18, 2024
42 checks passed
@kit-ty-kate kit-ty-kate deleted the check-repos-remove-exist branch December 18, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confused by opam remote remove when remote does not exist
2 participants