log failures in the git integration (#6441)

Cargo.lock

@@ -1220,6 +1220,7 @@ dependencies = [
 name = "helix-vcs"
 version = "0.6.0"
 dependencies = [
+ "anyhow",
  "arc-swap",
  "gix",
  "helix-core",

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"
 

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 @@
 pub struct Git;
 
 impl Git {
-    fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Option<ThreadSafeRepository> {
+    fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result<ThreadSafeRepository> {
         // custom open options
         let mut git_open_opts_map = gix::sec::trust::Mapping::<gix::open::Options>::default();
 
@@ -45,26 +46,31 @@ fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Option<ThreadSafeReposi
             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<Vec<u8>> {
+    fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>> {
         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 @@ fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>> {
             }
             data = normalized_file
         }
-        Some(data)
+        Ok(data)
     }
 
-    fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
+    fn get_current_head_name(&self, file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
         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<ObjectId> {
-    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<ObjectId> {
+    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()),
     }
 }

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));
 }

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<Vec<u8>>;
-    fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>>;
+    fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>>;
+    fn get_current_head_name(&self, file: &Path) -> Result<Arc<ArcSwap<Box<str>>>>;
 }
 
 #[doc(hidden)]
 pub struct Dummy;
 impl DiffProvider for Dummy {
-    fn get_diff_base(&self, _file: &Path) -> Option<Vec<u8>> {
-        None
+    fn get_diff_base(&self, _file: &Path) -> Result<Vec<u8>> {
+        bail!("helix was compiled without git support")
     }
 
-    fn get_current_head_name(&self, _file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
-        None
+    fn get_current_head_name(&self, _file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
+        bail!("helix was compiled without git support")
     }
 }
 
@@ -42,13 +43,27 @@ impl DiffProviderRegistry {
     pub fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>> {
         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<Arc<ArcSwap<Box<str>>>> {
         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
+                }
+            })
     }
 }