D7798: rust-nodemap: special case for prefixes of NULL_NODE

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We have to behave as though NULL_NODE was stored in the node tree,
  although we don't store it.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,8 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    node::get_nybble, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
-    RevlogIndex,
+    node::get_nybble, node::NULL_NODE, Node, NodeError, NodePrefix,
+    NodePrefixRef, Revision, RevlogIndex, NULL_REVISION,
 };
 use std::fmt;
 use std::mem;
@@ -209,6 +209,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -282,9 +307,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks
-    /// - special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -519,9 +541,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -645,8 +665,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -665,7 +686,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
gracinet updated this revision to Diff 19138.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=19047&id=19138

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,8 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    node::get_nybble, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
-    RevlogIndex,
+    node::get_nybble, node::NULL_NODE, Node, NodeError, NodePrefix,
+    NodePrefixRef, Revision, RevlogIndex, NULL_REVISION,
 };
 use std::fmt;
 use std::mem;
@@ -209,6 +209,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -282,9 +307,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks
-    /// - special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -519,9 +541,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -645,8 +665,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -665,7 +686,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
gracinet updated this revision to Diff 19330.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=19138&id=19330

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,8 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    node::get_nybble, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
-    RevlogIndex,
+    node::get_nybble, node::NULL_NODE, Node, NodeError, NodePrefix,
+    NodePrefixRef, Revision, RevlogIndex, NULL_REVISION,
 };
 use std::fmt;
 use std::mem;
@@ -207,6 +207,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -280,9 +305,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks
-    /// - special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -517,9 +539,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -643,8 +663,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -663,7 +684,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
gracinet updated this revision to Diff 19638.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=19330&id=19638

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,7 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    Node, NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex,
+    node::NULL_NODE, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
+    RevlogIndex, NULL_REVISION,
 };
 
 use std::fmt;
@@ -250,6 +251,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -330,8 +356,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -582,9 +606,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -713,8 +735,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -733,7 +756,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
marmoute added a comment.
marmoute accepted this revision.


  looks good to me too.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

To: gracinet, #hg-reviewers, kevincox, marmoute
Cc: marmoute, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
marmoute updated this revision to Diff 20027.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=19638&id=20027

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,7 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    Node, NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex,
+    node::NULL_NODE, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
+    RevlogIndex, NULL_REVISION,
 };
 
 use std::fmt;
@@ -250,6 +251,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -330,8 +356,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -582,9 +606,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -713,8 +735,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -733,7 +756,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers, kevincox, marmoute
Cc: marmoute, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
Alphare updated this revision to Diff 20217.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=20027&id=20217

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,7 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    Node, NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex,
+    node::NULL_NODE, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
+    RevlogIndex, NULL_REVISION,
 };
 
 use std::fmt;
@@ -250,6 +251,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate<'p>(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef<'p>,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -339,8 +365,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -591,9 +615,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -723,8 +745,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -743,7 +766,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers, kevincox, marmoute
Cc: marmoute, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
Alphare updated this revision to Diff 20236.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=20217&id=20236

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,7 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    Node, NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex,
+    node::NULL_NODE, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
+    RevlogIndex, NULL_REVISION,
 };
 
 use std::fmt;
@@ -268,6 +269,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -358,8 +384,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -610,9 +634,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -745,8 +767,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -765,7 +788,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers, kevincox, marmoute
Cc: marmoute, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D7798: rust-nodemap: special case for prefixes of NULL_NODE

marmoute (Pierre-Yves David)
In reply to this post by marmoute (Pierre-Yves David)
Alphare updated this revision to Diff 20282.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7798?vs=20236&id=20282

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7798/new/

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

AFFECTED FILES
  rust/hg-core/src/revlog/nodemap.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,7 +13,8 @@
 //! is used in a more abstract context.
 
 use super::{
-    Node, NodeError, NodePrefix, NodePrefixRef, Revision, RevlogIndex,
+    node::NULL_NODE, Node, NodeError, NodePrefix, NodePrefixRef, Revision,
+    RevlogIndex, NULL_REVISION,
 };
 
 use std::fmt;
@@ -270,6 +271,31 @@
         })
 }
 
+/// validate that the candidate's node starts indeed with given prefix,
+/// and treat ambiguities related to `NULL_REVISION`.
+///
+/// From the data in the NodeTree, one can only conclude that some
+/// revision is the only one for a *subprefix* of the one being looked up.
+fn validate_candidate(
+    idx: &impl RevlogIndex,
+    prefix: NodePrefixRef,
+    rev: Option<Revision>,
+) -> Result<Option<Revision>, NodeMapError> {
+    if prefix.is_prefix_of(&NULL_NODE) {
+        // NULL_REVISION always matches a prefix made only of zeros
+        // and any other *valid* result is an ambiguity
+        match rev {
+            None => Ok(Some(NULL_REVISION)),
+            Some(r) => match has_prefix_or_none(idx, prefix, r)? {
+                None => Ok(Some(NULL_REVISION)),
+                _ => Err(NodeMapError::MultipleResults),
+            },
+        }
+    } else {
+        rev.map_or(Ok(None), |r| has_prefix_or_none(idx, prefix, r))
+    }
+}
+
 impl NodeTree {
     /// Initiate a NodeTree from an immutable slice-like of `Block`
     ///
@@ -361,8 +387,6 @@
     }
 
     /// Main working method for `NodeTree` searches
-    ///
-    /// This partial implementation lacks special cases for NULL_REVISION
     fn lookup<'p>(
         &self,
         prefix: NodePrefixRef<'p>,
@@ -613,9 +637,7 @@
         idx: &impl RevlogIndex,
         prefix: NodePrefixRef<'a>,
     ) -> Result<Option<Revision>, NodeMapError> {
-        self.lookup(prefix.clone()).and_then(|opt| {
-            opt.map_or(Ok(None), |rev| has_prefix_or_none(idx, prefix, rev))
-        })
+        validate_candidate(idx, prefix.clone(), self.lookup(prefix)?)
     }
 }
 
@@ -748,8 +770,9 @@
 
         assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
         assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
+        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -768,7 +791,8 @@
         };
         assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
         assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.find_hex(&idx, "00")?, Some(0));
+        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
+        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
         assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
         Ok(())
     }



To: gracinet, #hg-reviewers, kevincox, marmoute
Cc: marmoute, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel