From f1359b55519726011642c0548846846608c13e6c Mon Sep 17 00:00:00 2001 From: Tommy Yu Date: Fri, 8 Nov 2024 03:16:56 +1300 Subject: [PATCH] Delay handle opening of repositories in pmrrepo - Goal is to allow the possibility to allow a repository to be non- existent and still have the option to give that feedback. - Also allow pathinfo to return empty repository data. - All related structs have been modified to include optional fields. --- pmrapp/src/server/workspace.rs | 6 +- pmrapp/src/workspace.rs | 33 ++-- pmrapp/src/workspace/api.rs | 22 ++- pmrcore/src/repo.rs | 6 +- pmrctrl/tests/pmrctrl_test.rs | 12 +- pmrrepo/src/backend.rs | 2 +- pmrrepo/src/bin/pmrrepo.rs | 8 +- pmrrepo/src/error/mod.rs | 4 + pmrrepo/src/handle/git.rs | 12 +- pmrrepo/src/handle/git/impls.rs | 303 +++++++++++++++++++------------- pmrrepo/src/handle/git/util.rs | 37 ++-- pmrrepo/src/handle/impls.rs | 19 +- 12 files changed, 278 insertions(+), 186 deletions(-) diff --git a/pmrapp/src/server/workspace.rs b/pmrapp/src/server/workspace.rs index 9a3656e..f4ef2ac 100644 --- a/pmrapp/src/server/workspace.rs +++ b/pmrapp/src/server/workspace.rs @@ -47,7 +47,7 @@ pub async fn raw_workspace_download( // Ok(blob) match &result.target() { - GitResultTarget::Object(object) => { + Some(GitResultTarget::Object(object)) => { let info: PathObjectInfo = object.into(); match info { PathObjectInfo::FileInfo(info) => { @@ -69,12 +69,12 @@ pub async fn raw_workspace_download( } } } - GitResultTarget::RemoteInfo(RemoteInfo { location, commit, subpath, .. }) => { - // XXX this should be a redirect + Some(GitResultTarget::RemoteInfo(RemoteInfo { location, commit, subpath, .. })) => { Ok(Redirect::temporary( &format!("{}/raw/{}/{}", location, commit, subpath) ).into_response()) }, + None => Err(AppError::NotFound), } }, Err(_) => { diff --git a/pmrapp/src/workspace.rs b/pmrapp/src/workspace.rs index 2817e16..3f85d8e 100644 --- a/pmrapp/src/workspace.rs +++ b/pmrapp/src/workspace.rs @@ -248,9 +248,12 @@ pub fn WorkspaceSynchronize() -> impl IntoView { #[component] fn WorkspaceListingView(repo_result: RepoResult) -> impl IntoView { let workspace_id = repo_result.workspace.id; - let commit_id = repo_result.commit.commit_id.clone(); - let path = repo_result.path.clone(); - let pardir = repo_result.path != ""; + let commit_id = repo_result.commit + .clone() + .map(|commit| commit.commit_id) + .unwrap_or_else(|| "".to_string()); + let path = repo_result.path.clone().unwrap_or_else(|| String::new()); + let pardir = path != ""; let pardir = move || { pardir.then(|| view! { impl IntoView { name=".."/> })}; - let commit_id = repo_result.commit.commit_id.clone(); - let path = repo_result.path.clone(); + let path = repo_result.path.clone().unwrap_or_else(|| String::new()); + let commit_id = repo_result.commit + .clone() + .map(|commit| commit.commit_id) + .unwrap_or_else(|| "".to_string()); view! { @@ -275,7 +281,7 @@ fn WorkspaceListingView(repo_result: RepoResult) -> impl IntoView { {pardir} { match repo_result.target { - PathObjectInfo::TreeInfo(tree_info) => + Some(PathObjectInfo::TreeInfo(tree_info)) => Some(view! { impl IntoView { #[component] fn WorkspaceFileInfoView(repo_result: RepoResult) -> impl IntoView { + let path = repo_result.path.clone().unwrap_or_else(|| String::new()); + let commit_id = repo_result.commit + .map(|commit| commit.commit_id) + .unwrap_or_else(|| "".to_string()) + .clone(); match repo_result.target { - PathObjectInfo::FileInfo(ref file_info) => { + Some(PathObjectInfo::FileInfo(ref file_info)) => { let href = format!( "/workspace/{}/rawfile/{}/{}", &repo_result.workspace.id, - &repo_result.commit.commit_id, - &repo_result.path, + &commit_id, + &path, ); let info = format!("{:?}", file_info); Some(view! { @@ -329,9 +340,9 @@ fn WorkspaceFileInfoView(repo_result: RepoResult) -> impl IntoView { #[component] fn WorkspaceRepoResultView(repo_result: RepoResult) -> impl IntoView { match repo_result.target { - PathObjectInfo::TreeInfo(_) => + Some(PathObjectInfo::TreeInfo(_)) => Some(view! {
}.into_any()), - PathObjectInfo::FileInfo(_) => + Some(PathObjectInfo::FileInfo(_)) => Some(view! {
}.into_any()), _ => None, } diff --git a/pmrapp/src/workspace/api.rs b/pmrapp/src/workspace/api.rs index de36db6..fdd7406 100644 --- a/pmrapp/src/workspace/api.rs +++ b/pmrapp/src/workspace/api.rs @@ -34,13 +34,23 @@ pub async fn get_workspace_info( ) -> Result> { enforcer(format!("/workspace/{id}/"), "").await?; let platform = platform().await?; - Ok(platform.repo_backend() + let handle = platform.repo_backend() .git_handle(id).await - .map_err(|_| AppError::InternalServerError)? - .pathinfo(commit, path) - .map_err(|_| AppError::InternalServerError)? - .into() - ) + .map_err(|_| AppError::InternalServerError)?; + match (commit.as_ref(), path.as_ref(), handle.repo()) { + (None, None, Err(_)) => Ok(RepoResult { + workspace: handle.workspace().clone_inner(), + commit: None, + path: None, + target: None, + }), + (_, _, Err(_)) => Err(AppError::InternalServerError)?, + (_, _, Ok(_)) => Ok(handle + .pathinfo(commit, path) + .map_err(|_| AppError::InternalServerError)? + .into() + ), + } } #[server] diff --git a/pmrcore/src/repo.rs b/pmrcore/src/repo.rs index ada4f40..03a6005 100644 --- a/pmrcore/src/repo.rs +++ b/pmrcore/src/repo.rs @@ -79,11 +79,11 @@ pub struct RepoResult { /// The workspace that this result was derived from. pub workspace: Workspace, /// The commit id that this result was derived from. - pub commit: CommitInfo, + pub commit: Option, /// The path to the target. - pub path: String, + pub path: Option, /// The target is the resource identified at the path. - pub target: PathObjectInfo, + pub target: Option, } /* diff --git a/pmrctrl/tests/pmrctrl_test.rs b/pmrctrl/tests/pmrctrl_test.rs index 17c3650..9df970f 100644 --- a/pmrctrl/tests/pmrctrl_test.rs +++ b/pmrctrl/tests/pmrctrl_test.rs @@ -178,11 +178,11 @@ async fn test_platform_exposure_ctrl_attach_file() -> anyhow::Result<()> { let efctrl = exposure.ctrl_file(ef_ref)?; let pathinfo = efctrl.pathinfo(); - assert_eq!(pathinfo.path(), "if1"); + assert_eq!(pathinfo.path(), Some("if1")); let efctrl = exposure.ctrl_path("if1").await?; let pathinfo = efctrl.pathinfo(); - assert_eq!(pathinfo.path(), "if1"); + assert_eq!(pathinfo.path(), Some("if1")); Ok(()) } @@ -223,28 +223,28 @@ async fn test_platform_exposure_ctrl_resolve_file() -> anyhow::Result<()> { .resolve_file_viewstr("dir1/nested/file_c") .await .expect("expected valid path not found"); - assert_eq!(efctrl.pathinfo().path(), "dir1/nested/file_c"); + assert_eq!(efctrl.pathinfo().path(), Some("dir1/nested/file_c")); assert_eq!(viewstr, None); let (efctrl, viewstr) = exposure .resolve_file_viewstr("dir1/nested/file_c/") .await .expect("expected valid path not found"); - assert_eq!(efctrl.pathinfo().path(), "dir1/nested/file_c"); + assert_eq!(efctrl.pathinfo().path(), Some("dir1/nested/file_c")); assert_eq!(viewstr, Some("")); let (efctrl, viewstr) = exposure .resolve_file_viewstr("dir1/nested/file_c/view") .await .expect("expected valid path not found"); - assert_eq!(efctrl.pathinfo().path(), "dir1/nested/file_c"); + assert_eq!(efctrl.pathinfo().path(), Some("dir1/nested/file_c")); assert_eq!(viewstr, Some("view")); let (efctrl, viewstr) = exposure .resolve_file_viewstr("dir1/nested/file_c/view/subpath/target") .await .expect("expected valid path not found"); - assert_eq!(efctrl.pathinfo().path(), "dir1/nested/file_c"); + assert_eq!(efctrl.pathinfo().path(), Some("dir1/nested/file_c")); assert_eq!(viewstr, Some("view/subpath/target")); Ok(()) diff --git a/pmrrepo/src/backend.rs b/pmrrepo/src/backend.rs index 669bf9f..7a2b654 100644 --- a/pmrrepo/src/backend.rs +++ b/pmrrepo/src/backend.rs @@ -35,7 +35,7 @@ impl Backend { pub async fn git_handle<'a>(&'a self, workspace_id: i64) -> Result, PmrRepoError> { let workspace = self.db_platform.get_workspace(workspace_id).await?; - Ok(GitHandle::new(&self, self.repo_root.clone(), workspace)?) + Ok(GitHandle::new(&self, self.repo_root.clone(), workspace)) } pub fn platform(&self) -> &(dyn MCPlatform + Send + Sync) { diff --git a/pmrrepo/src/bin/pmrrepo.rs b/pmrrepo/src/bin/pmrrepo.rs index 33bfba2..2c92916 100644 --- a/pmrrepo/src/bin/pmrrepo.rs +++ b/pmrrepo/src/bin/pmrrepo.rs @@ -113,11 +113,11 @@ fn stream_git_result_default<'a>( have path_object_info {:?} \n", item.repo().path(), - item.commit(&repo).id(), + item.commit(&repo).map(|c| c.id()), item.commit(&repo), item.path(), &item.target(), - ::from(item), + >::from(item), ).as_bytes()) } @@ -125,7 +125,7 @@ fn stream_git_result_as_json( writer: impl Write, item: &GitHandleResult, ) -> Result<(), serde_json::Error> { - serde_json::to_writer(writer, &::from(item)) + serde_json::to_writer(writer, &>::from(item)) } fn fetch_envvar(key: &str) -> anyhow::Result { @@ -220,7 +220,7 @@ async fn main(args: Args) -> anyhow::Result<()> { } Some(Command::Blob { workspace_id, obj_id }) => { let handle = backend.git_handle(workspace_id).await?; - let repo = handle.repo(); + let repo = handle.repo()?; let obj = repo.rev_parse_single(obj_id.deref())?.object()?; log::info!("Found object {} {}", obj.kind, obj.id); // info!("{:?}", object_to_info(&obj)); diff --git a/pmrrepo/src/error/mod.rs b/pmrrepo/src/error/mod.rs index acd51e2..179a31d 100644 --- a/pmrrepo/src/error/mod.rs +++ b/pmrrepo/src/error/mod.rs @@ -109,4 +109,8 @@ pub enum PathError { oid: String, path: String, }, + #[error("couldn't open repository for workspace `{workspace_id}`")] + Repository { + workspace_id: i64, + }, } diff --git a/pmrrepo/src/handle/git.rs b/pmrrepo/src/handle/git.rs index 50f1bd6..35c6357 100644 --- a/pmrrepo/src/handle/git.rs +++ b/pmrrepo/src/handle/git.rs @@ -7,11 +7,17 @@ use gix::{ ObjectDetached, ThreadSafeRepository, }; +use std::{ + path::PathBuf, + sync::OnceLock, +}; +use crate::error::GixError; pub struct GitHandle<'repo> { pub(super) backend: &'repo Backend, pub(super) workspace: WorkspaceRef<'repo>, - pub(super) repo: ThreadSafeRepository, + pub(super) repo_dir: PathBuf, + pub(super) repo: OnceLock>, } #[derive(Debug)] @@ -23,8 +29,8 @@ pub enum GitResultTarget { pub struct GitHandleResult<'repo> { pub(super) backend: &'repo Backend, pub(super) repo: &'repo ThreadSafeRepository, - pub(super) commit: ObjectDetached, - pub(super) target: GitResultTarget, + pub(super) commit: Option, + pub(super) target: Option, pub(super) workspace: &'repo WorkspaceRef<'repo>, } diff --git a/pmrrepo/src/handle/git/impls.rs b/pmrrepo/src/handle/git/impls.rs index fe29ef9..e383616 100644 --- a/pmrrepo/src/handle/git/impls.rs +++ b/pmrrepo/src/handle/git/impls.rs @@ -38,6 +38,7 @@ use std::{ Path, PathBuf, }, + sync::OnceLock, }; use crate::{ @@ -58,12 +59,14 @@ use super::{ util::*, }; -impl From<&GitHandleResult<'_>> for PathObjectInfo { +impl From<&GitHandleResult<'_>> for Option { fn from(item: &GitHandleResult) -> Self { - match &item.target { - GitResultTarget::Object(object) => object.into(), - GitResultTarget::RemoteInfo(remote_info) => PathObjectInfo::RemoteInfo(remote_info.clone()), - } + item.target + .as_ref() + .map(|target| match target { + GitResultTarget::Object(object) => object.into(), + GitResultTarget::RemoteInfo(remote_info) => PathObjectInfo::RemoteInfo(remote_info.clone()), + }) } } @@ -72,25 +75,23 @@ impl From> for RepoResult { RepoResult { target: (&item).into(), workspace: item.workspace.clone_inner(), - path: item.path().to_string(), - commit: item.commit.try_into() - .expect("commit should have been parsed during processing"), + path: item.path().map(str::to_string), + commit: item.commit + .map(|commit| commit.try_into() + .expect("commit should have been parsed during processing")), } } } -impl<'handle> TryFrom> for GitHandle<'handle> { - type Error = GixError; - - fn try_from(item: Handle<'handle>) -> Result { - let repo = gix::open::Options::isolated() - .open_path_as_is(true) - .open(&item.repo_dir)?; - Ok(Self { +impl<'handle> From> for GitHandle<'handle> { + fn from(item: Handle<'handle>) -> Self { + let repo = OnceLock::new(); + Self { backend: item.backend, workspace: item.workspace, - repo - }) + repo_dir: item.repo_dir, + repo, + } } } @@ -99,26 +100,35 @@ impl<'repo> GitHandle<'repo> { backend: &'repo Backend, repo_root: PathBuf, workspace: WorkspaceRef<'repo>, - ) -> Result { + ) -> Self { let repo_dir = repo_root.join(workspace.id().to_string()); - let repo = gix::open::Options::isolated() - .open_path_as_is(true) - .open(&repo_dir)?; - Ok(Self { backend, workspace, repo }) + let repo = OnceLock::new(); + Self { backend, workspace, repo_dir, repo } } pub fn workspace(&self) -> &WorkspaceRef<'repo> { &self.workspace } - pub fn repo(&self) -> Repository { - self.repo.to_thread_local() + pub fn repo(&self) -> Result { + self.repo.get_or_init(|| Ok( + gix::open::Options::isolated() + .open_path_as_is(true) + .open(&self.repo_dir)?) + ) + .as_ref() + .map(|repo| repo.to_thread_local()) + .map_err(|_| PathError::Repository { + workspace_id: self.workspace.id(), + }) } - fn repo_tags(&self) -> Result, GixError> { - Ok(self.repo() - .references()? - .tags()? + fn repo_tags(&self) -> Result, PmrRepoError> { + Ok(self.repo()? + .references() + .map_err(GixError::from)? + .tags() + .map_err(GixError::from)? .filter_map(|reference| { match reference { Ok(tag) => { @@ -146,7 +156,7 @@ impl<'repo> GitHandle<'repo> { .collect::>()) } - pub async fn index_tags(&self) -> Result<(), GixError> { + pub async fn index_tags(&self) -> Result<(), PmrRepoError> { let workspace = &self.workspace; self.repo_tags()? .into_iter() @@ -171,7 +181,7 @@ impl<'repo> GitHandle<'repo> { commit_id: S, ) -> Result<(), PmrRepoError> { let workspace_id = self.workspace.id(); - let repo = self.repo(); + let repo = self.repo()?; get_commit(&repo, workspace_id, Some(commit_id.as_ref()))?; Ok(()) } @@ -187,84 +197,100 @@ impl<'repo> GitHandle<'repo> { let path = path.map(|s| s.into()); let workspace_id = self.workspace.id(); - let repo = self.repo(); + let repo = self.repo()?; let commit = get_commit(&repo, workspace_id, commit_id.as_deref())?; - let tree = commit - .tree_id().map_err(GixError::from)? - .object().map_err(GixError::from)?; + match commit { + None => Ok(GitHandleResult { + backend: &self.backend, + repo: self.repo.get() + .expect("OnceLock should have been set with the self.repo()") + .as_ref() + .expect("Valid repo was resolved by here"), + commit: None, + target: None, + workspace: &self.workspace, + }), + Some(commit) => { + let tree = commit + .tree_id().map_err(GixError::from)? + .object().map_err(GixError::from)?; - let target = match path.as_ref().map(|s| s.as_ref()) { - Some("") | Some("/") | None => { - info!("No path provided; using root tree entry"); - GitResultTarget::Object( - PathObjectDetached::new("".to_string(), tree.into()), - ) - }, - Some(s) => { - let path = s.strip_prefix('/').unwrap_or(&s); - let mut comps = Path::new(path).components(); - let mut curr_path = PathBuf::new(); - let mut object = Some(tree); - let mut target: Option = None; + let target = match path.as_ref().map(|s| s.as_ref()) { + Some("") | Some("/") | None => { + info!("No path provided; using root tree entry"); + GitResultTarget::Object( + PathObjectDetached::new("".to_string(), tree.into()), + ) + }, + Some(s) => { + let path = s.strip_prefix('/').unwrap_or(&s); + let mut comps = Path::new(path).components(); + let mut curr_path = PathBuf::new(); + let mut object = Some(tree); + let mut target: Option = None; - while let Some(component) = comps.next() { - let entry = object - .expect("iteration has this set or look breaked") - .try_into_tree().map_err(GixError::from)? - .peel_to_entry_by_path( - Path::new(&component) - ).map_err(GixError::from)? - .ok_or_else( - || PmrRepoError::from(PathError::NoSuchPath { - workspace_id: workspace_id, - oid: commit.id.to_string(), - path: path.to_string(), - }) - )?; - curr_path.push(component); - match entry.mode() { - k if (k == EntryKind::Commit.into()) => { - info!("entry {:?} is a commit", entry.id()); - let location = get_submodule_target( - &commit, - workspace_id, - curr_path.to_str().unwrap(), - )?; - target = Some(GitResultTarget::RemoteInfo(RemoteInfo { - location: location, - commit: entry.id().to_string(), - subpath: comps.as_path().to_str().unwrap().to_string(), - path: path.to_string(), - })); - object = None; - break; + while let Some(component) = comps.next() { + let entry = object + .expect("iteration has this set or look breaked") + .try_into_tree().map_err(GixError::from)? + .peel_to_entry_by_path( + Path::new(&component) + ).map_err(GixError::from)? + .ok_or_else( + || PmrRepoError::from(PathError::NoSuchPath { + workspace_id: workspace_id, + oid: commit.id.to_string(), + path: path.to_string(), + }) + )?; + curr_path.push(component); + match entry.mode() { + k if (k == EntryKind::Commit.into()) => { + info!("entry {:?} is a commit", entry.id()); + let location = get_submodule_target( + &commit, + workspace_id, + curr_path.to_str().unwrap(), + )?; + target = Some(GitResultTarget::RemoteInfo(RemoteInfo { + location: location, + commit: entry.id().to_string(), + subpath: comps.as_path().to_str().unwrap().to_string(), + path: path.to_string(), + })); + object = None; + break; + } + _ => () + } + let next_object = entry + .object().map_err(GixError::from)?; + info!("got {} {:?}", next_object.kind, &next_object); + object = Some(next_object); + }; + match object { + Some(object) => + GitResultTarget::Object( + PathObjectDetached::new(path.to_string(), object.into()) + ), + None => + // Only way object is None is have target set. + target.expect("to be a RemoteInfo"), } - _ => () - } - let next_object = entry - .object().map_err(GixError::from)?; - info!("got {} {:?}", next_object.kind, &next_object); - object = Some(next_object); + }, }; - match object { - Some(object) => - GitResultTarget::Object( - PathObjectDetached::new(path.to_string(), object.into()) - ), - None => - // Only way object is None is have target set. - target.expect("to be a RemoteInfo"), - } - }, - }; - let item = GitHandleResult { - backend: &self.backend, - repo: &self.repo, - commit: commit.into(), - target: target, - workspace: &self.workspace, - }; - Ok(item) + Ok(GitHandleResult { + backend: &self.backend, + repo: &self.repo.get() + .expect("OnceLock should have been set with the self.repo()") + .as_ref() + .expect("Valid repo was resolved by here"), + commit: Some(commit.into()), + target: Some(target), + workspace: &self.workspace, + }) + } + } } pub fn loginfo( @@ -274,8 +300,12 @@ impl<'repo> GitHandle<'repo> { count: Option, ) -> Result { let workspace_id = self.workspace.id(); - let repo = self.repo(); + let repo = self.repo()?; let commit = get_commit(&repo, workspace_id, commit_id)?; + if commit.is_none() { + return Ok(LogInfo { entries: Vec::new() }); + } + let commit = commit.expect("None case should have been handled"); let mut filter = PathFilter::new(&repo, path); let log_entry_iter = repo.rev_walk([commit.id]) .sorting(Sorting::ByCommitTimeNewestFirst) @@ -313,9 +343,11 @@ impl<'repo> GitHandle<'repo> { commit_id: Option<&str>, ) -> Result, PmrRepoError> { let workspace_id = self.workspace.id(); - let repo = self.repo(); - let commit = get_commit(&repo, workspace_id, commit_id)?; - files(&commit) + let repo = self.repo()?; + let result = get_commit(&repo, workspace_id, commit_id)? + .map(|commit| files(&commit)) + .unwrap_or_else(|| Ok(Vec::new())); + result } pub fn checkout( @@ -324,8 +356,13 @@ impl<'repo> GitHandle<'repo> { dest_dir: &Path, ) -> Result<(), PmrRepoError> { let workspace_id = self.workspace.id(); - let repo = self.repo(); - let commit = get_commit(&repo, workspace_id, commit_id)?; + let repo = self.repo()?; + let commit = get_commit( + &repo, + workspace_id, + // force a HEAD commit at the minimum to trigger error handling + Some(commit_id.unwrap_or("HEAD")), + )?.expect("a commit was expected with Some(commit_id) provided"); checkout(&repo, &commit, dest_dir) } @@ -336,21 +373,27 @@ impl<'repo> GitHandleResult<'repo> { self.repo.to_thread_local() } - pub fn commit(&self, repo: &'repo Repository) -> Commit<'repo> { - self.commit.clone().attach(repo).into_commit() + pub fn commit(&self, repo: &'repo Repository) -> Option> { + self.commit + .as_ref() + .map(|commit| commit.clone() + .attach(repo) + .into_commit()) } - pub fn path(&self) -> &str { - match &self.target { - GitResultTarget::Object(object) => &object.path, - GitResultTarget::RemoteInfo(remote_info) => &remote_info.path, - } + pub fn path(&self) -> Option<&str> { + self.target + .as_ref() + .map(|target| match target { + GitResultTarget::Object(object) => object.path.as_str(), + GitResultTarget::RemoteInfo(remote_info) => remote_info.path.as_str(), + }) } // TODO could use an TryInto> or something along that line // for getting the final result. - pub fn target(&self) -> &GitResultTarget { - &self.target + pub fn target(&self) -> Option<&GitResultTarget> { + self.target.as_ref() } pub fn workspace(&'repo self) -> &WorkspaceRef<'repo> { @@ -363,16 +406,22 @@ impl<'repo> GitHandleResult<'repo> { mut writer: impl Write + Send + 'async_recursion, ) -> Result { match &self.target { - GitResultTarget::Object(object) => match object.object.kind { + None => Err(ContentError::Invalid { + workspace_id: self.workspace.id(), + oid: self.commit.as_ref().map(|c| c.id.to_string()).unwrap_or("".to_string()), + path: self.path().map(str::to_string).unwrap_or("".to_string()), + msg: format!("expected to be a blob"), + }.into()), + Some(GitResultTarget::Object(object)) => match object.object.kind { Kind::Blob => Ok(writer.write(&object.object.data)?), _ => Err(ContentError::Invalid { workspace_id: self.workspace.id(), - oid: self.commit.id.to_string(), - path: self.path().to_string(), + oid: self.commit.as_ref().map(|c| c.id.to_string()).unwrap_or("".to_string()), + path: self.path().map(str::to_string).unwrap_or("".to_string()), msg: format!("expected to be a blob"), - }.into()) + }.into()), }, - GitResultTarget::RemoteInfo(RemoteInfo { location, commit, subpath, .. }) => { + Some(GitResultTarget::RemoteInfo(RemoteInfo { location, commit, subpath, .. })) => { let workspaces = WorkspaceBackend::list_workspace_by_url( self.backend.db_platform.as_ref(), &location, ).await?; @@ -399,7 +448,11 @@ impl<'repo> GitHandleResult<'repo> { &self, repo: &'repo Repository, ) -> Result, PmrRepoError> { - files(&self.commit(repo)) + Ok(self.commit(repo) + .as_ref() + .map(files) + .transpose()? + .unwrap_or_else(|| Vec::new())) } } diff --git a/pmrrepo/src/handle/git/util.rs b/pmrrepo/src/handle/git/util.rs index becf1c9..e314279 100644 --- a/pmrrepo/src/handle/git/util.rs +++ b/pmrrepo/src/handle/git/util.rs @@ -80,24 +80,31 @@ pub(crate) fn get_commit<'a>( repo: &'a Repository, workspace_id: i64, commit_id: Option<&'a str>, -) -> Result, PmrRepoError> { - let obj = rev_parse_single(repo, &commit_id.unwrap_or("HEAD"))?; - match obj.kind { +) -> Result>, PmrRepoError> { + let obj = match commit_id { + Some(commit_id) => Some(rev_parse_single(repo, commit_id)?), + None => rev_parse_single(repo, "HEAD").ok(), + }; + obj.map(|obj| match obj.kind { kind if kind == Kind::Commit => { info!("Found {} {}", kind, obj.id); + match obj.try_into_commit() { + Ok(commit) => Ok(commit), + Err(obj) => Err(PmrRepoError::from(ExecutionError::Unexpected { + workspace_id: workspace_id, + msg: format!("gix said oid {:?} was a commit?", obj.id), + })), + } } - _ => return Err(PathError::NoSuchCommit { - workspace_id: workspace_id, - oid: commit_id.unwrap_or("HEAD?").into(), - }.into()) - } - match obj.try_into_commit() { - Ok(commit) => Ok(commit), - Err(obj) => Err(ExecutionError::Unexpected { - workspace_id: workspace_id, - msg: format!("gix said oid {:?} was a commit?", obj.id), - }.into()), - } + _ => { + Err(PmrRepoError::from(PathError::NoSuchCommit { + workspace_id: workspace_id, + // FIXME if it's HEAD it should be a variant along the lines of + // HEAD not leading to a valid commit + oid: commit_id.unwrap_or("HEAD").into(), + })) + } + }).transpose() } pub(crate) fn get_submodule_target( diff --git a/pmrrepo/src/handle/impls.rs b/pmrrepo/src/handle/impls.rs index 90a8b52..758ab2f 100644 --- a/pmrrepo/src/handle/impls.rs +++ b/pmrrepo/src/handle/impls.rs @@ -42,7 +42,7 @@ impl<'handle> Handle<'handle> { match super::git::util::fetch_or_clone(repo_dir, &url) { Ok(_) => { ticket.complete_sync().await?; - let handle: GitHandle<'handle> = self.try_into()?; + let handle: GitHandle<'handle> = self.into(); handle.index_tags().await?; Ok(handle) } @@ -310,7 +310,7 @@ mod tests { let backend = Backend::new(Arc::new(platform), repo_root.path().to_path_buf()); let handle = backend.git_handle(10).await?; let result = handle.pathinfo::(None, None).unwrap(); - assert_eq!(result.path(), ""); + assert_eq!(result.path(), Some("")); assert_eq!(result.workspace().description(), Some("Workspace 10")); Ok(()) } @@ -349,8 +349,8 @@ mod tests { assert_eq!( pathinfo.path(), - "ext/import1/README"); - let GitResultTarget::RemoteInfo(target) = pathinfo.target else { + Some("ext/import1/README")); + let Some(GitResultTarget::RemoteInfo(target)) = pathinfo.target else { unreachable!() }; assert_eq!( @@ -371,8 +371,9 @@ mod tests { ).unwrap(); assert_eq!( pathinfo.path(), - "ext/import2/import1/if1"); - let GitResultTarget::RemoteInfo(target) = pathinfo.target else{ + Some("ext/import2/import1/if1"), + ); + let Some(GitResultTarget::RemoteInfo(target)) = pathinfo.target else { unreachable!() }; assert_eq!( @@ -633,7 +634,7 @@ mod tests { ); let pathinfo = handle.pathinfo(None, Some("dir1/nested/file_a"))?; - assert_eq!(pathinfo.path(), "dir1/nested/file_a"); + assert_eq!(pathinfo.path(), Some("dir1/nested/file_a")); Ok(()) } @@ -656,8 +657,8 @@ mod tests { // this is no longer "reachable" through the pmrrepo API as most // are converted to the detached version for Send + Sync. let pathinfo = handle.pathinfo(Some("8ae6e9af37c8bd78614545d0ab807348fc46dcab"), None)?; - match PathObjectInfo::from(&pathinfo) { - PathObjectInfo::TreeInfo(tree_info) => { + match >::from(&pathinfo) { + Some(PathObjectInfo::TreeInfo(tree_info)) => { assert_eq!(tree_info.filecount, 6); assert_eq!(tree_info.entries[0], serde_json::from_str(r#"{ "filemode": "100644",