cleanup: remove useless Git struct, using free functions instead

helix-vcs/src/git.rs

@@ -17,191 +17,174 @@
 };
 use gix::{Commit, ObjectId, Repository, ThreadSafeRepository};
 
-use crate::{DiffProvider, FileChange};
+use crate::FileChange;
 
 #[cfg(test)]
 mod test;
 
-#[derive(Clone, Copy)]
+pub fn get_diff_base(file: &Path) -> Result<Vec<u8>> {
-pub struct Git;
+    debug_assert!(!file.exists() || file.is_file());
+    debug_assert!(file.is_absolute());
 
-impl Git {
+    // TODO cache repository lookup
-    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();
 
-        // On windows various configuration options are bundled as part of the installations
+    let repo_dir = file.parent().context("file has no parent directory")?;
-        // This path depends on the install location of git and therefore requires some overhead to lookup
+    let repo = open_repo(repo_dir, None)
-        // This is basically only used on windows and has some overhead hence it's disabled on other platforms.
+        .context("failed to open git repo")?
-        // `gitoxide` doesn't use this as default
+        .to_thread_local();
-        let config = gix::open::permissions::Config {
+    let head = repo.head_commit()?;
-            system: true,
+    let file_oid = find_file_in_commit(&repo, &head, file)?;
-            git: true,
-            user: true,
-            env: true,
-            includes: true,
-            git_binary: cfg!(windows),
-        };
-        // change options for config permissions without touching anything else
-        git_open_opts_map.reduced = git_open_opts_map
-            .reduced
-            .permissions(gix::open::Permissions {
-                config,
-                ..gix::open::Permissions::default_for_level(gix::sec::Trust::Reduced)
-            });
-        git_open_opts_map.full = git_open_opts_map.full.permissions(gix::open::Permissions {
-            config,
-            ..gix::open::Permissions::default_for_level(gix::sec::Trust::Full)
-        });
 
-        let open_options = gix::discover::upwards::Options {
+    let file_object = repo.find_object(file_oid)?;
-            ceiling_dirs: ceiling_dir
+    let data = file_object.detach().data;
-                .map(|dir| vec![dir.to_owned()])
+    // Get the actual data that git would make out of the git object.
-                .unwrap_or_default(),
+    // This will apply the user's git config or attributes like crlf conversions.
-            dot_git_only: true,
+    if let Some(work_dir) = repo.work_dir() {
-            ..Default::default()
+        let rela_path = file.strip_prefix(work_dir)?;
-        };
+        let rela_path = gix::path::try_into_bstr(rela_path)?;
-
+        let (mut pipeline, _) = repo.filter_pipeline(None)?;
-        let res = ThreadSafeRepository::discover_with_environment_overrides_opts(
+        let mut worktree_outcome =
-            path,
+            pipeline.convert_to_worktree(&data, rela_path.as_ref(), Delay::Forbid)?;
-            open_options,
+        let mut buf = Vec::with_capacity(data.len());
-            git_open_opts_map,
+        worktree_outcome.read_to_end(&mut buf)?;
-        )?;
+        Ok(buf)
-
+    } else {
-        Ok(res)
+        Ok(data)
     }
+}
 
-    /// Emulates the result of running `git status` from the command line.
+pub fn get_current_head_name(file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
-    fn status(repo: &Repository, f: impl Fn(Result<FileChange>) -> bool) -> Result<()> {
+    debug_assert!(!file.exists() || file.is_file());
-        let work_dir = repo
+    debug_assert!(file.is_absolute());
-            .work_dir()
+    let repo_dir = file.parent().context("file has no parent directory")?;
-            .ok_or_else(|| anyhow::anyhow!("working tree not found"))?
+    let repo = open_repo(repo_dir, None)
-            .to_path_buf();
+        .context("failed to open git repo")?
+        .to_thread_local();
+    let head_ref = repo.head_ref()?;
+    let head_commit = repo.head_commit()?;
 
-        let status_platform = repo
+    let name = match head_ref {
-            .status(gix::progress::Discard)?
+        Some(reference) => reference.name().shorten().to_string(),
-            // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in
+        None => head_commit.id.to_hex_with_len(8).to_string(),
-            // our case to not list new (untracked) files. We could have respected this config
+    };
-            // if the default value weren't `Collapsed` though, as this default value would render
-            // the feature unusable to many.
-            .untracked_files(UntrackedFiles::Files)
-            // Turn on file rename detection, which is off by default.
-            .index_worktree_rewrites(Some(Rewrites {
-                copies: None,
-                percentage: Some(0.5),
-                limit: 1000,
-            }));
 
-        // No filtering based on path
+    Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str())))
-        let empty_patterns = vec![];
+}
 
-        let status_iter = status_platform.into_index_worktree_iter(empty_patterns)?;
+pub fn for_each_changed_file(cwd: &Path, f: impl Fn(Result<FileChange>) -> bool) -> Result<()> {
+    status(&open_repo(cwd, None)?.to_thread_local(), f)
+}
 
-        for item in status_iter {
+fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result<ThreadSafeRepository> {
-            let Ok(item) = item.map_err(|err| f(Err(err.into()))) else {
+    // custom open options
+    let mut git_open_opts_map = gix::sec::trust::Mapping::<gix::open::Options>::default();
+
+    // On windows various configuration options are bundled as part of the installations
+    // This path depends on the install location of git and therefore requires some overhead to lookup
+    // This is basically only used on windows and has some overhead hence it's disabled on other platforms.
+    // `gitoxide` doesn't use this as default
+    let config = gix::open::permissions::Config {
+        system: true,
+        git: true,
+        user: true,
+        env: true,
+        includes: true,
+        git_binary: cfg!(windows),
+    };
+    // change options for config permissions without touching anything else
+    git_open_opts_map.reduced = git_open_opts_map
+        .reduced
+        .permissions(gix::open::Permissions {
+            config,
+            ..gix::open::Permissions::default_for_level(gix::sec::Trust::Reduced)
+        });
+    git_open_opts_map.full = git_open_opts_map.full.permissions(gix::open::Permissions {
+        config,
+        ..gix::open::Permissions::default_for_level(gix::sec::Trust::Full)
+    });
+
+    let open_options = gix::discover::upwards::Options {
+        ceiling_dirs: ceiling_dir
+            .map(|dir| vec![dir.to_owned()])
+            .unwrap_or_default(),
+        dot_git_only: true,
+        ..Default::default()
+    };
+
+    let res = ThreadSafeRepository::discover_with_environment_overrides_opts(
+        path,
+        open_options,
+        git_open_opts_map,
+    )?;
+
+    Ok(res)
+}
+
+/// Emulates the result of running `git status` from the command line.
+fn status(repo: &Repository, f: impl Fn(Result<FileChange>) -> bool) -> Result<()> {
+    let work_dir = repo
+        .work_dir()
+        .ok_or_else(|| anyhow::anyhow!("working tree not found"))?
+        .to_path_buf();
+
+    let status_platform = repo
+        .status(gix::progress::Discard)?
+        // Here we discard the `status.showUntrackedFiles` config, as it makes little sense in
+        // our case to not list new (untracked) files. We could have respected this config
+        // if the default value weren't `Collapsed` though, as this default value would render
+        // the feature unusable to many.
+        .untracked_files(UntrackedFiles::Files)
+        // Turn on file rename detection, which is off by default.
+        .index_worktree_rewrites(Some(Rewrites {
+            copies: None,
+            percentage: Some(0.5),
+            limit: 1000,
+        }));
+
+    // No filtering based on path
+    let empty_patterns = vec![];
+
+    let status_iter = status_platform.into_index_worktree_iter(empty_patterns)?;
+
+    for item in status_iter {
+        let Ok(item) = item.map_err(|err| f(Err(err.into()))) else {
                 continue;
             };
-            let change = match item {
+        let change = match item {
-                Item::Modification {
+            Item::Modification {
-                    rela_path, status, ..
+                rela_path, status, ..
-                } => {
+            } => {
-                    let path = work_dir.join(rela_path.to_path()?);
+                let path = work_dir.join(rela_path.to_path()?);
-                    match status {
+                match status {
-                        EntryStatus::Conflict(_) => FileChange::Conflict { path },
+                    EntryStatus::Conflict(_) => FileChange::Conflict { path },
-                        EntryStatus::Change(Change::Removed) => FileChange::Deleted { path },
+                    EntryStatus::Change(Change::Removed) => FileChange::Deleted { path },
-                        EntryStatus::Change(Change::Modification { .. }) => {
+                    EntryStatus::Change(Change::Modification { .. }) => {
-                            FileChange::Modified { path }
+                        FileChange::Modified { path }
-                        }
-                        _ => continue,
                     }
+                    _ => continue,
                 }
-                Item::DirectoryContents { entry, .. } if entry.status == Status::Untracked => {
-                    FileChange::Untracked {
-                        path: work_dir.join(entry.rela_path.to_path()?),
-                    }
-                }
-                Item::Rewrite {
-                    source,
-                    dirwalk_entry,
-                    ..
-                } => FileChange::Renamed {
-                    from_path: work_dir.join(source.rela_path().to_path()?),
-                    to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?),
-                },
-                _ => continue,
-            };
-            if !f(Ok(change)) {
-                break;
             }
-        }
+            Item::DirectoryContents { entry, .. } if entry.status == Status::Untracked => {
-
+                FileChange::Untracked {
-        Ok(())
+                    path: work_dir.join(entry.rela_path.to_path()?),
-    }
+                }
-}
+            }
-
+            Item::Rewrite {
-impl Git {
+                source,
-    pub fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>> {
+                dirwalk_entry,
-        debug_assert!(!file.exists() || file.is_file());
+                ..
-        debug_assert!(file.is_absolute());
+            } => FileChange::Renamed {
-
+                from_path: work_dir.join(source.rela_path().to_path()?),
-        // TODO cache repository lookup
+                to_path: work_dir.join(dirwalk_entry.rela_path.to_path()?),
-
+            },
-        let repo_dir = file.parent().context("file has no parent directory")?;
+            _ => continue,
-        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)?;
-        let data = file_object.detach().data;
-        // Get the actual data that git would make out of the git object.
-        // This will apply the user's git config or attributes like crlf conversions.
-        if let Some(work_dir) = repo.work_dir() {
-            let rela_path = file.strip_prefix(work_dir)?;
-            let rela_path = gix::path::try_into_bstr(rela_path)?;
-            let (mut pipeline, _) = repo.filter_pipeline(None)?;
-            let mut worktree_outcome =
-                pipeline.convert_to_worktree(&data, rela_path.as_ref(), Delay::Forbid)?;
-            let mut buf = Vec::with_capacity(data.len());
-            worktree_outcome.read_to_end(&mut buf)?;
-            Ok(buf)
-        } else {
-            Ok(data)
-        }
-    }
-
-    pub 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_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(),
         };
-
+        if !f(Ok(change)) {
-        Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str())))
+            break;
+        }
     }
 
-    pub fn for_each_changed_file(
+    Ok(())
-        &self,
-        cwd: &Path,
-        f: impl Fn(Result<FileChange>) -> bool,
-    ) -> Result<()> {
-        Self::status(&Self::open_repo(cwd, None)?.to_thread_local(), f)
-    }
-}
-
-impl From<Git> for DiffProvider {
-    fn from(value: Git) -> Self {
-        DiffProvider::Git(value)
-    }
 }
 
 /// Finds the object that contains the contents of a file at a specific commit.

helix-vcs/src/git/test.rs

@@ -2,7 +2,7 @@
 
 use tempfile::TempDir;
 
-use crate::git::Git;
+use crate::git;
 
 fn exec_git_cmd(args: &str, git_dir: &Path) {
     let res = Command::new("git")
@@ -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!(Git.get_diff_base(&file).is_err());
+    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).unwrap(), 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).unwrap(), 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!(Git.get_diff_base(&dir).is_err());
+    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!(Git.get_diff_base(&file_link).is_err());
+    assert!(git::get_diff_base(&file_link).is_err());
-    assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents));
+    assert_eq!(git::get_diff_base(&file).unwrap(), Vec::from(contents));
 }

helix-vcs/src/lib.rs

@@ -1,4 +1,4 @@
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, bail, Result};
 use arc_swap::ArcSwap;
 use std::{
     path::{Path, PathBuf},
@@ -74,7 +74,7 @@ fn default() -> Self {
         // TODO make this configurable when more providers are added
         let providers = vec![
             #[cfg(feature = "git")]
-            git::Git.into(),
+            DiffProvider::Git,
         ];
         DiffProviderRegistry { providers }
     }
@@ -82,24 +82,29 @@ fn default() -> Self {
 
 /// 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.
-#[derive(Clone)]
+///
+/// `Copy` is simply to ensure the `clone()` call is the simplest it can be.
+#[derive(Copy, Clone)]
 pub enum DiffProvider {
     #[cfg(feature = "git")]
-    Git(git::Git),
+    Git,
+    None,
 }
 
 impl DiffProvider {
     fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>> {
         match self {
             #[cfg(feature = "git")]
-            Self::Git(inner) => inner.get_diff_base(file),
+            Self::Git => git::get_diff_base(file),
+            Self::None => bail!("No diff support compiled in"),
         }
     }
 
     fn get_current_head_name(&self, file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
         match self {
             #[cfg(feature = "git")]
-            Self::Git(inner) => inner.get_current_head_name(file),
+            Self::Git => git::get_current_head_name(file),
+            Self::None => bail!("No diff support compiled in"),
         }
     }
 
@@ -110,7 +115,8 @@ fn for_each_changed_file(
     ) -> Result<()> {
         match self {
             #[cfg(feature = "git")]
-            Self::Git(inner) => inner.for_each_changed_file(cwd, f),
+            Self::Git => git::for_each_changed_file(cwd, f),
+            Self::None => bail!("No diff support compiled in"),
         }
     }
 }