From 55dd3a3747b781eff2dbe2b73bae3df91f2752a2 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 18 Jan 2023 20:48:50 +0900 Subject: [PATCH] index: do not build hex string to test prefix match, use .as_bytes() matches() is called from resolve_change_id() loop right now, so it's better to not allocate String there. Regarding new IdIndex integration, I'll probably make IdIndex store raw byte ids instead of hexes, and use HexPrefix to look up range and test prefixes. I think this is basically the same as prefix lookup in MutableIndex, but I have no idea if we can factor out a common interface. I made HexPrefix store (Vec, bool) instead of (Vec, Option) so both min/partial prefixes can be borrowed as slice. --- lib/src/index.rs | 118 ++++++++++++++++++++++++++++++++++------------ lib/src/revset.rs | 4 +- 2 files changed, 90 insertions(+), 32 deletions(-) diff --git a/lib/src/index.rs b/lib/src/index.rs index 165e3913c..a0c1bc61f 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -263,38 +263,60 @@ impl Debug for ReadonlyIndex { } #[derive(Debug, Clone, PartialEq, Eq)] -pub struct HexPrefix(String); +pub struct HexPrefix { + // For odd-length prefix, lower 4 bits of the last byte is padded with 0 + min_prefix_bytes: Vec, + has_odd_byte: bool, +} impl HexPrefix { - pub fn new(prefix: String) -> Option { - if prefix - .matches(|c: char| !c.is_ascii_hexdigit() || c.is_ascii_uppercase()) - .next() - .is_some() - { - None + pub fn new(prefix: &str) -> Option { + let has_odd_byte = prefix.len() & 1 != 0; + let min_prefix_bytes = if has_odd_byte { + hex::decode(prefix.to_owned() + "0").ok()? } else { - Some(HexPrefix(prefix)) - } + hex::decode(prefix).ok()? + }; + Some(HexPrefix { + min_prefix_bytes, + has_odd_byte, + }) } - pub fn hex(&self) -> &str { - self.0.as_str() + pub fn hex(&self) -> String { + let mut hex_string = hex::encode(&self.min_prefix_bytes); + if self.has_odd_byte { + hex_string.pop().unwrap(); + } + hex_string + } + + fn split_odd_byte(&self) -> (Option, &[u8]) { + if self.has_odd_byte { + let (&odd, prefix) = self.min_prefix_bytes.split_last().unwrap(); + (Some(odd), prefix) + } else { + (None, &self.min_prefix_bytes) + } } pub fn bytes_prefixes(&self) -> (Q, Q) { - if self.0.len() % 2 == 0 { - let bytes = hex::decode(&self.0).unwrap(); - (Q::new(bytes.clone()), Q::new(bytes)) - } else { - let min_bytes = hex::decode(self.0.clone() + "0").unwrap(); - let prefix = min_bytes[0..min_bytes.len() - 1].to_vec(); - (Q::new(prefix), Q::new(min_bytes)) - } + let (_, prefix) = self.split_odd_byte(); + (Q::from_bytes(prefix), Q::from_bytes(&self.min_prefix_bytes)) } pub fn matches(&self, id: &Q) -> bool { - id.hex().starts_with(&self.0) + let id_bytes = id.as_bytes(); + let (maybe_odd, prefix) = self.split_odd_byte(); + if id_bytes.starts_with(prefix) { + if let Some(odd) = maybe_odd { + matches!(id_bytes.get(prefix.len()), Some(v) if v & 0xf0 == odd) + } else { + true + } + } else { + false + } } } @@ -1912,44 +1934,44 @@ mod tests { // Can find commits given the full hex number assert_eq!( - index.resolve_prefix(&HexPrefix::new(id_0.hex()).unwrap()), + index.resolve_prefix(&HexPrefix::new(&id_0.hex()).unwrap()), PrefixResolution::SingleMatch(id_0) ); assert_eq!( - index.resolve_prefix(&HexPrefix::new(id_1.hex()).unwrap()), + index.resolve_prefix(&HexPrefix::new(&id_1.hex()).unwrap()), PrefixResolution::SingleMatch(id_1) ); assert_eq!( - index.resolve_prefix(&HexPrefix::new(id_2.hex()).unwrap()), + index.resolve_prefix(&HexPrefix::new(&id_2.hex()).unwrap()), PrefixResolution::SingleMatch(id_2) ); // Test nonexistent commits assert_eq!( - index.resolve_prefix(&HexPrefix::new("ffffff".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("ffffff").unwrap()), PrefixResolution::NoMatch ); assert_eq!( - index.resolve_prefix(&HexPrefix::new("000001".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("000001").unwrap()), PrefixResolution::NoMatch ); // Test ambiguous prefix assert_eq!( - index.resolve_prefix(&HexPrefix::new("0".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("0").unwrap()), PrefixResolution::AmbiguousMatch ); // Test a globally unique prefix in initial part assert_eq!( - index.resolve_prefix(&HexPrefix::new("009".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("009").unwrap()), PrefixResolution::SingleMatch(CommitId::from_hex("009999")) ); // Test a globally unique prefix in incremental part assert_eq!( - index.resolve_prefix(&HexPrefix::new("03".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("03").unwrap()), PrefixResolution::SingleMatch(CommitId::from_hex("033333")) ); // Test a locally unique but globally ambiguous prefix assert_eq!( - index.resolve_prefix(&HexPrefix::new("0554".to_string()).unwrap()), + index.resolve_prefix(&HexPrefix::new("0554").unwrap()), PrefixResolution::AmbiguousMatch ); } @@ -2327,4 +2349,40 @@ mod tests { // Merge commit and other commit assert_eq!(index.heads(&[id_5.clone(), id_3.clone()]), vec![id_3, id_5]); } + + #[test] + fn test_hex_prefix_prefixes() { + let (prefix, min_prefix) = HexPrefix::new("").unwrap().bytes_prefixes::(); + assert_eq!(prefix, CommitId::from_hex("")); + assert_eq!(min_prefix, CommitId::from_hex("")); + + let (prefix, min_prefix) = HexPrefix::new("1").unwrap().bytes_prefixes::(); + assert_eq!(prefix, CommitId::from_hex("")); + assert_eq!(min_prefix, CommitId::from_hex("10")); + + let (prefix, min_prefix) = HexPrefix::new("12").unwrap().bytes_prefixes::(); + assert_eq!(prefix, CommitId::from_hex("12")); + assert_eq!(min_prefix, CommitId::from_hex("12")); + + let (prefix, min_prefix) = HexPrefix::new("123").unwrap().bytes_prefixes::(); + assert_eq!(prefix, CommitId::from_hex("12")); + assert_eq!(min_prefix, CommitId::from_hex("1230")); + } + + #[test] + fn test_hex_prefix_matches() { + let id = CommitId::from_hex("1234"); + + assert!(HexPrefix::new("").unwrap().matches(&id)); + assert!(HexPrefix::new("1").unwrap().matches(&id)); + assert!(HexPrefix::new("12").unwrap().matches(&id)); + assert!(HexPrefix::new("123").unwrap().matches(&id)); + assert!(HexPrefix::new("1234").unwrap().matches(&id)); + assert!(!HexPrefix::new("12345").unwrap().matches(&id)); + + assert!(!HexPrefix::new("a").unwrap().matches(&id)); + assert!(!HexPrefix::new("1a").unwrap().matches(&id)); + assert!(!HexPrefix::new("12a").unwrap().matches(&id)); + assert!(!HexPrefix::new("123a").unwrap().matches(&id)); + } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 16a5b3874..6af43a501 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -105,7 +105,7 @@ fn resolve_short_commit_id( repo: RepoRef, symbol: &str, ) -> Result>, RevsetError> { - if let Some(prefix) = HexPrefix::new(symbol.to_owned()) { + if let Some(prefix) = HexPrefix::new(symbol) { match repo.index().resolve_prefix(&prefix) { PrefixResolution::NoMatch => Ok(None), PrefixResolution::AmbiguousMatch => { @@ -122,7 +122,7 @@ fn resolve_change_id( repo: RepoRef, change_id_prefix: &str, ) -> Result>, RevsetError> { - if let Some(hex_prefix) = HexPrefix::new(change_id_prefix.to_owned()) { + if let Some(hex_prefix) = HexPrefix::new(change_id_prefix) { let mut found_change_id = None; let mut commit_ids = vec![]; // TODO: Create a persistent lookup from change id to (visible?) commit ids.