[commitgraph] refactor Graph, Position, and access module

Author: Byron

README.md

@@ -197,7 +197,9 @@ become available.
   * [ ] API documentation with examples
 
 ### git-commitgraph
-  * Access to all capabilities provided by the file format, as well as their maintenance
+  * [x] read-only access
+     * [x] Graph lookup of commit information to obtain timestamps, generation and parents
+  * [ ] create and update graphs and graph files
   * [ ] API documentation with examples
 
 ### git-config

git-commitgraph/src/file/commit.rs

@@ -1,5 +1,5 @@
 use crate::file::{File, LexPosition};
-use crate::graph::GraphPosition;
+use crate::graph::Position;
 use byteorder::{BigEndian, ByteOrder};
 use git_object::{borrowed, owned, SHA1_SIZE};
 use quick_error::quick_error;
@@ -79,7 +79,7 @@ impl<'a> Commit<'a> {
         self.generation
     }
 
-    pub fn iter_parents(&'a self) -> impl Iterator<Item = Result<GraphPosition, Error>> + 'a {
+    pub fn iter_parents(&'a self) -> impl Iterator<Item = Result<Position, Error>> + 'a {
         // I didn't find a combinator approach that a) was as strict as ParentIterator, b) supported
         // fuse-after-first-error behavior, and b) was significantly shorter or more understandable
         // than ParentIterator. So here we are.
@@ -93,7 +93,7 @@ impl<'a> Commit<'a> {
         self.file.id_at(self.lex_pos)
     }
 
-    pub fn parent1(&self) -> Result<Option<GraphPosition>, Error> {
+    pub fn parent1(&self) -> Result<Option<Position>, Error> {
         self.iter_parents().next().transpose()
     }
 
@@ -131,7 +131,7 @@ pub struct ParentIterator<'a> {
 }
 
 impl<'a> Iterator for ParentIterator<'a> {
-    type Item = Result<GraphPosition, Error>;
+    type Item = Result<Position, Error>;
 
     fn next(&mut self) -> Option<Self::Item> {
         let state = std::mem::replace(&mut self.state, ParentIteratorState::Exhausted);
@@ -219,7 +219,7 @@ enum ParentIteratorState<'a> {
 #[derive(Clone, Copy, Debug)]
 enum ParentEdge {
     None,
-    GraphPosition(GraphPosition),
+    GraphPosition(Position),
     ExtraEdgeIndex(u32),
 }
 
@@ -231,24 +231,24 @@ impl ParentEdge {
         if raw & EXTENDED_EDGES_MASK != 0 {
             ParentEdge::ExtraEdgeIndex(raw & !EXTENDED_EDGES_MASK)
         } else {
-            ParentEdge::GraphPosition(GraphPosition(raw))
+            ParentEdge::GraphPosition(Position(raw))
         }
     }
 }
 
 const LAST_EXTENDED_EDGE_MASK: u32 = 0x8000_0000;
 
 enum ExtraEdge {
-    Internal(GraphPosition),
-    Last(GraphPosition),
+    Internal(Position),
+    Last(Position),
 }
 
 impl ExtraEdge {
     pub fn from_raw(raw: u32) -> Self {
         if raw & LAST_EXTENDED_EDGE_MASK != 0 {
-            Self::Last(GraphPosition(raw & !LAST_EXTENDED_EDGE_MASK))
+            Self::Last(Position(raw & !LAST_EXTENDED_EDGE_MASK))
         } else {
-            Self::Internal(GraphPosition(raw))
+            Self::Internal(Position(raw))
         }
     }
 }

git-commitgraph/src/graph/access.rs

@@ -1,9 +1,12 @@
-use crate::file::{Commit, File, LexPosition};
-use crate::graph::{Graph, GraphPosition};
+use crate::{
+    file::{Commit, File, LexPosition},
+    graph::{Graph, Position},
+};
 use git_object::borrowed;
 
+/// Access convenience
 impl Graph {
-    pub fn commit_at(&self, pos: GraphPosition) -> Commit<'_> {
+    pub fn commit_at(&self, pos: Position) -> Commit<'_> {
         let r = self.lookup_by_pos(pos);
         r.file.commit_at(r.lex_pos)
     }
@@ -13,7 +16,7 @@ impl Graph {
         Some(r.file.commit_at(r.lex_pos))
     }
 
-    pub fn id_at(&self, pos: GraphPosition) -> borrowed::Id<'_> {
+    pub fn id_at(&self, pos: Position) -> borrowed::Id<'_> {
         let r = self.lookup_by_pos(pos);
         r.file.id_at(r.lex_pos)
     }
@@ -28,7 +31,7 @@ impl Graph {
         self.files.iter().flat_map(|file| file.iter_ids())
     }
 
-    pub fn lookup(&self, id: borrowed::Id<'_>) -> Option<GraphPosition> {
+    pub fn lookup(&self, id: borrowed::Id<'_>) -> Option<Position> {
         Some(self.lookup_by_id(id)?.graph_pos)
     }
 
@@ -37,25 +40,26 @@ impl Graph {
     }
 }
 
+/// Access fundamentals
 impl Graph {
     fn lookup_by_id(&self, id: borrowed::Id<'_>) -> Option<LookupByIdResult<'_>> {
         let mut current_file_start = 0;
-        for file in self.files.iter() {
+        for file in &self.files {
             if let Some(lex_pos) = file.lookup(id) {
                 return Some(LookupByIdResult {
                     file,
                     lex_pos,
-                    graph_pos: GraphPosition(current_file_start + lex_pos.0),
+                    graph_pos: Position(current_file_start + lex_pos.0),
                 });
             }
             current_file_start += file.num_commits();
         }
         None
     }
 
-    fn lookup_by_pos(&self, pos: GraphPosition) -> LookupByPositionResult<'_> {
+    fn lookup_by_pos(&self, pos: Position) -> LookupByPositionResult<'_> {
         let mut remaining = pos.0;
-        for file in self.files.iter() {
+        for file in &self.files {
             match remaining.checked_sub(file.num_commits()) {
                 Some(v) => remaining = v,
                 None => {
@@ -70,12 +74,14 @@ impl Graph {
     }
 }
 
+#[derive(Clone)]
 struct LookupByIdResult<'a> {
     pub file: &'a File,
-    pub graph_pos: GraphPosition,
+    pub graph_pos: Position,
     pub lex_pos: LexPosition,
 }
 
+#[derive(Clone)]
 struct LookupByPositionResult<'a> {
     pub file: &'a File,
     pub lex_pos: LexPosition,

git-commitgraph/src/graph/init.rs

@@ -1,9 +1,13 @@
-use crate::file::File;
-use crate::{file, Graph, MAX_COMMITS};
+use crate::{
+    file::{self, File},
+    Graph, MAX_COMMITS,
+};
 use git_object::HashKind;
 use quick_error::quick_error;
-use std::io::{BufRead, BufReader};
-use std::path::{Path, PathBuf};
+use std::{
+    io::{BufRead, BufReader},
+    path::{Path, PathBuf},
+};
 
 quick_error! {
     #[derive(Debug)]

git-commitgraph/src/graph/mod.rs

@@ -3,7 +3,7 @@ mod access;
 mod init;
 
 use crate::file::File;
-use std::fmt::{Display, Formatter};
+use std::fmt;
 
 /// A complete commit graph.
 ///
@@ -14,11 +14,11 @@ pub struct Graph {
     files: Vec<File>,
 }
 
-#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)]
-pub struct GraphPosition(pub u32);
+#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd, Hash)]
+pub struct Position(pub u32);
 
-impl Display for GraphPosition {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+impl fmt::Display for Position {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         self.0.fmt(f)
     }
 }

git-commitgraph/src/lib.rs

@@ -4,7 +4,7 @@
 pub mod file;
 pub mod graph;
 
-pub use graph::{Graph, GraphPosition};
+pub use graph::Graph;
 
 /// The maximum number of commits that can be stored in a commit graph.
 pub const MAX_COMMITS: u32 = (1 << 30) + (1 << 29) + (1 << 28) - 1;

git-commitgraph/tests/commitgraph.rs

@@ -1,4 +1,4 @@
-use git_commitgraph::{Graph, GraphPosition};
+use git_commitgraph::{graph::Position as GraphPosition, Graph};
 use git_object::{borrowed, owned};
 use std::{
     collections::{HashMap, HashSet},

tasks.md

@@ -20,6 +20,9 @@
   * [ ] full docs
 * **git-commitgraph** review
   * [x] adjust tests to disable gpgsignatures
+  * [ ] ~~do graph results need a reference to their owning file?~~
+     * Yes, as it allows to obtain additional information related to the item in the file itself, like `File::commit_at(…)`
+  * [ ] feature-toggled support for serde
   * [ ] ~~make tests depend on checked-in fixtures, instead of generating them (and depend on git on CI), making it easy to recreate them~~
      * the tests currently rely on calling git, see `inspect_refs(…)`
 * **git-config**