diff --git a/Cargo.lock b/Cargo.lock index c74239d55dc4..c825778e49e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1742,6 +1742,7 @@ dependencies = [ "helix-event", "imara-diff", "log", + "memchr", "parking_lot", "tempfile", "tokio", diff --git a/book/src/workspace-trust.md b/book/src/workspace-trust.md index d501110c81a5..775dfe6b3dcc 100644 --- a/book/src/workspace-trust.md +++ b/book/src/workspace-trust.md @@ -156,7 +156,9 @@ are expanded. > genuinely disruptive to your workflow. An explicit `:workspace-exclude` > still overrides a matching pattern. -## Git trust +## VCS trust + +### Git Workspace trust also gates how Helix opens git repositories. Untrusted workspaces are opened in [gix](https://github.com/Byron/gitoxide)'s @@ -173,3 +175,20 @@ Helix forces this trust level explicitly rather than letting gix infer it from `.git` directory ownership — a malicious `.git/config` in a directory you happen to own is still treated as untrusted until you run `:workspace-trust`. + +### Jujutsu + +Jujutsu repositories are only supported when the workspace is fully trusted +because Jujutsu itself does not expose a trust setting (yet). + +Jujutsu itself is fairly secure in that it does not allow bringing a +configuration on clone (JJ repository and workspace configuration live +outside of their relevant directory) but there is no way to transmit the +`Trust::Reduced` setting to underlying Git operations so we choose to not +allow any JJ operation unless the workspace is fully trusted. + +> [!NOTE] +> If you use `level = "servers"`, you will not get a prompt to enable full +> workspace trust by default, which means (for JJ repositories) that you +> will not get diff markers, branch name or a populated changed files +> picker until you run `:workspace-trust` manually. diff --git a/helix-term/Cargo.toml b/helix-term/Cargo.toml index 2f3809da1bee..bd4743a45299 100644 --- a/helix-term/Cargo.toml +++ b/helix-term/Cargo.toml @@ -31,10 +31,12 @@ assets = [ ] [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-term/src/commands.rs b/helix-term/src/commands.rs index c9560f33f830..d0c238046948 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -79,6 +79,7 @@ use std::{ future::Future, io::Read, num::NonZeroUsize, + sync::Arc, }; use std::{ @@ -3481,7 +3482,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, @@ -3489,7 +3490,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"); @@ -3568,17 +3569,24 @@ fn changed_file_picker(cx: &mut Context) { helix_loader::workspace_trust::TrustQuery::Git, ) .is_trusted(); - cx.editor - .diff_providers - .clone() - .for_each_changed_file(cwd, trust_full, 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, trust_full); + 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 9a5bf97129fc..4efd462ac0ac 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -1593,7 +1593,7 @@ fn reload(cx: &mut compositor::Context, _args: Args, event: PromptEvent) -> anyh let scrolloff = cx.editor.config().scrolloff; let trust_full = doc_trust_full(cx.editor); let (view, doc) = current!(cx.editor); - doc.reload(view, &cx.editor.diff_providers, trust_full) + doc.reload(view, &mut cx.editor.diff_providers, trust_full) .map(|_| { view.ensure_cursor_in_view(doc, scrolloff); })?; @@ -1629,6 +1629,8 @@ fn reload_all(cx: &mut compositor::Context, _args: Args, event: PromptEvent) -> }) .collect(); + cx.editor.diff_providers.reset(); + for (doc_id, view_ids) in docs_view_ids { let doc = doc_mut!(cx.editor, &doc_id); @@ -1647,7 +1649,7 @@ fn reload_all(cx: &mut compositor::Context, _args: Args, event: PromptEvent) -> helix_loader::workspace_trust::TrustQuery::Git, ) .is_trusted(); - if let Err(error) = doc.reload(view, &cx.editor.diff_providers, trust_full) { + if let Err(error) = doc.reload(view, &mut cx.editor.diff_providers, trust_full) { cx.editor.set_error(format!("{}", error)); continue; } diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index b58589bdbcd2..3ff74ebfda7e 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -17,14 +17,16 @@ tokio = { version = "1", features = ["rt", "rt-multi-thread", "time", "sync", "p parking_lot.workspace = true arc-swap.workspace = true -gix = { version = "0.85.0", features = ["attributes", "status", "max-performance", "sha1"], default-features = false, optional = true } +gix = { version = "0.85.0", features = ["attributes", "parallel", "status", "max-performance", "sha1"], default-features = false, optional = true } imara-diff = "0.2.0" anyhow = "1" - log = "0.4" +memchr = { version = "2.7", optional = true } +tempfile = { version = "3.13", optional = true } [features] git = ["gix"] +jj = ["memchr", "tempfile"] [dev-dependencies] tempfile.workspace = true diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 133b77dab0bf..f83b24e3be84 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, trust_full: bool) -> Result> { +pub(super) 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 repo_dir = get_repo_dir(&file)?; - let repo = open_repo(repo_dir, trust_full) - .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)?; @@ -65,15 +55,8 @@ pub fn get_diff_base(file: &Path, trust_full: bool) -> Result> { } } -pub fn get_current_head_name(file: &Path, trust_full: bool) -> 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, trust_full) - .context("failed to open git repo")? - .to_thread_local(); +pub(super) 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()?; @@ -85,15 +68,14 @@ pub fn get_current_head_name(file: &Path, trust_full: bool) -> Result) -> bool, ) -> Result<()> { - status(&open_repo(cwd, trust_full)?.to_thread_local(), f) + status(&repo.to_thread_local(), f) } -fn open_repo(path: &Path, trust_full: bool) -> Result { +pub(super) fn open_repo(path: &Path, trust_full: bool) -> Result { // `trust_full` is the workspace-trust decision made by the caller, and it must be the // authority on the gix trust level. gix's own discovery (`discover_*`) ignores a // caller-supplied trust level: it always re-derives trust from `.git` ownership, so a malicious @@ -110,6 +92,10 @@ fn open_repo(path: &Path, trust_full: bool) -> Result { gix::sec::Trust::Reduced }; + // 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)?; + // On Windows various configuration options are bundled as part of the git installation. The // lookup is expensive; only do it there. let config = gix::open::permissions::Config { @@ -130,7 +116,7 @@ fn open_repo(path: &Path, trust_full: bool) -> Result { dot_git_only: true, ..Default::default() }; - let (repo_path, _trust_from_ownership) = gix::discover::upwards_opts(path, discover_options) + let (repo_path, _trust_from_ownership) = gix::discover::upwards_opts(&path, discover_options) .context("failed to discover git repo")?; let (git_dir, _work_dir) = repo_path.into_repository_and_work_tree_directories(); diff --git a/helix-vcs/src/git/test.rs b/helix-vcs/src/git/test.rs index fe941d2e3319..c7ed7512bcfa 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, true).is_err()); + let repo = git::open_repo(temp_git.path(), true).unwrap(); + assert!(git::get_diff_base(&repo, &file).is_err()); } #[test] @@ -64,8 +65,10 @@ fn unmodified_file() { let contents = b"foo".as_slice(); File::create(&file).unwrap().write_all(contents).unwrap(); create_commit(temp_git.path(), true); + + let repo = git::open_repo(temp_git.path(), true).unwrap(); assert_eq!( - git::get_diff_base(&file, true).unwrap(), + git::get_diff_base(&repo, &file).unwrap(), Vec::from(contents) ); } @@ -79,8 +82,9 @@ fn modified_file() { create_commit(temp_git.path(), true); File::create(&file).unwrap().write_all(b"bar").unwrap(); + let repo = git::open_repo(temp_git.path(), true).unwrap(); assert_eq!( - git::get_diff_base(&file, true).unwrap(), + git::get_diff_base(&repo, &file).unwrap(), Vec::from(contents) ); } @@ -101,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, true).is_err()); + + let repo = git::open_repo(temp_git.path(), true).unwrap(); + assert!(git::get_diff_base(&repo, &dir).is_err()); } /// Test that `get_diff_base` resolves symlinks so that the same diff base is @@ -128,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, true).unwrap(), contents); - assert_eq!(git::get_diff_base(&file, true).unwrap(), contents); + let repo = git::open_repo(temp_git.path(), true).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 @@ -153,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, true).unwrap(), contents); - assert_eq!(git::get_diff_base(&file, true).unwrap(), contents); + let repo = git::open_repo(temp_git.path(), true).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/jj.rs b/helix-vcs/src/jj.rs new file mode 100644 index 000000000000..473279dd2ce2 --- /dev/null +++ b/helix-vcs/src/jj.rs @@ -0,0 +1,531 @@ +//! 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::ffi::OsStr; +use std::path::{Path, PathBuf}; +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")?; + + /// Helper function to run `jj diff` with the same default arguments + fn run_jj_diff( + repo: &Path, + file: &Path, + option: impl AsRef, + value: impl AsRef, + ) -> Result> { + let output = jj_command(repo) + .args([ + "diff".as_ref(), + // Work with current revision only. + "--revision".as_ref(), + "@".as_ref(), + option.as_ref(), + value.as_ref(), + // If the filepath starts with `-` or `--` it could be interpreted as an option or + // flag, which is not what the user will want and could at worst allow malicious + // repos to run destructive commands. + "--".as_ref(), + // Restrict the diff to the current file + file.as_ref(), + ]) + .output() + .context("failed to execute `jj diff` to get diff base")?; + + anyhow::ensure!( + output.status.success(), + "`jj diff` executed but failed for {file:?}" + ); + + Ok(output.stdout) + } + + let stdout = run_jj_diff( + repo, + file, + // Check if file is newly added, existing or unmodified + "--template", + "status_char", + )?; + + match stdout.trim_ascii().first().copied() { + // File existed in previous change and has no diff + None => std::fs::read(file).context("could not read jj diff base from existing file"), + // File is new from current change + Some(b'A' | b'C') => Ok(Vec::new()), + // File existed in previous change and has diff, in which case we need to get the diff base + Some(_) => { + 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 _stdout = run_jj_diff( + repo, + file, + // Pass custom diff configuration. + "--config", + format!( + "ui.diff-formatter=['{exe}', '$left/{base}', '{target}']", + exe = copy_bin, + base = file_relative_to_root.display(), + // Where to copy the jujutsu-provided file + target = tmppath.display(), + ), + )?; + std::fs::read(tmppath).context("could not read jj diff base from target") + } + } +} + +pub(crate) fn get_current_head_name(repo: &Path) -> Result>>> { + let out = jj_command(repo) + .args([ + "log", + "--no-graph", + // Includes from last immutable revision to current change. + "--revisions", + // + "immutable_heads()::@ & bookmarks()", + "--template", + // See + // + // This will produce the following: + // + // quvlrxss + // kmlpqmrv main-1 + // sxrrsnun main-2 main-3* + // kzqnuykl + // + // There will be a `*` when a bookmark has been modified compared to its remote. + // + // We use a short ID with 8 characters because in practice the change ID is extremely + // unlikely to conflict since we only consider mutable commits (like most jj commands + // will do by default) and this leaves space for bookmarks to appear in the status bar + // even on narrower screens. + r#"change_id.short(8) ++ " " ++ bookmarks ++ "\n""#, + ]) + .output()?; + + anyhow::ensure!(out.status.success(), "`jj log` executed but failed"); + + let output = String::from_utf8(out.stdout).context("`jj log` did not output valid UTF-8")?; + let head_text = extract_head_name(&output)?; + + Ok(Arc::new(ArcSwap::from_pointee(head_text.into()))) +} + +pub(crate) fn for_each_changed_file( + repo: &Path, + callback: impl Fn(Result) -> bool, +) -> Result<()> { + // The forward slash is the only character that is disallowed in both Unix and Windows paths, + // meaning `//` cannot ever appear in them on any platform. + // + // + // + // Lines will be of the following format (examples) + // + // ``` + // C:text.txt // + // D:added // copied.txt // // copied.txt // file // + // D:renamed // to-rename.txt // file // renamed.txt // file // + // D:removed // to-delete.txt // file // to-delete.txt // // + // D:modified // to-modify.txt // file // to-modify.txt // file // + // ``` + // + // Note we use `//\n` as the end delimiter to allow for files that contains `\n` in their name. + // + // For the file types, we will only concern ourselves with `file` and `symlink`, anything else + // will get dropped just like `git.rs` does. + let template = r#"concat( + conflicted_files + .map(|file| concat("C:", file.path().display(), " //\n")) + .join(""), + diff + .files() + .filter(|file| !conflict || conflicted_files.all(|c| c.path().display() != file.path().display())) + .map(|file| concat( + "D:", + file.status(), + " // ", + file.source().path().display(), + " // ", + file.source().file_type(), + " // ", + file.target().path().display(), + " // ", + file.target().file_type(), + " //\n", + )) + .join(""), + )"#; + + let out = jj_command(repo) + .args([ + "show", + // Work with current revision only. + "@", + "--no-patch", + "--template", + template, + ]) + .output()?; + + if !out.status.success() { + log::warn!("`jj show` executed but failed:\n{out:#?}"); + anyhow::bail!("`jj show` executed but failed"); + } + + for entry in split_double_slash(&out.stdout, true) { + let change = match entry { + [b'C', b':', rest @ ..] => FileChange::Conflict { + path: make_pathbuf(rest), + }, + [b'D', b':', rest @ ..] => match entry_to_change(rest) { + Some(change) => change, + None => continue, + }, + _ => continue, + }; + + if !callback(Ok(change)) { + return Ok(()); + } + } + + Ok(()) +} + +pub(crate) fn open_repo(repo_path: &Path) -> Result<()> { + assert!( + repo_path.join(".jj").exists(), + "no .jj where one was expected: {repo_path:?}", + ); + + // Checking that the .jj we found is actually a JJ repo + let status = jj_command(repo_path) + .args(["workspace", "root"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + + if status.success() { + Ok(()) + } else { + anyhow::bail!("not a valid JJ repo") + } +} + +/// Prepare a JJ command with the common boilerplate +fn jj_command(repo: &Path) -> Command { + // Prevents argument injection when the repo is named `--my-repo`. + // The public interface in `./lib.rs` will canonicalize before trying to look for a JJ repo + // so this is here to avoid issues in case of refactors + assert!( + repo.is_absolute(), + "only absolute paths can be used as jj repo paths" + ); + + let mut command = Command::new("jj"); + command.args([ + "--color", + "never", + // Will search for diffs and such normally but not update the underlying repository + "--no-integrate-operation", + "--no-pager", + "--quiet", + "--repository", + ]); + command.arg(repo); + command +} + +/// Helper function to make the extracting logic testable +fn extract_head_name(output: &str) -> Result { + let mut lines = output.lines(); + let mut next = || lines.next().and_then(|line| line.split_once(' ')); + + let (rev, exact_bookmarks) = 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")?; + + let head_text = if !exact_bookmarks.is_empty() { + // Parentheses: bookmarks are exactly on current change. + format!("{rev} ({exact_bookmarks})") + } else { + let ancestor_bookmarks = std::iter::from_fn(next) + .map(|e| e.1) + .filter(|s| !s.is_empty()) + .collect::>(); + if ancestor_bookmarks.is_empty() { + // Found no bookmarks amongst ancestors. + rev.to_string() + } else { + // Angle brackets: bookmarks are on ancestors. + format!("{rev} [{}]", ancestor_bookmarks.join(" ").trim()) + } + }; + + Ok(head_text) +} + +/// Associate a status to a `FileChange`. +/// +/// Gets something like `modified // conflict.txt // conflict // conflict.txt // file` as input. +fn entry_to_change(entry: &[u8]) -> Option { + let mut sections = split_double_slash(entry, false); + + let kind = sections.next()?; + + let source_path = sections.next()?; + let source_file_type = sections.next()?; + + let target_path = sections.next()?; + let target_file_type = sections.next()?; + + // Never generated in practice but let's be thourough in case that changes. + // + if target_file_type == b"conflict" { + return Some(FileChange::Conflict { + path: make_pathbuf(target_path), + }); + } + + let file_types = [ + // The empty file type is used when the file didn't exist before or doesn't exist now, + // e.g. when added or removed. + "".as_bytes(), + "conflict".as_bytes(), + "file".as_bytes(), + "symlink".as_bytes(), + ]; + if !file_types.contains(&source_file_type) || !file_types.contains(&target_file_type) { + return None; + } + + let change = match kind { + b"added" | b"copied" => FileChange::Untracked { + path: make_pathbuf(target_path), + }, + b"modified" => FileChange::Modified { + path: make_pathbuf(target_path), + }, + b"removed" => FileChange::Deleted { + path: make_pathbuf(target_path), + }, + b"renamed" => FileChange::Renamed { + from_path: make_pathbuf(source_path), + to_path: make_pathbuf(target_path), + }, + _ => return None, + }; + + Some(change) +} + +#[cfg(any(unix, target_os = "wasi"))] +fn make_pathbuf(sl: &[u8]) -> PathBuf { + #[cfg(unix)] + use std::os::unix::ffi::OsStrExt; + #[cfg(target_os = "wasi")] + use std::os::wasi::ffi::OsStrExt; + + PathBuf::from(std::ffi::OsStr::from_bytes(sl)) +} + +// Imperfect fallback for platforms where we don't know about an always-correct method. +// In practice, non-UTF8 paths are vanishingly rare and should not be an issue for anyone running a +// Rust binary like Helix. +#[cfg(not(any(unix, target_os = "wasi")))] +fn make_pathbuf(sl: &[u8]) -> PathBuf { + let s = String::from_utf8_lossy(sl); + PathBuf::from(s.into_owned()) +} + +/// Split a byte slice on either ` // ` or ` //\n` depending on `with_newline`. +fn split_double_slash(slice: &[u8], with_newline: bool) -> impl Iterator { + let mut done = false; + let mut rest = slice; + let needle = if with_newline { " //\n" } else { " // " }.as_bytes(); + std::iter::from_fn(move || { + if done { + return None; + } + let result = match memchr::memmem::find(rest, needle) { + Some(pos) => { + // We use the non-panicking variants to avoid adding the panic machinery here when + // we know it won't ever panic in practice (unless there is a bug in memchr, which + // is unlikely given how much the crate is used). + let (before, after) = rest.split_at_checked(pos).unwrap_or_default(); + rest = after.get(4..).unwrap_or_default(); + before + } + None => { + done = true; + rest + } + }; + Some(result) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_split_double_slash_no_newline() { + let input = b"modified // test.rs // file // test.rs // file //\n"; + let expected = [ + "modified".as_bytes(), + "test.rs".as_bytes(), + "file".as_bytes(), + "test.rs".as_bytes(), + "file //\n".as_bytes(), // Not trimmed since we're not splitting on newlines + ]; + + let result = split_double_slash(input, false).collect::>(); + + assert_eq!(result, expected); + } + + #[test] + fn test_split_double_slash_with_newline() { + let input = concat!( + "modified // test.rs // file // test.rs // file //\n", + "modified // test.rs // file // test.rs // file //\n", + ) + .as_bytes(); + let expected = [ + "modified // test.rs // file // test.rs // file".as_bytes(), + "modified // test.rs // file // test.rs // file".as_bytes(), + // We expect an empty slice after the last split + &[], + ]; + + let result = split_double_slash(input, true).collect::>(); + + assert_eq!(result, expected); + } + + #[test] + fn test_entry_to_change() { + let p = "helix-vcs/src/lib.rs"; + let pb = PathBuf::from(p); + + let entry = |kind, (t1, t2)| { + entry_to_change(format!("{kind} // {p} // {t1} // {p} // {t2}").as_bytes()) + }; + + for types in [ + ("conflict", "file"), + ("conflict", "symlink"), + ("file", "file"), + ("file", "symlink"), + ("symlink", "file"), + ("symlink", "symlink"), + ] { + assert_eq!( + entry("modified", types).unwrap(), + FileChange::Modified { path: pb.clone() } + ); + } + + for types in [("", "file"), ("", "symlink")] { + assert_eq!( + entry("added", types).unwrap(), + FileChange::Untracked { path: pb.clone() } + ); + } + + for types in [ + ("file", "file"), + ("file", "symlink"), + ("symlink", "file"), + ("symlink", "symlink"), + ] { + assert_eq!( + entry("copied", types).unwrap(), + FileChange::Untracked { path: pb.clone() } + ); + } + + for types in [("conflict", ""), ("file", ""), ("symlink", "")] { + assert_eq!( + entry("removed", types).unwrap(), + FileChange::Deleted { path: pb.clone() } + ); + } + + for types in [ + ("", "conflict"), + ("conflict", "conflict"), + ("file", "conflict"), + ("symlink", "conflict"), + ] { + assert_eq!( + entry("conflict", types).unwrap(), + FileChange::Conflict { path: pb.clone() } + ); + } + + for invalid_kind in ["invalid", ""] { + assert_eq!(entry(invalid_kind, ("file", "file")), None); + } + + for invalid_types in [ + ("tree", "file"), + ("submodule", "file"), + ("abcdef", "file"), + ("file", "tree"), + ("file", "submodule"), + ("file", "abcdef"), + ] { + assert_eq!(entry("modified", invalid_types), None); + } + } + + #[test] + fn test_extract_head_name() { + // No bookmarks. + let result = extract_head_name("abcdefgh \nijklmnop \n").unwrap(); + assert_eq!(result, "abcdefgh"); + + // Single exact bookmark. + let result = extract_head_name("abcdefgh bookmark*\nijklmnop other-bookmark*\n").unwrap(); + assert_eq!(result, "abcdefgh (bookmark*)"); + + // Multiple exact bookmarks. + let result = extract_head_name(concat!( + "abcdefgh bookmark bookmark-v2\n", + "ijklmnop other-ookmark\n", + )) + .unwrap(); + assert_eq!(result, "abcdefgh (bookmark bookmark-v2)"); + + // Single inexact bookmark. + let result = extract_head_name("abcdefgh \nijklmnop other-bookmark\n").unwrap(); + assert_eq!(result, "abcdefgh [other-bookmark]"); + + // Multiple inexact bookmarks. + let result = extract_head_name("abcdefgh \nijklmnop bookmark* bookmark-v2\n").unwrap(); + assert_eq!(result, "abcdefgh [bookmark* bookmark-v2]"); + } +} diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index e337a6a89721..d0d266b7b615 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -2,15 +2,14 @@ //! Currently `git` is the only supported provider for diffs, but this architecture allows //! for other providers to be added in the future. -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; +#[cfg(feature = "jj")] +mod jj; mod diff; @@ -22,121 +21,345 @@ pub use status::FileChange; /// Contains all active diff providers. Diff providers are compiled in via features. Currently /// only `git` is supported. -#[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 { /// Get the given file from the VCS. This provides the unedited document as a "base" /// for a diff to be created. - pub fn get_diff_base(&self, file: &Path, trust_full: bool) -> Option> { - self.providers - .iter() - .find_map(|provider| match provider.get_diff_base(file, trust_full) { - Ok(res) => Some(res), - Err(err) => { - log::debug!("{err:#?}"); - log::debug!("failed to open diff base for {}", file.display()); - None - } - }) + pub fn get_diff_base(&self, file: &Path) -> Option> { + 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 + } + } } /// Get the current name of the current [HEAD](https://stackoverflow.com/questions/2304087/what-is-head-in-git). - pub fn get_current_head_name( - &self, - file: &Path, - trust_full: bool, - ) -> Option>>> { - self.providers.iter().find_map(|provider| { - match provider.get_current_head_name(file, trust_full) { - Ok(res) => Some(res), - Err(err) => { - log::debug!("{err:#?}"); - log::debug!("failed to obtain current head name for {}", file.display()); - None - } + pub fn get_current_head_name(&self, file: &Path) -> Option>>> { + 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, - trust_full: bool, + 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, trust_full, &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(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, trust_full: bool) { + 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: Result<(Arc, PossibleDiffProvider)> = match provider { #[cfg(feature = "git")] - DiffProvider::Git, - DiffProvider::None, - ]; - DiffProviderRegistry { providers } + PossibleDiffProvider::Git => self.add_file_git(repo_path, trust_full), + #[cfg(feature = "jj")] + PossibleDiffProvider::JJ => self.add_file_jj(repo_path, trust_full), + }; + + 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, trust_full: bool) { + self.remove(path); + self.add(path, trust_full); + } + + /// 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) { + // NOTE: we keep the allocated memory for reuse since this is mostly used through + // `reload_all`, in which case the underlying repository is most likely still there and the + // user just wants a clean slate for diffs and other features. + let Self { + providers, + counters, + } = self; + providers.clear(); + counters.clear(); + } +} + +/// Private methods +impl DiffProviderRegistry { + fn provider_for(&self, path: &Path) -> Option<&DiffProvider> { + let path = get_possible_provider(path)?.0; + self.providers.get(path) + } + + // Remove a provider, notably used when its trust setting has changed from the original setup. + fn remove_provider(&mut self, repo_path: &Path) { + // Ensure we d'ont + let Self { + providers, + counters, + } = self; + providers.remove(repo_path); + counters.remove(repo_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, + trust_full: bool, + ) -> Result<(Arc, PossibleDiffProvider)> { + // Don't build a git repo object if there is already one for that path. + if let Some(( + key, + DiffProvider::Git { + trust_full: originally_trusted, + .. + }, + )) = self.providers.get_key_value(repo_path) + { + if *originally_trusted == trust_full { + return Ok((Arc::clone(key), PossibleDiffProvider::Git)); + } else { + self.remove_provider(repo_path); + } + } + + match git::open_repo(repo_path, trust_full) { + Ok(repo) => { + let key = Arc::from(repo_path); + self.providers.insert( + Arc::clone(&key), + DiffProvider::Git { + repo: Box::new(repo), + trust_full, + }, + ); + Ok((key, PossibleDiffProvider::Git)) + } + 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, + trust_full: bool, + ) -> Result<(Arc, PossibleDiffProvider)> { + use anyhow::Context; + + if !trust_full { + log::warn!(concat!( + "JJ repositories are only supported when the repository is trusted in Helix", + " because JJ itself does not expose a trust setting yet.\n", + "\n", + "See and ", + "." + )); + anyhow::bail!("Only fully trusted workspaces are supported for JJ repositories"); + } + let repo_path = std::fs::canonicalize(repo_path) + .context("failed to canonicalize potential jj repo path")?; + + // 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(Copy, Clone)] -enum DiffProvider { +#[derive(Clone)] +pub enum DiffProvider { #[cfg(feature = "git")] - Git, - None, + Git { + repo: Box, + trust_full: bool, + }, + /// 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(any(feature = "git", feature = "jj")), allow(unused))] impl DiffProvider { - fn get_diff_base(&self, file: &Path, trust_full: bool) -> Result> { - match self { + 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. + match *self { #[cfg(feature = "git")] - Self::Git => git::get_diff_base(file, trust_full), - Self::None => bail!("No diff support compiled in"), + Self::Git { ref repo, .. } => git::get_diff_base(repo, file), + #[cfg(feature = "jj")] + Self::JJ(ref repo) => jj::get_diff_base(repo, file), } } - fn get_current_head_name( - &self, - file: &Path, - trust_full: bool, - ) -> Result>>> { - match self { + fn get_current_head_name(&self) -> Result>>> { + match *self { #[cfg(feature = "git")] - Self::Git => git::get_current_head_name(file, trust_full), - Self::None => bail!("No diff support compiled in"), + Self::Git { ref repo, .. } => git::get_current_head_name(repo), + #[cfg(feature = "jj")] + Self::JJ(ref repo) => jj::get_current_head_name(repo), } } - fn for_each_changed_file( - &self, - cwd: &Path, - trust_full: bool, - f: impl Fn(Result) -> bool, - ) -> Result<()> { - match self { + fn for_each_changed_file(&self, f: impl Fn(Result) -> bool) -> Result<()> { + match *self { #[cfg(feature = "git")] - Self::Git => git::for_each_changed_file(cwd, trust_full, f), - Self::None => bail!("No diff support compiled in"), + 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), } } } + +#[derive(Debug, Copy, Clone)] +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 +/// +/// 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)> { + // 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() { + for &(repo_indic, pdp) in checks { + if let Ok(true) = parent.join(repo_indic).try_exists() { + return Some((parent, pdp)); + } + } + } + } + + None +} diff --git a/helix-vcs/src/status.rs b/helix-vcs/src/status.rs index 0240cad1a020..5ef301b87780 100644 --- a/helix-vcs/src/status.rs +++ b/helix-vcs/src/status.rs @@ -1,6 +1,7 @@ use std::path::{Path, PathBuf}; /// States for a file having been changed. +#[derive(Debug, PartialEq, Eq)] pub enum FileChange { /// Not tracked by the VCS. Untracked { path: PathBuf }, diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 72aa97f9077f..50440c81f5c0 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1282,7 +1282,7 @@ impl Document { pub fn reload( &mut self, view: &mut View, - provider_registry: &DiffProviderRegistry, + provider_registry: &mut DiffProviderRegistry, trust_full: bool, ) -> Result<(), Error> { let encoding = self.encoding; @@ -1294,6 +1294,8 @@ impl Document { }, }; + provider_registry.reload(&path, trust_full); + // Once we have a valid path we check if its readonly status has changed self.detect_readonly(); @@ -1310,12 +1312,12 @@ impl Document { self.pickup_last_saved_time(); self.detect_indent_and_line_ending(); - match provider_registry.get_diff_base(&path, trust_full) { + match provider_registry.get_diff_base(&path) { Some(diff_base) => self.set_diff_base(diff_base), None => self.diff_handle = None, } - self.version_control_head = provider_registry.get_current_head_name(&path, trust_full); + self.version_control_head = provider_registry.get_current_head_name(&path); Ok(()) } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index adced1884f6a..bb2bdd5dab73 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -2122,12 +2122,12 @@ impl Editor { .workspace_trust .query(doc.workspace_root(), TrustQuery::Git) .is_trusted(); - if let Some(diff_base) = self.diff_providers.get_diff_base(&path, trust_full) { + // When opening a *new* file, ensure its diff provider is loaded. + self.diff_providers.add(&path, trust_full); + if let Some(diff_base) = self.diff_providers.get_diff_base(&path) { doc.set_diff_base(diff_base); } - doc.set_version_control_head( - self.diff_providers.get_current_head_name(&path, trust_full), - ); + doc.set_version_control_head(self.diff_providers.get_current_head_name(&path)); let id = self.new_document(doc); self.launch_language_servers(id); @@ -2163,6 +2163,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);