D5944: rust: stop putting NULL_REVISION in MissingAncestors.bases

gracinet (Georges Racinet) phabricator at mercurial-scm.org
Tue Feb 12 07:02:15 EST 2019


gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As noted in initial review of MissingAncestors, adding
  NULL_REVISION in constructor in case the given `bases` is
  empty wasn't really useful, yet it's been kept for identity
  with the Python implementation

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5944

AFFECTED FILES
  rust/hg-core/src/ancestors.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/ancestors.rs b/rust/hg-core/src/ancestors.rs
--- a/rust/hg-core/src/ancestors.rs
+++ b/rust/hg-core/src/ancestors.rs
@@ -209,15 +209,11 @@
 
 impl<G: Graph> MissingAncestors<G> {
     pub fn new(graph: G, bases: impl IntoIterator<Item = Revision>) -> Self {
-        let mut bases: HashSet<Revision> = bases.into_iter().collect();
-        if bases.is_empty() {
-            bases.insert(NULL_REVISION);
-        }
-        MissingAncestors { graph, bases }
+        MissingAncestors { graph: graph, bases: bases.into_iter().collect() }
     }
 
     pub fn has_bases(&self) -> bool {
-        self.bases.iter().any(|&b| b != NULL_REVISION)
+        !self.bases.is_empty()
     }
 
     /// Return a reference to current bases.
@@ -246,15 +242,19 @@
         new_bases: impl IntoIterator<Item = Revision>,
     ) {
         self.bases.extend(new_bases);
+        self.bases.remove(&NULL_REVISION);
     }
 
     /// Remove all ancestors of self.bases from the revs set (in place)
     pub fn remove_ancestors_from(
         &mut self,
         revs: &mut HashSet<Revision>,
     ) -> Result<(), GraphError> {
         revs.retain(|r| !self.bases.contains(r));
-        // the null revision is always an ancestor
+        // the null revision is always an ancestor. Logically speaking
+        // it's debatable in case bases is empty, but the Python
+        // implementation always adds NULL_REVISION to bases, making it
+        // unconditionnally true.
         revs.remove(&NULL_REVISION);
         if revs.is_empty() {
             return Ok(());
@@ -265,8 +265,7 @@
         // we shouldn't need to iterate each time on bases
         let start = match self.bases.iter().cloned().max() {
             Some(m) => m,
-            None => {
-                // bases is empty (shouldn't happen, but let's be safe)
+            None => {  // self.bases is empty
                 return Ok(());
             }
         };



To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list