Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 20 additions & 1 deletion book/src/workspace-trust.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
4 changes: 3 additions & 1 deletion helix-term/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
22 changes: 15 additions & 7 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ use std::{
future::Future,
io::Read,
num::NonZeroUsize,
sync::Arc,
};

use std::{
Expand Down Expand Up @@ -3481,15 +3482,15 @@ fn jumplist_picker(cx: &mut Context) {

fn changed_file_picker(cx: &mut Context) {
pub struct FileChangeData {
cwd: PathBuf,
cwd: Arc<Path>,
style_untracked: Style,
style_modified: Style,
style_conflict: Style,
style_deleted: Style,
style_renamed: Style,
}

let cwd = helix_stdx::env::current_working_dir();
let cwd: Arc<Path> = Arc::from(helix_stdx::env::current_working_dir().as_path());
if !cwd.exists() {
cx.editor
.set_error("Current working directory does not exist");
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 4 additions & 2 deletions helix-term/src/commands/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
})?;
Expand Down Expand Up @@ -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);

Expand All @@ -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;
}
Expand Down
6 changes: 4 additions & 2 deletions helix-vcs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
40 changes: 13 additions & 27 deletions helix-vcs/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<u8>> {
pub(super) fn get_diff_base(repo: &ThreadSafeRepository, file: &Path) -> Result<Vec<u8>> {
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)?;

Expand Down Expand Up @@ -65,15 +55,8 @@ pub fn get_diff_base(file: &Path, trust_full: bool) -> Result<Vec<u8>> {
}
}

pub fn get_current_head_name(file: &Path, trust_full: bool) -> Result<Arc<ArcSwap<Box<str>>>> {
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<Arc<ArcSwap<Box<str>>>> {
let repo = repo.to_thread_local();
let head_ref = repo.head_ref()?;
let head_commit = repo.head_commit()?;

Expand All @@ -85,15 +68,14 @@ pub fn get_current_head_name(file: &Path, trust_full: bool) -> Result<Arc<ArcSwa
Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str())))
}

pub fn for_each_changed_file(
cwd: &Path,
trust_full: bool,
pub(super) fn for_each_changed_file(
repo: &ThreadSafeRepository,
f: impl Fn(Result<FileChange>) -> 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<ThreadSafeRepository> {
pub(super) fn open_repo(path: &Path, trust_full: bool) -> Result<ThreadSafeRepository> {
// `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
Expand All @@ -110,6 +92,10 @@ fn open_repo(path: &Path, trust_full: bool) -> Result<ThreadSafeRepository> {
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 {
Expand All @@ -130,7 +116,7 @@ fn open_repo(path: &Path, trust_full: bool) -> Result<ThreadSafeRepository> {
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();

Expand Down
24 changes: 16 additions & 8 deletions helix-vcs/src/git/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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)
);
}
Expand All @@ -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)
);
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}
Loading