feat!: turn entry::Stage into an enum.

Byron · Byron · commit a6cc78176c5b · 2024-04-03T22:08:23.000+02:00
This way, it's much clearer what possible values are.
For performance, it also adds `Entry::stage_raw()` to avoid having
to match on a number.
diff --git a/gix-index/src/access/mod.rs b/gix-index/src/access/mod.rs
@@ -3,6 +3,7 @@ use std::{cmp::Ordering, ops::Range};
 use bstr::{BStr, ByteSlice, ByteVec};
 use filetime::FileTime;
 
+use crate::entry::{Stage, StageRaw};
 use crate::{entry, extension, AccelerateLookup, Entry, PathStorage, PathStorageRef, State, Version};
 
 // TODO: integrate this somehow, somewhere, depending on later usage.
@@ -81,7 +82,7 @@ impl State {
                 res
             })
             .ok()?;
-        self.entry_index_by_idx_and_stage(path, idx, stage, stage_cmp)
+        self.entry_index_by_idx_and_stage(path, idx, stage as StageRaw, stage_cmp)
     }
 
     /// Walk as far in `direction` as possible, with [`Ordering::Greater`] towards higher stages, and [`Ordering::Less`]
@@ -112,7 +113,7 @@ impl State {
         &self,
         path: &BStr,
         idx: usize,
-        wanted_stage: entry::Stage,
+        wanted_stage: entry::StageRaw,
         stage_cmp: Ordering,
     ) -> Option<usize> {
         match stage_cmp {
@@ -121,15 +122,15 @@ impl State {
                 .enumerate()
                 .rev()
                 .take_while(|(_, e)| e.path(self) == path)
-                .find_map(|(idx, e)| (e.stage() == wanted_stage).then_some(idx)),
+                .find_map(|(idx, e)| (e.stage_raw() == wanted_stage).then_some(idx)),
             Ordering::Equal => Some(idx),
             Ordering::Less => self
                 .entries
                 .get(idx + 1..)?
                 .iter()
                 .enumerate()
                 .take_while(|(_, e)| e.path(self) == path)
-                .find_map(|(ofs, e)| (e.stage() == wanted_stage).then_some(idx + ofs + 1)),
+                .find_map(|(ofs, e)| (e.stage_raw() == wanted_stage).then_some(idx + ofs + 1)),
         }
     }
 
@@ -291,15 +292,15 @@ impl State {
             .binary_search_by(|e| {
                 let res = e.path(self).cmp(path);
                 if res.is_eq() {
-                    stage_at_index = e.stage();
+                    stage_at_index = e.stage_raw();
                 }
                 res
             })
             .ok()?;
         let idx = if stage_at_index == 0 || stage_at_index == 2 {
             idx
         } else {
-            self.entry_index_by_idx_and_stage(path, idx, 2, stage_at_index.cmp(&2))?
+            self.entry_index_by_idx_and_stage(path, idx, Stage::Ours as StageRaw, stage_at_index.cmp(&2))?
         };
         Some(&self.entries[idx])
     }
@@ -334,13 +335,13 @@ impl State {
             + self.entries[low..].partition_point(|e| e.path(self).get(..prefix_len).map_or(false, |p| p <= prefix));
 
         let low_entry = &self.entries.get(low)?;
-        if low_entry.stage() != 0 {
+        if low_entry.stage_raw() != 0 {
             low = self
                 .walk_entry_stages(low_entry.path(self), low, Ordering::Less)
                 .unwrap_or(low);
         }
         if let Some(high_entry) = self.entries.get(high) {
-            if high_entry.stage() != 0 {
+            if high_entry.stage_raw() != 0 {
                 high = self
                     .walk_entry_stages(high_entry.path(self), high, Ordering::Less)
                     .unwrap_or(high);
@@ -374,7 +375,7 @@ impl State {
             .binary_search_by(|e| {
                 let res = e.path(self).cmp(path);
                 if res.is_eq() {
-                    stage_at_index = e.stage();
+                    stage_at_index = e.stage_raw();
                 }
                 res
             })
diff --git a/gix-index/src/entry/flags.rs b/gix-index/src/entry/flags.rs
@@ -62,6 +62,23 @@ bitflags! {
 impl Flags {
     /// Return the stage as extracted from the bits of this instance.
     pub fn stage(&self) -> Stage {
+        match self.stage_raw() {
+            0 => Stage::Unconflicted,
+            1 => Stage::Base,
+            2 => Stage::Ours,
+            3 => Stage::Theirs,
+            _ => unreachable!("BUG: Flags::STAGE_MASK is two bits, whose 4 possible values we have covered"),
+        }
+    }
+
+    /// Return an entry's stage as raw number between 0 and 4.
+    /// Possible values are:
+    ///
+    /// * 0 = no conflict,
+    /// * 1 = base,
+    /// * 2 = ours,
+    /// * 3 = theirs
+    pub fn stage_raw(&self) -> u32 {
         (*self & Flags::STAGE_MASK).bits() >> 12
     }
 
diff --git a/gix-index/src/entry/mod.rs b/gix-index/src/entry/mod.rs
@@ -1,9 +1,23 @@
-/// The stage of an entry, one of…
+/// The stage of an entry.
+#[derive(Default, Copy, Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash)]
+pub enum Stage {
+    /// This is the default, and most entries are in this stage.
+    #[default]
+    Unconflicted = 0,
+    /// The entry is the common base between 'our' change and 'their' change, for comparison.
+    Base = 1,
+    /// The entry represents our change.
+    Ours = 2,
+    /// The entry represents their change.
+    Theirs = 3,
+}
+
+// The stage of an entry, one of…
 /// * 0 = no conflict,
 /// * 1 = base,
 /// * 2 = ours,
 /// * 3 = theirs
-pub type Stage = u32;
+pub type StageRaw = u32;
 
 ///
 #[allow(clippy::empty_docs)]
@@ -78,6 +92,17 @@ mod access {
         pub fn stage(&self) -> entry::Stage {
             self.flags.stage()
         }
+
+        /// Return an entry's stage as raw number between 0 and 4.
+        /// Possible values are:
+        ///
+        /// * 0 = no conflict,
+        /// * 1 = base,
+        /// * 2 = ours,
+        /// * 3 = theirs
+        pub fn stage_raw(&self) -> u32 {
+            self.flags.stage_raw()
+        }
     }
 }
 
diff --git a/gix-index/src/lib.rs b/gix-index/src/lib.rs
@@ -144,6 +144,7 @@ pub struct State {
 }
 
 mod impls {
+    use crate::entry::Stage;
     use std::fmt::{Debug, Formatter};
 
     use crate::State;
@@ -155,11 +156,10 @@ mod impls {
                     f,
                     "{} {}{:?} {} {}",
                     match entry.flags.stage() {
-                        0 => "       ",
-                        1 => "BASE   ",
-                        2 => "OURS   ",
-                        3 => "THEIRS ",
-                        _ => "UNKNOWN",
+                        Stage::Unconflicted => "       ",
+                        Stage::Base => "BASE   ",
+                        Stage::Ours => "OURS   ",
+                        Stage::Theirs => "THEIRS ",
                     },
                     if entry.flags.is_empty() {
                         "".to_string()
diff --git a/gix-index/tests/index/access.rs b/gix-index/tests/index/access.rs
@@ -1,5 +1,6 @@
 use crate::index::Fixture;
 use bstr::{BString, ByteSlice};
+use gix_index::entry::Stage;
 
 fn icase_fixture() -> gix_index::File {
     Fixture::Generated("v2_icase_name_clashes").open()
@@ -11,7 +12,7 @@ fn entry_by_path() {
     for entry in file.entries() {
         let path = entry.path(&file);
         assert_eq!(file.entry_by_path(path), Some(entry));
-        assert_eq!(file.entry_by_path_and_stage(path, 0), Some(entry));
+        assert_eq!(file.entry_by_path_and_stage(path, Stage::Unconflicted), Some(entry));
     }
 }
 
@@ -140,27 +141,27 @@ fn entry_by_path_and_stage() {
     for entry in file.entries() {
         let path = entry.path(&file);
         assert_eq!(
-            file.entry_index_by_path_and_stage(path, 0)
+            file.entry_index_by_path_and_stage(path, Stage::Unconflicted)
                 .map(|idx| &file.entries()[idx]),
             Some(entry)
         );
-        assert_eq!(file.entry_by_path_and_stage(path, 0), Some(entry));
+        assert_eq!(file.entry_by_path_and_stage(path, Stage::Unconflicted), Some(entry));
     }
 }
 
 #[test]
 fn entry_by_path_with_conflicting_file() {
     let file = Fixture::Loose("conflicting-file").open();
-    for expected_stage in [1 /* common ancestor */, 2 /* ours */, 3 /* theirs */] {
+    for expected_stage in [Stage::Base,Stage::Ours, Stage::Theirs] {
         assert!(
             file.entry_by_path_and_stage("file".into(), expected_stage).is_some(),
-            "we have no stage 0 during a conflict, but all other ones. Missed {expected_stage}"
+            "we have no stage 0 during a conflict, but all other ones. Missed {expected_stage:?}"
         );
     }
 
     assert_eq!(
         file.entry_by_path("file".into()).expect("found").stage(),
-        2,
+        Stage::Ours,
         "we always find our stage while in a merge"
     );
 }
@@ -226,13 +227,13 @@ fn sort_entries() {
 
     for (idx, entry) in file.entries()[..valid_entries].iter().enumerate() {
         assert_eq!(
-            file.entry_index_by_path_and_stage_bounded(entry.path(&file), 0, valid_entries),
+            file.entry_index_by_path_and_stage_bounded(entry.path(&file), Stage::Unconflicted, valid_entries),
             Some(idx),
             "we can still find entries in the correctly sorted region"
         );
     }
     assert_eq!(
-        file.entry_by_path_and_stage(new_entry_path, 0),
+        file.entry_by_path_and_stage(new_entry_path, Stage::Unconflicted),
         None,
         "new entry can't be found due to incorrect order"
     );
@@ -241,7 +242,7 @@ fn sort_entries() {
     assert!(file.verify_entries().is_ok(), "sorting of entries restores invariants");
 
     assert_eq!(
-        file.entry_by_path_and_stage(new_entry_path, 0)
+        file.entry_by_path_and_stage(new_entry_path, Stage::Unconflicted)
             .expect("can be found")
             .path(&file),
         new_entry_path,
