changeset 52294:a3fa37bdb7ec

rust: normalize `_for_unchecked_rev` naming among revlogs and the index

This normalizes the naming scheme between the `Revlog`, `Changelog`, etc.
which is less suprising, though no real bugs could stem from this because of
the type signature mismatch.

The very high-level `Repo` object still uses an `UncheckedRevision` parameter
for its methods because that's what most callers will want.

--- a/rust/hg-core/src/operations/cat.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/operations/cat.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -87,7 +87,7 @@
     let manifest = repo.manifest_for_rev(rev.into())?;
     let node = *repo
         .changelog()?
-        .node_from_rev(rev.into())
+        .node_from_unchecked_rev(rev.into())
         .expect("should succeed when repo.manifest did");
     let mut results: Vec<(&'a HgPath, Vec<u8>)> = vec![];
     let mut found_any = false;

--- a/rust/hg-core/src/operations/debugdata.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/operations/debugdata.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -40,6 +40,6 @@
     )?;
     let rev =
         crate::revset::resolve_rev_number_or_hex_prefix(revset, &revlog)?;
-    let data = revlog.get_rev_data_for_checked_rev(rev)?;
+    let data = revlog.get_data(rev)?;
     Ok(data.into_owned())
 }

--- a/rust/hg-core/src/repo.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/repo.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -620,7 +620,7 @@
     ) -> Result<Manifest, RevlogError> {
         self.manifestlog()?.data_for_node(
             self.changelog()?
-                .data_for_rev(revision)?
+                .data_for_unchecked_rev(revision)?
                 .manifest_node()?
                 .into(),
         )
@@ -795,7 +795,7 @@
     pub fn node(&self, rev: UncheckedRevision) -> Option<crate::Node> {
         self.changelog()
             .ok()
-            .and_then(|c| c.node_from_rev(rev).copied())
+            .and_then(|c| c.node_from_unchecked_rev(rev).copied())
     }
 
     /// Change the current working directory parents cached in the repo.

--- a/rust/hg-core/src/revlog/changelog.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/changelog.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -40,24 +40,21 @@
         node: NodePrefix,
     ) -> Result<ChangelogRevisionData, RevlogError> {
         let rev = self.revlog.rev_from_node(node)?;
-        self.entry_for_checked_rev(rev)?.data()
+        self.entry(rev)?.data()
     }
 
     /// Return the [`ChangelogEntry`] for the given revision number.
-    pub fn entry_for_rev(
+    pub fn entry_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<ChangelogEntry, RevlogError> {
-        let revlog_entry = self.revlog.get_entry(rev)?;
+        let revlog_entry = self.revlog.get_entry_for_unchecked_rev(rev)?;
         Ok(ChangelogEntry { revlog_entry })
     }
 
-    /// Same as [`Self::entry_for_rev`] for checked revisions.
-    fn entry_for_checked_rev(
-        &self,
-        rev: Revision,
-    ) -> Result<ChangelogEntry, RevlogError> {
-        let revlog_entry = self.revlog.get_entry_for_checked_rev(rev)?;
+    /// Same as [`Self::entry_for_unchecked_rev`] for a checked revision
+    fn entry(&self, rev: Revision) -> Result<ChangelogEntry, RevlogError> {
+        let revlog_entry = self.revlog.get_entry(rev)?;
         Ok(ChangelogEntry { revlog_entry })
     }
 
@@ -66,15 +63,18 @@
     /// This is a useful shortcut in case the caller does not need the
     /// generic revlog information (parents, hashes etc). Otherwise
     /// consider taking a [`ChangelogEntry`] with
-    /// [entry_for_rev](`Self::entry_for_rev`) and doing everything from there.
-    pub fn data_for_rev(
+    /// [`Self::entry_for_unchecked_rev`] and doing everything from there.
+    pub fn data_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<ChangelogRevisionData, RevlogError> {
-        self.entry_for_rev(rev)?.data()
+        self.entry_for_unchecked_rev(rev)?.data()
     }
 
-    pub fn node_from_rev(&self, rev: UncheckedRevision) -> Option<&Node> {
+    pub fn node_from_unchecked_rev(
+        &self,
+        rev: UncheckedRevision,
+    ) -> Option<&Node> {
         self.revlog.node_from_rev(rev)
     }
 
@@ -570,12 +570,14 @@
 
         let changelog = Changelog { revlog };
         assert_eq!(
-            changelog.data_for_rev(NULL_REVISION.into())?,
+            changelog.data_for_unchecked_rev(NULL_REVISION.into())?,
             ChangelogRevisionData::null()
         );
         // same with the intermediate entry object
         assert_eq!(
-            changelog.entry_for_rev(NULL_REVISION.into())?.data()?,
+            changelog
+                .entry_for_unchecked_rev(NULL_REVISION.into())?
+                .data()?,
             ChangelogRevisionData::null()
         );
         Ok(())

--- a/rust/hg-core/src/revlog/filelog.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/filelog.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -56,16 +56,19 @@
         file_node: impl Into<NodePrefix>,
     ) -> Result<FilelogRevisionData, RevlogError> {
         let file_rev = self.revlog.rev_from_node(file_node.into())?;
-        self.data_for_rev(file_rev.into())
+        self.data_for_unchecked_rev(file_rev.into())
     }
 
     /// The given revision is that of the file as found in a filelog, not of a
     /// changeset.
-    pub fn data_for_rev(
+    pub fn data_for_unchecked_rev(
         &self,
         file_rev: UncheckedRevision,
     ) -> Result<FilelogRevisionData, RevlogError> {
-        let data: Vec<u8> = self.revlog.get_rev_data(file_rev)?.into_owned();
+        let data: Vec<u8> = self
+            .revlog
+            .get_data_for_unchecked_rev(file_rev)?
+            .into_owned();
         Ok(FilelogRevisionData(data))
     }
 
@@ -76,25 +79,26 @@
         file_node: impl Into<NodePrefix>,
     ) -> Result<FilelogEntry, RevlogError> {
         let file_rev = self.revlog.rev_from_node(file_node.into())?;
-        self.entry_for_checked_rev(file_rev)
+        self.entry(file_rev)
     }
 
     /// The given revision is that of the file as found in a filelog, not of a
     /// changeset.
-    pub fn entry_for_rev(
+    pub fn entry_for_unchecked_rev(
         &self,
         file_rev: UncheckedRevision,
     ) -> Result<FilelogEntry, RevlogError> {
-        Ok(FilelogEntry(self.revlog.get_entry(file_rev)?))
+        Ok(FilelogEntry(
+            self.revlog.get_entry_for_unchecked_rev(file_rev)?,
+        ))
     }
 
-    fn entry_for_checked_rev(
+    /// Same as [`Self::entry_for_unchecked_rev`] for a checked revision.
+    pub fn entry(
         &self,
         file_rev: Revision,
     ) -> Result<FilelogEntry, RevlogError> {
-        Ok(FilelogEntry(
-            self.revlog.get_entry_for_checked_rev(file_rev)?,
-        ))
+        Ok(FilelogEntry(self.revlog.get_entry(file_rev)?))
     }
 }
 

--- a/rust/hg-core/src/revlog/index.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/index.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -507,8 +507,7 @@
 
     /// Return the binary content of the index entry for the given revision
     ///
-    /// See [get_entry()](`Self::get_entry()`) for cases when `None` is
-    /// returned.
+    /// See [`Self::get_entry`] for cases when `None` is returned.
     pub fn entry_binary(&self, rev: Revision) -> Option<&[u8]> {
         self.get_entry(rev).map(|e| {
             let bytes = e.as_bytes();
@@ -536,11 +535,13 @@
                 .compressed_len()
                 .try_into()
                 .unwrap_or_else(|_| {
-                    // Python's `unionrepo` sets the compressed length to be
-                    // `-1` (or `u32::MAX` if transmuted to `u32`) because it
+                    // Python's `unionrepo` sets the compressed length to
+                    // be `-1` (or `u32::MAX` if
+                    // transmuted to `u32`) because it
                     // cannot know the correct compressed length of a given
-                    // revision. I'm not sure if this is true, but having this
-                    // edge case won't hurt other use cases, let's handle it.
+                    // revision. I'm not sure if this is true, but having
+                    // this edge case won't hurt
+                    // other use cases, let's handle it.
                     assert_eq!(e.compressed_len(), u32::MAX);
                     NULL_REVISION.0
                 }),

--- a/rust/hg-core/src/revlog/inner_revlog.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/inner_revlog.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -160,7 +160,7 @@
     }
 
     /// Return the [`RevlogEntry`] for a [`Revision`] that is known to exist
-    pub fn get_entry_for_checked_rev(
+    pub fn get_entry(
         &self,
         rev: Revision,
     ) -> Result<RevlogEntry, RevlogError> {
@@ -199,10 +199,7 @@
 
     /// Return the [`RevlogEntry`] for `rev`. If `rev` fails to check, this
     /// returns a [`RevlogError`].
-    /// TODO normalize naming across the index and all revlogs
-    /// (changelog, etc.) so that `get_entry` is always on an unchecked rev and
-    /// `get_entry_for_checked_rev` is for checked rev
-    pub fn get_entry(
+    pub fn get_entry_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<RevlogEntry, RevlogError> {
@@ -212,7 +209,7 @@
         let rev = self.index.check_revision(rev).ok_or_else(|| {
             RevlogError::corrupted(format!("rev {} is invalid", rev))
         })?;
-        self.get_entry_for_checked_rev(rev)
+        self.get_entry(rev)
     }
 
     /// Is the revlog currently delaying the visibility of written data?
@@ -471,7 +468,7 @@
             ) -> Result<(), RevlogError>,
         ) -> Result<(), RevlogError>,
     {
-        let entry = &self.get_entry_for_checked_rev(rev)?;
+        let entry = &self.get_entry(rev)?;
         let raw_size = entry.uncompressed_len();
         let mut mutex_guard = self
             .last_revision_cache

--- a/rust/hg-core/src/revlog/manifest.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/manifest.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -44,7 +44,7 @@
         node: NodePrefix,
     ) -> Result<Manifest, RevlogError> {
         let rev = self.revlog.rev_from_node(node)?;
-        self.data_for_checked_rev(rev)
+        self.data(rev)
     }
 
     /// Return the `Manifest` of a given revision number.
@@ -53,20 +53,17 @@
     /// changeset.
     ///
     /// See also `Repo::manifest_for_rev`
-    pub fn data_for_rev(
+    pub fn data_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<Manifest, RevlogError> {
-        let bytes = self.revlog.get_rev_data(rev)?.into_owned();
+        let bytes = self.revlog.get_data_for_unchecked_rev(rev)?.into_owned();
         Ok(Manifest { bytes })
     }
 
-    pub fn data_for_checked_rev(
-        &self,
-        rev: Revision,
-    ) -> Result<Manifest, RevlogError> {
-        let bytes =
-            self.revlog.get_rev_data_for_checked_rev(rev)?.into_owned();
+    /// Same as [`Self::data_for_unchecked_rev`] for a checked [`Revision`]
+    pub fn data(&self, rev: Revision) -> Result<Manifest, RevlogError> {
+        let bytes = self.revlog.get_data(rev)?.into_owned();
         Ok(Manifest { bytes })
     }
 }

--- a/rust/hg-core/src/revlog/mod.rs	Tue Nov 12 12:45:23 2024 +0100
+++ b/rust/hg-core/src/revlog/mod.rs	Tue Oct 29 11:00:04 2024 +0100
@@ -371,44 +371,41 @@
         self.index().check_revision(rev).is_some()
     }
 
-    pub fn get_entry_for_checked_rev(
+    pub fn get_entry(
         &self,
         rev: Revision,
     ) -> Result<RevlogEntry, RevlogError> {
-        self.inner.get_entry_for_checked_rev(rev)
+        self.inner.get_entry(rev)
     }
 
-    pub fn get_entry(
+    pub fn get_entry_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<RevlogEntry, RevlogError> {
-        self.inner.get_entry(rev)
+        self.inner.get_entry_for_unchecked_rev(rev)
     }
 
     /// Return the full data associated to a revision.
     ///
     /// All entries required to build the final data out of deltas will be
-    /// retrieved as needed, and the deltas will be applied to the inital
+    /// retrieved as needed, and the deltas will be applied to the initial
     /// snapshot to rebuild the final data.
-    pub fn get_rev_data(
+    pub fn get_data_for_unchecked_rev(
         &self,
         rev: UncheckedRevision,
     ) -> Result<Cow<[u8]>, RevlogError> {
         if rev == NULL_REVISION.into() {
             return Ok(Cow::Borrowed(&[]));
         };
-        self.get_entry(rev)?.data()
+        self.get_entry_for_unchecked_rev(rev)?.data()
     }
 
-    /// [`Self::get_rev_data`] for checked revisions.
-    pub fn get_rev_data_for_checked_rev(
-        &self,
-        rev: Revision,
-    ) -> Result<Cow<[u8]>, RevlogError> {
+    /// [`Self::get_data_for_unchecked_rev`] for a checked [`Revision`].
+    pub fn get_data(&self, rev: Revision) -> Result<Cow<[u8]>, RevlogError> {
         if rev == NULL_REVISION {
             return Ok(Cow::Borrowed(&[]));
         };
-        self.get_entry_for_checked_rev(rev)?.data()
+        self.get_entry(rev)?.data()
     }
 
     /// Check the hash of some given data against the recorded hash.
@@ -590,7 +587,7 @@
         if self.p1 == NULL_REVISION {
             Ok(None)
         } else {
-            Ok(Some(self.revlog.get_entry_for_checked_rev(self.p1)?))
+            Ok(Some(self.revlog.get_entry(self.p1)?))
         }
     }
 
@@ -600,7 +597,7 @@
         if self.p2 == NULL_REVISION {
             Ok(None)
         } else {
-            Ok(Some(self.revlog.get_entry_for_checked_rev(self.p2)?))
+            Ok(Some(self.revlog.get_entry(self.p2)?))
         }
     }
 
@@ -737,13 +734,16 @@
                 .unwrap();
         assert!(revlog.is_empty());
         assert_eq!(revlog.len(), 0);
-        assert!(revlog.get_entry(0.into()).is_err());
+        assert!(revlog.get_entry_for_unchecked_rev(0.into()).is_err());
         assert!(!revlog.has_rev(0.into()));
         assert_eq!(
             revlog.rev_from_node(NULL_NODE.into()).unwrap(),
             NULL_REVISION
         );
-        let null_entry = revlog.get_entry(NULL_REVISION.into()).ok().unwrap();
+        let null_entry = revlog
+            .get_entry_for_unchecked_rev(NULL_REVISION.into())
+            .ok()
+            .unwrap();
         assert_eq!(null_entry.revision(), NULL_REVISION);
         assert!(null_entry.data().unwrap().is_empty());
     }
@@ -779,7 +779,8 @@
             Revlog::open(&vfs, "foo.i", None, RevlogOpenOptions::default())
                 .unwrap();
 
-        let entry0 = revlog.get_entry(0.into()).ok().unwrap();
+        let entry0 =
+            revlog.get_entry_for_unchecked_rev(0.into()).ok().unwrap();
         assert_eq!(entry0.revision(), Revision(0));
         assert_eq!(*entry0.node(), node0);
         assert!(!entry0.has_p1());
@@ -790,7 +791,8 @@
         let p2_entry = entry0.p2_entry().unwrap();
         assert!(p2_entry.is_none());
 
-        let entry1 = revlog.get_entry(1.into()).ok().unwrap();
+        let entry1 =
+            revlog.get_entry_for_unchecked_rev(1.into()).ok().unwrap();
         assert_eq!(entry1.revision(), Revision(1));
         assert_eq!(*entry1.node(), node1);
         assert!(!entry1.has_p1());
@@ -801,7 +803,8 @@
         let p2_entry = entry1.p2_entry().unwrap();
         assert!(p2_entry.is_none());
 
-        let entry2 = revlog.get_entry(2.into()).ok().unwrap();
+        let entry2 =
+            revlog.get_entry_for_unchecked_rev(2.into()).ok().unwrap();
         assert_eq!(entry2.revision(), Revision(2));
         assert_eq!(*entry2.node(), node2);
         assert!(entry2.has_p1());
@@ -854,7 +857,11 @@
         .unwrap();
 
         // accessing the data shows the corruption
-        revlog.get_entry(0.into()).unwrap().data().unwrap_err();
+        revlog
+            .get_entry_for_unchecked_rev(0.into())
+            .unwrap()
+            .data()
+            .unwrap_err();
 
         assert_eq!(
             revlog.rev_from_node(NULL_NODE.into()).unwrap(),