Apply suggestions from code review

Co-authored-by: David Allsopp <[email protected]>
Co-authored-by: Kate <[email protected]>

Author: 3 people

src/state/opamFormatUpgrade.ml

@@ -1157,7 +1157,20 @@ let from_2_2_beta_to_2_2 ~on_the_fly:_ _ conf = conf, gtc_none
 
 let latest_version = OpamFile.Config.root_version
 
-(* Note to dev: try as possible to not change that value *)
+(* Development notes:
+   opam differentiates two kinds of format upgrade - "Hard" upgrades, which
+   must be written straight to disk, and "Light" upgrades, which can be
+   performed in-memory, and don't have to be written immediately.
+   This distinction was added in opam 2.1, as it allows users of a _newer_
+   version of opam-state to _read_ an opam root which is still being maintained
+   by an _older_ version of opam (for example, it allows a program compiled with
+   opam-state 2.1.0 to load the global configuration of an opam 2.0 user's root
+   without forcing the upgrade of that root to 2.1).
+
+   Essentially, a "Hard" upgrade is used where the change is difficult (or even
+   impossible) to perform in-memory. We try our hardest to keep all format
+   upgrades "Light" - i.e. the aim is that this version below should never
+   change. *)
 let latest_hard_upgrade = (* to *) v2_0_beta5
 
 (* intermediate roots that need a hard upgrade when upgrading from them *)
@@ -1360,14 +1373,16 @@ let as_necessary_repo_switch_t updates changes read_f lock_kind gt =
     (* If we don't have a written opam root version in a config file,
        we are unable to determine if there is an upgrade to do. This can happen in
        case there is only on the fly upgrades from 2.0. Should we fail ? *)
-    let written_root_version = OpamFile.Config.raw_root_version config_f
+    let written_root_version =
+      OpamFile.Config.raw_root_version config_f 
     in
     (* Config already upgraded *)
-    if OpamStd.Option.equal OpamVersion.equal
-        written_root_version
-        (Some OpamFile.Config.root_version) then None else
+    if OpamStd.Option.equal_some OpamVersion.equal
+        OpamFile.Config.root_version written_root_version then
+      None
+    else
       let written_root_version =
-        OpamStd.Option.default v2_0 written_root_version
+        OpamStd.Option.default OpamFile.Config.default_old_root_version written_root_version
       in
       let updates =
         List.filter (fun (v,_) ->

src/state/opamFormatUpgrade.mli

@@ -38,9 +38,9 @@ val as_necessary:
   OpamFile.Config.t ->
   OpamFile.Config.t * gt_changes
 
-(* [as_necessary_repo lock gt] does he upgrades at repo level. [lock] is the
+(* [as_necessary_repo lock gt] does the upgrades at repo level. [lock] is the
    required lock for repo state and gt the on-the-fly upgraded global lock. If
-   [lock] is non or read, it will perform an on-the-fly upgrade of repos-config
+   [lock] is none or read, it will perform an on-the-fly upgrade of repos-config
    and return it. if [lock] is a write lock, it locks opam root and perform and
    apply all needed upgrades (write global, repo & state config if needed).
    At global state loading, [as_necessary] is called, so this function will do

src/state/opamRepositoryState.ml

@@ -159,7 +159,7 @@ let load lock_kind gt =
   log "LOAD-REPOSITORY-STATE %@ %a" (slog OpamFilename.Dir.to_string) gt.root;
   let lock = OpamFilename.flock lock_kind (OpamPath.repos_lock gt.root) in
   let repos_map =
-    match repos_config with
+    match OpamFormatUpgrade.as_necessary_switch lock_kind switch gt with
     | Some repos_map -> repos_map
     | None -> OpamStateConfig.Repos.safe_read ~lock_kind gt
   in

src/state/opamSwitchState.ml

@@ -259,7 +259,7 @@ let load lock_kind gt rt switch =
     OpamFilename.flock lock_kind (OpamPath.Switch.lock gt.root switch)
   in
   let switch_config =
-    match switch_config with
+    match OpamFormatUpgrade.as_necessary_switch lock_kind switch gt with
     | Some switch_config -> switch_config
     | None -> load_switch_config ~lock_kind gt switch
   in