# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1681372674 -7200
#      Thu Apr 13 09:57:54 2023 +0200
# Node ID a273ee467f4927a582d4e64af0f7ec34fc072146
# Parent  0df79102a3e88c8f326139d124c329efd35c12d6
RHGitaly: separation of repo errors into specification and load errors

As the Gitaly Comparison tests showed us in the Python case, the errors
to return on wrong `Repository` specifications depend at least on the service,
hence we need an added flexibility.

For that, we need internally to make a difference between client-side errors
(e.g., repo does not exist) and internal errors (failed to load repo, e.g., due
to a filesystem issue).

diff --git a/rust/Cargo.lock b/rust/Cargo.lock
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -1255,6 +1255,7 @@
 version = "0.1.0"
 dependencies = [
  "build_const",
+ "derive_more",
  "hg-core",
  "prost",
  "prost-types",
diff --git a/rust/rhgitaly/Cargo.toml b/rust/rhgitaly/Cargo.toml
--- a/rust/rhgitaly/Cargo.toml
+++ b/rust/rhgitaly/Cargo.toml
@@ -14,6 +14,7 @@
 tracing = "0.1.37"
 tracing-subscriber = "0.3.16"
 build_const = "0.2"
+derive_more = "0.99.17"
 
 [build-dependencies]
 tonic-build = "0.8"
diff --git a/rust/rhgitaly/src/repository.rs b/rust/rhgitaly/src/repository.rs
--- a/rust/rhgitaly/src/repository.rs
+++ b/rust/rhgitaly/src/repository.rs
@@ -4,30 +4,67 @@
 // GNU General Public License version 2 or any later version.
 // SPDX-License-Identifier: GPL-2.0-or-later
 use std::path::PathBuf;
+
 use tonic::Status;
 
+use hg::repo::RepoError;
+
 use super::config::Config;
 use super::gitaly::Repository;
 
+/// Represent errors that are due to a wrong repository specification.
+///
+/// In terms of gRPC methods, the specifications is usually enclosed in  a [`Repository`] message
+/// and these errors are considered to be client errors.
 #[derive(Debug, PartialEq, Eq)]
-pub enum PathError {
+pub enum RepoSpecError {
+    MissingSpecification,
     UnknownStorage(String),
+    RepoNotFound(PathBuf),
+}
+
+/// Represent errors loading a repository (bad specification or internal errors)
+#[derive(Debug, derive_more::From)] // TODO add PartialEq, but do it in core for RepoError first
+pub enum RepoLoadError {
+    #[from]
+    SpecError(RepoSpecError),
+    LoadError(RepoError),
 }
 
-impl From<PathError> for Status {
-    fn from(value: PathError) -> Self {
-        match value {
-            PathError::UnknownStorage(storage) => Status::invalid_argument(format!(
-                "GetStorageByName: no such storage: \"{}\"",
-                storage
-            )),
+impl From<RepoError> for RepoLoadError {
+    fn from(value: RepoError) -> Self {
+        if let RepoError::NotFound { at } = value {
+            return Self::SpecError(RepoSpecError::RepoNotFound(at));
+        }
+        Self::LoadError(value)
+    }
+}
+
+/// Default conversion of ['RepoSpecError'] into a gRPC ['Status']
+///
+/// This function does not care to precisely match the error details, focusing on the error
+/// codes instead.
+///
+/// The resulting codes match the most common behaviour of Gitaly, which actually behaves more
+/// and more like this with time (e.g., as internal Git error get catched and handled).
+pub fn default_repo_spec_error_status(err: RepoSpecError) -> Status {
+    match err {
+        RepoSpecError::MissingSpecification => {
+            Status::invalid_argument("missing repository specification")
+        }
+        RepoSpecError::UnknownStorage(storage) => Status::invalid_argument(format!(
+            "GetStorageByName: no such storage: \"{}\"",
+            storage
+        )),
+        RepoSpecError::RepoNotFound(at) => {
+            Status::not_found(format!("repository at \"{}\" not found", at.display()))
         }
     }
 }
 
-pub fn repo_path(config: &Config, repo: &Repository) -> Result<PathBuf, PathError> {
+pub fn repo_path(config: &Config, repo: &Repository) -> Result<PathBuf, RepoSpecError> {
     if repo.storage_name != "default" {
-        return Err(PathError::UnknownStorage(repo.storage_name.clone()));
+        return Err(RepoSpecError::UnknownStorage(repo.storage_name.clone()));
     }
     let root = &config.repositories_root;
 
diff --git a/rust/rhgitaly/src/service/repository.rs b/rust/rhgitaly/src/service/repository.rs
--- a/rust/rhgitaly/src/service/repository.rs
+++ b/rust/rhgitaly/src/service/repository.rs
@@ -11,7 +11,7 @@
 use crate::config::Config;
 use crate::gitaly::repository_service_server::{RepositoryService, RepositoryServiceServer};
 use crate::gitaly::{RepositoryExistsRequest, RepositoryExistsResponse};
-use crate::repository::repo_path;
+use crate::repository::{default_repo_spec_error_status, repo_path};
 
 #[derive(Debug)]
 pub struct RepositoryServiceImpl {
@@ -28,7 +28,7 @@
         info!("Processing");
 
         if let Some(gl_repo) = &request.get_ref().repository {
-            let path = repo_path(&self.config, gl_repo)?;
+            let path = repo_path(&self.config, gl_repo).map_err(default_repo_spec_error_status)?;
             let res = match fs::metadata(path).await {
                 Ok(md) => md.is_dir(),
                 Err(_) => false,