From f1461b49fa547a50a28f85d5de69e45d7b8143aa Mon Sep 17 00:00:00 2001 From: "Alexis (Poliorcetics) Bourget" Date: Sat, 27 Apr 2024 14:36:48 +0200 Subject: [PATCH] cleanup: remove useless Git struct, using free functions instead --- helix-vcs/src/git.rs | 319 ++++++++++++++++++-------------------- helix-vcs/src/git/test.rs | 14 +- helix-vcs/src/lib.rs | 20 ++- 3 files changed, 171 insertions(+), 182 deletions(-) diff --git a/helix-vcs/src/git.rs b/helix-vcs/src/git.rs index 8d935b5fc..080f9b633 100644 --- a/helix-vcs/src/git.rs +++ b/helix-vcs/src/git.rs @@ -17,191 +17,174 @@ use gix::status::{ }; use gix::{Commit, ObjectId, Repository, ThreadSafeRepository}; -use crate::{DiffProvider, FileChange}; +use crate::FileChange; #[cfg(test)] mod test; -#[derive(Clone, Copy)] -pub struct Git; - -impl Git { - fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { - // custom open options - let mut git_open_opts_map = gix::sec::trust::Mapping::::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) - }); +pub fn get_diff_base(file: &Path) -> Result> { + debug_assert!(!file.exists() || file.is_file()); + debug_assert!(file.is_absolute()); + + // TODO cache repository lookup + + let repo_dir = file.parent().context("file has no parent directory")?; + let repo = 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) + } +} - 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() - }; +pub fn get_current_head_name(file: &Path) -> Result>>> { + 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 = 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(), + }; + + Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) +} - let res = ThreadSafeRepository::discover_with_environment_overrides_opts( - path, - open_options, - git_open_opts_map, - )?; +pub fn for_each_changed_file(cwd: &Path, f: impl Fn(Result) -> bool) -> Result<()> { + status(&open_repo(cwd, None)?.to_thread_local(), f) +} - Ok(res) - } +fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result { + // custom open options + let mut git_open_opts_map = gix::sec::trust::Mapping::::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) -> 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 { +/// Emulates the result of running `git status` from the command line. +fn status(repo: &Repository, f: impl Fn(Result) -> 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 { - Item::Modification { - rela_path, status, .. - } => { - let path = work_dir.join(rela_path.to_path()?); - match status { - EntryStatus::Conflict(_) => FileChange::Conflict { path }, - EntryStatus::Change(Change::Removed) => FileChange::Deleted { path }, - EntryStatus::Change(Change::Modification { .. }) => { - FileChange::Modified { path } - } - _ => continue, + let change = match item { + Item::Modification { + rela_path, status, .. + } => { + let path = work_dir.join(rela_path.to_path()?); + match status { + EntryStatus::Conflict(_) => FileChange::Conflict { path }, + EntryStatus::Change(Change::Removed) => FileChange::Deleted { path }, + EntryStatus::Change(Change::Modification { .. }) => { + FileChange::Modified { path } } + _ => continue, } - Item::DirectoryContents { entry, .. } if entry.status == Status::Untracked => { - FileChange::Untracked { - path: work_dir.join(entry.rela_path.to_path()?), - } + } + 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; } - } - - Ok(()) - } -} - -impl Git { - pub 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_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)?; - 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>>> { - 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(), + 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, }; - - Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str()))) - } - - pub fn for_each_changed_file( - &self, - cwd: &Path, - f: impl Fn(Result) -> bool, - ) -> Result<()> { - Self::status(&Self::open_repo(cwd, None)?.to_thread_local(), f) + if !f(Ok(change)) { + break; + } } -} -impl From for DiffProvider { - fn from(value: Git) -> Self { - DiffProvider::Git(value) - } + Ok(()) } /// Finds the object that contains the contents of a file at a specific commit. diff --git a/helix-vcs/src/git/test.rs b/helix-vcs/src/git/test.rs index cb3397323..95ff10b23 100644 --- a/helix-vcs/src/git/test.rs +++ b/helix-vcs/src/git/test.rs @@ -2,7 +2,7 @@ use std::{fs::File, io::Write, path::Path, process::Command}; 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_eq!(Git.get_diff_base(&file).unwrap(), 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 6382928d4..539be779a 100644 --- a/helix-vcs/src/lib.rs +++ b/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 @@ impl Default for DiffProviderRegistry { // 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 @@ impl Default for DiffProviderRegistry { /// 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> { 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>>> { 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 @@ impl DiffProvider { ) -> 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"), } } }