From 6a7e64d04a8b0ad932169df43e2d93efe8ceedf8 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Tue, 18 Aug 2026 07:18:07 +0900 Subject: fix: propagate ported exceptions instead of flattening them `ProcessExecutor::execute_args` existed only to turn `execute`'s `anyhow::Result` into an exit code of 1, so every one of its ~87 call sites silently took the "command failed" branch on an error PHP would have thrown. It is gone; callers use `execute` and propagate with `?`. Where the enclosing function had no `Result` to propagate into, its signature grew one, up to and including `Git::get_version`, `Svn::binary_version`, `GitHub`/`GitLab`/`Bitbucket::authorize_oauth`, `InitCommand::get_git_config` and `DiagnoseCommand::check_git`. The VCS drivers had the same problem in the other direction: their `get_contents` returned `Result>`, a type too narrow for the PHP method, which lets any Throwable out of the `catch (TransportException $e)` block. Every non-transport error was therefore rewritten into a `TransportException` with code 0, which the callers switch on. They now return `anyhow::Result>>`: the outer `Result` carries what PHP does not catch, the inner one the exception the drivers handle. That signature also restores `GitLabDriver::getContents`: the 400/401 `TransportException`s it raises to force authentication are thrown inside its own `try` block and handled by its own `catch`, but the port returned them straight to the caller, so the authentication flow behind them never ran. `impl_php_exception!` gains `From> for anyhow::Error` so a caught exception can be re-propagated with `?`. Co-Authored-By: Claude Opus 5 (1M context) --- crates/shirabe/src/repository/vcs/gitlab_driver.rs | 212 ++++++++++----------- 1 file changed, 104 insertions(+), 108 deletions(-) (limited to 'crates/shirabe/src/repository/vcs/gitlab_driver.rs') diff --git a/crates/shirabe/src/repository/vcs/gitlab_driver.rs b/crates/shirabe/src/repository/vcs/gitlab_driver.rs index b40297da..d21a9ca1 100644 --- a/crates/shirabe/src/repository/vcs/gitlab_driver.rs +++ b/crates/shirabe/src/repository/vcs/gitlab_driver.rs @@ -396,7 +396,7 @@ impl GitLabDriver { identifier, ); - let content = match self.get_contents(&resource, false) { + let content = match self.get_contents(&resource, false)? { Ok(response) => response.get_body().map(|s| s.to_string()), Err(e) => { if e.get_code() != 404 { @@ -587,9 +587,7 @@ impl GitLabDriver { let mut references: IndexMap = IndexMap::new(); loop { - let response = self - .get_contents(resource.as_deref().unwrap_or(""), false) - .map_err(|e| anyhow::anyhow!("{}", e.get_message()))?; + let response = self.get_contents(resource.as_deref().unwrap_or(""), false)??; let data = response.decode_json()?; if let PhpMixed::List(ref list) = data { @@ -646,10 +644,7 @@ impl GitLabDriver { // we need to fetch the default branch from the api let resource = self.get_api_url(); - let project = self - .get_contents(&resource, true) - .map_err(|e| anyhow::anyhow!("{}", e.get_message()))? - .decode_json()?; + let project = self.get_contents(&resource, true)??.decode_json()?; self.project = match project { PhpMixed::Array(m) => Some(m), _ => None, @@ -741,14 +736,18 @@ impl GitLabDriver { &mut self, url: &str, fetching_repo_data: bool, - ) -> anyhow::Result> { - let response_result = self.inner.get_contents(url); - match response_result { - Ok(response) => { + ) -> anyhow::Result>> { + // PHP body of the `try` block: a TransportException raised anywhere in here, including the + // ones thrown below to force authentication, is handled by the `catch` that follows. + let response_result = + (|this: &mut Self| -> anyhow::Result>> { + let response = match this.inner.get_contents(url)? { + Ok(response) => response, + Err(e) => return Ok(Err(e)), + }; + if fetching_repo_data { - let json = response - .decode_json() - .map_err(|e| TransportException::new(e.to_string(), 0))?; + let json = response.decode_json()?; let json_map = match json { PhpMixed::Array(ref m) => m.clone(), _ => IndexMap::new(), @@ -760,7 +759,7 @@ impl GitLabDriver { if !json_map.contains_key("default_branch") && json_map.contains_key("permissions") { - self.is_private = json_map + this.is_private = json_map .get("visibility") .and_then(|v| v.as_string()) .map(|s| s != "public") @@ -785,21 +784,20 @@ impl GitLabDriver { } if !more_than_guest_access { - self.inner.io.write_error3( - "GitLab token with Guest or Planner only access detected", - true, - io_interface::NORMAL, - ); + this.inner.io.write_error3( + "GitLab token with Guest or Planner only access detected", + true, + io_interface::NORMAL, + ); - self.attempt_clone_fallback() - .map_err(|e| TransportException::new(e.to_string(), 0))?; + this.attempt_clone_fallback()?; - return Ok(Response::new( + return Ok(Ok(Response::new( "dummy".to_string(), Some(200), vec![], Some("null".to_string()), - )); + ))); } } @@ -811,110 +809,108 @@ impl GitLabDriver { .and_then(|v| v.as_string()) == Some("disabled") { - return Err(Box::new(TransportException::new( + return Ok(Err(Box::new(TransportException::new( "The GitLab repository is disabled in the project".to_string(), 400, - ))); + )))); } if !empty(&json_map.get("id").cloned().unwrap_or(PhpMixed::Null)) { - self.is_private = false; + this.is_private = false; } - return Err(Box::new(TransportException::new( - "GitLab API seems to not be authenticated as it did not return a default_branch" - .to_string(), - 401, - ))); + return Ok(Err(Box::new(TransportException::new( + "GitLab API seems to not be authenticated as it did not return a default_branch" + .to_string(), + 401, + )))); } } - Ok(response) - } - Err(e) => { - let mut git_lab_util = GitLab::new( - self.inner.io.clone(), - self.inner.config.clone(), - Some(self.inner.process.clone()), - Some(self.inner.http_downloader.clone()), - ) - .map_err(|err| TransportException::new(err.to_string(), 0))?; + Ok(Ok(response)) + })(self); - match e.get_code() { - 401 | 404 => { - // try to authorize only if we are fetching the main /repos/foo/bar data, otherwise it must be a real 404 - if !fetching_repo_data { - return Err(e); - } + let e = match response_result? { + Ok(response) => return Ok(Ok(response)), + Err(e) => e, + }; - if git_lab_util.authorize_oauth(&self.inner.origin_url) { - return self.inner.get_contents(url); - } + let mut git_lab_util = GitLab::new( + self.inner.io.clone(), + self.inner.config.clone(), + Some(self.inner.process.clone()), + Some(self.inner.http_downloader.clone()), + )?; + + match e.get_code() { + 401 | 404 => { + // try to authorize only if we are fetching the main /repos/foo/bar data, otherwise it must be a real 404 + if !fetching_repo_data { + return Ok(Err(e)); + } - if git_lab_util.is_oauth_expired(&self.inner.origin_url) - && git_lab_util - .authorize_oauth_refresh(&self.scheme, &self.inner.origin_url) - .map_err(|err| TransportException::new(err.to_string(), 0))? - { - return self.inner.get_contents(url); - } + if git_lab_util.authorize_oauth(&self.inner.origin_url)? { + return self.inner.get_contents(url); + } - if !self.inner.io.is_interactive() { - self.attempt_clone_fallback() - .map_err(|err| TransportException::new(err.to_string(), 0))?; + if git_lab_util.is_oauth_expired(&self.inner.origin_url) + && git_lab_util.authorize_oauth_refresh(&self.scheme, &self.inner.origin_url)? + { + return self.inner.get_contents(url); + } - return Ok(Response::new( - "dummy".to_string(), - Some(200), - vec![], - Some("null".to_string()), - )); - } - self.inner.io.write_error3( - &format!( - "Failed to download {}/{}:{}", - self.namespace, - self.repository, - e.get_message() - ), - true, - io_interface::NORMAL, - ); - git_lab_util.authorize_oauth_interactively( - &self.scheme, - &self.inner.origin_url, - Some(&format!( - "Your credentials are required to fetch private repository metadata ({})", - self.inner.url - )), - ); + if !self.inner.io.is_interactive() { + self.attempt_clone_fallback()?; - self.inner.get_contents(url) - } - 403 => { - if !self.inner.io.has_authentication(&self.inner.origin_url) - && git_lab_util.authorize_oauth(&self.inner.origin_url) - { - return self.inner.get_contents(url); - } + return Ok(Ok(Response::new( + "dummy".to_string(), + Some(200), + vec![], + Some("null".to_string()), + ))); + } + self.inner.io.write_error3( + &format!( + "Failed to download {}/{}:{}", + self.namespace, + self.repository, + e.get_message() + ), + true, + io_interface::NORMAL, + ); + git_lab_util.authorize_oauth_interactively( + &self.scheme, + &self.inner.origin_url, + Some(&format!( + "Your credentials are required to fetch private repository metadata ({})", + self.inner.url + )), + )?; - if !self.inner.io.is_interactive() && fetching_repo_data { - self.attempt_clone_fallback() - .map_err(|err| TransportException::new(err.to_string(), 0))?; + self.inner.get_contents(url) + } + 403 => { + if !self.inner.io.has_authentication(&self.inner.origin_url) + && git_lab_util.authorize_oauth(&self.inner.origin_url)? + { + return self.inner.get_contents(url); + } - return Ok(Response::new( - "dummy".to_string(), - Some(200), - vec![], - Some("null".to_string()), - )); - } + if !self.inner.io.is_interactive() && fetching_repo_data { + self.attempt_clone_fallback()?; - Err(e) - } - _ => Err(e), + return Ok(Ok(Response::new( + "dummy".to_string(), + Some(200), + vec![], + Some("null".to_string()), + ))); } + + Ok(Err(e)) } + _ => Ok(Err(e)), } } -- cgit v1.3.1-4-g156e