Merge pull request #41 from mbarbin/set-ref

Prepare for 0.0.9

CHANGES.md

@@ -1,4 +1,4 @@
-## 0.0.9 (unreleased)
+## 0.0.9 (2024-10-23)
 
 ### Added
 
@@ -16,10 +16,9 @@
 - Rename what was `Vcs.Result` to `Vcs.Rresult` and introduce `Vcs.Result` whose type is simpler (#33, @mbarbin).
 - Moved `ocaml-vcs more-tests` commands at top-level (#28, @mbarbin).
 
-### Deprecated
-
 ### Fixed
 
+- Fixed stale refs information leaked by `Vcs.Graph.set_ref` (#40, @mbarbin).
 - Fixed some odoc warnings related to `Vcs_base` (#38, @mbarbin).
 - Changed some exceptions raised by the `vcs` related libraries to the `Vcs.E` exception (#34, @mbarbin).
 

doc/blog/2024-07-24-hello/index.md

@@ -6,3 +6,5 @@ tags: [hello]
 ---
 
 Hello! I've just launched a blog section within the ocaml-vcs documentation, powered by Docusaurus. This new space is designed to keep you updated with all things related to ocaml-vcs. Stay tuned for more updates and insights. Happy reading!
+
+<!-- truncate -->

dune-project

@@ -340,6 +340,8 @@
     (< v0.18)))
   (vcs
    (= :version))
+  (vcs-base
+   (= :version))
   (vcs-command
    (= :version))
   (vcs-git-blocking

example/hello_blocking.ml

@@ -36,7 +36,7 @@ let%expect_test "hello commit" =
     let path = Stdlib.Filename.temp_dir ~temp_dir:(Unix.getcwd ()) "vcs" "test" in
     Vcs_test_helpers.init vcs ~path:(Absolute_path.v path)
   in
-  (* Ok, we are all set, we are now inside a Git repo and we can start using
+  (* Ok, we are all set, [repo_root] points to a Git repo and we can start using
      [Vcs]. What we do in this example is simply create a new file and commit it
      to the repository, and query it from the store afterwards. *)
   let hello_file = Vcs.Path_in_repo.v "hello.txt" in

example/hello_error.ml

@@ -20,7 +20,7 @@
 (*******************************************************************************)
 
 (* In this test we run a function from the Vcs API, let it raise an exception
-   and show out to catch it. This allows for a basic coverage of the raising
+   and show how to catch it. This allows for a basic coverage of the raising
    case of the API too. *)
 
 let%expect_test "hello error" =

lib/vcs/src/err.mli

@@ -21,21 +21,24 @@
 
 (** The type of errors raised by [Vcs].
 
-    Under the hood, it is lazily constructed human-readable information which
-    also carry some context (a sort of a high level stack trace manipulated by
-    the programmers of Vcs).
+    Under the hood, it is human-readable information which also carries some
+    context (a sort of a high level stack trace manipulated by the programmers
+    of Vcs).
 
     It is not meant to be matched on, but rather to be printed on stderr (or
     perhaps logged). *)
 type t
 
-(** [sexp_of_t t] forces the lazy message, and allow printing the information
-    contained by [t]. *)
+(** {1 Printing} *)
+
+(** [sexp_of_t t] allows printing the information contained by [t]. *)
 val sexp_of_t : t -> Sexp.t
 
 (** [to_string_hum t] is a convenience wrapper around [t |> sexp_of_t |> Sexp.to_string_hum]. *)
 val to_string_hum : t -> string
 
+(** {1 Building} *)
+
 val error_string : string -> t
 val create_s : Sexp.t -> t
 val of_exn : exn -> t

lib/vcs/src/git.mli

@@ -19,8 +19,8 @@
 (*_  <http://www.gnu.org/licenses/> and <https://spdx.org>, respectively.       *)
 (*_******************************************************************************)
 
-(** Manipulating the output of process run by vcs and providers - typically the
-    ["git"] command. *)
+(** Manipulating the output of processes run by vcs and providers - typically
+    the ["git"] command. *)
 
 module Output : sig
   type t = Git_output0.t =

lib/vcs/src/graph.ml

@@ -234,6 +234,16 @@ let set_ref t ~rev ~ref_kind =
   match Rev_table.find t.revs rev with
   | None -> raise (Exn0.E (Err.create_s [%sexp "Rev not found", (rev : Rev.t)]))
   | Some index ->
+    (match Ref_kind_table.find t.refs ref_kind with
+     | None -> ()
+     | Some previous_node ->
+       (match Int_table.find t.reverse_refs previous_node with
+        | None -> assert false (* Inconsistency between [t.refs] and [t.reverse_refs]. *)
+        | Some refs ->
+          let refs = List.filter refs ~f:(fun r -> not (Ref_kind.equal r ref_kind)) in
+          if List.is_empty refs
+          then Int_table.remove t.reverse_refs previous_node
+          else Int_table.set t.reverse_refs ~key:previous_node ~data:refs));
     Ref_kind_table.set t.refs ~key:ref_kind ~data:index;
     Int_table.add_multi t.reverse_refs ~key:index ~data:ref_kind
 ;;

lib/vcs/src/graph.mli

@@ -49,8 +49,8 @@ val create : unit -> t
     simply operations that needs to be called to feed to [t] the information
     that already exists in the git log. *)
 
-(** [add t ~log] add to [t] all the nodes from the log. This is idempotent
-    this doesn't add the nodes that if [t] already knows.*)
+(** [add t ~log] add to [t] all the nodes from the log. This is idempotent -
+    this doesn't add the nodes that [t] already knows. *)
 val add_nodes : t -> log:Log.t -> unit
 
 (** [set_refs t ~refs] add to [t] all the refs from the log. *)
@@ -119,16 +119,16 @@ val prepend_parents : t -> node:Node.t -> prepend_to:Node.t list -> Node.t list
 val node_kind : t -> node:Node.t -> Node_kind.t
 
 (** If the graph has refs (such as tags or branches) attached to this node, they
-    will all be returned by [node_refs graph ~node]. The order of the refs in
-    the resulting list is not specified. *)
+    will all be returned by [node_refs graph ~node]. The refs are returned
+    ordered increasingly according to [Ref_kind.compare]. *)
 val node_refs : t -> node:Node.t -> Ref_kind.t list
 
 (** Return the number of nodes the graph currently holds. *)
 val node_count : t -> int
 
 (** {1 Refs} *)
 
-(** List known refs. *)
+(** List known refs, ordered increasingly according to [Ref_kind.compare]. *)
 val refs : t -> Refs.t
 
 (** Find a ref if it is present. *)
@@ -143,7 +143,7 @@ val find_rev : t -> rev:Rev.t -> Node.t option
     [find_rev graph ~rev = Some _]. *)
 val mem_rev : t -> rev:Rev.t -> bool
 
-(** {1 Roots & Tips} *)
+(** {1 Roots & Leaves} *)
 
 (** Return the list of nodes that do not have any parents. *)
 val roots : t -> Node.t list
@@ -175,12 +175,12 @@ val is_ancestor_or_equal : t -> ancestor:Node.t -> descendant:Node.t -> bool
     of all the nodes in the set and is not a strict ancestor of any other common
     ancestor of the nodes.
 
-    If the nodes in [nodes] are unrelated, the function returns an empty list. If
-    there are multiple greatest common ancestors, all of them are included in
-    the returned list.
+    If the nodes in [nodes] do not have common ancestors, the function returns
+    an empty list. If there are multiple greatest common ancestors, all of them
+    are included in the returned list.
 
-    Multiple nodes may have multiple greatest common ancestors, especially in
-    cases of complex merge histories, hence the list return type. *)
+    A set of nodes may have multiple greatest common ancestors, especially in
+    cases of complex merge histories, hence the list returned type. *)
 val greatest_common_ancestors : t -> nodes:Node.t list -> Node.t list
 
 module Descendance : sig

lib/vcs/test/test__graph.ml

@@ -590,15 +590,15 @@ let%expect_test "debug graph" =
     Vcs.Graph.add_nodes graph ~log:[ Vcs.Log.Line.Merge { rev; parent1; parent2 } ];
     rev
   in
-  let r1 = root () in
-  let r2 = commit ~parent:r1 in
-  let r3 = commit ~parent:r1 in
-  let m1 = merge ~parent1:r2 ~parent2:r3 in
+  let r0 = root () in
+  let r1 = commit ~parent:r0 in
+  let r2 = commit ~parent:r0 in
+  let m1 = merge ~parent1:r1 ~parent2:r2 in
   let r4 = commit ~parent:m1 in
   Vcs.Graph.set_refs
     graph
     ~refs:
-      [ { rev = r2
+      [ { rev = r1
         ; ref_kind =
             Remote_branch { remote_branch_name = Vcs.Remote_branch_name.v "origin/main" }
         }
@@ -651,9 +651,9 @@ let%expect_test "debug graph" =
     let node = node ~rev in
     print_s [%sexp (Vcs.Graph.node_kind graph ~node : Vcs.Graph.Node_kind.t)]
   in
-  node_kind r1;
+  node_kind r0;
   [%expect {| (Root (rev 5cd237e9598b11065c344d1eb33bc8c15cd237e9)) |}];
-  node_kind r2;
+  node_kind r1;
   [%expect
     {|
     (Commit
@@ -679,19 +679,19 @@ let%expect_test "debug graph" =
   let print_ancestors rev =
     print_s [%sexp (ancestors graph (node ~rev) : Set.M(Vcs.Graph.Node).t)]
   in
-  print_ancestors r1;
+  print_ancestors r0;
   [%expect {| (#0) |}];
-  print_ancestors r2;
+  print_ancestors r1;
   [%expect {| (#0 #1) |}];
-  print_ancestors r3;
+  print_ancestors r2;
   [%expect {| (#0 #2) |}];
   print_ancestors m1;
   [%expect {| (#0 #1 #2 #3) |}];
   print_ancestors r4;
   [%expect {| (#0 #1 #2 #3 #4) |}];
   (* Low level int indexing. *)
   let node_index node = print_s [%sexp (Vcs.Graph.node_index node : int)] in
-  node_index (node ~rev:r1);
+  node_index (node ~rev:r0);
   [%expect {| 0 |}];
   node_index (node ~rev:r4);
   [%expect {| 4 |}];
@@ -718,5 +718,105 @@ let%expect_test "debug graph" =
         (index      -1)
         (node_count 5))))
     |}];
+  (* Here we monitor for a regression of a bug where [set_ref] would not
+     properly update pre-existing bindings. *)
+  let upstream =
+    Vcs.Ref_kind.Remote_branch
+      { remote_branch_name = Vcs.Remote_branch_name.v "origin/main" }
+  in
+  let show_upstream () =
+    print_s
+      [%sexp (Vcs.Graph.find_ref graph ~ref_kind:upstream : Vcs.Graph.Node.t option)]
+  in
+  show_upstream ();
+  [%expect {| (#1) |}];
+  (* We are now simulating that we pushed [main] and thus upstream [origin/main]
+     has advanced to [r4]. *)
+  Vcs.Graph.set_ref graph ~rev:r4 ~ref_kind:upstream;
+  show_upstream ();
+  [%expect {| (#4) |}];
+  let show_refs rev =
+    print_s [%sexp (Vcs.Graph.node_refs graph ~node:(node ~rev) : Vcs.Ref_kind.t list)]
+  in
+  (* There are no longer any refs pointing to [r1]. *)
+  show_refs r1;
+  [%expect {| () |}];
+  (* Both [main] and [origin/main] now point to [r4]. *)
+  show_refs r4;
+  [%expect
+    {|
+    ((Local_branch (branch_name main))
+     (Remote_branch (
+       remote_branch_name (
+         (remote_name origin)
+         (branch_name main))))
+     (Tag (tag_name 0.1.0)))
+    |}];
+  print_s [%sexp (Vcs.Graph.refs graph : Vcs.Refs.t)];
+  [%expect
+    {|
+    (((rev 7216231cd107946841cc3eebe5da287b7216231c)
+      (ref_kind (Local_branch (branch_name main))))
+     ((rev 7216231cd107946841cc3eebe5da287b7216231c)
+      (ref_kind (
+        Remote_branch (
+          remote_branch_name (
+            (remote_name origin)
+            (branch_name main))))))
+     ((rev 7216231cd107946841cc3eebe5da287b7216231c)
+      (ref_kind (Tag (tag_name 0.1.0)))))
+    |}];
+  print_s [%sexp (graph : Vcs.Graph.t)];
+  [%expect
+    {|
+    ((nodes (
+       (#4 (
+         Commit
+         (rev    7216231cd107946841cc3eebe5da287b7216231c)
+         (parent #3)))
+       (#3 (
+         Merge
+         (rev     9a81fba7a18f740120f1141b1ed109bb9a81fba7)
+         (parent1 #1)
+         (parent2 #2)))
+       (#2 (
+         Commit
+         (rev    5deb4aaec51a75ef58765038b7c20b3f5deb4aae)
+         (parent #0)))
+       (#1 (
+         Commit
+         (rev    f453b802f640c6888df978c712057d17f453b802)
+         (parent #0)))
+       (#0 (Root (rev 5cd237e9598b11065c344d1eb33bc8c15cd237e9)))))
+     (revs (
+       (#4 7216231cd107946841cc3eebe5da287b7216231c)
+       (#3 9a81fba7a18f740120f1141b1ed109bb9a81fba7)
+       (#2 5deb4aaec51a75ef58765038b7c20b3f5deb4aae)
+       (#1 f453b802f640c6888df978c712057d17f453b802)
+       (#0 5cd237e9598b11065c344d1eb33bc8c15cd237e9)))
+     (refs ((
+       #4 (
+         (Local_branch (branch_name main))
+         (Remote_branch (
+           remote_branch_name (
+             (remote_name origin)
+             (branch_name main))))
+         (Tag (tag_name 0.1.0)))))))
+    |}];
+  (* We also test a case where [set_ref] leaves at least one ref at the previous
+     location. *)
+  let custom_A = Vcs.Ref_kind.Other { name = "custom-A" } in
+  Vcs.Graph.set_ref graph ~rev:r0 ~ref_kind:custom_A;
+  Vcs.Graph.set_ref graph ~rev:r0 ~ref_kind:(Other { name = "custom-B" });
+  show_refs r0;
+  [%expect {|
+    ((Other (name custom-A))
+     (Other (name custom-B)))
+    |}];
+  Vcs.Graph.set_ref graph ~rev:r1 ~ref_kind:custom_A;
+  show_refs r0;
+  [%expect {| ((Other (name custom-B))) |}];
+  show_refs r1;
+  [%expect {| ((Other (name custom-A))) |}];
   ()
 ;;

lib/vcs_base/src/vcs_base.mli

@@ -19,6 +19,46 @@
 (*_  <http://www.gnu.org/licenses/> and <https://spdx.org>, respectively.       *)
 (*_******************************************************************************)
 
+(** An extension of the Vcs library for use with Base.
+
+    [Vcs_base] is a library that extends the [Vcs] library with additional
+    modules and functionalities, aimed to improve the compatibility of [Vcs] for
+    programs using [Base].
+
+    For example, it adds [Comparable.S] to all container keys modules so that
+    they can be used with Base-style containers:
+
+    {[
+      let create_path_in_repo_table () = Hashtbl.create (module Vcs.Path_in_repo)
+    ]}
+
+    There's also a new module [Vcs.Or_error] which allows using [Vcs] with the
+    [Or_error] monad.
+
+    The library is designed to be used as a drop-in replacement for [Vcs]. For
+    this, it includes a single module named [Vcs] which must be setup to shadow
+    the regular [Vcs] module.
+
+    You may do so by defining the following module alias in a place that's
+    available to your scope:
+
+    {[
+      module Vcs = Vcs_base.Vcs
+    ]}
+
+    Another way to achieve this is to open [Vcs_base] via dune flags. When doing
+    that, all the files in your library will use [Vcs_base.Vcs] consistently.
+
+    {v
+      (library
+        (name my_library)
+        (flags (:standard -open Vcs_base))
+        (libraries vcs-base))
+    v}
+
+    This pattern is Vcs's authors favorite way of using [Vcs_base] and is the
+    way we're setting up [Vcs_base] in the examples of the Vcs repository. *)
+
 module Vcs : sig
   (** {1 Extended Vcs API} *)
 

test/data/README.md

@@ -4,11 +4,9 @@ This directory contains some files that are used by tests.
 
 ## Repos source
 
-Data files are the result of git commands run inside a clone of different repos,
-keeping the repo name as basename for the files.
+Data files are the result of git commands run inside a clone of different repos, keeping the repo name as basename for the files.
 
-The repos were chosen somewhat arbitrarily, or given as part of a bug report,
-etc. We can add more files from other repos as needed.
+The repos were chosen somewhat arbitrarily, or given as part of a bug report, etc. We can add more files from other repos as needed.
 
 - [super-master-mind](https://github.com/mbarbin/super-master-mind)
 - [eio](https://github.com/ocaml-multicore/eio.git)