From abef92a9b341209aeae8802d30fc8c1f971a43df Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 26 Mar 2023 11:44:07 +0200 Subject: [PATCH] log failures in the git integration (#6441) --- Cargo.lock | 1 + helix-vcs/Cargo.toml | 1 + helix-vcs/src/git.rs | 55 ++++++++++++++++++++++++--------------- helix-vcs/src/git/test.rs | 12 ++++----- helix-vcs/src/lib.rs | 31 ++++++++++++++++------ 5 files changed, 65 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c86b5aac0..0bef317e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1220,6 +1220,7 @@ dependencies = [ name = "helix-vcs" version = "0.6.0" dependencies = [ + "anyhow", "arc-swap", "gix", "helix-core", diff --git a/helix-vcs/Cargo.toml b/helix-vcs/Cargo.toml index 5ad6c91b7..b32c028bd 100644 --- a/helix-vcs/Cargo.toml +++ b/helix-vcs/Cargo.toml @@ -19,6 +19,7 @@ arc-swap = { version = "1.6.0" } gix = { version = "0.41.0", default-features = false , optional = true } imara-diff = "0.1.5" +anyhow = "1" log = "0.4" diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 1732bdd0d..00a2c596d 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -1,3 +1,4 @@ +use anyhow::{bail, Context, Result}; use arc_swap::ArcSwap; use std::path::Path; use std::sync::Arc; @@ -14,7 +15,7 @@ mod test; pub struct Git; impl Git { - fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Option { + fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { // custom open options let mut git_open_opts_map = gix::sec::trust::Mapping::::default(); @@ -45,26 +46,31 @@ impl Git { open_options.ceiling_dirs = vec![ceiling_dir.to_owned()]; } - ThreadSafeRepository::discover_with_environment_overrides_opts( + let res = ThreadSafeRepository::discover_with_environment_overrides_opts( path, open_options, git_open_opts_map, - ) - .ok() + )?; + + Ok(res) } } impl DiffProvider for Git { - fn get_diff_base(&self, file: &Path) -> Option> { + fn get_diff_base(&self, file: &Path) -> Result> { debug_assert!(!file.exists() || file.is_file()); debug_assert!(file.is_absolute()); // TODO cache repository lookup - let repo = Git::open_repo(file.parent()?, None)?.to_thread_local(); - let head = repo.head_commit().ok()?; + + let repo_dir = file.parent().context("file has no parent directory")?; + let repo = Git::open_repo(repo_dir, None) + .context("failed to open git repo")? + .to_thread_local(); + let head = repo.head_commit()?; let file_oid = find_file_in_commit(&repo, &head, file)?; - let file_object = repo.find_object(file_oid).ok()?; + let file_object = repo.find_object(file_oid)?; let mut data = file_object.detach().data; // convert LF to CRLF if configured to avoid showing every line as changed if repo @@ -87,35 +93,42 @@ impl DiffProvider for Git { } data = normalized_file } - Some(data) + Ok(data) } - fn get_current_head_name(&self, file: &Path) -> Option>>> { + fn get_current_head_name(&self, file: &Path) -> Result>>> { debug_assert!(!file.exists() || file.is_file()); debug_assert!(file.is_absolute()); - let repo = Git::open_repo(file.parent()?, None)?.to_thread_local(); - let head_ref = repo.head_ref().ok()?; - let head_commit = repo.head_commit().ok()?; + let repo_dir = file.parent().context("file has no parent directory")?; + let repo = Git::open_repo(repo_dir, None) + .context("failed to open git repo")? + .to_thread_local(); + let head_ref = repo.head_ref()?; + let head_commit = repo.head_commit()?; let name = match head_ref { Some(reference) => reference.name().shorten().to_string(), None => head_commit.id.to_hex_with_len(8).to_string(), }; - Some(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) + Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) } } /// 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) -> Option { - let repo_dir = repo.work_dir()?; - let rel_path = file.strip_prefix(repo_dir).ok()?; - let tree = commit.tree().ok()?; - let tree_entry = tree.lookup_entry_by_path(rel_path).ok()??; +fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Result { + let repo_dir = repo.work_dir().context("repo has no worktree")?; + let rel_path = file.strip_prefix(repo_dir)?; + let tree = commit.tree()?; + let tree_entry = tree + .lookup_entry_by_path(rel_path)? + .context("file is untracked")?; match tree_entry.mode() { // not a file, everything is new, do not show diff - EntryMode::Tree | EntryMode::Commit | EntryMode::Link => None, + mode @ (EntryMode::Tree | EntryMode::Commit | EntryMode::Link) => { + bail!("entry at {} is not a file but a {mode:?}", file.display()) + } // found a file - EntryMode::Blob | EntryMode::BlobExecutable => Some(tree_entry.object_id()), + EntryMode::Blob | EntryMode::BlobExecutable => Ok(tree_entry.object_id()), } } diff --git a/helix-vcs/src/git/test.rs b/helix-vcs/src/git/test.rs index 6b1aba7f5..9c67d2c33 100644 --- a/helix-vcs/src/git/test.rs +++ b/helix-vcs/src/git/test.rs @@ -54,7 +54,7 @@ fn missing_file() { let file = temp_git.path().join("file.txt"); File::create(&file).unwrap().write_all(b"foo").unwrap(); - assert_eq!(Git.get_diff_base(&file), None); + assert!(Git.get_diff_base(&file).is_err()); } #[test] @@ -64,7 +64,7 @@ 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), Some(Vec::from(contents))); + assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents)); } #[test] @@ -76,7 +76,7 @@ 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), Some(Vec::from(contents))); + assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents)); } /// Test that `get_file_head` does not return content for a directory. @@ -95,7 +95,7 @@ fn directory() { std::fs::remove_dir_all(&dir).unwrap(); File::create(&dir).unwrap().write_all(b"bar").unwrap(); - assert_eq!(Git.get_diff_base(&dir), None); + assert!(Git.get_diff_base(&dir).is_err()); } /// Test that `get_file_head` does not return content for a symlink. @@ -116,6 +116,6 @@ fn symlink() { symlink("file.txt", &file_link).unwrap(); create_commit(temp_git.path(), true); - assert_eq!(Git.get_diff_base(&file_link), None); - assert_eq!(Git.get_diff_base(&file), Some(Vec::from(contents))); + assert!(Git.get_diff_base(&file_link).is_err()); + assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents)); } diff --git a/helix-vcs/src/lib.rs b/helix-vcs/src/lib.rs index 6f5e40d0c..4d3a3623b 100644 --- a/helix-vcs/src/lib.rs +++ b/helix-vcs/src/lib.rs @@ -1,3 +1,4 @@ +use anyhow::{bail, Result}; use arc_swap::ArcSwap; use std::{path::Path, sync::Arc}; @@ -18,19 +19,19 @@ pub trait DiffProvider { /// 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) -> Option>; - fn get_current_head_name(&self, file: &Path) -> Option>>>; + fn get_diff_base(&self, file: &Path) -> Result>; + fn get_current_head_name(&self, file: &Path) -> Result>>>; } #[doc(hidden)] pub struct Dummy; impl DiffProvider for Dummy { - fn get_diff_base(&self, _file: &Path) -> Option> { - None + fn get_diff_base(&self, _file: &Path) -> Result> { + bail!("helix was compiled without git support") } - fn get_current_head_name(&self, _file: &Path) -> Option>>> { - None + fn get_current_head_name(&self, _file: &Path) -> Result>>> { + bail!("helix was compiled without git support") } } @@ -42,13 +43,27 @@ impl DiffProviderRegistry { pub fn get_diff_base(&self, file: &Path) -> Option> { self.providers .iter() - .find_map(|provider| provider.get_diff_base(file)) + .find_map(|provider| match provider.get_diff_base(file) { + Ok(res) => Some(res), + Err(err) => { + log::error!("{err:#?}"); + log::error!("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| provider.get_current_head_name(file)) + .find_map(|provider| match provider.get_current_head_name(file) { + Ok(res) => Some(res), + Err(err) => { + log::error!("{err:#?}"); + log::error!("failed to obtain current head name for {}", file.display()); + None + } + }) } }