just playing with tangled
0
fork

Configure Feed

Select the types of activity you want to include in your feed.

repo_path: turn RepoPath into String wrapper

RepoPath::from_components() is removed since it is no longer a primitive
function.

The components iterator could be implemented on top of str::split(), but
it's not as we'll probably want to add components.as_path() -> &RepoPath.

Tree walking and tree_states map construction get slightly faster thanks to
fewer allocations and/or better cache locality. If we add a borrowed RepoPath
type, we can also implement a cheap &str to &RepoPath conversion on top. Then,
we can get rid of BTreeMap<RepoPath, FileState> construction at all.

Snapshot without watchman:
```
% hyperfine --sort command --warmup 3 --runs 10 -L bin jj-0,jj-1 \
"target/release-with-debug/{bin} -R ~/mirrors/linux status"
Benchmark 1: target/release-with-debug/jj-0 -R ~/mirrors/linux status
Time (mean ± σ): 950.1 ms ± 24.9 ms [User: 1642.4 ms, System: 681.1 ms]
Range (min … max): 913.8 ms … 990.9 ms 10 runs

Benchmark 2: target/release-with-debug/jj-1 -R ~/mirrors/linux status
Time (mean ± σ): 872.1 ms ± 14.5 ms [User: 1922.3 ms, System: 625.8 ms]
Range (min … max): 853.2 ms … 895.9 ms 10 runs

Relative speed comparison
1.09 ± 0.03 target/release-with-debug/jj-0 -R ~/mirrors/linux status
1.00 target/release-with-debug/jj-1 -R ~/mirrors/linux status
```

Tree walk:
```
% hyperfine --sort command --warmup 3 --runs 10 -L bin jj-0,jj-1 \
"target/release-with-debug/{bin} -R ~/mirrors/linux files --ignore-working-copy"
Benchmark 1: target/release-with-debug/jj-0 -R ~/mirrors/linux files --ignore-working-copy
Time (mean ± σ): 375.3 ms ± 15.4 ms [User: 223.3 ms, System: 151.8 ms]
Range (min … max): 359.4 ms … 394.1 ms 10 runs

Benchmark 2: target/release-with-debug/jj-1 -R ~/mirrors/linux files --ignore-working-copy
Time (mean ± σ): 357.1 ms ± 16.2 ms [User: 214.7 ms, System: 142.6 ms]
Range (min … max): 341.6 ms … 378.9 ms 10 runs

Relative speed comparison
1.05 ± 0.06 target/release-with-debug/jj-0 -R ~/mirrors/linux files --ignore-working-copy
1.00 target/release-with-debug/jj-1 -R ~/mirrors/linux files --ignore-working-copy
```

+145 -60
+144 -59
lib/src/repo_path.rs
··· 15 15 #![allow(missing_docs)] 16 16 17 17 use std::borrow::Borrow; 18 + use std::cmp::Ordering; 18 19 use std::fmt::{Debug, Error, Formatter}; 20 + use std::iter::FusedIterator; 19 21 use std::ops::Deref; 20 22 use std::path::{Component, Path, PathBuf}; 21 23 22 - use itertools::Itertools; 23 24 use ref_cast::{ref_cast_custom, RefCastCustom}; 24 25 use thiserror::Error; 25 26 ··· 112 113 } 113 114 114 115 /// Iterator over `RepoPath` components. 115 - pub type RepoPathComponentsIter<'a> = std::iter::Map< 116 - std::slice::Iter<'a, RepoPathComponentBuf>, 117 - fn(&RepoPathComponentBuf) -> &RepoPathComponent, 118 - >; 116 + #[derive(Clone, Debug)] 117 + pub struct RepoPathComponentsIter<'a> { 118 + value: &'a str, 119 + } 119 120 120 - #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] 121 + impl RepoPathComponentsIter<'_> { 122 + // TODO: add borrowed RepoPath type and implement as_path() instead 123 + fn to_path(&self) -> RepoPath { 124 + RepoPath { 125 + value: self.value.to_owned(), 126 + } 127 + } 128 + } 129 + 130 + impl<'a> Iterator for RepoPathComponentsIter<'a> { 131 + type Item = &'a RepoPathComponent; 132 + 133 + fn next(&mut self) -> Option<Self::Item> { 134 + if self.value.is_empty() { 135 + return None; 136 + } 137 + let (name, remainder) = self 138 + .value 139 + .split_once('/') 140 + .unwrap_or_else(|| (&self.value, &self.value[self.value.len()..])); 141 + self.value = remainder; 142 + Some(RepoPathComponent::new_unchecked(name)) 143 + } 144 + } 145 + 146 + impl DoubleEndedIterator for RepoPathComponentsIter<'_> { 147 + fn next_back(&mut self) -> Option<Self::Item> { 148 + if self.value.is_empty() { 149 + return None; 150 + } 151 + let (remainder, name) = self 152 + .value 153 + .rsplit_once('/') 154 + .unwrap_or_else(|| (&self.value[..0], &self.value)); 155 + self.value = remainder; 156 + Some(RepoPathComponent::new_unchecked(name)) 157 + } 158 + } 159 + 160 + impl FusedIterator for RepoPathComponentsIter<'_> {} 161 + 162 + #[derive(Clone, Eq, Hash, PartialEq)] 121 163 pub struct RepoPath { 122 - components: Vec<RepoPathComponentBuf>, 164 + value: String, 123 165 } 124 166 125 167 impl Debug for RepoPath { 126 168 fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { 127 - f.write_fmt(format_args!("{:?}", &self.to_internal_file_string())) 169 + f.write_fmt(format_args!("{:?}", &self.value)) 128 170 } 129 171 } 130 172 131 173 impl RepoPath { 132 - pub fn root() -> Self { 133 - RepoPath { components: vec![] } 174 + pub const fn root() -> Self { 175 + RepoPath { 176 + value: String::new(), 177 + } 134 178 } 135 179 136 180 /// Creates `RepoPath` from valid string representation. ··· 139 183 /// `"/"`, `"/foo"`, `"foo/"`, `"foo//bar"` are all invalid. 140 184 pub fn from_internal_string(value: &str) -> Self { 141 185 assert!(is_valid_repo_path_str(value)); 142 - if value.is_empty() { 143 - RepoPath::root() 144 - } else { 145 - let components = value 146 - .split('/') 147 - .map(|value| RepoPathComponentBuf { 148 - value: value.to_string(), 149 - }) 150 - .collect(); 151 - RepoPath { components } 186 + RepoPath { 187 + value: value.to_owned(), 152 188 } 153 189 } 154 190 ··· 157 193 /// The input path should not contain `.` or `..`. 158 194 pub fn from_relative_path(relative_path: impl AsRef<Path>) -> Option<Self> { 159 195 let relative_path = relative_path.as_ref(); 160 - let components = relative_path 196 + let mut components = relative_path 161 197 .components() 162 198 .map(|c| match c { 163 - Component::Normal(a) => Some(RepoPathComponentBuf::from(a.to_str().unwrap())), 199 + Component::Normal(name) => Some(name.to_str().unwrap()), 164 200 // TODO: better to return Err instead of None? 165 201 _ => None, 166 202 }) 167 - .collect::<Option<_>>()?; 168 - Some(RepoPath::from_components(components)) 169 - } 170 - 171 - pub fn from_components(components: Vec<RepoPathComponentBuf>) -> Self { 172 - RepoPath { components } 203 + .fuse(); 204 + let mut value = String::with_capacity(relative_path.as_os_str().len()); 205 + if let Some(name) = components.next() { 206 + value.push_str(name?); 207 + } 208 + for name in components { 209 + value.push('/'); 210 + value.push_str(name?); 211 + } 212 + Some(RepoPath { value }) 173 213 } 174 214 175 215 /// Parses an `input` path into a `RepoPath` relative to `base`. ··· 197 237 /// trailing slash, unless this path represents the root directory. That 198 238 /// way it can be concatenated with a basename and produce a valid path. 199 239 pub fn to_internal_dir_string(&self) -> String { 200 - let mut result = String::new(); 201 - for component in &self.components { 202 - result.push_str(component.as_str()); 203 - result.push('/'); 240 + if self.value.is_empty() { 241 + String::new() 242 + } else { 243 + [&self.value, "/"].concat() 204 244 } 205 - result 206 245 } 207 246 208 247 /// The full string form used internally, not for presenting to users (where 209 248 /// we may want to use the platform's separator). 210 249 pub fn to_internal_file_string(&self) -> String { 211 - let strings = self 212 - .components 213 - .iter() 214 - .map(|component| component.value.clone()) 215 - .collect_vec(); 216 - strings.join("/") 250 + self.value.to_owned() 217 251 } 218 252 219 253 pub fn to_fs_path(&self, base: &Path) -> PathBuf { 220 - let repo_path_len: usize = self.components.iter().map(|x| x.as_str().len() + 1).sum(); 221 - let mut result = PathBuf::with_capacity(base.as_os_str().len() + repo_path_len); 254 + let mut result = PathBuf::with_capacity(base.as_os_str().len() + self.value.len() + 1); 222 255 result.push(base); 223 - result.extend(self.components.iter().map(|dir| &dir.value)); 256 + result.extend(self.components().map(RepoPathComponent::as_str)); 224 257 result 225 258 } 226 259 227 260 pub fn is_root(&self) -> bool { 228 - self.components.is_empty() 261 + self.value.is_empty() 229 262 } 230 263 264 + // TODO: might be better to add .starts_with() instead 231 265 pub fn contains(&self, other: &RepoPath) -> bool { 232 - other.components.starts_with(&self.components) 266 + other.strip_prefix(self).is_some() 233 267 } 234 268 235 - pub fn parent(&self) -> Option<RepoPath> { 236 - if self.is_root() { 237 - None 269 + // TODO: make it return borrowed RepoPath type 270 + fn strip_prefix(&self, base: &RepoPath) -> Option<&str> { 271 + if base.value.is_empty() { 272 + Some(&self.value) 238 273 } else { 239 - Some(RepoPath { 240 - components: self.components[0..self.components.len() - 1].to_vec(), 241 - }) 274 + let tail = self.value.strip_prefix(&base.value)?; 275 + if tail.is_empty() { 276 + Some(tail) 277 + } else { 278 + tail.strip_prefix('/') 279 + } 242 280 } 243 281 } 244 282 283 + pub fn parent(&self) -> Option<RepoPath> { 284 + self.split().map(|(parent, _)| parent) 285 + } 286 + 245 287 pub fn split(&self) -> Option<(RepoPath, &RepoPathComponent)> { 246 - if self.is_root() { 247 - None 248 - } else { 249 - Some((self.parent().unwrap(), self.components.last().unwrap())) 250 - } 288 + let mut components = self.components(); 289 + let basename = components.next_back()?; 290 + Some((components.to_path(), basename)) 251 291 } 252 292 253 293 pub fn components(&self) -> RepoPathComponentsIter<'_> { 254 - self.components.iter().map(AsRef::as_ref) 294 + RepoPathComponentsIter { value: &self.value } 255 295 } 256 296 257 297 pub fn join(&self, entry: &RepoPathComponent) -> RepoPath { 258 - let components = 259 - itertools::chain(self.components.iter().cloned(), [entry.to_owned()]).collect(); 260 - RepoPath { components } 298 + let value = if self.value.is_empty() { 299 + entry.as_str().to_owned() 300 + } else { 301 + [&self.value, "/", entry.as_str()].concat() 302 + }; 303 + RepoPath { value } 304 + } 305 + } 306 + 307 + impl Ord for RepoPath { 308 + fn cmp(&self, other: &Self) -> Ordering { 309 + // If there were leading/trailing slash, components-based Ord would 310 + // disagree with str-based Eq. 311 + debug_assert!(is_valid_repo_path_str(&self.value)); 312 + self.components().cmp(other.components()) 313 + } 314 + } 315 + 316 + impl PartialOrd for RepoPath { 317 + fn partial_cmp(&self, other: &Self) -> Option<Ordering> { 318 + Some(self.cmp(other)) 261 319 } 262 320 } 263 321 ··· 278 336 #[cfg(test)] 279 337 mod tests { 280 338 use std::panic; 339 + 340 + use itertools::Itertools as _; 281 341 282 342 use super::*; 283 343 ··· 306 366 assert_eq!(RepoPath::root().to_internal_file_string(), ""); 307 367 assert_eq!(repo_path("dir").to_internal_file_string(), "dir"); 308 368 assert_eq!(repo_path("dir/file").to_internal_file_string(), "dir/file"); 369 + } 370 + 371 + #[test] 372 + fn test_to_internal_dir_string() { 373 + assert_eq!(RepoPath::root().to_internal_dir_string(), ""); 374 + assert_eq!(repo_path("dir").to_internal_dir_string(), "dir/"); 375 + assert_eq!(repo_path("dir/file").to_internal_dir_string(), "dir/file/"); 376 + } 377 + 378 + #[test] 379 + fn test_contains() { 380 + assert!(repo_path("").contains(&repo_path(""))); 381 + assert!(repo_path("").contains(&repo_path("x"))); 382 + assert!(!repo_path("x").contains(&repo_path(""))); 383 + 384 + assert!(repo_path("x").contains(&repo_path("x"))); 385 + assert!(repo_path("x").contains(&repo_path("x/y"))); 386 + assert!(!repo_path("x").contains(&repo_path("xy"))); 387 + assert!(!repo_path("y").contains(&repo_path("x/y"))); 388 + 389 + assert!(repo_path("x/y").contains(&repo_path("x/y"))); 390 + assert!(repo_path("x/y").contains(&repo_path("x/y/z"))); 391 + assert!(!repo_path("x/y").contains(&repo_path("x/yz"))); 392 + assert!(!repo_path("x/y").contains(&repo_path("x"))); 393 + assert!(!repo_path("x/y").contains(&repo_path("xy"))); 309 394 } 310 395 311 396 #[test]
+1 -1
lib/tests/test_merge_trees.rs
··· 495 495 match further_rebased_tree.value(component).unwrap() { 496 496 TreeValue::Conflict(id) => { 497 497 let conflict = store 498 - .read_conflict(&RepoPath::from_components(vec![component.to_owned()]), id) 498 + .read_conflict(&RepoPath::from_internal_string(component.as_str()), id) 499 499 .unwrap(); 500 500 assert_eq!( 501 501 conflict.removes().map(|v| v.as_ref()).collect_vec(),