From 3fcf157214396b194d8edfa6411b585819f9f4f3 Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Thu, 3 Jun 2021 00:06:28 +0200 Subject: [PATCH] fix path handling on windows (#762) * this reduces memory overhead where nothing is folded up * makes folding work with windows path seperators --- CHANGELOG.md | 1 + asyncgit/src/sync/blame.rs | 10 ++- filetreelist/src/error.rs | 3 - filetreelist/src/filetree.rs | 34 ++++---- filetreelist/src/filetreeitems.rs | 139 ++++++++++++++---------------- filetreelist/src/item.rs | 131 ++++++++++++++++------------ src/components/revision_files.rs | 13 ++- 7 files changed, 176 insertions(+), 155 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17fed8aaffc..bf6972ef2c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Fixed - wrong file with same name shown in file tree ([#748](https://github.com/extrawurst/gitui/issues/748)) +- filetree collapsing broken on windows ([#761](https://github.com/extrawurst/gitui/issues/761)) ### Internal - use git_repository_message [[@kosayoda](https://github.com/kosayoda)] ([#751](https://github.com/extrawurst/gitui/issues/751)) diff --git a/asyncgit/src/sync/blame.rs b/asyncgit/src/sync/blame.rs index 4b5b02883b2..f1aecae178d 100644 --- a/asyncgit/src/sync/blame.rs +++ b/asyncgit/src/sync/blame.rs @@ -50,7 +50,15 @@ pub fn blame_file( let commit_id = utils::get_head_repo(&repo)?; - let spec = format!("{}:{}", commit_id.to_string(), file_path); + let spec = if cfg!(unix) { + format!("{}:{}", commit_id.to_string(), file_path) + } else { + format!( + "{}:{}", + commit_id.to_string(), + file_path.replace("\\", "/") + ) + }; let object = repo.revparse_single(&spec)?; let blob = repo.find_blob(object.id())?; diff --git a/filetreelist/src/error.rs b/filetreelist/src/error.rs index a68fcaf32c1..6aa0f2c2d87 100644 --- a/filetreelist/src/error.rs +++ b/filetreelist/src/error.rs @@ -6,9 +6,6 @@ pub enum Error { #[error("InvalidPath: `{0}`")] InvalidPath(PathBuf), - #[error("InvalidFilePath: `{0}`")] - InvalidFilePath(String), - #[error("TryFromInt error:{0}")] IntConversion(#[from] TryFromIntError), } diff --git a/filetreelist/src/filetree.rs b/filetreelist/src/filetree.rs index 26c7a5af273..a04b7bf75c4 100644 --- a/filetreelist/src/filetree.rs +++ b/filetreelist/src/filetree.rs @@ -2,7 +2,7 @@ use crate::{ error::Result, filetreeitems::FileTreeItems, tree_iter::TreeIterator, TreeItemInfo, }; -use std::{collections::BTreeSet, usize}; +use std::{collections::BTreeSet, path::Path, usize}; /// #[derive(Copy, Clone, Debug)] @@ -35,7 +35,7 @@ pub struct FileTree { impl FileTree { /// pub fn new( - list: &[&str], + list: &[&Path], collapsed: &BTreeSet<&String>, ) -> Result { let mut new_self = Self { @@ -318,12 +318,12 @@ impl FileTree { mod test { use crate::{FileTree, MoveSelection}; use pretty_assertions::assert_eq; - use std::collections::BTreeSet; + use std::{collections::BTreeSet, path::Path}; #[test] fn test_selection() { let items = vec![ - "a/b", // + Path::new("a/b"), // ]; let mut tree = @@ -341,8 +341,8 @@ mod test { #[test] fn test_selection_skips_collapsed() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -364,8 +364,8 @@ mod test { #[test] fn test_selection_left_collapse() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -390,8 +390,8 @@ mod test { #[test] fn test_selection_left_parent() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -417,8 +417,8 @@ mod test { #[test] fn test_selection_right_expand() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -449,8 +449,8 @@ mod test { #[test] fn test_selection_top() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -470,9 +470,9 @@ mod test { #[test] fn test_visible_selection() { let items = vec![ - "a/b/c", // - "a/b/c2", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/b/c2"), // + Path::new("a/d"), // ]; //0 a/ diff --git a/filetreelist/src/filetreeitems.rs b/filetreelist/src/filetreeitems.rs index c49edfd202f..dec003a9df6 100644 --- a/filetreelist/src/filetreeitems.rs +++ b/filetreelist/src/filetreeitems.rs @@ -6,7 +6,7 @@ use crate::{ use crate::{error::Result, treeitems_iter::TreeItemsIterator}; use std::{ collections::{BTreeSet, HashMap}, - path::Path, + path::{Path, PathBuf}, usize, }; @@ -20,7 +20,7 @@ pub struct FileTreeItems { impl FileTreeItems { /// pub fn new( - list: &[&str], + list: &[&Path], collapsed: &BTreeSet<&String>, ) -> Result { let (mut items, paths) = Self::create_items(list, collapsed)?; @@ -34,7 +34,7 @@ impl FileTreeItems { } fn create_items<'a>( - list: &'a [&str], + list: &'a [&Path], collapsed: &BTreeSet<&String>, ) -> Result<(Vec, HashMap<&'a Path, usize>)> { // scopetime::scope_time!("create_items"); @@ -45,9 +45,8 @@ impl FileTreeItems { for e in list { { - let item_path = Path::new(e); Self::push_dirs( - item_path, + e, &mut items, &mut paths_added, collapsed, @@ -103,13 +102,10 @@ impl FileTreeItems { } } + //TODO: make non alloc let path_string = Self::path_to_string(c)?; let is_collapsed = collapsed.contains(&path_string); - nodes.push(FileTreeItem::new_path( - c, - path_string, - is_collapsed, - )?); + nodes.push(FileTreeItem::new_path(c, is_collapsed)?); } } @@ -121,6 +117,7 @@ impl FileTreeItems { Ok(()) } + //TODO: return ref fn path_to_string(p: &Path) -> Result { Ok(p.to_str() .map_or_else( @@ -134,9 +131,8 @@ impl FileTreeItems { if self.tree_items[index].kind().is_path() { self.tree_items[index].collapse_path(); - let path = format!( - "{}/", - self.tree_items[index].info().full_path() + let path = PathBuf::from( + self.tree_items[index].info().full_path_str(), ); for i in index + 1..self.tree_items.len() { @@ -146,7 +142,8 @@ impl FileTreeItems { item.collapse_path(); } - let item_path = &item.info().full_path(); + let item_path = + Path::new(item.info().full_path_str()); if item_path.starts_with(&path) { item.hide(); @@ -161,18 +158,15 @@ impl FileTreeItems { if self.tree_items[index].kind().is_path() { self.tree_items[index].expand_path(); - let full_path = format!( - "{}/", - self.tree_items[index].info().full_path() + let full_path = PathBuf::from( + self.tree_items[index].info().full_path_str(), ); if recursive { for i in index + 1..self.tree_items.len() { let item = &mut self.tree_items[i]; - if !item - .info() - .full_path() + if !Path::new(item.info().full_path_str()) .starts_with(&full_path) { break; @@ -187,7 +181,7 @@ impl FileTreeItems { } self.update_visibility( - Some(full_path.as_str()), + &Some(full_path), index + 1, false, ); @@ -196,16 +190,18 @@ impl FileTreeItems { fn update_visibility( &mut self, - prefix: Option<&str>, + prefix: &Option, start_idx: usize, set_defaults: bool, ) { // if we are in any subpath that is collapsed we keep skipping over it - let mut inner_collapsed: Option = None; + let mut inner_collapsed: Option = None; for i in start_idx..self.tree_items.len() { if let Some(ref collapsed_path) = inner_collapsed { - let p = self.tree_items[i].info().full_path(); + let p = Path::new( + self.tree_items[i].info().full_path_str(), + ); if p.starts_with(collapsed_path) { if set_defaults { self.tree_items[i] @@ -219,15 +215,17 @@ impl FileTreeItems { } let item_kind = self.tree_items[i].kind().clone(); - let item_path = self.tree_items[i].info().full_path(); + let item_path = + Path::new(self.tree_items[i].info().full_path_str()); if matches!(item_kind, FileTreeItemKind::Path(PathCollapsed(collapsed)) if collapsed) { // we encountered an inner path that is still collapsed - inner_collapsed = Some(format!("{}/", &item_path)); + inner_collapsed = Some(item_path.into()); } if prefix + .as_ref() .map_or(true, |prefix| item_path.starts_with(prefix)) { self.tree_items[i].info_mut().set_visible(true); @@ -251,8 +249,8 @@ impl FileTreeItems { while i < items.len() { let item = &items[i]; if item.kind().is_path() { - let children = - paths.get(&Path::new(item.info().full_path())); + let children = paths + .get(&Path::new(item.info().full_path_str())); if let Some(children) = children { if *children == 1 { @@ -271,7 +269,7 @@ impl FileTreeItems { let prefix = item_mut .info() - .full_path() + .full_path_str() .to_owned(); Self::unindent(items, &prefix, i + 1); @@ -291,7 +289,7 @@ impl FileTreeItems { start: usize, ) { for elem in items.iter_mut().skip(start) { - if elem.info().full_path().starts_with(prefix) { + if elem.info().full_path_str().starts_with(prefix) { elem.info_mut().unindent(); } else { return; @@ -308,7 +306,7 @@ mod tests { #[test] fn test_simple() { let items = vec![ - "file.txt", // + Path::new("file.txt"), // ]; let res = @@ -320,8 +318,8 @@ mod tests { assert_eq!(res.tree_items[0].info().full_path(), items[0]); let items = vec![ - "file.txt", // - "file2.txt", // + Path::new("file.txt"), // + Path::new("file2.txt"), // ]; let res = @@ -329,10 +327,7 @@ mod tests { assert_eq!(res.tree_items.len(), 2); assert_eq!(res.tree_items.len(), res.len()); - assert_eq!( - res.tree_items[1].info().path(), - items[1].to_string() - ); + assert_eq!(res.tree_items[1].info().path(), items[1]); } #[test] @@ -392,14 +387,14 @@ mod tests { #[test] fn test_folder() { let items = vec![ - "a/file.txt", // + Path::new("a/file.txt"), // ]; let res = FileTreeItems::new(&items, &BTreeSet::new()) .unwrap() .tree_items .iter() - .map(|i| i.info().full_path().to_string()) + .map(|i| i.info().full_path_str().to_string()) .collect::>(); assert_eq!( @@ -411,7 +406,7 @@ mod tests { #[test] fn test_indent() { let items = vec![ - "a/b/file.txt", // + Path::new("a/b/file.txt"), // ]; let list = @@ -421,15 +416,15 @@ mod tests { .iter() .map(|i| (i.info().indent(), i.info().path())); - assert_eq!(res.next(), Some((0, "a/b"))); - assert_eq!(res.next(), Some((1, "file.txt"))); + assert_eq!(res.next(), Some((0, Path::new("a/b")))); + assert_eq!(res.next(), Some((1, Path::new("file.txt")))); } #[test] fn test_indent_folder_file_name() { let items = vec![ - "a/b", // - "a.txt", // + Path::new("a/b"), // + Path::new("a.txt"), // ]; let list = @@ -437,7 +432,7 @@ mod tests { let mut res = list .tree_items .iter() - .map(|i| (i.info().indent(), i.info().path())); + .map(|i| (i.info().indent(), i.info().path_str())); assert_eq!(res.next(), Some((0, "a"))); assert_eq!(res.next(), Some((1, "b"))); @@ -447,8 +442,8 @@ mod tests { #[test] fn test_folder_dup() { let items = vec![ - "a/file.txt", // - "a/file2.txt", // + Path::new("a/file.txt"), // + Path::new("a/file2.txt"), // ]; let tree = @@ -460,7 +455,7 @@ mod tests { let res = tree .tree_items .iter() - .map(|i| i.info().full_path().to_string()) + .map(|i| i.info().full_path_str().to_string()) .collect::>(); assert_eq!( @@ -476,8 +471,8 @@ mod tests { #[test] fn test_collapse() { let items = vec![ - "a/file1.txt", // - "b/file2.txt", // + Path::new("a/file1.txt"), // + Path::new("b/file2.txt"), // ]; let mut tree = @@ -493,8 +488,8 @@ mod tests { #[test] fn test_iterate_collapsed() { let items = vec![ - "a/file1.txt", // - "b/file2.txt", // + Path::new("a/file1.txt"), // + Path::new("b/file2.txt"), // ]; let mut tree = @@ -520,8 +515,8 @@ mod tests { #[test] fn test_expand() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -564,8 +559,8 @@ mod tests { #[test] fn test_expand_bug() { let items = vec![ - "a/b/c", // - "a/b2/d", // + Path::new("a/b/c"), // + Path::new("a/b2/d"), // ]; //0 a/ @@ -608,8 +603,8 @@ mod tests { #[test] fn test_collapse_too_much() { let items = vec![ - "a/b", // - "a2/c", // + Path::new("a/b"), // + Path::new("a2/c"), // ]; //0 a/ @@ -638,8 +633,8 @@ mod tests { #[test] fn test_expand_with_collapsed_sub_parts() { let items = vec![ - "a/b/c", // - "a/d", // + Path::new("a/b/c"), // + Path::new("a/d"), // ]; //0 a/ @@ -701,7 +696,7 @@ mod test_merging { #[test] fn test_merge_simple() { - let list = vec!["a/b/c"]; + let list = vec![Path::new("a/b/c")]; let (mut items, paths) = FileTreeItems::create_items(&list, &BTreeSet::new()) .unwrap(); @@ -716,8 +711,8 @@ mod test_merging { #[test] fn test_merge_simple2() { let list = vec![ - "a/b/c", // - "a/b/d", + Path::new("a/b/c"), // + Path::new("a/b/d"), // ]; let (mut items, paths) = FileTreeItems::create_items(&list, &BTreeSet::new()) @@ -736,8 +731,8 @@ mod test_merging { #[test] fn test_merge_indent() { let list = vec![ - "a/b/c/d", // - "a/e/f", + Path::new("a/b/c/d"), // + Path::new("a/e/f"), // ]; //0:0 a/ @@ -758,7 +753,7 @@ mod test_merging { assert_eq!(*paths.get(&Path::new("a/b/c")).unwrap(), 1); assert_eq!(*paths.get(&Path::new("a/e")).unwrap(), 1); - FileTreeItems::fold_paths(&mut items, dbg!(&paths)); + FileTreeItems::fold_paths(&mut items, &paths); let indents: Vec = items.iter().map(|i| i.info().indent()).collect(); @@ -768,8 +763,8 @@ mod test_merging { #[test] fn test_merge_single_paths() { let items = vec![ - "a/b/c", // - "a/b/d", // + Path::new("a/b/c"), // + Path::new("a/b/d"), // ]; //0 a/b/ @@ -781,7 +776,7 @@ mod test_merging { let mut it = tree .iterate(0, 10) - .map(|(_, item)| item.info().full_path()); + .map(|(_, item)| item.info().full_path_str()); assert_eq!(it.next().unwrap(), "a/b"); assert_eq!(it.next().unwrap(), "a/b/c"); @@ -792,8 +787,8 @@ mod test_merging { #[test] fn test_merge_nothing() { let items = vec![ - "a/b/c", // - "a/b2/d", // + Path::new("a/b/c"), // + Path::new("a/b2/d"), // ]; //0 a/ @@ -807,7 +802,7 @@ mod test_merging { let mut it = tree .iterate(0, 10) - .map(|(_, item)| item.info().full_path()); + .map(|(_, item)| item.info().full_path_str()); assert_eq!(it.next().unwrap(), "a"); assert_eq!(it.next().unwrap(), "a/b"); diff --git a/filetreelist/src/item.rs b/filetreelist/src/item.rs index 1f14203a9d1..5936b05ffb6 100644 --- a/filetreelist/src/item.rs +++ b/filetreelist/src/item.rs @@ -1,5 +1,8 @@ -use crate::error::{Error, Result}; -use std::{convert::TryFrom, path::Path}; +use crate::error::Result; +use std::{ + convert::TryFrom, + path::{Path, PathBuf}, +}; /// holds the information shared among all `FileTreeItem` in a `FileTree` #[derive(Debug, Clone)] @@ -8,23 +11,20 @@ pub struct TreeItemInfo { indent: u8, /// currently visible depending on the folder collapse states visible: bool, - /// just the last path element - path: String, + /// contains this paths last component and folded up paths added to it + /// if this is `None` nothing was folding into here + folded: Option, /// the full path - full_path: String, + full_path: PathBuf, } impl TreeItemInfo { /// - pub const fn new( - indent: u8, - path: String, - full_path: String, - ) -> Self { + pub const fn new(indent: u8, full_path: PathBuf) -> Self { Self { indent, visible: true, - path, + folded: None, full_path, } } @@ -35,13 +35,36 @@ impl TreeItemInfo { } /// - pub fn full_path(&self) -> &str { - &self.full_path + //TODO: remove + pub fn full_path_str(&self) -> &str { + self.full_path.to_str().unwrap_or_default() } /// - pub fn path(&self) -> &str { - &self.path + pub fn full_path(&self) -> &Path { + self.full_path.as_path() + } + + /// like `path` but as `&str` + pub fn path_str(&self) -> &str { + self.path().as_os_str().to_str().unwrap_or_default() + } + + /// returns the last component of `full_path` + /// or the last components plus folded up children paths + pub fn path(&self) -> &Path { + self.folded.as_ref().map_or_else( + || { + Path::new( + self.full_path + .components() + .last() + .and_then(|c| c.as_os_str().to_str()) + .unwrap_or_default(), + ) + }, + |folding| folding.as_path(), + ) } /// @@ -91,64 +114,38 @@ pub struct FileTreeItem { } impl FileTreeItem { - pub fn new_file(path: &str) -> Result { - let item_path = Path::new(&path); + pub fn new_file(path: &Path) -> Result { + let item_path = PathBuf::from(path); let indent = u8::try_from( item_path.ancestors().count().saturating_sub(2), )?; - let filename = item_path - .file_name() - .map_or_else( - || Err(Error::InvalidFilePath(path.to_string())), - Ok, - )? - .to_string_lossy() - .to_string(); - Ok(Self { - info: TreeItemInfo::new( - indent, - filename, - item_path.to_string_lossy().to_string(), - ), + info: TreeItemInfo::new(indent, item_path), kind: FileTreeItemKind::File, }) } - pub fn new_path( - path: &Path, - path_string: String, - collapsed: bool, - ) -> Result { + pub fn new_path(path: &Path, collapsed: bool) -> Result { let indent = u8::try_from(path.ancestors().count().saturating_sub(2))?; - let last_path_component = - path.components().last().map_or_else( - || Err(Error::InvalidPath(path.to_path_buf())), - Ok, - )?; - let last_path_component = last_path_component - .as_os_str() - .to_string_lossy() - .to_string(); - Ok(Self { - info: TreeItemInfo::new( - indent, - last_path_component, - path_string, - ), + info: TreeItemInfo::new(indent, path.to_owned()), kind: FileTreeItemKind::Path(PathCollapsed(collapsed)), }) } /// pub fn fold(&mut self, next: Self) { - self.info.path = - format!("{}/{}", self.info.path, next.info.path); + if let Some(folded) = self.info.folded.as_mut() { + *folded = folded.join(next.info.path()); + } else { + self.info.folded = + Some(self.info.path().join(next.info.path())) + } + self.info.full_path = next.info.full_path; } @@ -202,6 +199,32 @@ impl PartialOrd for FileTreeItem { impl Ord for FileTreeItem { fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.info.path.cmp(&other.info.path) + self.info.path().cmp(other.info.path()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn test_smoke() { + let mut a = + FileTreeItem::new_path(Path::new("a"), false).unwrap(); + + assert_eq!(a.info.full_path_str(), "a"); + assert_eq!(a.info.path_str(), "a"); + + let b = + FileTreeItem::new_path(Path::new("a/b"), false).unwrap(); + a.fold(b); + + assert_eq!(a.info.full_path_str(), "a/b"); + assert_eq!( + &a.info.folded.as_ref().unwrap(), + &Path::new("a/b") + ); + assert_eq!(a.info.path(), Path::new("a/b")); } } diff --git a/src/components/revision_files.rs b/src/components/revision_files.rs index 5864a064130..5f077ac2763 100644 --- a/src/components/revision_files.rs +++ b/src/components/revision_files.rs @@ -80,11 +80,8 @@ impl RevisionFilesComponent { self.revision.map(|c| c == commit).unwrap_or_default(); if !same_id { self.files = sync::tree_files(CWD, commit)?; - let filenames: Vec<&str> = self - .files - .iter() - .map(|f| f.path.to_str().unwrap_or_default()) - .collect(); + let filenames: Vec<&Path> = + self.files.iter().map(|f| f.path.as_path()).collect(); self.tree = FileTree::new(&filenames, &BTreeSet::new())?; self.tree.collapse_but_root(); self.revision = Some(commit); @@ -108,7 +105,7 @@ impl RevisionFilesComponent { theme: &SharedTheme, selected: bool, ) -> Span<'a> { - let path = item.info().path(); + let path = item.info().path_str(); let indent = item.info().indent(); let indent_str = if indent == 0 { @@ -136,7 +133,7 @@ impl RevisionFilesComponent { self.tree.selected_file().map_or(false, |file| { self.queue.borrow_mut().push_back( InternalEvent::BlameFile( - file.full_path() + file.full_path_str() .strip_prefix("./") .unwrap_or_default() .to_string(), @@ -151,7 +148,7 @@ impl RevisionFilesComponent { if let Some(file) = self .tree .selected_file() - .map(|file| file.full_path().to_string()) + .map(|file| file.full_path_str().to_string()) { let path = Path::new(&file); if let Some(item) =