git.iko.soy/helix
on master help sign in
log failures in the git integration (#6441)
abef92aPascal Kuthe
Cargo.lock+1
compare with master
unifiedsplit
@@ -1220,6 +1220,7 @@ dependencies = [
1220name = "helix-vcs"
1220name = "helix-vcs"
1221version = "0.6.0"
1221version = "0.6.0"
1222dependencies = [
1222dependencies = [
1223+ "anyhow",
1223 "arc-swap",
1224 "arc-swap",
1224 "gix",
1225 "gix",
1225 "helix-core",
1226 "helix-core",
helix-vcs/Cargo.toml+1
@@ -19,6 +19,7 @@ arc-swap = { version = "1.6.0" }
19
19
20gix = { version = "0.41.0", default-features = false , optional = true }
20gix = { version = "0.41.0", default-features = false , optional = true }
21imara-diff = "0.1.5"
21imara-diff = "0.1.5"
22+anyhow = "1"
22
23
23log = "0.4"
24log = "0.4"
24
25
helix-vcs/src/git.rs+34−21
@@ -1,3 +1,4 @@
1+use anyhow::{bail, Context, Result};
1use arc_swap::ArcSwap;
2use arc_swap::ArcSwap;
2use std::path::Path;
3use std::path::Path;
3use std::sync::Arc;
4use std::sync::Arc;
@@ -14,7 +15,7 @@ mod test;
14pub struct Git;
15pub struct Git;
15
16
16impl Git {
17impl Git {
17− fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Option<ThreadSafeRepository> {
18+ fn open_repo(path: &Path, ceiling_dir: Option<&Path>) -> Result<ThreadSafeRepository> {
18 // custom open options
19 // custom open options
19 let mut git_open_opts_map = gix::sec::trust::Mapping::<gix::open::Options>::default();
20 let mut git_open_opts_map = gix::sec::trust::Mapping::<gix::open::Options>::default();
20
21
@@ -45,26 +46,31 @@ impl Git {
45 open_options.ceiling_dirs = vec![ceiling_dir.to_owned()];
46 open_options.ceiling_dirs = vec![ceiling_dir.to_owned()];
46 }
47 }
47
48
48− ThreadSafeRepository::discover_with_environment_overrides_opts(
49+ let res = ThreadSafeRepository::discover_with_environment_overrides_opts(
49 path,
50 path,
50 open_options,
51 open_options,
51 git_open_opts_map,
52 git_open_opts_map,
52− )
53+ )?;
53− .ok()
54+
55+ Ok(res)
54 }
56 }
55}
57}
56
58
57impl DiffProvider for Git {
59impl DiffProvider for Git {
58− fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>> {
60+ fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>> {
59 debug_assert!(!file.exists() || file.is_file());
61 debug_assert!(!file.exists() || file.is_file());
60 debug_assert!(file.is_absolute());
62 debug_assert!(file.is_absolute());
61
63
62 // TODO cache repository lookup
64 // TODO cache repository lookup
65+
66+ let repo_dir = file.parent().context("file has no parent directory")?;
63− let repo = Git::open_repo(file.parent()?, None)?.to_thread_local();
67+ let repo = Git::open_repo(repo_dir, None)
68+ .context("failed to open git repo")?
69+ .to_thread_local();
64− let head = repo.head_commit().ok()?;
70+ let head = repo.head_commit()?;
65 let file_oid = find_file_in_commit(&repo, &head, file)?;
71 let file_oid = find_file_in_commit(&repo, &head, file)?;
66
72
67− let file_object = repo.find_object(file_oid).ok()?;
73+ let file_object = repo.find_object(file_oid)?;
68 let mut data = file_object.detach().data;
74 let mut data = file_object.detach().data;
69 // convert LF to CRLF if configured to avoid showing every line as changed
75 // convert LF to CRLF if configured to avoid showing every line as changed
70 if repo
76 if repo
@@ -87,35 +93,42 @@ impl DiffProvider for Git {
87 }
93 }
88 data = normalized_file
94 data = normalized_file
89 }
95 }
90− Some(data)
96+ Ok(data)
91 }
97 }
92
98
93− fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
99+ fn get_current_head_name(&self, file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
94 debug_assert!(!file.exists() || file.is_file());
100 debug_assert!(!file.exists() || file.is_file());
95 debug_assert!(file.is_absolute());
101 debug_assert!(file.is_absolute());
102+ let repo_dir = file.parent().context("file has no parent directory")?;
96− let repo = Git::open_repo(file.parent()?, None)?.to_thread_local();
103+ let repo = Git::open_repo(repo_dir, None)
104+ .context("failed to open git repo")?
105+ .to_thread_local();
97− let head_ref = repo.head_ref().ok()?;
106+ let head_ref = repo.head_ref()?;
98− let head_commit = repo.head_commit().ok()?;
107+ let head_commit = repo.head_commit()?;
99
108
100 let name = match head_ref {
109 let name = match head_ref {
101 Some(reference) => reference.name().shorten().to_string(),
110 Some(reference) => reference.name().shorten().to_string(),
102 None => head_commit.id.to_hex_with_len(8).to_string(),
111 None => head_commit.id.to_hex_with_len(8).to_string(),
103 };
112 };
104
113
105− Some(Arc::new(ArcSwap::from_pointee(name.into_boxed_str())))
114+ Ok(Arc::new(ArcSwap::from_pointee(name.into_boxed_str())))
106 }
115 }
107}
116}
108
117
109/// Finds the object that contains the contents of a file at a specific commit.
118/// Finds the object that contains the contents of a file at a specific commit.
110−fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Option<ObjectId> {
119+fn find_file_in_commit(repo: &Repository, commit: &Commit, file: &Path) -> Result<ObjectId> {
111− let repo_dir = repo.work_dir()?;
120+ let repo_dir = repo.work_dir().context("repo has no worktree")?;
112− let rel_path = file.strip_prefix(repo_dir).ok()?;
121+ let rel_path = file.strip_prefix(repo_dir)?;
113− let tree = commit.tree().ok()?;
122+ let tree = commit.tree()?;
123+ let tree_entry = tree
114− let tree_entry = tree.lookup_entry_by_path(rel_path).ok()??;
124+ .lookup_entry_by_path(rel_path)?
125+ .context("file is untracked")?;
115 match tree_entry.mode() {
126 match tree_entry.mode() {
116 // not a file, everything is new, do not show diff
127 // not a file, everything is new, do not show diff
117− EntryMode::Tree | EntryMode::Commit | EntryMode::Link => None,
128+ mode @ (EntryMode::Tree | EntryMode::Commit | EntryMode::Link) => {
129+ bail!("entry at {} is not a file but a {mode:?}", file.display())
130+ }
118 // found a file
131 // found a file
119− EntryMode::Blob | EntryMode::BlobExecutable => Some(tree_entry.object_id()),
132+ EntryMode::Blob | EntryMode::BlobExecutable => Ok(tree_entry.object_id()),
120 }
133 }
121}
134}
helix-vcs/src/git/test.rs+6−6
@@ -54,7 +54,7 @@ fn missing_file() {
54 let file = temp_git.path().join("file.txt");
54 let file = temp_git.path().join("file.txt");
55 File::create(&file).unwrap().write_all(b"foo").unwrap();
55 File::create(&file).unwrap().write_all(b"foo").unwrap();
56
56
57− assert_eq!(Git.get_diff_base(&file), None);
57+ assert!(Git.get_diff_base(&file).is_err());
58}
58}
59
59
60#[test]
60#[test]
@@ -64,7 +64,7 @@ fn unmodified_file() {
64 let contents = b"foo".as_slice();
64 let contents = b"foo".as_slice();
65 File::create(&file).unwrap().write_all(contents).unwrap();
65 File::create(&file).unwrap().write_all(contents).unwrap();
66 create_commit(temp_git.path(), true);
66 create_commit(temp_git.path(), true);
67− assert_eq!(Git.get_diff_base(&file), Some(Vec::from(contents)));
67+ assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents));
68}
68}
69
69
70#[test]
70#[test]
@@ -76,7 +76,7 @@ fn modified_file() {
76 create_commit(temp_git.path(), true);
76 create_commit(temp_git.path(), true);
77 File::create(&file).unwrap().write_all(b"bar").unwrap();
77 File::create(&file).unwrap().write_all(b"bar").unwrap();
78
78
79− assert_eq!(Git.get_diff_base(&file), Some(Vec::from(contents)));
79+ assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents));
80}
80}
81
81
82/// Test that `get_file_head` does not return content for a directory.
82/// Test that `get_file_head` does not return content for a directory.
@@ -95,7 +95,7 @@ fn directory() {
95
95
96 std::fs::remove_dir_all(&dir).unwrap();
96 std::fs::remove_dir_all(&dir).unwrap();
97 File::create(&dir).unwrap().write_all(b"bar").unwrap();
97 File::create(&dir).unwrap().write_all(b"bar").unwrap();
98− assert_eq!(Git.get_diff_base(&dir), None);
98+ assert!(Git.get_diff_base(&dir).is_err());
99}
99}
100
100
101/// Test that `get_file_head` does not return content for a symlink.
101/// Test that `get_file_head` does not return content for a symlink.
@@ -116,6 +116,6 @@ fn symlink() {
116 symlink("file.txt", &file_link).unwrap();
116 symlink("file.txt", &file_link).unwrap();
117
117
118 create_commit(temp_git.path(), true);
118 create_commit(temp_git.path(), true);
119− assert_eq!(Git.get_diff_base(&file_link), None);
119+ assert!(Git.get_diff_base(&file_link).is_err());
120− assert_eq!(Git.get_diff_base(&file), Some(Vec::from(contents)));
120+ assert_eq!(Git.get_diff_base(&file).unwrap(), Vec::from(contents));
121}
121}
helix-vcs/src/lib.rs+23−8
@@ -1,3 +1,4 @@
1+use anyhow::{bail, Result};
1use arc_swap::ArcSwap;
2use arc_swap::ArcSwap;
2use std::{path::Path, sync::Arc};
3use std::{path::Path, sync::Arc};
3
4
@@ -18,19 +19,19 @@ pub trait DiffProvider {
18 /// if this provider is used.
19 /// if this provider is used.
19 /// The data is returned as raw byte without any decoding or encoding performed
20 /// The data is returned as raw byte without any decoding or encoding performed
20 /// to ensure all file encodings are handled correctly.
21 /// to ensure all file encodings are handled correctly.
21− fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>>;
22+ fn get_diff_base(&self, file: &Path) -> Result<Vec<u8>>;
22− fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>>;
23+ fn get_current_head_name(&self, file: &Path) -> Result<Arc<ArcSwap<Box<str>>>>;
23}
24}
24
25
25#[doc(hidden)]
26#[doc(hidden)]
26pub struct Dummy;
27pub struct Dummy;
27impl DiffProvider for Dummy {
28impl DiffProvider for Dummy {
28− fn get_diff_base(&self, _file: &Path) -> Option<Vec<u8>> {
29+ fn get_diff_base(&self, _file: &Path) -> Result<Vec<u8>> {
29− None
30+ bail!("helix was compiled without git support")
30 }
31 }
31
32
32− fn get_current_head_name(&self, _file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
33+ fn get_current_head_name(&self, _file: &Path) -> Result<Arc<ArcSwap<Box<str>>>> {
33− None
34+ bail!("helix was compiled without git support")
34 }
35 }
35}
36}
36
37
@@ -42,13 +43,27 @@ impl DiffProviderRegistry {
42 pub fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>> {
43 pub fn get_diff_base(&self, file: &Path) -> Option<Vec<u8>> {
43 self.providers
44 self.providers
44 .iter()
45 .iter()
45− .find_map(|provider| provider.get_diff_base(file))
46+ .find_map(|provider| match provider.get_diff_base(file) {
47+ Ok(res) => Some(res),
48+ Err(err) => {
49+ log::error!("{err:#?}");
50+ log::error!("failed to open diff base for {}", file.display());
51+ None
52+ }
53+ })
46 }
54 }
47
55
48 pub fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
56 pub fn get_current_head_name(&self, file: &Path) -> Option<Arc<ArcSwap<Box<str>>>> {
49 self.providers
57 self.providers
50 .iter()
58 .iter()
51− .find_map(|provider| provider.get_current_head_name(file))
59+ .find_map(|provider| match provider.get_current_head_name(file) {
60+ Ok(res) => Some(res),
61+ Err(err) => {
62+ log::error!("{err:#?}");
63+ log::error!("failed to obtain current head name for {}", file.display());
64+ None
65+ }
66+ })
52 }
67 }
53}
68}
54
69