From a5fde22587a4ff9bef4c1dee9f0a432b4d50e4c3 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Wed, 25 Jan 2023 02:48:17 +0000 Subject: [PATCH 01/15] Changed file picker --- Cargo.lock | 68 ++++++ helix-term/src/commands.rs | 41 ++++ helix-term/src/keymap/default.rs | 3 +- helix-term/src/ui/menu.rs | 33 +++ helix-vcs/Cargo.toml | 5 +- helix-vcs/src/diff.rs | 28 +++ helix-vcs/src/git.rs | 358 ++++++++++++++++++++++++++++++- helix-vcs/src/lib.rs | 19 +- 8 files changed, 550 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 13c1ea333364..9473fd4202eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -105,6 +105,15 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf4b9d6a944f767f8e5e0db018570623c85f3d925ac718db4e06d0187adb21c1" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bstr" version = "1.8.0" @@ -215,6 +224,15 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ffa3d3e0138386cd4361f63537765cac7ee40698028844635a54495a92f67f3" +[[package]] +name = "cpufeatures" +version = "0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53fe5e26ff1b7aef8bca9c6080520cfb8d9333c7568e1829cef191a9723e5504" +dependencies = [ + "libc", +] + [[package]] name = "crc32fast" version = "1.3.2" @@ -294,6 +312,16 @@ dependencies = [ "winapi", ] +[[package]] +name = "crypto-common" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "cxx" version = "1.0.94" @@ -338,6 +366,16 @@ dependencies = [ "syn 2.0.48", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "dunce" version = "1.0.4" @@ -506,6 +544,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getrandom" version = "0.2.9" @@ -1411,12 +1459,15 @@ version = "23.10.0" dependencies = [ "anyhow", "arc-swap", + "content_inspector", "gix", "helix-core", "helix-event", + "ignore", "imara-diff", "log", "parking_lot", + "sha1", "tempfile", "tokio", ] @@ -2070,6 +2121,17 @@ dependencies = [ "serde", ] +[[package]] +name = "sha1" +version = "0.10.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "sha1_smol" version = "1.0.0" @@ -2408,6 +2470,12 @@ dependencies = [ "regex", ] +[[package]] +name = "typenum" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "42ff0bf0c66b8238c6f3b578df37d0b7848e55df8577b3f74f92a69acceeb825" + [[package]] name = "unicase" version = "2.6.0" diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index d545480b5308..d5aabc3cc634 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -324,6 +324,7 @@ impl MappableCommand { buffer_picker, "Open buffer picker", jumplist_picker, "Open jumplist picker", symbol_picker, "Open symbol picker", + changed_file_picker, "Open changed file picker", select_references_to_symbol_under_cursor, "Select symbol references", workspace_symbol_picker, "Open workspace symbol picker", diagnostics_picker, "Open diagnostic picker", @@ -2994,6 +2995,46 @@ fn jumplist_picker(cx: &mut Context) { cx.push_layer(Box::new(overlaid(picker))); } +fn changed_file_picker(cx: &mut Context) { + let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); + + let entries = match cx.editor.diff_providers.get_changed_files(&cwd) { + Ok(entries) => entries, + Err(err) => { + cx.editor.set_error(format!("{err}")); + return; + } + }; + + let added = cx.editor.theme.get("diff.plus"); + let deleted = cx.editor.theme.get("diff.minus"); + let modified = cx.editor.theme.get("diff.delta"); + + let picker = Picker::new( + entries, + ui::menu::FileChangeData { + cwd, + style_untracked: added, + style_modified: modified, + style_deleted: deleted, + style_renamed: modified, + }, + |cx, meta, action| { + let path_to_open = meta.path(); + if let Err(e) = cx.editor.open(path_to_open, action) { + let err = if let Some(err) = e.source() { + format!("{}", err) + } else { + format!("unable to open \"{}\"", path_to_open.display()) + }; + cx.editor.set_error(err); + } + }, + ) + .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); + cx.push_layer(Box::new(overlaid(picker))); +} + impl ui::menu::Item for MappableCommand { type Data = ReverseKeymap; diff --git a/helix-term/src/keymap/default.rs b/helix-term/src/keymap/default.rs index ca5a21d26a16..f27f42b71914 100644 --- a/helix-term/src/keymap/default.rs +++ b/helix-term/src/keymap/default.rs @@ -223,9 +223,10 @@ pub fn default() -> HashMap { "S" => workspace_symbol_picker, "d" => diagnostics_picker, "D" => workspace_diagnostics_picker, + "g" => changed_file_picker, "a" => code_action, "'" => last_picker, - "g" => { "Debug (experimental)" sticky=true + "G" => { "Debug (experimental)" sticky=true "l" => dap_launch, "r" => dap_restart, "b" => dap_toggle_breakpoint, diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index c0e60b33e344..43f67fe5e33d 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -14,9 +14,11 @@ use tui::{ pub use tui::widgets::{Cell, Row}; +use helix_vcs::FileChange; use helix_view::{ editor::SmartTabConfig, graphics::{Margin, Rect}, + theme::Style, Editor, }; use tui::layout::Constraint; @@ -52,6 +54,37 @@ impl Item for PathBuf { pub type MenuCallback = Box, MenuEvent)>; +pub struct FileChangeData { + pub cwd: PathBuf, + pub style_untracked: Style, + pub style_modified: Style, + pub style_deleted: Style, + pub style_renamed: Style, +} + +impl Item for FileChange { + type Data = FileChangeData; + + fn format(&self, data: &Self::Data) -> Row { + let (sign, style) = match self { + Self::Untracked { .. } => ("[+]", data.style_untracked), + Self::Modified { .. } => ("[~]", data.style_modified), + Self::Deleted { .. } => ("[-]", data.style_deleted), + Self::Renamed { .. } => ("[>]", data.style_modified), + }; + let path = self.path(); + + Row::new(vec![ + sign.to_owned(), + path.strip_prefix(&data.cwd) + .unwrap_or(path) + .to_string_lossy() + .to_string(), + ]) + .style(style) + } +} + pub struct Menu { options: Vec, editor_data: T::Data, diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index 49b3661e22b3..93427841cfda 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -19,9 +19,12 @@ tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "p parking_lot = "0.12" arc-swap = { version = "1.7.0" } -gix = { version = "0.61.0", features = ["attributes"], default-features = false, optional = true } +content_inspector = "0.2.4" +gix = { version = "0.61.0", features = ["attributes", "index"], default-features = false, optional = true } +ignore = "0.4" imara-diff = "0.1.5" anyhow = "1" +sha1 = "0.10.5" log = "0.4" diff --git a/helix-vcs/src/diff.rs b/helix-vcs/src/diff.rs index c72deb7ea7f3..8a9a6e8ecbf7 100644 --- a/helix-vcs/src/diff.rs +++ b/helix-vcs/src/diff.rs @@ -1,4 +1,5 @@ use std::ops::Range; +use std::path::{Path, PathBuf}; use std::sync::Arc; use helix_core::Rope; @@ -290,3 +291,30 @@ impl Diff<'_> { } } } + +pub enum FileChange { + Untracked { + path: PathBuf, + }, + Modified { + path: PathBuf, + }, + Deleted { + path: PathBuf, + }, + Renamed { + from_path: PathBuf, + to_path: PathBuf, + }, +} + +impl FileChange { + pub fn path(&self) -> &Path { + match self { + Self::Untracked { path } => path, + Self::Modified { path } => path, + Self::Deleted { path } => path, + Self::Renamed { to_path, .. } => to_path, + } + } +} diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 995bade06e0d..812f8c0de74f 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -1,3 +1,8 @@ +use std::{ + collections::{hash_map::Entry, HashMap, HashSet}, + path::PathBuf, +}; + use anyhow::{bail, Context, Result}; use arc_swap::ArcSwap; use gix::filter::plumbing::driver::apply::Delay; @@ -5,17 +10,60 @@ use std::io::Read; use std::path::Path; use std::sync::Arc; +use gix::index::{entry::Mode, State}; use gix::objs::tree::EntryKind; use gix::sec::trust::DefaultForLevel; use gix::{Commit, ObjectId, Repository, ThreadSafeRepository}; +use ignore::WalkBuilder; +use sha1::Digest; -use crate::DiffProvider; +use crate::{DiffProvider, FileChange}; #[cfg(test)] mod test; pub struct Git; +/// A subset of `git_repository::objs::tree::EntryMode` that actually makes sense for tree nodes. +#[derive(Hash, PartialEq, Eq)] +enum FileEntryMode { + Blob, + BlobExecutable, + Link, +} + +#[derive(Default)] +struct RawChanges { + additions: Vec, + deletions: HashMap>, + modifications: Vec, +} + +#[derive(Hash, PartialEq, Eq)] +struct RawAddition { + entry_mode: FileEntryMode, + oid: ObjectId, + path: PathBuf, +} + +#[derive(Hash, PartialEq, Eq)] +struct RawDeletion { + entry_mode: FileEntryMode, + oid: ObjectId, + path: PathBuf, +} + +#[allow(unused)] +struct RawModification { + previous_entry_mode: FileEntryMode, + previous_oid: ObjectId, + + entry_mode: FileEntryMode, + oid: ObjectId, + + path: PathBuf, +} + impl Git { fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { // custom open options @@ -61,6 +109,109 @@ impl Git { Ok(res) } + + /// Emulates the result of running `git status` from the command line. + fn status(repo: &Repository) -> Result> { + let autocrlf = repo + .config_snapshot() + .boolean("core.autocrlf") + .unwrap_or(false); + + let work_dir = repo + .work_dir() + .ok_or_else(|| anyhow::anyhow!("working tree not found"))?; + + // TODO: allow diffing against another ref + let head_tree = repo.head_commit()?.tree()?; + let head_state = State::from_tree(&head_tree.id, &repo.objects)?; + + let mut head_tree_set = HashSet::new(); + let mut submodule_paths = vec![]; + + let mut raw_changes = RawChanges::default(); + + for item in head_state.entries() { + let full_path = work_dir.join(&PathBuf::from(item.path(&head_state).to_string())); + + if item.mode == Mode::COMMIT { + submodule_paths.push(full_path); + } else { + let old_entry_mode = match item.mode { + Mode::FILE => FileEntryMode::Blob, + Mode::FILE_EXECUTABLE => FileEntryMode::BlobExecutable, + Mode::SYMLINK => FileEntryMode::Link, + _ => anyhow::bail!("unexpected entry mode"), + }; + + match git_meta_from_path(&full_path, autocrlf)? { + Some((new_entry_mode, new_oid)) => { + // On Windows, physical files are _always_ inferred as `Blob`. We simply don't + // compare the entry mode as it's pointless. + let entry_mode_changed = { + #[cfg(unix)] + { + new_entry_mode != old_entry_mode + } + + #[cfg(not(unix))] + { + false + } + }; + + if entry_mode_changed || new_oid != item.id { + raw_changes.add_modification(RawModification { + previous_entry_mode: old_entry_mode, + previous_oid: item.id, + entry_mode: new_entry_mode, + oid: new_oid, + path: full_path.clone(), + }); + } + } + None => { + raw_changes.add_deletion(RawDeletion { + entry_mode: old_entry_mode, + oid: item.id, + path: full_path.clone(), + }); + } + } + + head_tree_set.insert(full_path); + } + } + + // Looks for untracked files by walking the fs and probing the (cached) head tree + // TODO: use build_parallel() to speed this up + for entry in WalkBuilder::new(work_dir) + .hidden(false) + .ignore(false) + .filter_entry(move |entry| { + entry.file_name() != ".git" + && !submodule_paths + .iter() + .any(|submodule| entry.path().starts_with(submodule)) + }) + .build() + { + let entry = entry?; + if !entry.file_type().map_or(false, |ft| ft.is_dir()) { + let full_path = entry.path(); + let meta = git_meta_from_path(full_path, autocrlf)? + .ok_or_else(|| anyhow::anyhow!("file moved between checks"))?; + if !head_tree_set.contains(full_path) { + raw_changes.add_addition(RawAddition { + entry_mode: meta.0, + oid: meta.1, + path: full_path.to_path_buf(), + }) + } + } + } + + Ok(raw_changes.into()) + } } impl DiffProvider for Git { @@ -112,6 +263,86 @@ impl DiffProvider for Git { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } + + fn get_changed_files(&self, cwd: &Path) -> Result> { + Self::status(&Self::open_repo(cwd, None)?.to_thread_local()) + } +} + +impl RawChanges { + pub fn add_addition(&mut self, addition: RawAddition) { + self.additions.push(addition); + } + + pub fn add_deletion(&mut self, deletion: RawDeletion) { + match self.deletions.entry(deletion.oid) { + Entry::Occupied(entry) => { + entry.into_mut().push(deletion); + } + Entry::Vacant(entry) => { + entry.insert(vec![deletion]); + } + } + } + + pub fn add_modification(&mut self, modification: RawModification) { + self.modifications.push(modification); + } +} + +impl From for Vec { + // Unlike Git, we only look for pure renames at the moment. + // TODO: detect renames with minor changes + fn from(mut raw: RawChanges) -> Self { + let mut status_entries = vec![]; + + let additions_left = if !raw.additions.is_empty() && !raw.deletions.is_empty() { + let mut unmatched_additions = vec![]; + + for add in raw.additions.into_iter() { + let matched_deletions = match raw.deletions.entry(add.oid) { + Entry::Occupied(entry) => entry.into_mut(), + Entry::Vacant(_) => { + unmatched_additions.push(add); + continue; + } + }; + + // Impossible to have an empty vec inside + let chosen_deletion = matched_deletions.pop().expect("unexpected empty vec"); + if matched_deletions.is_empty() { + raw.deletions.remove(&add.oid); + } + + status_entries.push(FileChange::Renamed { + from_path: chosen_deletion.path.to_owned(), + to_path: add.path.to_owned(), + }); + } + + unmatched_additions + } else { + raw.additions + }; + + additions_left + .into_iter() + .for_each(|item| status_entries.push(FileChange::Untracked { path: item.path })); + raw.deletions + .values() + .into_iter() + .flat_map(|val| val.iter()) + .for_each(|item| { + status_entries.push(FileChange::Deleted { + path: item.path.to_owned(), + }) + }); + raw.modifications + .into_iter() + .for_each(|item| status_entries.push(FileChange::Modified { path: item.path })); + + status_entries + } } /// Finds the object that contains the contents of a file at a specific commit. @@ -131,3 +362,128 @@ fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Resul EntryKind::Blob | EntryKind::BlobExecutable => Ok(tree_entry.object_id()), } } + +fn git_meta_from_path( + path: &Path, + autocrlf: bool, +) -> Result, std::io::Error> { + // Windows doesn't support symlinks. This block runs fine but is just wasting CPU cycles. + #[cfg(not(windows))] + match path.symlink_metadata() { + Ok(meta) => { + if meta.is_symlink() { + let link_content = std::fs::read_link(path)?; + let link_content = link_content.to_string_lossy(); + let link_content = link_content.as_bytes(); + + let mut hasher = sha1::Sha1::default(); + hasher.update(b"blob "); + hasher.update(format!("{}", link_content.len()).as_bytes()); + hasher.update(b"\0"); + hasher.update(link_content); + + let hash: [u8; 20] = hasher.finalize().into(); + + return Ok(Some((FileEntryMode::Link, ObjectId::from(hash)))); + } + } + Err(_) => return Ok(None), + }; + + // Not a symlink for sure from this point + Ok(match path.metadata() { + Ok(meta) => { + if meta.is_file() { + let entry_mode = { + #[cfg(unix)] + { + use std::os::unix::prelude::PermissionsExt; + if meta.permissions().mode() & 0o111 != 0 { + FileEntryMode::BlobExecutable + } else { + FileEntryMode::Blob + } + } + + #[cfg(not(unix))] + { + FileEntryMode::Blob + } + }; + + let oid = { + let mut file = std::fs::File::open(path)?; + + // `git::features::hash::Sha1` doesn't implement `Write` so we use the + // underlying crate directly for max perf. + let mut hasher = sha1::Sha1::default(); + hasher.update(b"blob "); + + if autocrlf { + // When autocrlf is on, we either have to fit the whole file into memory, + // or we read the file twice. Either way is not optimal. How should we + // handle this? + // + // With the current implementation, there's no way we can handle huge files + // that do not fit into memory. Maybe we can set a size limit? Anything + // over a certain size will simply be read twice: once for getting the + // normalized size, and once for the hasher updates? + const BUFFER_SIZE: usize = 8 * 1024; + let mut buffer = [0u8; BUFFER_SIZE]; + + let mut len = file.read(&mut buffer)?; + if content_inspector::inspect(&buffer[..len]) + == content_inspector::ContentType::BINARY + { + // No CRLF handling! We update the part already read + the remaining + // content in the file. + hasher.update(format!("{}", meta.len()).as_bytes()); + hasher.update(b"\0"); + + hasher.update(&buffer[..len]); + std::io::copy(&mut file, &mut hasher)?; + } else { + // It's a text file. CRLF transformation as planned. + let mut normalized_file = Vec::with_capacity(meta.len() as usize); + let mut was_cr = false; + + loop { + buffer[..len].iter().for_each(|byte| { + if was_cr && *byte == b'\n' { + normalized_file.pop(); + } + normalized_file.push(*byte); + was_cr = *byte == b'\r'; + }); + + if len < BUFFER_SIZE { + break; + } + len = file.read(&mut buffer)?; + } + + hasher.update(format!("{}", normalized_file.len()).as_bytes()); + hasher.update(b"\0"); + + hasher.update(&normalized_file); + } + } else { + hasher.update(format!("{}", meta.len()).as_bytes()); + hasher.update(b"\0"); + + std::io::copy(&mut file, &mut hasher)?; + } + + let hash: [u8; 20] = hasher.finalize().into(); + ObjectId::from(hash) + }; + + Some((entry_mode, oid)) + } else { + // It's a non-symlink folder. Git doesn't track folders. Same as deletion. + None + } + } + Err(_) => None, + }) +} diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 63487fbcde6c..e69ddd27785c 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,7 +1,8 @@ -use anyhow::{bail, Result}; use arc_swap::ArcSwap; use std::{path::Path, sync::Arc}; +use anyhow::{bail, Result}; + #[cfg(feature = "git")] pub use git::Git; #[cfg(not(feature = "git"))] @@ -12,7 +13,7 @@ mod git; mod diff; -pub use diff::{DiffHandle, Hunk}; +pub use diff::{DiffHandle, FileChange, Hunk}; pub trait DiffProvider { /// Returns the data that a diff should be computed against @@ -20,7 +21,10 @@ pub trait DiffProvider { /// The data is returned as raw byte without any decoding or encoding performed /// to ensure all file encodings are handled correctly. fn get_diff_base(&self, file: &Path) -> Result>; + fn get_current_head_name(&self, file: &Path) -> Result>>>; + + fn get_changed_files(&self, cwd: &Path) -> Result>; } #[doc(hidden)] @@ -33,6 +37,10 @@ impl DiffProvider for Dummy { fn get_current_head_name(&self, _file: &Path) -> Result>>> { bail!("helix was compiled without git support") } + + fn get_changed_files(&self, _cwd: &Path) -> Result> { + anyhow::bail!("dummy diff provider") + } } pub struct DiffProviderRegistry { @@ -65,6 +73,13 @@ impl DiffProviderRegistry { } }) } + + pub fn get_changed_files(&self, cwd: &Path) -> Result> { + self.providers + .iter() + .find_map(|provider| provider.get_changed_files(cwd).ok()) + .ok_or_else(|| anyhow::anyhow!("no diff provider returns success")) + } } impl Default for DiffProviderRegistry { From 8021ca5278286ef158377b3a328c91fc73806087 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Thu, 9 Mar 2023 19:41:33 +0800 Subject: [PATCH 02/15] refactor: make better use of chained iterators Co-authored-by: WJH --- helix-vcs/src/git.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 812f8c0de74f..1f91cc0fd6a6 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -325,21 +325,23 @@ impl From for Vec { raw.additions }; - additions_left + let status_entries = additions_left .into_iter() - .for_each(|item| status_entries.push(FileChange::Untracked { path: item.path })); - raw.deletions - .values() - .into_iter() - .flat_map(|val| val.iter()) - .for_each(|item| { - status_entries.push(FileChange::Deleted { - path: item.path.to_owned(), - }) - }); - raw.modifications - .into_iter() - .for_each(|item| status_entries.push(FileChange::Modified { path: item.path })); + .map(|item| FileChange::Untracked { path: item.path }) + .chain( + raw.deletions + .values() + .flat_map(|val| val.iter()) + .map(|item| FileChange::Deleted { + path: item.path.to_owned(), + }), + ) + .chain( + raw.modifications + .into_iter() + .map(|item| FileChange::Modified { path: item.path }), + ) + .collect::>(); status_entries } From 61574f1360a4909d040a5f674e4c1761bfa1713d Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Sun, 24 Mar 2024 02:59:27 +0000 Subject: [PATCH 03/15] refactor: replace changed file detection with gix --- Cargo.lock | 125 ++++++------ helix-term/src/ui/menu.rs | 31 +-- helix-vcs/Cargo.toml | 5 +- helix-vcs/src/git.rs | 396 ++++++-------------------------------- helix-vcs/src/lib.rs | 3 +- 5 files changed, 135 insertions(+), 425 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9473fd4202eb..9b4be942a6ca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -105,15 +105,6 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf4b9d6a944f767f8e5e0db018570623c85f3d925ac718db4e06d0187adb21c1" -[[package]] -name = "block-buffer" -version = "0.10.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" -dependencies = [ - "generic-array", -] - [[package]] name = "bstr" version = "1.8.0" @@ -224,15 +215,6 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ffa3d3e0138386cd4361f63537765cac7ee40698028844635a54495a92f67f3" -[[package]] -name = "cpufeatures" -version = "0.2.12" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53fe5e26ff1b7aef8bca9c6080520cfb8d9333c7568e1829cef191a9723e5504" -dependencies = [ - "libc", -] - [[package]] name = "crc32fast" version = "1.3.2" @@ -312,16 +294,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "crypto-common" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" -dependencies = [ - "generic-array", - "typenum", -] - [[package]] name = "cxx" version = "1.0.94" @@ -367,13 +339,16 @@ dependencies = [ ] [[package]] -name = "digest" -version = "0.10.7" +name = "dashmap" +version = "5.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +checksum = "907076dfda823b0b36d2a1bb5f90c96660a5bbcd7729e10727f07858f22c4edc" dependencies = [ - "block-buffer", - "crypto-common", + "cfg-if", + "hashbrown 0.12.3", + "lock_api", + "once_cell", + "parking_lot_core", ] [[package]] @@ -544,16 +519,6 @@ dependencies = [ "slab", ] -[[package]] -name = "generic-array" -version = "0.14.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" -dependencies = [ - "typenum", - "version_check", -] - [[package]] name = "getrandom" version = "0.2.9" @@ -584,6 +549,7 @@ dependencies = [ "gix-config", "gix-date", "gix-diff", + "gix-dir", "gix-discover", "gix-features", "gix-filter", @@ -605,6 +571,7 @@ dependencies = [ "gix-revision", "gix-revwalk", "gix-sec", + "gix-status", "gix-submodule", "gix-tempfile", "gix-trace", @@ -747,8 +714,36 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "78e605593c2ef74980a534ade0909c7dc57cca72baa30cbb67d2dda621f99ac4" dependencies = [ "bstr", + "gix-command", + "gix-filter", + "gix-fs", "gix-hash", "gix-object", + "gix-path", + "gix-tempfile", + "gix-trace", + "gix-worktree", + "imara-diff", + "thiserror", +] + +[[package]] +name = "gix-dir" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3413ccd29130900c17574678aee640e4847909acae9febf6424dc77b782c6d32" +dependencies = [ + "bstr", + "gix-discover", + "gix-fs", + "gix-ignore", + "gix-index", + "gix-object", + "gix-path", + "gix-pathspec", + "gix-trace", + "gix-utils", + "gix-worktree", "thiserror", ] @@ -1102,6 +1097,28 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "gix-status" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ca216db89947eca709f69ec5851aa76f9628e7c7aab7aa5a927d0c619d046bf2" +dependencies = [ + "bstr", + "filetime", + "gix-diff", + "gix-dir", + "gix-features", + "gix-filter", + "gix-fs", + "gix-hash", + "gix-index", + "gix-object", + "gix-path", + "gix-pathspec", + "gix-worktree", + "thiserror", +] + [[package]] name = "gix-submodule" version = "0.10.0" @@ -1123,6 +1140,7 @@ version = "13.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2d337955b7af00fb87120d053d87cdfb422a80b9ff7a3aa4057a99c79422dc30" dependencies = [ + "dashmap", "gix-fs", "libc", "once_cell", @@ -1172,6 +1190,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0066432d4c277f9877f091279a597ea5331f68ca410efc874f0bdfb1cd348f92" dependencies = [ + "bstr", "fastrand", "unicode-normalization", ] @@ -1459,15 +1478,12 @@ version = "23.10.0" dependencies = [ "anyhow", "arc-swap", - "content_inspector", "gix", "helix-core", "helix-event", - "ignore", "imara-diff", "log", "parking_lot", - "sha1", "tempfile", "tokio", ] @@ -2121,17 +2137,6 @@ dependencies = [ "serde", ] -[[package]] -name = "sha1" -version = "0.10.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" -dependencies = [ - "cfg-if", - "cpufeatures", - "digest", -] - [[package]] name = "sha1_smol" version = "1.0.0" @@ -2470,12 +2475,6 @@ dependencies = [ "regex", ] -[[package]] -name = "typenum" -version = "1.17.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "42ff0bf0c66b8238c6f3b578df37d0b7848e55df8577b3f74f92a69acceeb825" - [[package]] name = "unicase" version = "2.6.0" diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index 43f67fe5e33d..1b65421498e1 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -66,22 +66,25 @@ impl Item for FileChange { type Data = FileChangeData; fn format(&self, data: &Self::Data) -> Row { - let (sign, style) = match self { - Self::Untracked { .. } => ("[+]", data.style_untracked), - Self::Modified { .. } => ("[~]", data.style_modified), - Self::Deleted { .. } => ("[-]", data.style_deleted), - Self::Renamed { .. } => ("[>]", data.style_modified), - }; - let path = self.path(); - - Row::new(vec![ - sign.to_owned(), + let process_path = |path: &PathBuf| { path.strip_prefix(&data.cwd) .unwrap_or(path) - .to_string_lossy() - .to_string(), - ]) - .style(style) + .display() + .to_string() + }; + + let (sign, style, content) = match self { + Self::Untracked { path } => ("[+]", data.style_untracked, process_path(path)), + Self::Modified { path } => ("[~]", data.style_modified, process_path(path)), + Self::Deleted { path } => ("[-]", data.style_deleted, process_path(path)), + Self::Renamed { from_path, to_path } => ( + "[>]", + data.style_modified, + format!("{} -> {}", process_path(from_path), process_path(to_path)), + ), + }; + + Row::new(vec![sign.to_owned(), content]).style(style) } } diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index 93427841cfda..d38c3fc879da 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -19,12 +19,9 @@ tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "p parking_lot = "0.12" arc-swap = { version = "1.7.0" } -content_inspector = "0.2.4" -gix = { version = "0.61.0", features = ["attributes", "index"], default-features = false, optional = true } -ignore = "0.4" +gix = { version = "0.61.0", features = ["attributes", "status"], default-features = false, optional = true } imara-diff = "0.1.5" anyhow = "1" -sha1 = "0.10.5" log = "0.4" diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 1f91cc0fd6a6..10b621180599 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -1,8 +1,3 @@ -use std::{ - collections::{hash_map::Entry, HashMap, HashSet}, - path::PathBuf, -}; - use anyhow::{bail, Context, Result}; use arc_swap::ArcSwap; use gix::filter::plumbing::driver::apply::Delay; @@ -10,12 +5,15 @@ use std::io::Read; use std::path::Path; use std::sync::Arc; -use gix::index::{entry::Mode, State}; +use gix::bstr::ByteSlice; use gix::objs::tree::EntryKind; use gix::sec::trust::DefaultForLevel; +use gix::status::{ + index_worktree::iter::Item, + plumbing::index_as_worktree::{Change, EntryStatus}, + UntrackedFiles, +}; use gix::{Commit, ObjectId, Repository, ThreadSafeRepository}; -use ignore::WalkBuilder; -use sha1::Digest; use crate::{DiffProvider, FileChange}; @@ -24,46 +22,6 @@ mod test; pub struct Git; -/// A subset of `git_repository::objs::tree::EntryMode` that actually makes sense for tree nodes. -#[derive(Hash, PartialEq, Eq)] -enum FileEntryMode { - Blob, - BlobExecutable, - Link, -} - -#[derive(Default)] -struct RawChanges { - additions: Vec, - deletions: HashMap>, - modifications: Vec, -} - -#[derive(Hash, PartialEq, Eq)] -struct RawAddition { - entry_mode: FileEntryMode, - oid: ObjectId, - path: PathBuf, -} - -#[derive(Hash, PartialEq, Eq)] -struct RawDeletion { - entry_mode: FileEntryMode, - oid: ObjectId, - path: PathBuf, -} - -#[allow(unused)] -struct RawModification { - previous_entry_mode: FileEntryMode, - previous_oid: ObjectId, - - entry_mode: FileEntryMode, - oid: ObjectId, - - path: PathBuf, -} - impl Git { fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { // custom open options @@ -112,105 +70,62 @@ impl Git { /// Emulates the result of running `git status` from the command line. fn status(repo: &Repository) -> Result> { - let autocrlf = repo - .config_snapshot() - .boolean("core.autocrlf") - .unwrap_or(false); - let work_dir = repo .work_dir() .ok_or_else(|| anyhow::anyhow!("working tree not found"))?; - // TODO: allow diffing against another ref - let head_tree = repo.head_commit()?.tree()?; - let head_state = State::from_tree(&head_tree.id, &repo.objects)?; - - let mut head_tree_set = HashSet::new(); - let mut submodule_paths = vec![]; - - let mut raw_changes = RawChanges::default(); - - for item in head_state.entries() { - let full_path = work_dir.join(&PathBuf::from(item.path(&head_state).to_string())); - - if item.mode == Mode::COMMIT { - submodule_paths.push(full_path); - } else { - let old_entry_mode = match item.mode { - Mode::FILE => FileEntryMode::Blob, - Mode::FILE_EXECUTABLE => FileEntryMode::BlobExecutable, - Mode::SYMLINK => FileEntryMode::Link, - _ => anyhow::bail!("unexpected entry mode"), - }; - - match git_meta_from_path(&full_path, autocrlf)? { - Some((new_entry_mode, new_oid)) => { - // On Windows, physical files are _always_ inferred as `Blob`. We simply don't - // compare the entry mode as it's pointless. - let entry_mode_changed = { - #[cfg(unix)] - { - new_entry_mode != old_entry_mode + // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in our + // case to not list new (untracked) files. We could have respected this config if the + // default value weren't `Collapsed` though, as this default value would render the feature + // unusable to many. + let status_platform = repo + .status(gix::progress::Discard)? + .untracked_files(UntrackedFiles::Files) + .index_worktree_rewrites(Some(Default::default())); + + Ok(status_platform + .into_index_worktree_iter(Default::default())? + .map(|item| { + let item = item?; + + Result::, _>::Ok(match item { + Item::Modification { + rela_path, status, .. + } => { + let full_path = work_dir.join(rela_path.to_path()?); + + match status { + EntryStatus::Conflict(_) => { + // We treat conflict as change for now for simplicity + Some(FileChange::Modified { path: full_path }) } - - #[cfg(not(unix))] - { - false - } - }; - - if entry_mode_changed || new_oid != item.id { - raw_changes.add_modification(RawModification { - previous_entry_mode: old_entry_mode, - previous_oid: item.id, - entry_mode: new_entry_mode, - oid: new_oid, - path: full_path.clone(), - }); + EntryStatus::Change(change) => match change { + Change::Removed => Some(FileChange::Deleted { path: full_path }), + Change::Modification { .. } => { + Some(FileChange::Modified { path: full_path }) + } + Change::Type | Change::SubmoduleModification(_) => None, + }, + EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, } } - None => { - raw_changes.add_deletion(RawDeletion { - entry_mode: old_entry_mode, - oid: item.id, - path: full_path.clone(), - }); - } - } - - head_tree_set.insert(full_path); - } - } - - // Looks for untracked files by walking the fs and probing the (cached) head tree - // TODO: use build_parallel() to speed this up - for entry in WalkBuilder::new(work_dir) - .hidden(false) - .ignore(false) - .filter_entry(move |entry| { - entry.file_name() != ".git" - && !submodule_paths - .iter() - .any(|submodule| entry.path().starts_with(submodule)) + Item::DirectoryContents { entry, .. } => Some(FileChange::Untracked { + path: work_dir.join(entry.rela_path.to_path()?), + }), + Item::Rewrite { + source, + dirwalk_entry, + .. + } => Some(FileChange::Renamed { + from_path: work_dir.join(source.rela_path().to_path()?), + to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?), + }), + }) }) - .build() - { - let entry = entry?; - if !entry.file_type().map_or(false, |ft| ft.is_dir()) { - let full_path = entry.path(); - let meta = git_meta_from_path(full_path, autocrlf)? - .ok_or_else(|| anyhow::anyhow!("file moved between checks"))?; - if !head_tree_set.contains(full_path) { - raw_changes.add_addition(RawAddition { - entry_mode: meta.0, - oid: meta.1, - path: full_path.to_path_buf(), - }) - } - } - } - - Ok(raw_changes.into()) + .collect::>>>()? + .into_iter() + .flatten() + .collect::>()) } } @@ -269,84 +184,6 @@ impl DiffProvider for Git { } } -impl RawChanges { - pub fn add_addition(&mut self, addition: RawAddition) { - self.additions.push(addition); - } - - pub fn add_deletion(&mut self, deletion: RawDeletion) { - match self.deletions.entry(deletion.oid) { - Entry::Occupied(entry) => { - entry.into_mut().push(deletion); - } - Entry::Vacant(entry) => { - entry.insert(vec![deletion]); - } - } - } - - pub fn add_modification(&mut self, modification: RawModification) { - self.modifications.push(modification); - } -} - -impl From for Vec { - // Unlike Git, we only look for pure renames at the moment. - // TODO: detect renames with minor changes - fn from(mut raw: RawChanges) -> Self { - let mut status_entries = vec![]; - - let additions_left = if !raw.additions.is_empty() && !raw.deletions.is_empty() { - let mut unmatched_additions = vec![]; - - for add in raw.additions.into_iter() { - let matched_deletions = match raw.deletions.entry(add.oid) { - Entry::Occupied(entry) => entry.into_mut(), - Entry::Vacant(_) => { - unmatched_additions.push(add); - continue; - } - }; - - // Impossible to have an empty vec inside - let chosen_deletion = matched_deletions.pop().expect("unexpected empty vec"); - if matched_deletions.is_empty() { - raw.deletions.remove(&add.oid); - } - - status_entries.push(FileChange::Renamed { - from_path: chosen_deletion.path.to_owned(), - to_path: add.path.to_owned(), - }); - } - - unmatched_additions - } else { - raw.additions - }; - - let status_entries = additions_left - .into_iter() - .map(|item| FileChange::Untracked { path: item.path }) - .chain( - raw.deletions - .values() - .flat_map(|val| val.iter()) - .map(|item| FileChange::Deleted { - path: item.path.to_owned(), - }), - ) - .chain( - raw.modifications - .into_iter() - .map(|item| FileChange::Modified { path: item.path }), - ) - .collect::>(); - - status_entries - } -} - /// Finds the object that contains the contents of a file at a specific commit. fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Result { let repo_dir = repo.work_dir().context("repo has no worktree")?; @@ -364,128 +201,3 @@ fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Resul EntryKind::Blob | EntryKind::BlobExecutable => Ok(tree_entry.object_id()), } } - -fn git_meta_from_path( - path: &Path, - autocrlf: bool, -) -> Result, std::io::Error> { - // Windows doesn't support symlinks. This block runs fine but is just wasting CPU cycles. - #[cfg(not(windows))] - match path.symlink_metadata() { - Ok(meta) => { - if meta.is_symlink() { - let link_content = std::fs::read_link(path)?; - let link_content = link_content.to_string_lossy(); - let link_content = link_content.as_bytes(); - - let mut hasher = sha1::Sha1::default(); - hasher.update(b"blob "); - hasher.update(format!("{}", link_content.len()).as_bytes()); - hasher.update(b"\0"); - hasher.update(link_content); - - let hash: [u8; 20] = hasher.finalize().into(); - - return Ok(Some((FileEntryMode::Link, ObjectId::from(hash)))); - } - } - Err(_) => return Ok(None), - }; - - // Not a symlink for sure from this point - Ok(match path.metadata() { - Ok(meta) => { - if meta.is_file() { - let entry_mode = { - #[cfg(unix)] - { - use std::os::unix::prelude::PermissionsExt; - if meta.permissions().mode() & 0o111 != 0 { - FileEntryMode::BlobExecutable - } else { - FileEntryMode::Blob - } - } - - #[cfg(not(unix))] - { - FileEntryMode::Blob - } - }; - - let oid = { - let mut file = std::fs::File::open(path)?; - - // `git::features::hash::Sha1` doesn't implement `Write` so we use the - // underlying crate directly for max perf. - let mut hasher = sha1::Sha1::default(); - hasher.update(b"blob "); - - if autocrlf { - // When autocrlf is on, we either have to fit the whole file into memory, - // or we read the file twice. Either way is not optimal. How should we - // handle this? - // - // With the current implementation, there's no way we can handle huge files - // that do not fit into memory. Maybe we can set a size limit? Anything - // over a certain size will simply be read twice: once for getting the - // normalized size, and once for the hasher updates? - const BUFFER_SIZE: usize = 8 * 1024; - let mut buffer = [0u8; BUFFER_SIZE]; - - let mut len = file.read(&mut buffer)?; - if content_inspector::inspect(&buffer[..len]) - == content_inspector::ContentType::BINARY - { - // No CRLF handling! We update the part already read + the remaining - // content in the file. - hasher.update(format!("{}", meta.len()).as_bytes()); - hasher.update(b"\0"); - - hasher.update(&buffer[..len]); - std::io::copy(&mut file, &mut hasher)?; - } else { - // It's a text file. CRLF transformation as planned. - let mut normalized_file = Vec::with_capacity(meta.len() as usize); - let mut was_cr = false; - - loop { - buffer[..len].iter().for_each(|byte| { - if was_cr && *byte == b'\n' { - normalized_file.pop(); - } - normalized_file.push(*byte); - was_cr = *byte == b'\r'; - }); - - if len < BUFFER_SIZE { - break; - } - len = file.read(&mut buffer)?; - } - - hasher.update(format!("{}", normalized_file.len()).as_bytes()); - hasher.update(b"\0"); - - hasher.update(&normalized_file); - } - } else { - hasher.update(format!("{}", meta.len()).as_bytes()); - hasher.update(b"\0"); - - std::io::copy(&mut file, &mut hasher)?; - } - - let hash: [u8; 20] = hasher.finalize().into(); - ObjectId::from(hash) - }; - - Some((entry_mode, oid)) - } else { - // It's a non-symlink folder. Git doesn't track folders. Same as deletion. - None - } - } - Err(_) => None, - }) -} diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index e69ddd27785c..35d39bc86b56 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,8 +1,7 @@ +use anyhow::{bail, Result}; use arc_swap::ArcSwap; use std::{path::Path, sync::Arc}; -use anyhow::{bail, Result}; - #[cfg(feature = "git")] pub use git::Git; #[cfg(not(feature = "git"))] From f56ed060ee2252b48f63f29249c4aa6875b68c16 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Mon, 25 Mar 2024 10:37:16 +0000 Subject: [PATCH 04/15] feat: async vcs changed file streaming --- helix-term/src/commands.rs | 37 +---------- helix-term/src/ui/mod.rs | 51 ++++++++++++++- helix-vcs/src/git.rs | 124 +++++++++++++++++++++++-------------- helix-vcs/src/lib.rs | 64 +++++++++++++++++-- 4 files changed, 185 insertions(+), 91 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index d5aabc3cc634..bbe9609cf1df 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -2996,42 +2996,7 @@ fn jumplist_picker(cx: &mut Context) { } fn changed_file_picker(cx: &mut Context) { - let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); - - let entries = match cx.editor.diff_providers.get_changed_files(&cwd) { - Ok(entries) => entries, - Err(err) => { - cx.editor.set_error(format!("{err}")); - return; - } - }; - - let added = cx.editor.theme.get("diff.plus"); - let deleted = cx.editor.theme.get("diff.minus"); - let modified = cx.editor.theme.get("diff.delta"); - - let picker = Picker::new( - entries, - ui::menu::FileChangeData { - cwd, - style_untracked: added, - style_modified: modified, - style_deleted: deleted, - style_renamed: modified, - }, - |cx, meta, action| { - let path_to_open = meta.path(); - if let Err(e) = cx.editor.open(path_to_open, action) { - let err = if let Some(err) = e.source() { - format!("{}", err) - } else { - format!("unable to open \"{}\"", path_to_open.display()) - }; - cx.editor.set_error(err); - } - }, - ) - .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); + let picker = ui::changed_file_picker(cx.editor); cx.push_layer(Box::new(overlaid(picker))); } diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index b5969818cf56..787c53566288 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -19,8 +19,9 @@ use crate::job::{self, Callback}; pub use completion::{Completion, CompletionItem}; pub use editor::EditorView; use helix_stdx::rope; +use helix_vcs::FileChange; pub use markdown::Markdown; -pub use menu::Menu; +pub use menu::{FileChangeData, Menu}; pub use picker::{DynamicPicker, FileLocation, Picker}; pub use popup::Popup; pub use prompt::{Prompt, PromptEvent}; @@ -255,6 +256,54 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> Picker picker } +pub fn changed_file_picker(editor: &helix_view::Editor) -> Picker { + let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); + + let added = editor.theme.get("diff.plus"); + let deleted = editor.theme.get("diff.minus"); + let modified = editor.theme.get("diff.delta"); + + let picker = Picker::new( + Vec::new(), + menu::FileChangeData { + cwd: cwd.clone(), + style_untracked: added, + style_modified: modified, + style_deleted: deleted, + style_renamed: modified, + }, + |cx, meta: &FileChange, action| { + let path_to_open = meta.path(); + if let Err(e) = cx.editor.open(path_to_open, action) { + let err = if let Some(err) = e.source() { + format!("{}", err) + } else { + format!("unable to open \"{}\"", path_to_open.display()) + }; + cx.editor.set_error(err); + } + }, + ) + .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); + let injector = picker.injector(); + + let diff_providers = editor.diff_providers.clone(); + + std::thread::spawn(move || { + // There's no way we can notify the editor since we're in a background thread. This just + // silently fails. + if let Ok(change_iter) = diff_providers.get_changed_files(&cwd) { + // We ignore errors as they could be caused by permission issues etc + for change in change_iter.flatten() { + if injector.push(change).is_err() { + break; + } + } + } + }); + picker +} + pub mod completers { use crate::ui::prompt::Completion; use helix_core::fuzzy::fuzzy_match; diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 10b621180599..606e0325e904 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -2,7 +2,7 @@ use anyhow::{bail, Context, Result}; use arc_swap::ArcSwap; use gix::filter::plumbing::driver::apply::Delay; use std::io::Read; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::Arc; use gix::bstr::ByteSlice; @@ -15,13 +15,20 @@ use gix::status::{ }; use gix::{Commit, ObjectId, Repository, ThreadSafeRepository}; -use crate::{DiffProvider, FileChange}; +use crate::{DiffProvider, DiffProviderImpls, FileChange}; #[cfg(test)] mod test; +#[derive(Clone, Copy)] pub struct Git; +/// An adapter around the status iter from `gix` that outputs [FileChange]. +pub struct StatusIter { + work_dir: PathBuf, + inner: gix::status::index_worktree::Iter, +} + impl Git { fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { // custom open options @@ -69,10 +76,11 @@ impl Git { } /// Emulates the result of running `git status` from the command line. - fn status(repo: &Repository) -> Result> { + fn status(repo: &Repository) -> Result>>> { let work_dir = repo .work_dir() - .ok_or_else(|| anyhow::anyhow!("working tree not found"))?; + .ok_or_else(|| anyhow::anyhow!("working tree not found"))? + .to_path_buf(); // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in our // case to not list new (untracked) files. We could have respected this config if the @@ -83,49 +91,10 @@ impl Git { .untracked_files(UntrackedFiles::Files) .index_worktree_rewrites(Some(Default::default())); - Ok(status_platform - .into_index_worktree_iter(Default::default())? - .map(|item| { - let item = item?; - - Result::, _>::Ok(match item { - Item::Modification { - rela_path, status, .. - } => { - let full_path = work_dir.join(rela_path.to_path()?); - - match status { - EntryStatus::Conflict(_) => { - // We treat conflict as change for now for simplicity - Some(FileChange::Modified { path: full_path }) - } - EntryStatus::Change(change) => match change { - Change::Removed => Some(FileChange::Deleted { path: full_path }), - Change::Modification { .. } => { - Some(FileChange::Modified { path: full_path }) - } - Change::Type | Change::SubmoduleModification(_) => None, - }, - EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, - } - } - Item::DirectoryContents { entry, .. } => Some(FileChange::Untracked { - path: work_dir.join(entry.rela_path.to_path()?), - }), - Item::Rewrite { - source, - dirwalk_entry, - .. - } => Some(FileChange::Renamed { - from_path: work_dir.join(source.rela_path().to_path()?), - to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?), - }), - }) - }) - .collect::>>>()? - .into_iter() - .flatten() - .collect::>()) + Ok(Box::new(StatusIter { + work_dir, + inner: status_platform.into_index_worktree_iter(Default::default())?, + })) } } @@ -179,11 +148,70 @@ impl DiffProvider for Git { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } - fn get_changed_files(&self, cwd: &Path) -> Result> { + fn get_changed_files( + &self, + cwd: &Path, + ) -> Result>>> { Self::status(&Self::open_repo(cwd, None)?.to_thread_local()) } } +impl From for DiffProviderImpls { + fn from(value: Git) -> Self { + DiffProviderImpls::Git(value) + } +} + +impl std::iter::Iterator for StatusIter { + type Item = Result; + + fn next(&mut self) -> Option { + let mapper = |item: std::result::Result| { + Result::>::Ok(match item? { + Item::Modification { + rela_path, status, .. + } => { + let full_path = self.work_dir.join(rela_path.to_path()?); + + match status { + EntryStatus::Conflict(_) => { + // We treat conflict as change for now for simplicity + Some(FileChange::Modified { path: full_path }) + } + EntryStatus::Change(change) => match change { + Change::Removed => Some(FileChange::Deleted { path: full_path }), + Change::Modification { .. } => { + Some(FileChange::Modified { path: full_path }) + } + // No submodule support for now + Change::Type | Change::SubmoduleModification(_) => None, + }, + EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, + } + } + Item::DirectoryContents { entry, .. } => Some(FileChange::Untracked { + path: self.work_dir.join(entry.rela_path.to_path()?), + }), + Item::Rewrite { + source, + dirwalk_entry, + .. + } => Some(FileChange::Renamed { + from_path: self.work_dir.join(source.rela_path().to_path()?), + to_path: self.work_dir.join(dirwalk_entry.rela_path.to_path()?), + }), + }) + }; + + self.inner.by_ref().map(mapper).find_map(|item| match item { + Ok(Some(item)) => Some(Ok(item)), + Err(err) => Some(Err(err)), + // Our mapper returns `None` for gix items that we're not interested in + Ok(None) => None, + }) + } +} + /// Finds the object that contains the contents of a file at a specific commit. fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Result { let repo_dir = repo.work_dir().context("repo has no worktree")?; diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 35d39bc86b56..6357bc35d225 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -23,10 +23,12 @@ pub trait DiffProvider { fn get_current_head_name(&self, file: &Path) -> Result>>>; - fn get_changed_files(&self, cwd: &Path) -> Result>; + fn get_changed_files(&self, cwd: &Path) + -> Result>>>; } #[doc(hidden)] +#[derive(Clone, Copy)] pub struct Dummy; impl DiffProvider for Dummy { fn get_diff_base(&self, _file: &Path) -> Result> { @@ -37,13 +39,23 @@ impl DiffProvider for Dummy { bail!("helix was compiled without git support") } - fn get_changed_files(&self, _cwd: &Path) -> Result> { + fn get_changed_files( + &self, + _cwd: &Path, + ) -> Result>>> { anyhow::bail!("dummy diff provider") } } +impl From for DiffProviderImpls { + fn from(value: Dummy) -> Self { + DiffProviderImpls::Dummy(value) + } +} + +#[derive(Clone)] pub struct DiffProviderRegistry { - providers: Vec>, + providers: Vec, } impl DiffProviderRegistry { @@ -73,7 +85,10 @@ impl DiffProviderRegistry { }) } - pub fn get_changed_files(&self, cwd: &Path) -> Result> { + pub fn get_changed_files( + &self, + cwd: &Path, + ) -> Result>>> { self.providers .iter() .find_map(|provider| provider.get_changed_files(cwd).ok()) @@ -85,8 +100,45 @@ impl Default for DiffProviderRegistry { fn default() -> Self { // currently only git is supported // TODO make this configurable when more providers are added - let git: Box = Box::new(Git); - let providers = vec![git]; + let providers = vec![Git.into()]; DiffProviderRegistry { providers } } } + +/// A union type that includes all types that implement [DiffProvider]. We need this type to allow +/// cloning [DiffProviderRegistry] as `Clone` cannot be used in trait objects (or use `dyn-clone`?). +#[derive(Clone)] +pub enum DiffProviderImpls { + Dummy(Dummy), + #[cfg(feature = "git")] + Git(Git), +} + +impl DiffProvider for DiffProviderImpls { + fn get_diff_base(&self, file: &Path) -> Result> { + match self { + Self::Dummy(inner) => inner.get_diff_base(file), + #[cfg(feature = "git")] + Self::Git(inner) => inner.get_diff_base(file), + } + } + + fn get_current_head_name(&self, file: &Path) -> Result>>> { + match self { + Self::Dummy(inner) => inner.get_current_head_name(file), + #[cfg(feature = "git")] + Self::Git(inner) => inner.get_current_head_name(file), + } + } + + fn get_changed_files( + &self, + cwd: &Path, + ) -> Result>>> { + match self { + Self::Dummy(inner) => inner.get_changed_files(cwd), + #[cfg(feature = "git")] + Self::Git(inner) => inner.get_changed_files(cwd), + } + } +} From 0b630dc798c1b3216b3e05b64a3b1e52061c6e96 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 07:15:34 +0000 Subject: [PATCH 05/15] fix: readability and missing entry status check --- helix-vcs/src/git.rs | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 606e0325e904..1794ef44ba17 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -6,6 +6,8 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use gix::bstr::ByteSlice; +use gix::diff::Rewrites; +use gix::dir::entry::Status; use gix::objs::tree::EntryKind; use gix::sec::trust::DefaultForLevel; use gix::status::{ @@ -82,18 +84,26 @@ impl Git { .ok_or_else(|| anyhow::anyhow!("working tree not found"))? .to_path_buf(); - // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in our - // case to not list new (untracked) files. We could have respected this config if the - // default value weren't `Collapsed` though, as this default value would render the feature - // unusable to many. let status_platform = repo .status(gix::progress::Discard)? + // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in + // our case to not list new (untracked) files. We could have respected this config + // if the default value weren't `Collapsed` though, as this default value would render + // the feature unusable to many. .untracked_files(UntrackedFiles::Files) - .index_worktree_rewrites(Some(Default::default())); + // Turn on file rename detection, which is off by default. + .index_worktree_rewrites(Some(Rewrites { + copies: None, + percentage: Some(0.5), + limit: 1000, + })); + + // No filtering based on path + let empty_patterns = vec![]; Ok(Box::new(StatusIter { work_dir, - inner: status_platform.into_index_worktree_iter(Default::default())?, + inner: status_platform.into_index_worktree_iter(empty_patterns)?, })) } } @@ -189,9 +199,12 @@ impl std::iter::Iterator for StatusIter { EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, } } - Item::DirectoryContents { entry, .. } => Some(FileChange::Untracked { - path: self.work_dir.join(entry.rela_path.to_path()?), - }), + Item::DirectoryContents { entry, .. } => match entry.status { + Status::Untracked => Some(FileChange::Untracked { + path: self.work_dir.join(entry.rela_path.to_path()?), + }), + Status::Pruned | Status::Tracked | Status::Ignored(_) => None, + }, Item::Rewrite { source, dirwalk_entry, From 138846fce6238fe6fed43b966fd34249edbe7048 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 12:53:23 +0000 Subject: [PATCH 06/15] refactor: use external iteration for changed files --- helix-term/src/ui/mod.rs | 21 ++---- helix-vcs/src/git.rs | 144 ++++++++++++++++++++------------------- helix-vcs/src/lib.rs | 64 +++++++++++------ 3 files changed, 124 insertions(+), 105 deletions(-) diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index 787c53566288..1f195bdf71b5 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -287,20 +287,13 @@ pub fn changed_file_picker(editor: &helix_view::Editor) -> Picker { .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); let injector = picker.injector(); - let diff_providers = editor.diff_providers.clone(); - - std::thread::spawn(move || { - // There's no way we can notify the editor since we're in a background thread. This just - // silently fails. - if let Ok(change_iter) = diff_providers.get_changed_files(&cwd) { - // We ignore errors as they could be caused by permission issues etc - for change in change_iter.flatten() { - if injector.push(change).is_err() { - break; - } - } - } - }); + editor.diff_providers.clone().for_each_changed_file( + cwd, + move |change| injector.push(change).is_ok(), + |err| { + helix_event::status::report_blocking(err); + }, + ); picker } diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 1794ef44ba17..3c70cf232943 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -2,7 +2,7 @@ use anyhow::{bail, Context, Result}; use arc_swap::ArcSwap; use gix::filter::plumbing::driver::apply::Delay; use std::io::Read; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::Arc; use gix::bstr::ByteSlice; @@ -25,12 +25,6 @@ mod test; #[derive(Clone, Copy)] pub struct Git; -/// An adapter around the status iter from `gix` that outputs [FileChange]. -pub struct StatusIter { - work_dir: PathBuf, - inner: gix::status::index_worktree::Iter, -} - impl Git { fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { // custom open options @@ -78,12 +72,56 @@ impl Git { } /// Emulates the result of running `git status` from the command line. - fn status(repo: &Repository) -> Result>>> { + fn status(repo: &Repository, on_change: FC, on_err: FE) -> Result<()> + where + FC: Fn(FileChange) -> bool, + FE: Fn(anyhow::Error), + { let work_dir = repo .work_dir() .ok_or_else(|| anyhow::anyhow!("working tree not found"))? .to_path_buf(); + let mapper = |item: std::result::Result| { + Result::>::Ok(match item? { + Item::Modification { + rela_path, status, .. + } => { + let full_path = work_dir.join(rela_path.to_path()?); + + match status { + EntryStatus::Conflict(_) => { + // We treat conflict as change for now for simplicity + Some(FileChange::Modified { path: full_path }) + } + EntryStatus::Change(change) => match change { + Change::Removed => Some(FileChange::Deleted { path: full_path }), + Change::Modification { .. } => { + Some(FileChange::Modified { path: full_path }) + } + // No submodule support for now + Change::Type | Change::SubmoduleModification(_) => None, + }, + EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, + } + } + Item::DirectoryContents { entry, .. } => match entry.status { + Status::Untracked => Some(FileChange::Untracked { + path: work_dir.join(entry.rela_path.to_path()?), + }), + Status::Pruned | Status::Tracked | Status::Ignored(_) => None, + }, + Item::Rewrite { + source, + dirwalk_entry, + .. + } => Some(FileChange::Renamed { + from_path: work_dir.join(source.rela_path().to_path()?), + to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?), + }), + }) + }; + let status_platform = repo .status(gix::progress::Discard)? // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in @@ -101,10 +139,24 @@ impl Git { // No filtering based on path let empty_patterns = vec![]; - Ok(Box::new(StatusIter { - work_dir, - inner: status_platform.into_index_worktree_iter(empty_patterns)?, - })) + let status_iter = status_platform.into_index_worktree_iter(empty_patterns)?; + + for item in status_iter.map(mapper) { + match item { + Ok(Some(item)) => { + if !on_change(item) { + break; + } + } + Err(err) => { + on_err(err); + } + // Our mapper returns `None` for gix items that we're not interested in + Ok(None) => {} + } + } + + Ok(()) } } @@ -158,11 +210,16 @@ impl DiffProvider for Git { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } - fn get_changed_files( - &self, - cwd: &Path, - ) -> Result>>> { - Self::status(&Self::open_repo(cwd, None)?.to_thread_local()) + fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> + where + FC: Fn(FileChange) -> bool, + FE: Fn(anyhow::Error), + { + Self::status( + &Self::open_repo(cwd, None)?.to_thread_local(), + on_change, + on_err, + ) } } @@ -172,59 +229,6 @@ impl From for DiffProviderImpls { } } -impl std::iter::Iterator for StatusIter { - type Item = Result; - - fn next(&mut self) -> Option { - let mapper = |item: std::result::Result| { - Result::>::Ok(match item? { - Item::Modification { - rela_path, status, .. - } => { - let full_path = self.work_dir.join(rela_path.to_path()?); - - match status { - EntryStatus::Conflict(_) => { - // We treat conflict as change for now for simplicity - Some(FileChange::Modified { path: full_path }) - } - EntryStatus::Change(change) => match change { - Change::Removed => Some(FileChange::Deleted { path: full_path }), - Change::Modification { .. } => { - Some(FileChange::Modified { path: full_path }) - } - // No submodule support for now - Change::Type | Change::SubmoduleModification(_) => None, - }, - EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, - } - } - Item::DirectoryContents { entry, .. } => match entry.status { - Status::Untracked => Some(FileChange::Untracked { - path: self.work_dir.join(entry.rela_path.to_path()?), - }), - Status::Pruned | Status::Tracked | Status::Ignored(_) => None, - }, - Item::Rewrite { - source, - dirwalk_entry, - .. - } => Some(FileChange::Renamed { - from_path: self.work_dir.join(source.rela_path().to_path()?), - to_path: self.work_dir.join(dirwalk_entry.rela_path.to_path()?), - }), - }) - }; - - self.inner.by_ref().map(mapper).find_map(|item| match item { - Ok(Some(item)) => Some(Ok(item)), - Err(err) => Some(Err(err)), - // Our mapper returns `None` for gix items that we're not interested in - Ok(None) => None, - }) - } -} - /// Finds the object that contains the contents of a file at a specific commit. fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Result { let repo_dir = repo.work_dir().context("repo has no worktree")?; diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 6357bc35d225..855bb21f94cc 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,6 +1,9 @@ use anyhow::{bail, Result}; use arc_swap::ArcSwap; -use std::{path::Path, sync::Arc}; +use std::{ + path::{Path, PathBuf}, + sync::Arc, +}; #[cfg(feature = "git")] pub use git::Git; @@ -23,8 +26,12 @@ pub trait DiffProvider { fn get_current_head_name(&self, file: &Path) -> Result>>>; - fn get_changed_files(&self, cwd: &Path) - -> Result>>>; + /// Returns `Err` in case of an _initialization_ failure. Iteration errors must be reported via + /// `on_err` instead. + fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> + where + FC: Fn(FileChange) -> bool, + FE: Fn(anyhow::Error); } #[doc(hidden)] @@ -39,10 +46,11 @@ impl DiffProvider for Dummy { bail!("helix was compiled without git support") } - fn get_changed_files( - &self, - _cwd: &Path, - ) -> Result>>> { + fn for_each_changed_file(&self, _cwd: &Path, _on_item: FC, _on_err: FE) -> Result<()> + where + FC: Fn(FileChange) -> bool, + FE: Fn(anyhow::Error), + { anyhow::bail!("dummy diff provider") } } @@ -85,14 +93,27 @@ impl DiffProviderRegistry { }) } - pub fn get_changed_files( - &self, - cwd: &Path, - ) -> Result>>> { - self.providers - .iter() - .find_map(|provider| provider.get_changed_files(cwd).ok()) - .ok_or_else(|| anyhow::anyhow!("no diff provider returns success")) + /// Fire-and-forget changed file iteration. Runs everything in a background task. Keeps + /// iteration until `on_change` returns `false`. + pub fn for_each_changed_file(self, cwd: PathBuf, on_change: FC, on_err: FE) + where + FC: Fn(FileChange) -> bool + Clone + Send + 'static, + FE: Fn(anyhow::Error) + Clone + Send + 'static, + { + tokio::task::spawn_blocking(move || { + if self + .providers + .iter() + .find_map(|provider| { + provider + .for_each_changed_file(&cwd, on_change.clone(), on_err.clone()) + .ok() + }) + .is_none() + { + on_err(anyhow::anyhow!("no diff provider returns success")) + } + }); } } @@ -131,14 +152,15 @@ impl DiffProvider for DiffProviderImpls { } } - fn get_changed_files( - &self, - cwd: &Path, - ) -> Result>>> { + fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> + where + FC: Fn(FileChange) -> bool, + FE: Fn(anyhow::Error), + { match self { - Self::Dummy(inner) => inner.get_changed_files(cwd), + Self::Dummy(inner) => inner.for_each_changed_file(cwd, on_change, on_err), #[cfg(feature = "git")] - Self::Git(inner) => inner.get_changed_files(cwd), + Self::Git(inner) => inner.for_each_changed_file(cwd, on_change, on_err), } } } From 3b50c76052a6862ded6b1e88ffe99792c1dc7614 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 13:36:55 +0000 Subject: [PATCH 07/15] refactor: move changed file picker to `commands` --- helix-term/src/commands.rs | 40 ++++++++++++++++++++++++++++++++++-- helix-term/src/ui/mod.rs | 42 -------------------------------------- 2 files changed, 38 insertions(+), 44 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index bbe9609cf1df..f3b57d53ee3e 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -4,7 +4,7 @@ pub(crate) mod typed; pub use dap::*; use helix_stdx::rope::{self, RopeSliceExt}; -use helix_vcs::Hunk; +use helix_vcs::{FileChange, Hunk}; pub use lsp::*; use tui::widgets::Row; pub use typed::*; @@ -2996,7 +2996,43 @@ fn jumplist_picker(cx: &mut Context) { } fn changed_file_picker(cx: &mut Context) { - let picker = ui::changed_file_picker(cx.editor); + let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); + + let added = cx.editor.theme.get("diff.plus"); + let deleted = cx.editor.theme.get("diff.minus"); + let modified = cx.editor.theme.get("diff.delta"); + + let picker = Picker::new( + Vec::new(), + ui::menu::FileChangeData { + cwd: cwd.clone(), + style_untracked: added, + style_modified: modified, + style_deleted: deleted, + style_renamed: modified, + }, + |cx, meta: &FileChange, action| { + let path_to_open = meta.path(); + if let Err(e) = cx.editor.open(path_to_open, action) { + let err = if let Some(err) = e.source() { + format!("{}", err) + } else { + format!("unable to open \"{}\"", path_to_open.display()) + }; + cx.editor.set_error(err); + } + }, + ) + .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); + let injector = picker.injector(); + + cx.editor.diff_providers.clone().for_each_changed_file( + cwd, + move |change| injector.push(change).is_ok(), + |err| { + helix_event::status::report_blocking(err); + }, + ); cx.push_layer(Box::new(overlaid(picker))); } diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index 1f195bdf71b5..a7a8f9cc5990 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -19,7 +19,6 @@ use crate::job::{self, Callback}; pub use completion::{Completion, CompletionItem}; pub use editor::EditorView; use helix_stdx::rope; -use helix_vcs::FileChange; pub use markdown::Markdown; pub use menu::{FileChangeData, Menu}; pub use picker::{DynamicPicker, FileLocation, Picker}; @@ -256,47 +255,6 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> Picker picker } -pub fn changed_file_picker(editor: &helix_view::Editor) -> Picker { - let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); - - let added = editor.theme.get("diff.plus"); - let deleted = editor.theme.get("diff.minus"); - let modified = editor.theme.get("diff.delta"); - - let picker = Picker::new( - Vec::new(), - menu::FileChangeData { - cwd: cwd.clone(), - style_untracked: added, - style_modified: modified, - style_deleted: deleted, - style_renamed: modified, - }, - |cx, meta: &FileChange, action| { - let path_to_open = meta.path(); - if let Err(e) = cx.editor.open(path_to_open, action) { - let err = if let Some(err) = e.source() { - format!("{}", err) - } else { - format!("unable to open \"{}\"", path_to_open.display()) - }; - cx.editor.set_error(err); - } - }, - ) - .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); - let injector = picker.injector(); - - editor.diff_providers.clone().for_each_changed_file( - cwd, - move |change| injector.push(change).is_ok(), - |err| { - helix_event::status::report_blocking(err); - }, - ); - picker -} - pub mod completers { use crate::ui::prompt::Completion; use helix_core::fuzzy::fuzzy_match; From 2afb4ffdc54213e8cb428d0cdb0c35fae2febb01 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 14:33:45 +0000 Subject: [PATCH 08/15] feat: only style signs for the changed file picker --- helix-term/src/ui/menu.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index 1b65421498e1..cd43619fa5f7 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -9,6 +9,7 @@ use nucleo::pattern::{Atom, AtomKind, CaseMatching}; use nucleo::{Config, Utf32Str}; use tui::{ buffer::Buffer as Surface, + text::Span, widgets::{Block, Borders, Table, Widget}, }; @@ -84,7 +85,10 @@ impl Item for FileChange { ), }; - Row::new(vec![sign.to_owned(), content]).style(style) + Row::new(vec![ + Cell::from(Span::styled(sign, style)), + Cell::from(content), + ]) } } From 4375d17c8ddfdc024ab719c4c7b2685ed432ab3b Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 14:42:38 +0000 Subject: [PATCH 09/15] feat: add conflict file change type --- helix-term/src/commands.rs | 1 + helix-term/src/ui/menu.rs | 2 ++ helix-vcs/src/diff.rs | 28 ---------------------------- helix-vcs/src/git.rs | 5 +---- helix-vcs/src/lib.rs | 6 +++++- helix-vcs/src/status.rs | 32 ++++++++++++++++++++++++++++++++ 6 files changed, 41 insertions(+), 33 deletions(-) create mode 100644 helix-vcs/src/status.rs diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index f3b57d53ee3e..762c813eb326 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -3008,6 +3008,7 @@ fn changed_file_picker(cx: &mut Context) { cwd: cwd.clone(), style_untracked: added, style_modified: modified, + style_conflict: deleted, style_deleted: deleted, style_renamed: modified, }, diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index cd43619fa5f7..694b9e54ecea 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -59,6 +59,7 @@ pub struct FileChangeData { pub cwd: PathBuf, pub style_untracked: Style, pub style_modified: Style, + pub style_conflict: Style, pub style_deleted: Style, pub style_renamed: Style, } @@ -77,6 +78,7 @@ impl Item for FileChange { let (sign, style, content) = match self { Self::Untracked { path } => ("[+]", data.style_untracked, process_path(path)), Self::Modified { path } => ("[~]", data.style_modified, process_path(path)), + Self::Conflict { path } => ("[x]", data.style_conflict, process_path(path)), Self::Deleted { path } => ("[-]", data.style_deleted, process_path(path)), Self::Renamed { from_path, to_path } => ( "[>]", diff --git a/helix-vcs/src/diff.rs b/helix-vcs/src/diff.rs index 8a9a6e8ecbf7..c72deb7ea7f3 100644 --- a/helix-vcs/src/diff.rs +++ b/helix-vcs/src/diff.rs @@ -1,5 +1,4 @@ use std::ops::Range; -use std::path::{Path, PathBuf}; use std::sync::Arc; use helix_core::Rope; @@ -291,30 +290,3 @@ impl Diff<'_> { } } } - -pub enum FileChange { - Untracked { - path: PathBuf, - }, - Modified { - path: PathBuf, - }, - Deleted { - path: PathBuf, - }, - Renamed { - from_path: PathBuf, - to_path: PathBuf, - }, -} - -impl FileChange { - pub fn path(&self) -> &Path { - match self { - Self::Untracked { path } => path, - Self::Modified { path } => path, - Self::Deleted { path } => path, - Self::Renamed { to_path, .. } => to_path, - } - } -} diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 3c70cf232943..3fe985d4a6f1 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -90,10 +90,7 @@ impl Git { let full_path = work_dir.join(rela_path.to_path()?); match status { - EntryStatus::Conflict(_) => { - // We treat conflict as change for now for simplicity - Some(FileChange::Modified { path: full_path }) - } + EntryStatus::Conflict(_) => Some(FileChange::Conflict { path: full_path }), EntryStatus::Change(change) => match change { Change::Removed => Some(FileChange::Deleted { path: full_path }), Change::Modification { .. } => { diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 855bb21f94cc..464eade38530 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -15,7 +15,11 @@ mod git; mod diff; -pub use diff::{DiffHandle, FileChange, Hunk}; +pub use diff::{DiffHandle, Hunk}; + +mod status; + +pub use status::FileChange; pub trait DiffProvider { /// Returns the data that a diff should be computed against diff --git a/helix-vcs/src/status.rs b/helix-vcs/src/status.rs new file mode 100644 index 000000000000..f34334909554 --- /dev/null +++ b/helix-vcs/src/status.rs @@ -0,0 +1,32 @@ +use std::path::{Path, PathBuf}; + +pub enum FileChange { + Untracked { + path: PathBuf, + }, + Modified { + path: PathBuf, + }, + Conflict { + path: PathBuf, + }, + Deleted { + path: PathBuf, + }, + Renamed { + from_path: PathBuf, + to_path: PathBuf, + }, +} + +impl FileChange { + pub fn path(&self) -> &Path { + match self { + Self::Untracked { path } => path, + Self::Modified { path } => path, + Self::Conflict { path } => path, + Self::Deleted { path } => path, + Self::Renamed { to_path, .. } => to_path, + } + } +} From 3c63c59318d690958ee0ecacea972e6518e29b9f Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 15:02:46 +0000 Subject: [PATCH 10/15] fix: use proper theme keys for changed file picker --- book/src/themes.md | 1 + helix-term/src/commands.rs | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/book/src/themes.md b/book/src/themes.md index 29a8c4ba82e8..0a49053f5921 100644 --- a/book/src/themes.md +++ b/book/src/themes.md @@ -251,6 +251,7 @@ We use a similar set of scopes as - `gutter` - gutter indicator - `delta` - modifications - `moved` - renamed or moved files/changes + - `conflict` - merge conflicts - `gutter` - gutter indicator #### Interface diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 762c813eb326..611fce91308e 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -2999,8 +2999,10 @@ fn changed_file_picker(cx: &mut Context) { let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); let added = cx.editor.theme.get("diff.plus"); - let deleted = cx.editor.theme.get("diff.minus"); let modified = cx.editor.theme.get("diff.delta"); + let conflict = cx.editor.theme.get("diff.delta.conflict"); + let deleted = cx.editor.theme.get("diff.minus"); + let renamed = cx.editor.theme.get("diff.delta.moved"); let picker = Picker::new( Vec::new(), @@ -3008,9 +3010,9 @@ fn changed_file_picker(cx: &mut Context) { cwd: cwd.clone(), style_untracked: added, style_modified: modified, - style_conflict: deleted, + style_conflict: conflict, style_deleted: deleted, - style_renamed: modified, + style_renamed: renamed, }, |cx, meta: &FileChange, action| { let path_to_open = meta.path(); From 7fd75b50409704343399c60ec15a9306dafe7370 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Tue, 26 Mar 2024 16:57:34 +0000 Subject: [PATCH 11/15] refactor: move changed file picker data def --- helix-term/src/commands.rs | 49 +++++++++++++++++++++++++++++++++++--- helix-term/src/ui/menu.rs | 42 -------------------------------- helix-term/src/ui/mod.rs | 2 +- 3 files changed, 47 insertions(+), 46 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 611fce91308e..53eea28de435 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -6,7 +6,10 @@ pub use dap::*; use helix_stdx::rope::{self, RopeSliceExt}; use helix_vcs::{FileChange, Hunk}; pub use lsp::*; -use tui::widgets::Row; +use tui::{ + text::Span, + widgets::{Cell, Row}, +}; pub use typed::*; use helix_core::{ @@ -39,6 +42,7 @@ use helix_view::{ info::Info, input::KeyEvent, keyboard::KeyCode, + theme::Style, tree, view::View, Document, DocumentId, Editor, ViewId, @@ -54,7 +58,7 @@ use crate::{ filter_picker_entry, job::Callback, keymap::ReverseKeymap, - ui::{self, overlay::overlaid, Picker, Popup, Prompt, PromptEvent}, + ui::{self, menu::Item, overlay::overlaid, Picker, Popup, Prompt, PromptEvent}, }; use crate::job::{self, Jobs}; @@ -2996,6 +3000,45 @@ fn jumplist_picker(cx: &mut Context) { } fn changed_file_picker(cx: &mut Context) { + pub struct FileChangeData { + cwd: PathBuf, + style_untracked: Style, + style_modified: Style, + style_conflict: Style, + style_deleted: Style, + style_renamed: Style, + } + + impl Item for FileChange { + type Data = FileChangeData; + + fn format(&self, data: &Self::Data) -> Row { + let process_path = |path: &PathBuf| { + path.strip_prefix(&data.cwd) + .unwrap_or(path) + .display() + .to_string() + }; + + let (sign, style, content) = match self { + Self::Untracked { path } => ("[+]", data.style_untracked, process_path(path)), + Self::Modified { path } => ("[~]", data.style_modified, process_path(path)), + Self::Conflict { path } => ("[x]", data.style_conflict, process_path(path)), + Self::Deleted { path } => ("[-]", data.style_deleted, process_path(path)), + Self::Renamed { from_path, to_path } => ( + "[>]", + data.style_renamed, + format!("{} -> {}", process_path(from_path), process_path(to_path)), + ), + }; + + Row::new(vec![ + Cell::from(Span::styled(sign, style)), + Cell::from(content), + ]) + } + } + let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); let added = cx.editor.theme.get("diff.plus"); @@ -3006,7 +3049,7 @@ fn changed_file_picker(cx: &mut Context) { let picker = Picker::new( Vec::new(), - ui::menu::FileChangeData { + FileChangeData { cwd: cwd.clone(), style_untracked: added, style_modified: modified, diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index 694b9e54ecea..c0e60b33e344 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -9,17 +9,14 @@ use nucleo::pattern::{Atom, AtomKind, CaseMatching}; use nucleo::{Config, Utf32Str}; use tui::{ buffer::Buffer as Surface, - text::Span, widgets::{Block, Borders, Table, Widget}, }; pub use tui::widgets::{Cell, Row}; -use helix_vcs::FileChange; use helix_view::{ editor::SmartTabConfig, graphics::{Margin, Rect}, - theme::Style, Editor, }; use tui::layout::Constraint; @@ -55,45 +52,6 @@ impl Item for PathBuf { pub type MenuCallback = Box, MenuEvent)>; -pub struct FileChangeData { - pub cwd: PathBuf, - pub style_untracked: Style, - pub style_modified: Style, - pub style_conflict: Style, - pub style_deleted: Style, - pub style_renamed: Style, -} - -impl Item for FileChange { - type Data = FileChangeData; - - fn format(&self, data: &Self::Data) -> Row { - let process_path = |path: &PathBuf| { - path.strip_prefix(&data.cwd) - .unwrap_or(path) - .display() - .to_string() - }; - - let (sign, style, content) = match self { - Self::Untracked { path } => ("[+]", data.style_untracked, process_path(path)), - Self::Modified { path } => ("[~]", data.style_modified, process_path(path)), - Self::Conflict { path } => ("[x]", data.style_conflict, process_path(path)), - Self::Deleted { path } => ("[-]", data.style_deleted, process_path(path)), - Self::Renamed { from_path, to_path } => ( - "[>]", - data.style_modified, - format!("{} -> {}", process_path(from_path), process_path(to_path)), - ), - }; - - Row::new(vec![ - Cell::from(Span::styled(sign, style)), - Cell::from(content), - ]) - } -} - pub struct Menu { options: Vec, editor_data: T::Data, diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index a7a8f9cc5990..b5969818cf56 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -20,7 +20,7 @@ pub use completion::{Completion, CompletionItem}; pub use editor::EditorView; use helix_stdx::rope; pub use markdown::Markdown; -pub use menu::{FileChangeData, Menu}; +pub use menu::Menu; pub use picker::{DynamicPicker, FileLocation, Picker}; pub use popup::Popup; pub use prompt::{Prompt, PromptEvent}; From e138982dff3d809499f43a4370db087a115f0739 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Wed, 27 Mar 2024 22:42:37 +0800 Subject: [PATCH 12/15] refactor: remove allocation on Row::new Co-authored-by: Michael Davis --- helix-term/src/commands.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 53eea28de435..d9fc92aadd82 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -3032,7 +3032,7 @@ fn changed_file_picker(cx: &mut Context) { ), }; - Row::new(vec![ + Row::new([ Cell::from(Span::styled(sign, style)), Cell::from(content), ]) From 661d772ddcf5082e1d97bdb176f52bbcd7cda864 Mon Sep 17 00:00:00 2001 From: Jonathan LEI Date: Wed, 27 Mar 2024 14:46:58 +0000 Subject: [PATCH 13/15] fix: use `helix_stdx::env::current_working_dir` --- helix-term/src/commands.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index d9fc92aadd82..cbbb54f00d85 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -3032,14 +3032,16 @@ fn changed_file_picker(cx: &mut Context) { ), }; - Row::new([ - Cell::from(Span::styled(sign, style)), - Cell::from(content), - ]) + Row::new([Cell::from(Span::styled(sign, style)), Cell::from(content)]) } } - let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("./")); + let cwd = helix_stdx::env::current_working_dir(); + if !cwd.exists() { + cx.editor + .set_error("Current working directory does not exist"); + return; + } let added = cx.editor.theme.get("diff.plus"); let modified = cx.editor.theme.get("diff.delta"); From 4cd2cfef47ae74215ce2f25e39923aeea5b8cb7c Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 31 Mar 2024 21:37:18 +0200 Subject: [PATCH 14/15] minor cleanup --- helix-term/src/commands.rs | 18 ++++--- helix-vcs/src/git.rs | 100 +++++++++++++++---------------------- helix-vcs/src/lib.rs | 57 ++++++++++----------- 3 files changed, 77 insertions(+), 98 deletions(-) diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index cbbb54f00d85..2eb931dec162 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -3,6 +3,7 @@ pub(crate) mod lsp; pub(crate) mod typed; pub use dap::*; +use helix_event::status; use helix_stdx::rope::{self, RopeSliceExt}; use helix_vcs::{FileChange, Hunk}; pub use lsp::*; @@ -3074,13 +3075,16 @@ fn changed_file_picker(cx: &mut Context) { .with_preview(|_editor, meta| Some((meta.path().to_path_buf().into(), None))); let injector = picker.injector(); - cx.editor.diff_providers.clone().for_each_changed_file( - cwd, - move |change| injector.push(change).is_ok(), - |err| { - helix_event::status::report_blocking(err); - }, - ); + cx.editor + .diff_providers + .clone() + .for_each_changed_file(cwd, move |change| match change { + Ok(change) => injector.push(change).is_ok(), + Err(err) => { + status::report_blocking(err); + true + } + }); cx.push_layer(Box::new(overlaid(picker))); } diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 3fe985d4a6f1..1ab5e5f71c7d 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -72,53 +72,12 @@ impl Git { } /// Emulates the result of running `git status` from the command line. - fn status(repo: &Repository, on_change: FC, on_err: FE) -> Result<()> - where - FC: Fn(FileChange) -> bool, - FE: Fn(anyhow::Error), - { + fn status(repo: &Repository, f: impl Fn(Result) -> bool) -> Result<()> { let work_dir = repo .work_dir() .ok_or_else(|| anyhow::anyhow!("working tree not found"))? .to_path_buf(); - let mapper = |item: std::result::Result| { - Result::>::Ok(match item? { - Item::Modification { - rela_path, status, .. - } => { - let full_path = work_dir.join(rela_path.to_path()?); - - match status { - EntryStatus::Conflict(_) => Some(FileChange::Conflict { path: full_path }), - EntryStatus::Change(change) => match change { - Change::Removed => Some(FileChange::Deleted { path: full_path }), - Change::Modification { .. } => { - Some(FileChange::Modified { path: full_path }) - } - // No submodule support for now - Change::Type | Change::SubmoduleModification(_) => None, - }, - EntryStatus::NeedsUpdate(_) | EntryStatus::IntentToAdd => None, - } - } - Item::DirectoryContents { entry, .. } => match entry.status { - Status::Untracked => Some(FileChange::Untracked { - path: work_dir.join(entry.rela_path.to_path()?), - }), - Status::Pruned | Status::Tracked | Status::Ignored(_) => None, - }, - Item::Rewrite { - source, - dirwalk_entry, - .. - } => Some(FileChange::Renamed { - from_path: work_dir.join(source.rela_path().to_path()?), - to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?), - }), - }) - }; - let status_platform = repo .status(gix::progress::Discard)? // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in @@ -138,18 +97,41 @@ impl Git { let status_iter = status_platform.into_index_worktree_iter(empty_patterns)?; - for item in status_iter.map(mapper) { - match item { - Ok(Some(item)) => { - if !on_change(item) { - break; + for item in status_iter { + let Ok(item) = item.map_err(|err| f(Err(err.into()))) else { + continue; + }; + let change = match item { + Item::Modification { + rela_path, status, .. + } => { + let path = work_dir.join(rela_path.to_path()?); + match status { + EntryStatus::Conflict(_) => FileChange::Conflict { path }, + EntryStatus::Change(Change::Removed) => FileChange::Deleted { path }, + EntryStatus::Change(Change::Modification { .. }) => { + FileChange::Modified { path } + } + _ => continue, } } - Err(err) => { - on_err(err); + Item::DirectoryContents { entry, .. } if entry.status == Status::Untracked => { + FileChange::Untracked { + path: work_dir.join(entry.rela_path.to_path()?), + } } - // Our mapper returns `None` for gix items that we're not interested in - Ok(None) => {} + Item::Rewrite { + source, + dirwalk_entry, + .. + } => FileChange::Renamed { + from_path: work_dir.join(source.rela_path().to_path()?), + to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?), + }, + _ => continue, + }; + if !f(Ok(change)) { + break; } } @@ -207,16 +189,12 @@ impl DiffProvider for Git { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } - fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> - where - FC: Fn(FileChange) -> bool, - FE: Fn(anyhow::Error), - { - Self::status( - &Self::open_repo(cwd, None)?.to_thread_local(), - on_change, - on_err, - ) + fn for_each_changed_file( + &self, + cwd: &Path, + f: impl Fn(Result) -> bool, + ) -> Result<()> { + Self::status(&Self::open_repo(cwd, None)?.to_thread_local(), f) } } diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 464eade38530..dd532248079e 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,4 +1,4 @@ -use anyhow::{bail, Result}; +use anyhow::{anyhow, bail, Result}; use arc_swap::ArcSwap; use std::{ path::{Path, PathBuf}, @@ -32,10 +32,11 @@ pub trait DiffProvider { /// Returns `Err` in case of an _initialization_ failure. Iteration errors must be reported via /// `on_err` instead. - fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> - where - FC: Fn(FileChange) -> bool, - FE: Fn(anyhow::Error); + fn for_each_changed_file( + &self, + cwd: &Path, + f: impl Fn(Result) -> bool, + ) -> Result<()>; } #[doc(hidden)] @@ -50,12 +51,12 @@ impl DiffProvider for Dummy { bail!("helix was compiled without git support") } - fn for_each_changed_file(&self, _cwd: &Path, _on_item: FC, _on_err: FE) -> Result<()> - where - FC: Fn(FileChange) -> bool, - FE: Fn(anyhow::Error), - { - anyhow::bail!("dummy diff provider") + fn for_each_changed_file( + &self, + _cwd: &Path, + _f: impl Fn(Result) -> bool, + ) -> Result<()> { + bail!("helix was compiled without git support") } } @@ -99,23 +100,19 @@ impl DiffProviderRegistry { /// Fire-and-forget changed file iteration. Runs everything in a background task. Keeps /// iteration until `on_change` returns `false`. - pub fn for_each_changed_file(self, cwd: PathBuf, on_change: FC, on_err: FE) - where - FC: Fn(FileChange) -> bool + Clone + Send + 'static, - FE: Fn(anyhow::Error) + Clone + Send + 'static, - { + pub fn for_each_changed_file( + self, + cwd: PathBuf, + f: impl Fn(Result) -> bool + Send + 'static, + ) { tokio::task::spawn_blocking(move || { if self .providers .iter() - .find_map(|provider| { - provider - .for_each_changed_file(&cwd, on_change.clone(), on_err.clone()) - .ok() - }) + .find_map(|provider| provider.for_each_changed_file(&cwd, &f).ok()) .is_none() { - on_err(anyhow::anyhow!("no diff provider returns success")) + f(Err(anyhow!("no diff provider returns success"))); } }); } @@ -131,7 +128,7 @@ impl Default for DiffProviderRegistry { } /// A union type that includes all types that implement [DiffProvider]. We need this type to allow -/// cloning [DiffProviderRegistry] as `Clone` cannot be used in trait objects (or use `dyn-clone`?). +/// cloning [DiffProviderRegistry] as `Clone` cannot be used in trait objects. #[derive(Clone)] pub enum DiffProviderImpls { Dummy(Dummy), @@ -156,15 +153,15 @@ impl DiffProvider for DiffProviderImpls { } } - fn for_each_changed_file(&self, cwd: &Path, on_change: FC, on_err: FE) -> Result<()> - where - FC: Fn(FileChange) -> bool, - FE: Fn(anyhow::Error), - { + fn for_each_changed_file( + &self, + cwd: &Path, + f: impl Fn(Result) -> bool, + ) -> Result<()> { match self { - Self::Dummy(inner) => inner.for_each_changed_file(cwd, on_change, on_err), + Self::Dummy(inner) => inner.for_each_changed_file(cwd, f), #[cfg(feature = "git")] - Self::Git(inner) => inner.for_each_changed_file(cwd, on_change, on_err), + Self::Git(inner) => inner.for_each_changed_file(cwd, f), } } } From cb92c983552cf07b953fca53e261701a20f4d205 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 31 Mar 2024 21:39:38 +0200 Subject: [PATCH 15/15] remove trait object in favor of enum --- helix-vcs/src/git.rs | 14 +++++++------- helix-vcs/src/git/test.rs | 2 +- helix-vcs/src/lib.rs | 30 ++++++------------------------ 3 files changed, 14 insertions(+), 32 deletions(-) diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 1ab5e5f71c7d..8d935b5fce53 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -17,7 +17,7 @@ use gix::status::{ }; use gix::{Commit, ObjectId, Repository, ThreadSafeRepository}; -use crate::{DiffProvider, DiffProviderImpls, FileChange}; +use crate::{DiffProvider, FileChange}; #[cfg(test)] mod test; @@ -139,8 +139,8 @@ impl Git { } } -impl DiffProvider for Git { - fn get_diff_base(&self, file: &Path) -> Result> { +impl Git { + pub fn get_diff_base(&self, file: &Path) -> Result> { debug_assert!(!file.exists() || file.is_file()); debug_assert!(file.is_absolute()); @@ -171,7 +171,7 @@ impl DiffProvider for Git { } } - fn get_current_head_name(&self, file: &Path) -> Result>>> { + pub fn get_current_head_name(&self, file: &Path) -> Result>>> { debug_assert!(!file.exists() || file.is_file()); debug_assert!(file.is_absolute()); let repo_dir = file.parent().context("file has no parent directory")?; @@ -189,7 +189,7 @@ impl DiffProvider for Git { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } - fn for_each_changed_file( + pub fn for_each_changed_file( &self, cwd: &Path, f: impl Fn(Result) -> bool, @@ -198,9 +198,9 @@ impl DiffProvider for Git { } } -impl From for DiffProviderImpls { +impl From for DiffProvider { fn from(value: Git) -> Self { - DiffProviderImpls::Git(value) + DiffProvider::Git(value) } } diff --git a/helix-vcs/src/git/test.rs b/helix-vcs/src/git/test.rs index 9c67d2c331de..0f928204a354 100644 --- a/helix-vcs/src/git/test.rs +++ b/helix-vcs/src/git/test.rs @@ -2,7 +2,7 @@ use std::{fs::File, io::Write, path::Path, process::Command}; use tempfile::TempDir; -use crate::{DiffProvider, Git}; +use crate::Git; fn exec_git_cmd(args: &str, git_dir: &Path) { let res = Command::new("git") diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index dd532248079e..38378fc271a1 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -21,28 +21,10 @@ mod status; pub use status::FileChange; -pub trait DiffProvider { - /// Returns the data that a diff should be computed against - /// if this provider is used. - /// The data is returned as raw byte without any decoding or encoding performed - /// to ensure all file encodings are handled correctly. - fn get_diff_base(&self, file: &Path) -> Result>; - - fn get_current_head_name(&self, file: &Path) -> Result>>>; - - /// Returns `Err` in case of an _initialization_ failure. Iteration errors must be reported via - /// `on_err` instead. - fn for_each_changed_file( - &self, - cwd: &Path, - f: impl Fn(Result) -> bool, - ) -> Result<()>; -} - #[doc(hidden)] #[derive(Clone, Copy)] pub struct Dummy; -impl DiffProvider for Dummy { +impl Dummy { fn get_diff_base(&self, _file: &Path) -> Result> { bail!("helix was compiled without git support") } @@ -60,15 +42,15 @@ impl DiffProvider for Dummy { } } -impl From for DiffProviderImpls { +impl From for DiffProvider { fn from(value: Dummy) -> Self { - DiffProviderImpls::Dummy(value) + DiffProvider::Dummy(value) } } #[derive(Clone)] pub struct DiffProviderRegistry { - providers: Vec, + providers: Vec, } impl DiffProviderRegistry { @@ -130,13 +112,13 @@ impl Default for DiffProviderRegistry { /// A union type that includes all types that implement [DiffProvider]. We need this type to allow /// cloning [DiffProviderRegistry] as `Clone` cannot be used in trait objects. #[derive(Clone)] -pub enum DiffProviderImpls { +pub enum DiffProvider { Dummy(Dummy), #[cfg(feature = "git")] Git(Git), } -impl DiffProvider for DiffProviderImpls { +impl DiffProvider { fn get_diff_base(&self, file: &Path) -> Result> { match self { Self::Dummy(inner) => inner.get_diff_base(file),