From ec7b62e551f88091ae5685228b9fe5138d26860a Mon Sep 17 00:00:00 2001 From: "Alexis (Poliorcetics) Bourget" Date: Sat, 27 Apr 2024 19:49:45 +0200 Subject: [PATCH 1/2] feat: Diff source caching and autodetection --- Cargo.lock | 11 ++ helix-term/src/commands.rs | 22 ++- helix-term/src/commands/typed.rs | 6 +- helix-vcs/Cargo.toml | 2 +- helix-vcs/src/git.rs | 39 ++--- helix-vcs/src/git/test.rs | 30 +++- helix-vcs/src/lib.rs | 262 ++++++++++++++++++++++++------- helix-view/src/document.rs | 4 +- helix-view/src/editor.rs | 6 + 9 files changed, 277 insertions(+), 105 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 86dace16ccd8..4b6551f31fdc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -211,6 +211,15 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "crossbeam-channel" +version = "0.5.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "33480d6946193aa8033910124896ca395333cae7e2d1113d1fef6c3272217df2" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-deque" version = "0.8.5" @@ -710,12 +719,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8e0eb9efdf96c35c0bed7596d1bef2d4ce6360a1d09738001f9d3e402aa7ba3e" dependencies = [ "crc32fast", + "crossbeam-channel", "flate2", "gix-hash", "gix-trace", "gix-utils", "libc", "once_cell", + "parking_lot", "prodash", "sha1_smol", "thiserror", diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index ee2949fa0f05..7f8003306093 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -71,6 +71,7 @@ use std::{ future::Future, io::Read, num::NonZeroUsize, + sync::Arc, }; use std::{ @@ -3109,7 +3110,7 @@ fn jumplist_picker(cx: &mut Context) { fn changed_file_picker(cx: &mut Context) { pub struct FileChangeData { - cwd: PathBuf, + cwd: Arc, style_untracked: Style, style_modified: Style, style_conflict: Style, @@ -3117,7 +3118,7 @@ fn changed_file_picker(cx: &mut Context) { style_renamed: Style, } - let cwd = helix_stdx::env::current_working_dir(); + let cwd: Arc = Arc::from(helix_stdx::env::current_working_dir().as_path()); if !cwd.exists() { cx.editor .set_error("Current working directory does not exist"); @@ -3188,17 +3189,24 @@ fn changed_file_picker(cx: &mut Context) { .with_preview(|_editor, meta| Some((meta.path().into(), None))); let injector = picker.injector(); - cx.editor - .diff_providers - .clone() - .for_each_changed_file(cwd, move |change| match change { + // Helix can be launched without arguments, in which case no diff provider will be loaded since + // there is no file to provide infos for. + // + // This ensures we have one to work with for cwd (and as a bonus it means any file opened + // from this picker will have its diff provider already in cache). + cx.editor.diff_providers.add(&cwd); + cx.editor.diff_providers.clone().for_each_changed_file( + cwd.clone(), + 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))); + cx.editor.diff_providers.remove(&cwd); } pub fn command_palette(cx: &mut Context) { diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 68ba9bab556e..015661a2c102 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -1279,7 +1279,7 @@ fn reload( let scrolloff = cx.editor.config().scrolloff; let (view, doc) = current!(cx.editor); - doc.reload(view, &cx.editor.diff_providers).map(|_| { + doc.reload(view, &mut cx.editor.diff_providers).map(|_| { view.ensure_cursor_in_view(doc, scrolloff); })?; if let Some(path) = doc.path() { @@ -1318,6 +1318,8 @@ fn reload_all( }) .collect(); + cx.editor.diff_providers.reset(); + for (doc_id, view_ids) in docs_view_ids { let doc = doc_mut!(cx.editor, &doc_id); @@ -1327,7 +1329,7 @@ fn reload_all( // Ensure that the view is synced with the document's history. view.sync_changes(doc); - if let Err(error) = doc.reload(view, &cx.editor.diff_providers) { + if let Err(error) = doc.reload(view, &mut cx.editor.diff_providers) { cx.editor.set_error(format!("{}", error)); continue; } diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index 919df04acc3e..6352eca7ebfc 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -19,7 +19,7 @@ tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "p parking_lot = "0.12" arc-swap = { version = "1.7.1" } -gix = { version = "0.67.0", features = ["attributes", "status"], default-features = false, optional = true } +gix = { version = "0.67.0", features = ["attributes", "parallel", "status"], default-features = false, optional = true } imara-diff = "0.1.7" anyhow = "1" diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 189f6e22bea5..bba87684ced2 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -22,22 +22,12 @@ use crate::FileChange; #[cfg(test)] mod test; -#[inline] -fn get_repo_dir(file: &Path) -> Result<&Path> { - file.parent().context("file has no parent directory") -} - -pub fn get_diff_base(file: &Path) -> Result> { +pub fn get_diff_base(repo: &ThreadSafeRepository, file: &Path) -> Result> { debug_assert!(!file.exists() || file.is_file()); debug_assert!(file.is_absolute()); - let file = gix::path::realpath(file).context("resolve symlinks")?; - - // TODO cache repository lookup + let file = gix::path::realpath(file).context("resolve symlink")?; - let repo_dir = get_repo_dir(&file)?; - let repo = open_repo(repo_dir) - .context("failed to open git repo")? - .to_thread_local(); + let repo = repo.to_thread_local(); let head = repo.head_commit()?; let file_oid = find_file_in_commit(&repo, &head, &file)?; @@ -59,15 +49,8 @@ pub fn get_diff_base(file: &Path) -> Result> { } } -pub fn get_current_head_name(file: &Path) -> Result>>> { - debug_assert!(!file.exists() || file.is_file()); - debug_assert!(file.is_absolute()); - let file = gix::path::realpath(file).context("resolve symlinks")?; - - let repo_dir = get_repo_dir(&file)?; - let repo = open_repo(repo_dir) - .context("failed to open git repo")? - .to_thread_local(); +pub fn get_current_head_name(repo: &ThreadSafeRepository) -> Result>>> { + let repo = repo.to_thread_local(); let head_ref = repo.head_ref()?; let head_commit = repo.head_commit()?; @@ -79,11 +62,17 @@ pub fn get_current_head_name(file: &Path) -> Result>>> { Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } -pub fn for_each_changed_file(cwd: &Path, f: impl Fn(Result) -> bool) -> Result<()> { - status(&open_repo(cwd)?.to_thread_local(), f) +pub fn for_each_changed_file( + repo: &ThreadSafeRepository, + f: impl Fn(Result) -> bool, +) -> Result<()> { + status(&repo.to_thread_local(), f) } -fn open_repo(path: &Path) -> Result { +pub(super) fn open_repo(path: &Path) -> Result { + // Ensure the repo itself is an absolute real path, else we'll not match prefixes with + // symlink-resolved files in `get_diff_base()` above. + let path = gix::path::realpath(path)?; // custom open options let mut git_open_opts_map = gix::sec::trust::Mapping::::default(); diff --git a/helix-vcs/src/git/test.rs b/helix-vcs/src/git/test.rs index 164040f50cd7..069a08b50c15 100644 --- a/helix-vcs/src/git/test.rs +++ b/helix-vcs/src/git/test.rs @@ -54,7 +54,8 @@ fn missing_file() { let file = temp_git.path().join("file.txt"); File::create(&file).unwrap().write_all(b"foo").unwrap(); - assert!(git::get_diff_base(&file).is_err()); + let repo = git::open_repo(temp_git.path()).unwrap(); + assert!(git::get_diff_base(&repo, &file).is_err()); } #[test] @@ -64,7 +65,12 @@ fn unmodified_file() { let contents = b"foo".as_slice(); File::create(&file).unwrap().write_all(contents).unwrap(); create_commit(temp_git.path(), true); - assert_eq!(git::get_diff_base(&file).unwrap(), Vec::from(contents)); + + let repo = git::open_repo(temp_git.path()).unwrap(); + assert_eq!( + git::get_diff_base(&repo, &file).unwrap(), + Vec::from(contents) + ); } #[test] @@ -76,7 +82,11 @@ fn modified_file() { create_commit(temp_git.path(), true); File::create(&file).unwrap().write_all(b"bar").unwrap(); - assert_eq!(git::get_diff_base(&file).unwrap(), Vec::from(contents)); + let repo = git::open_repo(temp_git.path()).unwrap(); + assert_eq!( + git::get_diff_base(&repo, &file).unwrap(), + Vec::from(contents) + ); } /// Test that `get_file_head` does not return content for a directory. @@ -95,7 +105,9 @@ fn directory() { std::fs::remove_dir_all(&dir).unwrap(); File::create(&dir).unwrap().write_all(b"bar").unwrap(); - assert!(git::get_diff_base(&dir).is_err()); + + let repo = git::open_repo(temp_git.path()).unwrap(); + assert!(git::get_diff_base(&repo, &dir).is_err()); } /// Test that `get_diff_base` resolves symlinks so that the same diff base is @@ -122,8 +134,9 @@ fn symlink() { symlink("file.txt", &file_link).unwrap(); create_commit(temp_git.path(), true); - assert_eq!(git::get_diff_base(&file_link).unwrap(), contents); - assert_eq!(git::get_diff_base(&file).unwrap(), contents); + let repo = git::open_repo(temp_git.path()).unwrap(); + assert_eq!(git::get_diff_base(&repo, &file_link).unwrap(), contents); + assert_eq!(git::get_diff_base(&repo, &file).unwrap(), contents); } /// Test that `get_diff_base` returns content when the file is a symlink to @@ -147,6 +160,7 @@ fn symlink_to_git_repo() { let file_link = temp_dir.path().join("file_link.txt"); symlink(&file, &file_link).unwrap(); - assert_eq!(git::get_diff_base(&file_link).unwrap(), contents); - assert_eq!(git::get_diff_base(&file).unwrap(), contents); + let repo = git::open_repo(temp_git.path()).unwrap(); + assert_eq!(git::get_diff_base(&repo, &file_link).unwrap(), contents); + assert_eq!(git::get_diff_base(&repo, &file).unwrap(), contents); } diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 539be779a900..2508be6b32d1 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,9 +1,6 @@ -use anyhow::{anyhow, bail, Result}; +use anyhow::Result; use arc_swap::ArcSwap; -use std::{ - path::{Path, PathBuf}, - sync::Arc, -}; +use std::{collections::HashMap, path::Path, sync::Arc}; #[cfg(feature = "git")] mod git; @@ -16,67 +13,182 @@ mod status; pub use status::FileChange; -#[derive(Clone)] +#[derive(Default, Clone)] pub struct DiffProviderRegistry { - providers: Vec, + /// Repository root path mapped to their provider. + /// + /// When a root path cannot be found after having called `add_file`, it means there is no + /// provider to speak of. + providers: HashMap, DiffProvider>, + /// Count the number of files added for a specific provider path. + /// Providers themselves don't care about that, this is handled entirely in `Self::add_file`, + /// without knowledge from the `Self::add_file_` methods. + /// + /// Note: it *could* happen that a provider for a path is changed without the number of + /// associated files changing, e.g deleting a .git/ and initializing a .jj/ repo. + counters: HashMap, u32>, } +/// Diff-related methods impl DiffProviderRegistry { pub fn get_diff_base(&self, file: &Path) -> Option> { - self.providers - .iter() - .find_map(|provider| match provider.get_diff_base(file) { - Ok(res) => Some(res), - Err(err) => { - log::debug!("{err:#?}"); - log::debug!("failed to open diff base for {}", file.display()); - None - } - }) + match self.provider_for(file)?.get_diff_base(file) { + Ok(diff_base) => Some(diff_base), + Err(err) => { + log::debug!("{err:#?}"); + log::debug!("failed to open diff base for {}", file.display()); + None + } + } } pub fn get_current_head_name(&self, file: &Path) -> Option>>> { - self.providers - .iter() - .find_map(|provider| match provider.get_current_head_name(file) { - Ok(res) => Some(res), - Err(err) => { - log::debug!("{err:#?}"); - log::debug!("failed to obtain current head name for {}", file.display()); - None - } - }) + match self.provider_for(file)?.get_current_head_name() { + Ok(head_name) => Some(head_name), + Err(err) => { + log::debug!("{err:#?}"); + log::debug!("failed to obtain current head name for {}", file.display()); + None + } + } } /// 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, + cwd: Arc, 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, &f).ok()) - .is_none() - { - f(Err(anyhow!("no diff provider returns success"))); + let Some(diff_provider) = self.provider_for(&cwd) else { + return; + }; + if let Err(err) = diff_provider.for_each_changed_file(&f) { + f(Err(err)); } }); } } -impl Default for DiffProviderRegistry { - fn default() -> Self { - // currently only git is supported - // TODO make this configurable when more providers are added - let providers = vec![ +/// Creation and update methods +#[cfg_attr(not(feature = "git"), allow(unused))] +impl DiffProviderRegistry { + /// Register a provider (if any is found) for the given path. + pub fn add(&mut self, path: &Path) { + let Some((repo_path, provider)) = get_possible_provider(path) else { + // Do nothing here: there is no path to use and so the actual methods to get infos + // like `get_diff_base` just won't do anything since they won't find a source to + // work with. + log::debug!("Found no potential diff provider for {}", path.display()); + // Note: if a `./` dir is deleted, we may end up in a situation where we lose track + // of a now unused provider. This is acceptable because it doesn't happen that often in + // practice and people can just reload to force an update. + // + // If it becomes an issue in the future, we could fix it by recomputing the providers + // for each stored paths here. + return; + }; + + let result = match provider { #[cfg(feature = "git")] - DiffProvider::Git, - ]; - DiffProviderRegistry { providers } + PossibleDiffProvider::Git => self.add_file_git(repo_path), + }; + + match result { + Ok((key, prov)) => { + // Increase the count for this path. + let count = self.counters.entry(key).or_default(); + let created = *count == 0; + *count += 1; + + // Only log at info level when adding a new provider + if created { + log::info!( + "Added {prov:?} (repo: {}) from {}", + repo_path.display(), + path.display() + ) + } else { + log::debug!( + "Reused {prov:?} (repo: {}) for {}", + repo_path.display(), + path.display() + ); + } + } + Err(err) => log::debug!( + "Failed to open repo at {} for {}: {:?}", + repo_path.display(), + path.display(), + err + ), + } + } + + /// Reload the provider for the given path. + pub fn reload(&mut self, path: &Path) { + self.remove(path); + self.add(path); + } + + /// Remove the given path from the provider cache. If it was the last one using it, this will + /// free up the provider. + pub fn remove(&mut self, path: &Path) { + let Some((repo_path, _)) = get_possible_provider(path) else { + return; + }; + + let Some(count) = self.counters.get_mut(repo_path) else { + return; + }; + + *count -= 1; + if *count == 0 { + // Cleanup the provider when the last user disappears + self.counters.remove(repo_path); + self.providers.remove(repo_path); + + // While reallocating is costly, in most sessions of Helix there will be one main + // workspace and sometimes a jump to some temporary one (for example from a jump-to-def + // in an LSP) that will be closed after some time. We want to avoid keeping unused + // RAM for this. + self.providers.shrink_to_fit(); + self.counters.shrink_to_fit(); + } + } + + /// Clears the saved providers completely. + pub fn reset(&mut self) { + self.providers = Default::default(); + self.counters = Default::default(); + } +} + +/// Private methods +impl DiffProviderRegistry { + fn provider_for(&self, path: &Path) -> Option<&DiffProvider> { + let path = get_possible_provider(path)?.0; + self.providers.get(path) + } + + /// Add the git repo to the known providers *if* it isn't already known. + #[cfg(feature = "git")] + fn add_file_git(&mut self, repo_path: &Path) -> Result<(Arc, PossibleDiffProvider)> { + // Don't build a git repo object if there is already one for that path. + if let Some((key, DiffProvider::Git(_))) = self.providers.get_key_value(repo_path) { + return Ok((Arc::clone(key), PossibleDiffProvider::Git)); + } + + match git::open_repo(repo_path) { + Ok(repo) => { + let key = Arc::from(repo_path); + self.providers + .insert(Arc::clone(&key), DiffProvider::Git(repo)); + Ok((key, PossibleDiffProvider::Git)) + } + Err(err) => Err(err), + } } } @@ -84,39 +196,67 @@ impl Default for DiffProviderRegistry { /// cloning [DiffProviderRegistry] as `Clone` cannot be used in trait objects. /// /// `Copy` is simply to ensure the `clone()` call is the simplest it can be. -#[derive(Copy, Clone)] +#[derive(Clone)] pub enum DiffProvider { #[cfg(feature = "git")] - Git, - None, + Git(gix::ThreadSafeRepository), } +#[cfg_attr(not(feature = "git"), allow(unused))] impl DiffProvider { fn get_diff_base(&self, file: &Path) -> Result> { - match self { + // We need the */ref else we're matching on a reference and Rust considers all references + // inhabited. In our case + match *self { + #[cfg(feature = "git")] + Self::Git(ref repo) => git::get_diff_base(repo, file), + } + } + + fn get_current_head_name(&self) -> Result>>> { + match *self { #[cfg(feature = "git")] - Self::Git => git::get_diff_base(file), - Self::None => bail!("No diff support compiled in"), + Self::Git(ref repo) => git::get_current_head_name(repo), } } - fn get_current_head_name(&self, file: &Path) -> Result>>> { - match self { + fn for_each_changed_file(&self, f: impl Fn(Result) -> bool) -> Result<()> { + match *self { #[cfg(feature = "git")] - Self::Git => git::get_current_head_name(file), - Self::None => bail!("No diff support compiled in"), + Self::Git(ref repo) => git::for_each_changed_file(repo, f), } } +} - fn for_each_changed_file( - &self, - cwd: &Path, - f: impl Fn(Result) -> bool, - ) -> Result<()> { - match self { +#[derive(Debug, Copy, Clone)] +pub enum PossibleDiffProvider { + /// Possibly a git repo rooted at the stored path (i.e. `/.git` exists) + #[cfg(feature = "git")] + Git, +} + +/// Does *possible* diff provider auto detection. Returns the 'root' of the workspace +/// +/// We say possible because this function doesn't open the actual repository to check if that's +/// actually the case. +fn get_possible_provider(path: &Path) -> Option<(&Path, PossibleDiffProvider)> { + if cfg!(feature = "git") { + #[cfg_attr(not(feature = "git"), allow(unused))] + fn check_path(path: &Path) -> Option<(&Path, PossibleDiffProvider)> { #[cfg(feature = "git")] - Self::Git => git::for_each_changed_file(cwd, f), - Self::None => bail!("No diff support compiled in"), + if path.join(".git").try_exists().ok()? { + return Some((path, PossibleDiffProvider::Git)); + } + + None + } + + for parent in path.ancestors() { + if let Some(path_and_provider) = check_path(parent) { + return Some(path_and_provider); + } } } + + None } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 91ec27874853..c4c0b3b4c756 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1111,7 +1111,7 @@ impl Document { pub fn reload( &mut self, view: &mut View, - provider_registry: &DiffProviderRegistry, + provider_registry: &mut DiffProviderRegistry, ) -> Result<(), Error> { let encoding = self.encoding; let path = match self.path() { @@ -1122,6 +1122,8 @@ impl Document { }, }; + provider_registry.reload(&path); + // Once we have a valid path we check if its readonly status has changed self.detect_readonly(); diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 26dea3a21e59..9be423ed2641 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1719,6 +1719,8 @@ impl Editor { Editor::doc_diagnostics(&self.language_servers, &self.diagnostics, &doc); doc.replace_diagnostics(diagnostics, &[], None); + // When opening a *new* file, ensure its diff provider is loaded. + self.diff_providers.add(&path); if let Some(diff_base) = self.diff_providers.get_diff_base(&path) { doc.set_diff_base(diff_base); } @@ -1752,6 +1754,10 @@ impl Editor { return Err(CloseError::BufferModified(doc.display_name().into_owned())); } + if let Some(path) = doc.path() { + self.diff_providers.remove(path); + } + // This will also disallow any follow-up writes self.saves.remove(&doc_id); From 601e3a54498142754c6d74e85850c3baefa9b3b1 Mon Sep 17 00:00:00 2001 From: "Alexis (Poliorcetics) Bourget" Date: Thu, 7 Nov 2024 13:42:48 +0100 Subject: [PATCH 2/2] feat: vcs: support Jujutsu as a diff-provider --- helix-term/Cargo.toml | 4 +- helix-vcs/Cargo.toml | 2 + helix-vcs/src/jj.rs | 282 ++++++++++++++++++++++++++++++++++++++++ helix-vcs/src/lib.rs | 70 +++++++--- helix-vcs/src/status.rs | 1 + 5 files changed, 340 insertions(+), 19 deletions(-) create mode 100644 helix-vcs/src/jj.rs diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index cd2cfe0340b3..044ac070aee8 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -13,10 +13,12 @@ repository.workspace = true homepage.workspace = true [features] -default = ["git"] +default = ["git", "jj"] unicode-lines = ["helix-core/unicode-lines", "helix-view/unicode-lines"] integration = ["helix-event/integration_test"] +# VCS features git = ["helix-vcs/git"] +jj = ["helix-vcs/jj"] [[bin]] name = "hx" diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index 6352eca7ebfc..2a54012e8d7e 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -24,9 +24,11 @@ imara-diff = "0.1.7" anyhow = "1" log = "0.4" +tempfile = { version = "3.13", optional = true } [features] git = ["gix"] +jj = ["tempfile"] [dev-dependencies] tempfile = "3.14" diff --git a/helix-vcs/src/jj.rs b/helix-vcs/src/jj.rs new file mode 100644 index 000000000000..10d2b08b8900 --- /dev/null +++ b/helix-vcs/src/jj.rs @@ -0,0 +1,282 @@ +//! Jujutsu works with several backends and could add new ones in the future. Private builds of +//! it could also have private backends. Those make it hard to use `jj-lib` since it won't have +//! access to newer or private backends and fail to compute the diffs for them. +//! +//! Instead in case there *is* a diff to base ourselves on, we copy it to a tempfile or just use the +//! current file if not. + +use std::path::Path; +use std::process::Command; +use std::sync::Arc; + +use anyhow::{Context, Result}; +use arc_swap::ArcSwap; + +use crate::FileChange; + +pub(super) fn get_diff_base(repo: &Path, file: &Path) -> Result> { + let file_relative_to_root = file + .strip_prefix(repo) + .context("failed to strip JJ repo root path from file")?; + + let tmpfile = tempfile::NamedTempFile::with_prefix("helix-jj-diff-") + .context("could not create tempfile to save jj diff base")?; + let tmppath = tmpfile.path(); + + let copy_bin = if cfg!(windows) { "copy.exe" } else { "cp" }; + + let status = Command::new("jj") + .arg("--repository") + .arg(repo) + .args([ + "--ignore-working-copy", + "diff", + "--revision", + "@", + "--config-toml", + ]) + // Copy the temporary file provided by jujutsu to a temporary path of our own, + // because the `$left` directory is deleted when `jj` finishes executing. + .arg(format!( + "ui.diff.tool = ['{exe}', '$left/{base}', '{target}']", + exe = copy_bin, + base = file_relative_to_root.display(), + // Where to copy the jujutsu-provided file + target = tmppath.display(), + )) + // Restrict the diff to the current file + .arg(file) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .context("failed to execute jj diff command")?; + + let use_jj_path = status.success() && std::fs::metadata(tmppath).map_or(false, |m| m.len() > 0); + // If the copy call inside `jj diff` succeeded, the tempfile is the one containing the base + // else it's just the original file (so no diff). We check for size since `jj` can return + // 0-sized files when there are no diffs to present for the file. + let diff_base_path = if use_jj_path { tmppath } else { file }; + + // If the command succeeded, it means we either copied the jujutsu base or the current file, + // so there should always be something to read and compare to. + std::fs::read(diff_base_path).context("could not read jj diff base from the target") +} + +pub(crate) fn get_current_head_name(repo: &Path) -> Result>>> { + // See + // + // This will produce the following: + // + // - If there are no branches: `vyvqwlmsvnlkmqrvqktpuluvuknuxpmm` + // - If there is a single branch: `vyvqwlmsvnlkmqrvqktpuluvuknuxpmm (master)` + // - If there are 2+ branches: `vyvqwlmsvnlkmqrvqktpuluvuknuxpmm (master, jj-diffs)` + // + // Always using the long id makes it easy to share it with others, which would not be the + // case for shorter ones: they could have a local change that renders it ambiguous. + let template = r#"separate(" ", change_id, surround("(", ")", branches.join(", ")))"#; + + let out = Command::new("jj") + .arg("--repository") + .arg(repo) + .args([ + "--ignore-working-copy", + "log", + "--color", + "never", + "--revisions", + "@", // Only display the current revision + "--no-graph", + "--no-pager", + "--template", + template, + ]) + .output()?; + + if !out.status.success() { + anyhow::bail!("jj log command executed but failed"); + } + + let out = String::from_utf8(out.stdout)?; + + let rev = out + .lines() + .next() + // Contrary to git, if a JJ repo exists, it always has at least two revisions: + // the root (zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz), which cannot be focused, and the current + // one, which exists even for brand new repos. + .context("should always find at least one line")?; + + Ok(Arc::new(ArcSwap::from_pointee(rev.into()))) +} + +pub(crate) fn for_each_changed_file( + repo: &Path, + callback: impl Fn(Result) -> bool, +) -> Result<()> { + let out = Command::new("jj") + .arg("--repository") + .arg(repo) + .args([ + "--ignore-working-copy", + "diff", + "--color", + "never", + "--revision", + "@", // Only display the current revision + "--no-pager", + "--types", + ]) + .output()?; + + if !out.status.success() { + anyhow::bail!("jj log command executed but failed"); + } + + let out = String::from_utf8(out.stdout)?; + + for line in out.lines() { + let Some((status, path)) = line.split_once(' ') else { + continue; + }; + + let Some(change) = status_to_change(status, path) else { + continue; + }; + + if !callback(Ok(change)) { + break; + } + } + + Ok(()) +} + +pub(crate) fn open_repo(repo_path: &Path) -> Result<()> { + assert!( + repo_path.join(".jj").exists(), + "no .jj where one was expected: {}", + repo_path.display(), + ); + + let status = Command::new("jj") + .args(["--ignore-working-copy", "root"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + + if status.success() { + Ok(()) + } else { + anyhow::bail!("not a valid JJ repo") + } +} + +/// Associate a status to a `FileChange`. +fn status_to_change(status: &str, path: &str) -> Option { + if let rename @ Some(_) = find_rename(path) { + return rename; + } + + // Syntax: + Some(match status { + "FF" | "LL" | "CF" | "CL" | "FL" | "LF" => FileChange::Modified { path: path.into() }, + "-F" | "-L" => FileChange::Untracked { path: path.into() }, + "F-" | "L-" => FileChange::Deleted { path: path.into() }, + "FC" | "LC" => FileChange::Conflict { path: path.into() }, + // We ignore gitsubmodules here since they not interesting in the context of + // a file editor. + _ => return None, + }) +} + +fn find_rename(path: &str) -> Option { + let (start, rest) = path.split_once('{')?; + let (from, rest) = rest.split_once(" => ")?; + let (to, end) = rest.split_once('}')?; + + Some(FileChange::Renamed { + from_path: format!("{start}{from}{end}").into(), + to_path: format!("{start}{to}{end}").into(), + }) +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use super::*; + + #[test] + fn test_status_to_change() { + let p = "helix-vcs/src/lib.rs"; + let pb = PathBuf::from(p); + + for s in ["FF", "LL", "CF", "CL", "FL", "LF"] { + assert_eq!( + status_to_change(s, p).unwrap(), + FileChange::Modified { path: pb.clone() } + ); + } + for s in ["-F", "-L"] { + assert_eq!( + status_to_change(s, p).unwrap(), + FileChange::Untracked { path: pb.clone() } + ); + } + for s in ["F-", "L-"] { + assert_eq!( + status_to_change(s, p).unwrap(), + FileChange::Deleted { path: pb.clone() } + ); + } + for s in ["FC", "LC"] { + assert_eq!( + status_to_change(s, p).unwrap(), + FileChange::Conflict { path: pb.clone() } + ); + } + for s in ["GG", "LG", "ARO", "", " ", " "] { + assert_eq!(status_to_change(s, p), None); + } + } + + #[test] + fn test_find_rename() { + fn check(path: &str, expected: Option<(&str, &str)>) { + let result = find_rename(path); + + assert_eq!( + result, + expected.map(|(f, t)| FileChange::Renamed { + from_path: f.into(), + to_path: t.into() + }) + ) + } + + // No renames + check("helix-term/Cargo.toml", None); + check("helix-term/src/lib.rs", None); + + // Rename of first element in path + check( + "{helix-term => helix-term2}/Cargo.toml", + Some(("helix-term/Cargo.toml", "helix-term2/Cargo.toml")), + ); + // Rename of final element in path + check( + "helix-term/{Cargo.toml => Cargo.toml2}", + Some(("helix-term/Cargo.toml", "helix-term/Cargo.toml2")), + ); + // Rename of a single dir in the middle + check( + "helix-term/{src => src2}/lib.rs", + Some(("helix-term/src/lib.rs", "helix-term/src2/lib.rs")), + ); + // Rename of two dirs in the middle + check( + "helix-term/{src/ui => src2/ui2}/text.rs", + Some(("helix-term/src/ui/text.rs", "helix-term/src2/ui2/text.rs")), + ); + } +} diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 2508be6b32d1..b4b29a79e267 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -4,6 +4,8 @@ use std::{collections::HashMap, path::Path, sync::Arc}; #[cfg(feature = "git")] mod git; +#[cfg(feature = "jj")] +mod jj; mod diff; @@ -72,7 +74,7 @@ impl DiffProviderRegistry { } /// Creation and update methods -#[cfg_attr(not(feature = "git"), allow(unused))] +#[cfg_attr(not(any(feature = "git", feature = "jj")), allow(unused))] impl DiffProviderRegistry { /// Register a provider (if any is found) for the given path. pub fn add(&mut self, path: &Path) { @@ -90,9 +92,11 @@ impl DiffProviderRegistry { return; }; - let result = match provider { + let result: Result<(Arc, PossibleDiffProvider)> = match provider { #[cfg(feature = "git")] PossibleDiffProvider::Git => self.add_file_git(repo_path), + #[cfg(feature = "jj")] + PossibleDiffProvider::JJ => self.add_file_jj(repo_path), }; match result { @@ -190,26 +194,49 @@ impl DiffProviderRegistry { Err(err) => Err(err), } } + + /// Add the JJ repo to the known providers *if* it isn't already known. + #[cfg(feature = "jj")] + fn add_file_jj(&mut self, repo_path: &Path) -> Result<(Arc, PossibleDiffProvider)> { + // Don't build a JJ repo object if there is already one for that path. + if let Some((key, DiffProvider::JJ(_))) = self.providers.get_key_value(repo_path) { + return Ok((Arc::clone(key), PossibleDiffProvider::JJ)); + } + + match jj::open_repo(repo_path) { + Ok(()) => { + let key = Arc::from(repo_path); + self.providers + .insert(Arc::clone(&key), DiffProvider::JJ(Arc::clone(&key))); + Ok((key, PossibleDiffProvider::JJ)) + } + Err(err) => Err(err), + } + } } /// 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. -/// -/// `Copy` is simply to ensure the `clone()` call is the simplest it can be. #[derive(Clone)] pub enum DiffProvider { #[cfg(feature = "git")] Git(gix::ThreadSafeRepository), + /// For [`jujutsu`](https://github.com/martinvonz/jj), we don't use the library but instead we + /// call the binary because it can dynamically load backends, which the JJ library doesn't know about. + #[cfg(feature = "jj")] + JJ(Arc), } -#[cfg_attr(not(feature = "git"), allow(unused))] +#[cfg_attr(not(any(feature = "git", feature = "jj")), allow(unused))] impl DiffProvider { fn get_diff_base(&self, file: &Path) -> Result> { // We need the */ref else we're matching on a reference and Rust considers all references - // inhabited. In our case + // inhabited. match *self { #[cfg(feature = "git")] Self::Git(ref repo) => git::get_diff_base(repo, file), + #[cfg(feature = "jj")] + Self::JJ(ref repo) => jj::get_diff_base(repo, file), } } @@ -217,6 +244,8 @@ impl DiffProvider { match *self { #[cfg(feature = "git")] Self::Git(ref repo) => git::get_current_head_name(repo), + #[cfg(feature = "jj")] + Self::JJ(ref repo) => jj::get_current_head_name(repo), } } @@ -224,6 +253,8 @@ impl DiffProvider { match *self { #[cfg(feature = "git")] Self::Git(ref repo) => git::for_each_changed_file(repo, f), + #[cfg(feature = "jj")] + Self::JJ(ref repo) => jj::for_each_changed_file(repo, f), } } } @@ -233,6 +264,9 @@ pub enum PossibleDiffProvider { /// Possibly a git repo rooted at the stored path (i.e. `/.git` exists) #[cfg(feature = "git")] Git, + /// Possibly a git repo rooted at the stored path (i.e. `/.jj` exists) + #[cfg(feature = "jj")] + JJ, } /// Does *possible* diff provider auto detection. Returns the 'root' of the workspace @@ -240,20 +274,20 @@ pub enum PossibleDiffProvider { /// We say possible because this function doesn't open the actual repository to check if that's /// actually the case. fn get_possible_provider(path: &Path) -> Option<(&Path, PossibleDiffProvider)> { - if cfg!(feature = "git") { - #[cfg_attr(not(feature = "git"), allow(unused))] - fn check_path(path: &Path) -> Option<(&Path, PossibleDiffProvider)> { - #[cfg(feature = "git")] - if path.join(".git").try_exists().ok()? { - return Some((path, PossibleDiffProvider::Git)); - } - - None - } + // TODO(poliorcetics): make checking order configurable + let checks: &[(&str, PossibleDiffProvider)] = &[ + #[cfg(feature = "jj")] + (".jj", PossibleDiffProvider::JJ), + #[cfg(feature = "git")] + (".git", PossibleDiffProvider::Git), + ]; + if !checks.is_empty() { for parent in path.ancestors() { - if let Some(path_and_provider) = check_path(parent) { - return Some(path_and_provider); + for &(repo_indic, pdp) in checks { + if let Ok(true) = parent.join(repo_indic).try_exists() { + return Some((parent, pdp)); + } } } } diff --git a/helix-vcs/src/status.rs b/helix-vcs/src/status.rs index f34334909554..b221dc77236e 100644 --- a/helix-vcs/src/status.rs +++ b/helix-vcs/src/status.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; +#[derive(Debug, PartialEq, Eq)] pub enum FileChange { Untracked { path: PathBuf,