diff --git a/app/src/lib/backend/ipc.ts b/app/src/lib/backend/ipc.ts index 04bb6129d5..312d5993e6 100644 --- a/app/src/lib/backend/ipc.ts +++ b/app/src/lib/backend/ipc.ts @@ -5,11 +5,7 @@ import type { EventCallback, EventName } from '@tauri-apps/api/event'; export enum Code { Unknown = 'errors.unknown', Validation = 'errors.validation', - Projects = 'errors.projects', - ProjectsGitAuth = 'errors.projects.git.auth', - ProjectsGitRemote = 'errors.projects.git.remote', - ProjectHead = 'errors.projects.head', - ProjectConflict = 'errors.projects.conflict' + ProjectsGitAuth = 'errors.projects.git.auth' } export class UserError extends Error { @@ -26,10 +22,17 @@ export class UserError extends Error { const cause = error instanceof Error ? error : undefined; const code = error.code ?? Code.Unknown; const message = error.message ?? error; - return new UserError(message, code, cause); + return new UserError(capitalize(message), code, cause); } } +function capitalize(str: string): string { + if (str.length === 0) { + return str; + } + return str.charAt(0).toUpperCase() + str.slice(1); +} + export async function invoke(command: string, params: Record = {}): Promise { // This commented out code can be used to delay/reject an api call // return new Promise((resolve, reject) => { diff --git a/crates/gitbutler-core/src/error.rs b/crates/gitbutler-core/src/error.rs index a26de15201..969d345f9e 100644 --- a/crates/gitbutler-core/src/error.rs +++ b/crates/gitbutler-core/src/error.rs @@ -2,170 +2,154 @@ //! //! This is a primer on how to use the types provided here. //! -//! Generally, if you do not care about attaching an error code for error-classification or -//! and/or messages that may show up in the user interface, read no further. Just use `anyhow` -//! or `thiserror` like before. +//! **tl;dr** - use `anyhow::Result` by direct import so a typical function looks like this: //! -//! ### Adding Context -//! -//! The [`Context`] type is the richest context we may attach to either `anyhow` errors or `thiserror`, -//! albeit using a different mechanism. This context is maintained as the error propagates and the -//! context higest up the error chain, the one most recently added, can be used by higher layers -//! of the GitButler application. Currently, a [`Context::message`] is shown in the user-interface, -//! whereas [`Context::code`] can be provided to help the user interface to make decisions, as it uses -//! the code for classifying errors. +//! ```rust +//!# use anyhow::{Result, bail}; +//! fn f() -> Result<()> { +//! bail!("this went wrong") +//! } +//! ``` //! -//! #### With `anyhow` +//! ### Providing Context //! -//! The basis is an error without context, just by using `anyhow` in any way. +//! To inform about what you were trying to do when it went wrong, assign some [`context`](anyhow::Context::context) +//! directly to [results](Result), to [`options`](Option) or to `anyhow` errors. //! -//!```rust -//!# use anyhow::bail; -//! fn f() -> anyhow::Result<()> { -//! bail!("internal information") +//! ```rust +//!# use anyhow::{anyhow, Result, bail, Context}; +//! fn maybe() -> Option<()> { +//! None //! } -//!``` //! -//! Adding context is as easy as using the `context()` method on any `Result` or [`anyhow::Error`]. -//! This can be a [`Code`], which automatically uses the message provided previously in the -//! frontend (note, though, that this is an implementation detail, which may change). -//! It serves as marker to make these messages show up, even if the code is [`Code::Unknown`]. +//! fn a() -> Result<()> { +//! maybe().context("didn't get it at this time") +//! } //! -//!```rust -//!# use anyhow::{anyhow}; -//!# use gitbutler_core::error::Code; -//! fn f() -> anyhow::Result<()> { -//! return Err(anyhow!("user information").context(Code::Unknown)) +//! fn b() -> Result<()> { +//! a().context("an operation couldn't be performed") //! } -//!``` //! -//! Finally, it's also possible to specify the user-message by using a [`Context`]. +//! fn c() -> Result<()> { +//! b().map_err(|err| err.context("sometimes useful")) +//! } //! -//!```rust -//!# use anyhow::{anyhow}; -//!# use gitbutler_core::error::{Code, Context}; -//! fn f() -> anyhow::Result<()> { -//! return Err(anyhow!("internal information").context(Context::new_static(Code::Unknown, "user information"))) +//! fn main() { +//! assert_eq!(format!("{:#}", c().unwrap_err()), +//! "sometimes useful: an operation couldn't be performed: didn't get it at this time"); //! } -//!``` +//! ``` //! -//! #### Backtraces and `anyhow` +//! ### Frontend Interactions //! -//! Backtraces are automatically collected when `anyhow` errors are instantiated, as long as the -//! `RUST_BACKTRACE` variable is set. +//! We don't know anything about frontends here, but we also have to know something to be able to control +//! which error messages show up. Sometimes the frontend needs to decide what to do based on a particular +//! error that happened, hence it has to classify errors and it shouldn't do that by matching strings. //! -//! #### With `thiserror` +//! #### Meet the `Code` //! -//! `thiserror` doesn't have a mechanism for generic context, which is why it has to be attached to -//! each type that is generated by thiserror. +//! The [`Code`] is a classifier for errors, and it can be attached as [`anyhow context`](anyhow::Context) +//! to be visible to `tauri`, which looks at the error chain to obtain such metadata. //! -//! By default, `thiserror` instances have no context. +//! By default, the frontend will show the stringified root error if a `tauri` command fails. +//! However, **sometimes we want to cut that short and display a particular message**. +//! +//! ```rust +//!# use anyhow::{Result, Context}; +//!# use gitbutler_core::error::Code; +//! +//! fn do_io() -> std::io::Result<()> { +//! Err(std::io::Error::new(std::io::ErrorKind::Other, "this didn't work")) +//! } //! -//!```rust -//! # use gitbutler_core::error::{Code, Context, ErrorWithContext}; -//! #[derive(thiserror::Error, Debug)] -//! #[error("user message")] -//! struct Error; +//! fn a() -> Result<()> { +//! do_io() +//! .context("whatever comes before a `Code` context shows in frontend, so THIS") +//! .context(Code::Unknown) +//! } //! -//! // But context can be added like this: -//! impl ErrorWithContext for Error {fn context(&self) -> Option { -//! // attach a code to make it show up in higher layers. -//! Some(Context::from(Code::Unknown)) -//! } +//! fn main() { +//! assert_eq!(format!("{:#}", a().unwrap_err()), +//! "errors.unknown: whatever comes before a `Code` context shows in frontend, so THIS: this didn't work", +//! "however, that Code also shows up in the error chain in logs - context is just like an Error for anyhow"); //! } //! ``` //! -//! Note that it's up to the implementation of [`ErrorWithContext`] to collect all context of errors in variants. +//! #### Tuning error chains //! -//!```rust -//! # use gitbutler_core::error::{AnyhowContextExt, Code, Context, ErrorWithContext}; +//! The style above was most convenient and can be used without hesitation, but if for some reason it's important +//! for `Code` not to show up in the error chain, one can use the [`error::Context`](Context) directly. //! -//! #[derive(thiserror::Error, Debug)] -//! #[error("user message")] -//! struct TinyError; +//! ```rust +//!# use anyhow::{Result, Context}; +//!# use gitbutler_core::error; //! -//! // But context can be added like this: -//! impl ErrorWithContext for TinyError { -//! fn context(&self) -> Option { -//! Some(Context::new_static(Code::Unknown, "tiny message")) -//! } +//! fn do_io() -> std::io::Result<()> { +//! Err(std::io::Error::new(std::io::ErrorKind::Other, "this didn't work")) //! } -//! #[derive(thiserror::Error, Debug)] -//! enum Error { -//! #[error(transparent)] -//! Tiny(#[from] TinyError), -//! #[error(transparent)] -//! Other(#[from] anyhow::Error) -//! }; //! -//! // But context can be added like this: -//! impl ErrorWithContext for Error { -//! fn context(&self) -> Option { -//! match self { -//! Error::Tiny(err) => err.context(), -//! Error::Other(err) => err.custom_context() -//! } -//! } +//! fn a() -> Result<()> { +//! do_io().context(error::Context::new("This message is shown and only this meessage") +//! .with_code(error::Code::Validation)) +//! } +//! +//! fn main() { +//! assert_eq!(format!("{:#}", a().unwrap_err()), +//! "This message is shown and only this meessage: this didn't work", +//! "now the added context just looks like an error, even though it also contains a `Code` which can be queried"); //! } //! ``` //! -//! ### Assuring Context +//! ### Backtraces and `anyhow` +//! +//! Backtraces are automatically collected when `anyhow` errors are instantiated, as long as the +//! `RUST_BACKTRACE` variable is set. +//! +//! #### With `thiserror` //! -//! Currently, the consumers of errors with context are quite primitive and thus rely on `anyhow` -//! to collect and find context hidden in the error chain. -//! To make that work, it's important that `thiserror` based errors never silently convert into -//! `anyhow::Error`, as the context-consumers wouldn't find the context anymore. +//! `thiserror` doesn't have a mechanism for generic context, and if it's needed the error can be converted +//! to `anyhow::Error`. //! -//! To prevent issues around this, make sure that relevant methods use the [`Error`] type provided -//! here. It is made to only automatically convert from types that have context information. -//! Those who have not will need to be converted by hand using [`Error::from_err()`]. +//! By default, `thiserror` instances have no context. use std::borrow::Cow; -use std::fmt::{self, Debug, Display}; +use std::fmt::Debug; /// A unique code that consumers of the API may rely on to identify errors. +/// +/// ### Important +/// +/// **Only add variants if a consumer, like the *frontend*, is actually using them**. +/// Remove variants when no longer in use. +/// +/// In practice, it should match its [frontend counterpart](https://github.com/gitbutlerapp/gitbutler/blob/fa973fd8f1ae8807621f47601803d98b8a9cf348/app/src/lib/backend/ipc.ts#L5). #[derive(Debug, Default, Copy, Clone, PartialOrd, PartialEq)] pub enum Code { + /// Much like a catch-all error code. It shouldn't be attached explicitly unless + /// a message is provided as well as part of a [`Context`]. #[default] Unknown, Validation, - Projects, - Branches, ProjectGitAuth, - ProjectGitRemote, - /// The push operation failed, specifically because the remote rejected it. - ProjectGitPush, - ProjectConflict, - ProjectHead, - Menu, - PreCommitHook, - CommitMsgHook, } impl std::fmt::Display for Code { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let code = match self { - Code::Menu => "errors.menu", Code::Unknown => "errors.unknown", Code::Validation => "errors.validation", - Code::Projects => "errors.projects", - Code::Branches => "errors.branches", Code::ProjectGitAuth => "errors.projects.git.auth", - Code::ProjectGitRemote => "errors.projects.git.remote", - Code::ProjectGitPush => "errors.projects.git.push", - Code::ProjectHead => "errors.projects.head", - Code::ProjectConflict => "errors.projects.conflict", - //TODO: rename js side to be more precise what kind of hook error this is - Code::PreCommitHook => "errors.hook", - Code::CommitMsgHook => "errors.hooks.commit.msg", }; f.write_str(code) } } -/// A context to wrap around lower errors to allow its classification, along with a message for the user. +/// A context for classifying errors. +/// +/// It provides a [`Code`], which may be [unknown](Code::Unknown), and a `message` which explains +/// more about the problem at hand. #[derive(Default, Debug, Clone)] pub struct Context { - /// The identifier of the error. + /// The classification of the error. pub code: Code, /// A description of what went wrong, if available. pub message: Option>, @@ -188,9 +172,9 @@ impl From for Context { impl Context { /// Create a new instance with `code` and an owned `message`. - pub fn new(code: Code, message: impl Into) -> Self { + pub fn new(message: impl Into) -> Self { Context { - code, + code: Code::Unknown, message: Some(Cow::Owned(message.into())), } } @@ -202,6 +186,12 @@ impl Context { message: Some(Cow::Borrowed(message)), } } + + /// Adjust the `code` of this instance to the given one. + pub fn with_code(mut self, code: Code) -> Self { + self.code = code; + self + } } mod private { @@ -237,92 +227,3 @@ impl AnyhowContextExt for anyhow::Error { }) } } - -/// A trait that if implemented on `thiserror` instance, allows to extract context we provide -/// in its variants. -/// -/// Note that this is a workaround for the inability to control or implement the `provide()` method -/// on the `std::error::Error` implementation of `thiserror`. -pub trait ErrorWithContext: std::error::Error { - /// Obtain the [`Context`], if present. - fn context(&self) -> Option; -} - -/// Convert `err` into an `anyhow` error, but also add provided `Code` or `Context` as anyhow context. -/// This uses the new `provide()` API to attach arbitrary information to error implementations. -pub fn into_anyhow(err: impl ErrorWithContext + Send + Sync + 'static) -> anyhow::Error { - let context = err.context(); - let err = anyhow::Error::from(err); - if let Some(context) = context { - err.context(context) - } else { - err - } -} - -/// A wrapper around an `anyhow` error which automatically extracts the context that might be attached -/// to `thiserror` instances. -/// -/// Whenever `thiserror` is involved, this error type should be used if the alternative would be to write -/// a `thiserror` which just forwards its context (like `app::Error` previously). -#[derive(Debug)] -pub struct Error(anyhow::Error); - -impl From for Error { - fn from(value: anyhow::Error) -> Self { - Self(value) - } -} - -impl From for anyhow::Error { - fn from(value: Error) -> Self { - value.0 - } -} - -impl Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - Display::fmt(&self.0, f) - } -} - -impl From for Error -where - E: ErrorWithContext + Send + Sync + 'static, -{ - fn from(value: E) -> Self { - Self(into_anyhow(value)) - } -} - -impl Error { - /// A manual, none-overlapping implementation of `From` (or else there are conflicts). - pub fn from_err(err: impl std::error::Error + Send + Sync + 'static) -> Self { - Self(err.into()) - } - - /// Associated more context to the contained anyhow error - pub fn context(self, context: C) -> Self - where - C: Display + Send + Sync + 'static, - { - let err = self.0; - Self(err.context(context)) - } - - /// Returns `true` if `E` is contained in our error chain. - pub fn is(&self) -> bool - where - E: Display + Debug + Send + Sync + 'static, - { - self.0.is::() - } - - /// Downcast our instance to the given type `E`, or `None` if it's not contained in our context or error chain. - pub fn downcast_ref(&self) -> Option<&E> - where - E: Display + Debug + Send + Sync + 'static, - { - self.0.downcast_ref::() - } -} diff --git a/crates/gitbutler-core/src/fs.rs b/crates/gitbutler-core/src/fs.rs index 727081e17f..9c2174b7b8 100644 --- a/crates/gitbutler-core/src/fs.rs +++ b/crates/gitbutler-core/src/fs.rs @@ -5,7 +5,7 @@ use std::{ path::{Path, PathBuf}, }; -use anyhow::Result; +use anyhow::{Context, Result}; use bstr::BString; use gix::{ dir::walk::EmissionMode, @@ -104,9 +104,7 @@ fn persist_tempfile( /// Reads and parses the state file. /// /// If the file does not exist, it will be created. -pub(crate) fn read_toml_file_or_default( - path: &Path, -) -> Result { +pub(crate) fn read_toml_file_or_default(path: &Path) -> Result { let mut file = match File::open(path) { Ok(f) => f, Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(T::default()), @@ -114,9 +112,7 @@ pub(crate) fn read_toml_file_or_default( }; let mut contents = String::new(); file.read_to_string(&mut contents)?; - let value: T = toml::from_str(&contents).map_err(|err| crate::reader::Error::ParseError { - path: path.to_owned(), - source: err, - })?; + let value: T = + toml::from_str(&contents).with_context(|| format!("Failed to parse {}", path.display()))?; Ok(value) } diff --git a/crates/gitbutler-core/src/git/credentials.rs b/crates/gitbutler-core/src/git/credentials.rs index a7b49b7b24..b6d02d6092 100644 --- a/crates/gitbutler-core/src/git/credentials.rs +++ b/crates/gitbutler-core/src/git/credentials.rs @@ -1,7 +1,6 @@ use std::path::PathBuf; -use crate::error::{AnyhowContextExt, Code, Context, ErrorWithContext}; -use crate::{error, keys, project_repository, projects, users}; +use crate::{keys, project_repository, projects, users}; #[derive(Debug, Clone, PartialEq, Eq)] pub enum SshCredential { @@ -87,19 +86,6 @@ pub enum HelpError { Other(#[from] anyhow::Error), } -impl ErrorWithContext for HelpError { - fn context(&self) -> Option { - Some(match self { - HelpError::NoUrlSet => { - error::Context::new_static(Code::ProjectGitRemote, "no url set for remote") - } - HelpError::UrlConvertError(_) => Code::ProjectGitRemote.into(), - HelpError::Git(_) => return None, - HelpError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - impl Helper { pub fn new( keys: keys::Controller, diff --git a/crates/gitbutler-core/src/git/error.rs b/crates/gitbutler-core/src/git/error.rs index 63c5e2e7f8..d87c43e6fd 100644 --- a/crates/gitbutler-core/src/git/error.rs +++ b/crates/gitbutler-core/src/git/error.rs @@ -1,18 +1,11 @@ use std::str::Utf8Error; -use crate::{ - error::{Context, ErrorWithContext}, - keys, -}; - #[derive(Debug, thiserror::Error)] pub enum Error { #[error("not found: {0}")] NotFound(git2::Error), #[error("authentication failed")] Auth(git2::Error), - #[error("sign error: {0}")] - Signing(keys::SignError), #[error("remote url error: {0}")] Url(super::url::ParseError), #[error("io error: {0}")] @@ -54,18 +47,6 @@ impl From for Error { } } -impl ErrorWithContext for Error { - fn context(&self) -> Option { - None - } -} - -impl From for Error { - fn from(err: keys::SignError) -> Self { - Error::Signing(err) - } -} - impl From for Error { fn from(err: super::url::ParseError) -> Self { Error::Url(err) diff --git a/crates/gitbutler-core/src/git/repository.rs b/crates/gitbutler-core/src/git/repository.rs index 8908793fcc..a9da6ad845 100644 --- a/crates/gitbutler-core/src/git/repository.rs +++ b/crates/gitbutler-core/src/git/repository.rs @@ -589,7 +589,7 @@ impl Repository { /// Returns a list of remotes /// - /// Returns Vec instead of StringArray because StringArray cannot safly be sent between threads + /// Returns `Vec` instead of StringArray because StringArray cannot safly be sent between threads pub fn remotes(&self) -> Result> { self.0 .remotes() diff --git a/crates/gitbutler-core/src/keys/key.rs b/crates/gitbutler-core/src/keys/key.rs index 066919e247..d1b32fd5d1 100644 --- a/crates/gitbutler-core/src/keys/key.rs +++ b/crates/gitbutler-core/src/keys/key.rs @@ -7,12 +7,6 @@ use ssh_key::{HashAlg, LineEnding, SshSig}; #[derive(Debug, Clone, Eq)] pub struct PrivateKey(ssh_key::PrivateKey); -#[derive(Debug, thiserror::Error)] -pub enum SignError { - #[error(transparent)] - Ssh(#[from] ssh_key::Error), -} - impl PrivateKey { pub fn generate() -> Self { Self::default() @@ -22,7 +16,7 @@ impl PrivateKey { PublicKey::from(self) } - pub fn sign(&self, bytes: &[u8]) -> Result { + pub fn sign(&self, bytes: &[u8]) -> anyhow::Result { let sig = SshSig::sign(&self.0, "git", HashAlg::Sha512, bytes)?; sig.to_pem(LineEnding::default()).map_err(Into::into) } diff --git a/crates/gitbutler-core/src/keys/mod.rs b/crates/gitbutler-core/src/keys/mod.rs index 852f758bc6..9649448e14 100644 --- a/crates/gitbutler-core/src/keys/mod.rs +++ b/crates/gitbutler-core/src/keys/mod.rs @@ -3,4 +3,4 @@ mod key; pub mod storage; pub use controller::*; -pub use key::{PrivateKey, PublicKey, SignError}; +pub use key::{PrivateKey, PublicKey}; diff --git a/crates/gitbutler-core/src/keys/storage.rs b/crates/gitbutler-core/src/keys/storage.rs index fc44a0832b..5721bad6b5 100644 --- a/crates/gitbutler-core/src/keys/storage.rs +++ b/crates/gitbutler-core/src/keys/storage.rs @@ -1,5 +1,6 @@ use super::PrivateKey; use crate::storage; +use anyhow::Result; use std::path::PathBuf; // TODO(ST): get rid of this type, it's more trouble than it's worth. @@ -8,14 +9,6 @@ pub struct Storage { inner: storage::Storage, } -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error(transparent)] - Storage(#[from] std::io::Error), - #[error("SSH key error: {0}")] - SSHKey(#[from] ssh_key::Error), -} - impl Storage { pub fn new(storage: storage::Storage) -> Storage { Storage { inner: storage } @@ -25,13 +18,13 @@ impl Storage { Storage::new(storage::Storage::new(path)) } - pub fn get(&self) -> Result, Error> { + pub fn get(&self) -> Result> { let key = self.inner.read("keys/ed25519")?; key.map(|s| s.parse().map_err(Into::into)).transpose() } // TODO(ST): see if Key should rather deal with bytes instead for this kind of serialization. - pub fn create(&self, key: &PrivateKey) -> Result<(), Error> { + pub fn create(&self, key: &PrivateKey) -> Result<()> { self.inner.write("keys/ed25519", &key.to_string())?; self.inner .write("keys/ed25519.pub", &key.public_key().to_string())?; diff --git a/crates/gitbutler-core/src/ops/entry.rs b/crates/gitbutler-core/src/ops/entry.rs index 3a03eec0df..574fbaebca 100644 --- a/crates/gitbutler-core/src/ops/entry.rs +++ b/crates/gitbutler-core/src/ops/entry.rs @@ -35,7 +35,7 @@ pub struct Snapshot { /// The payload of a snapshot commit /// -/// This is persisted as a commit message in the title, body and trailers format (https://git-scm.com/docs/git-interpret-trailers) +/// This is persisted as a commit message in the title, body and trailers format () #[derive(Debug, PartialEq, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct SnapshotDetails { @@ -196,7 +196,7 @@ impl FromStr for Version { } /// Represents a key value pair stored in a snapshot, like `key: value\n` -/// Using the git trailer format (https://git-scm.com/docs/git-interpret-trailers) +/// Using the git trailer format () #[derive(Debug, PartialEq, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct Trailer { diff --git a/crates/gitbutler-core/src/ops/reflog.rs b/crates/gitbutler-core/src/ops/reflog.rs index cfe86f9237..deab75f441 100644 --- a/crates/gitbutler-core/src/ops/reflog.rs +++ b/crates/gitbutler-core/src/ops/reflog.rs @@ -174,6 +174,10 @@ mod set_target_ref { let contents = std::fs::read_to_string(&log_file_path)?; assert_eq!(reflog_lines(&contents).len(), 2); + + let contents = std::fs::read_to_string(&log_file_path)?; + let lines = reflog_lines(&contents); + assert_signature(lines[0].signature); Ok(()) } @@ -222,12 +226,16 @@ mod set_target_ref { let oplog = git::Oid::from_str("0123456789abcdef0123456789abcdef0123456")?; set_reference_to_oplog(worktree_dir, commit_id.into(), oplog).expect("success"); - let loose_ref_log_path = worktree_dir.join(".git/logs/refs/heads/gitbutler/target"); - std::fs::remove_file(&loose_ref_log_path)?; + let log_file_path = worktree_dir.join(".git/logs/refs/heads/gitbutler/target"); + std::fs::remove_file(&log_file_path)?; set_reference_to_oplog(worktree_dir, commit_id.into(), oplog) .expect("missing reflog files are recreated"); - assert!(loose_ref_log_path.is_file(), "the file was recreated"); + assert!(log_file_path.is_file(), "the file was recreated"); + + let contents = std::fs::read_to_string(&log_file_path)?; + let lines = reflog_lines(&contents); + assert_signature(lines[0].signature); Ok(()) } @@ -346,6 +354,10 @@ mod set_target_ref { fn assert_signature(sig: gix::actor::SignatureRef<'_>) { assert_eq!(sig.name, GITBUTLER_INTEGRATION_COMMIT_AUTHOR_NAME); assert_eq!(sig.email, GITBUTLER_INTEGRATION_COMMIT_AUTHOR_EMAIL); + assert_ne!( + sig.time.seconds, 0, + "we don't accidentally use the default time as it would caues GC as well" + ); } fn setup_repo() -> anyhow::Result<(tempfile::TempDir, git2::Oid)> { diff --git a/crates/gitbutler-core/src/ops/snapshot.rs b/crates/gitbutler-core/src/ops/snapshot.rs index a5b33b3524..fc7410dae3 100644 --- a/crates/gitbutler-core/src/ops/snapshot.rs +++ b/crates/gitbutler-core/src/ops/snapshot.rs @@ -1,4 +1,4 @@ -use crate::error::Error; +use anyhow::Result; use std::vec; use crate::projects::Project; @@ -15,7 +15,7 @@ impl Project { pub(crate) fn snapshot_branch_applied( &self, snapshot_tree: git::Oid, - result: Result<&String, &Error>, + result: Result<&String, &anyhow::Error>, ) -> anyhow::Result<()> { let result = result.map(|o| Some(o.clone())); let details = SnapshotDetails::new(OperationKind::ApplyBranch) @@ -26,7 +26,7 @@ impl Project { pub(crate) fn snapshot_branch_unapplied( &self, snapshot_tree: git::Oid, - result: Result<&Option, &Error>, + result: Result<&Option, &anyhow::Error>, ) -> anyhow::Result<()> { let result = result.map(|o| o.clone().map(|b| b.name)); let details = SnapshotDetails::new(OperationKind::UnapplyBranch) @@ -37,7 +37,7 @@ impl Project { pub(crate) fn snapshot_commit_undo( &self, snapshot_tree: git::Oid, - result: Result<&(), &Error>, + result: Result<&(), &anyhow::Error>, commit_sha: git::Oid, ) -> anyhow::Result<()> { let result = result.map(|_| Some(commit_sha.to_string())); @@ -49,7 +49,7 @@ impl Project { pub(crate) fn snapshot_commit_creation( &self, snapshot_tree: git::Oid, - error: Option<&Error>, + error: Option<&anyhow::Error>, commit_message: String, sha: Option, ) -> anyhow::Result<()> { @@ -162,7 +162,7 @@ impl Project { } } -fn result_trailer(result: Result, &Error>, key: String) -> Vec { +fn result_trailer(result: Result, &anyhow::Error>, key: String) -> Vec { match result { Ok(v) => { if let Some(v) = v { @@ -181,7 +181,7 @@ fn result_trailer(result: Result, &Error>, key: String) -> Vec) -> Vec { +fn error_trailer(error: Option<&anyhow::Error>) -> Vec { error .map(|e| { vec![Trailer { diff --git a/crates/gitbutler-core/src/ops/state.rs b/crates/gitbutler-core/src/ops/state.rs index 85edbef957..c235caca6c 100644 --- a/crates/gitbutler-core/src/ops/state.rs +++ b/crates/gitbutler-core/src/ops/state.rs @@ -88,7 +88,7 @@ impl OplogHandle { /// /// If the file does not exist, it will be created. fn read_file(&self) -> Result { - Ok(read_toml_file_or_default(&self.file_path)?) + read_toml_file_or_default(&self.file_path) } fn write_file(&self, mut oplog: Oplog) -> Result<()> { diff --git a/crates/gitbutler-core/src/project_repository/conflicts.rs b/crates/gitbutler-core/src/project_repository/conflicts.rs index 833d2d3f3b..ade20a2f74 100644 --- a/crates/gitbutler-core/src/project_repository/conflicts.rs +++ b/crates/gitbutler-core/src/project_repository/conflicts.rs @@ -97,7 +97,7 @@ pub fn conflicting_files(repository: &Repository) -> Result> { /// Check if `path` is conflicting in `repository`, or if `None`, check if there is any conflict. // TODO(ST): Should this not rather check the conflicting state in the index? -pub fn is_conflicting>(repository: &Repository, path: Option

) -> Result { +pub fn is_conflicting(repository: &Repository, path: Option<&Path>) -> Result { let conflicts_path = repository.git_repository.path().join("conflicts"); if !conflicts_path.exists() { return Ok(false); @@ -105,11 +105,9 @@ pub fn is_conflicting>(repository: &Repository, path: Option

) let file = std::fs::File::open(conflicts_path)?; let reader = std::io::BufReader::new(file); + // TODO(ST): This shouldn't work on UTF8 strings. let mut files = reader.lines().map_ok(PathBuf::from); if let Some(pathname) = path { - // TODO(ST): This shouldn't work on UTF8 strings. - let pathname = pathname.as_ref(); - // check if pathname is one of the lines in conflicts_path file for line in files { let line = line?; diff --git a/crates/gitbutler-core/src/project_repository/mod.rs b/crates/gitbutler-core/src/project_repository/mod.rs index 79ba8b1d0b..6b823024fd 100644 --- a/crates/gitbutler-core/src/project_repository/mod.rs +++ b/crates/gitbutler-core/src/project_repository/mod.rs @@ -3,6 +3,6 @@ pub mod conflicts; mod repository; pub use config::Config; -pub use repository::{LogUntil, OpenError, RemoteError, Repository}; +pub use repository::{LogUntil, Repository}; pub mod signatures; diff --git a/crates/gitbutler-core/src/project_repository/repository.rs b/crates/gitbutler-core/src/project_repository/repository.rs index 8ecabc2491..df0cb12915 100644 --- a/crates/gitbutler-core/src/project_repository/repository.rs +++ b/crates/gitbutler-core/src/project_repository/repository.rs @@ -4,108 +4,103 @@ use std::{ sync::{atomic::AtomicUsize, Arc}, }; -use anyhow::{Context, Result}; +use anyhow::{anyhow, Context, Result}; use super::conflicts; +use crate::virtual_branches::errors::Marker; use crate::{ - askpass, error, - git::{self, credentials::HelpError, Url}, + askpass, + git::{self, Url}, projects::{self, AuthKey}, ssh, users, virtual_branches::{Branch, BranchId}, }; -use crate::{ - error::{AnyhowContextExt, Code, ErrorWithContext}, - git::Oid, -}; +use crate::{error::Code, git::Oid}; pub struct Repository { pub git_repository: git::Repository, project: projects::Project, } -#[derive(Debug, thiserror::Error)] -pub enum OpenError { - #[error("repository not found at {0}")] - NotFound(path::PathBuf), - #[error(transparent)] - Other(anyhow::Error), -} - -impl ErrorWithContext for OpenError { - fn context(&self) -> Option { - match self { - OpenError::NotFound(path) => { - error::Context::new(Code::Projects, format!("{} not found", path.display())).into() - } - OpenError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - impl Repository { - pub fn open(project: &projects::Project) -> Result { - git::Repository::open(&project.path) - .map_err(|error| match error { - git::Error::NotFound(_) => OpenError::NotFound(project.path.clone()), - other => OpenError::Other(other.into()), - }) - .map(|git_repository| { - // XXX(qix-): This is a temporary measure to disable GC on the project repository. - // XXX(qix-): We do this because the internal repository we use to store the "virtual" - // XXX(qix-): refs and information use Git's alternative-objects mechanism to refer - // XXX(qix-): to the project repository's objects. However, the project repository - // XXX(qix-): has no knowledge of these refs, and will GC them away (usually after - // XXX(qix-): about 2 weeks) which will corrupt the internal repository. - // XXX(qix-): - // XXX(qix-): We will ultimately move away from an internal repository for a variety - // XXX(qix-): of reasons, but for now, this is a simple, short-term solution that we - // XXX(qix-): can clean up later on. We're aware this isn't ideal. - if let Ok(config) = git_repository.config().as_mut() { - let should_set = match config.get_bool("gitbutler.didSetPrune") { - Ok(None | Some(false)) => true, - Ok(Some(true)) => false, - Err(error) => { - tracing::warn!( + pub fn open(project: &projects::Project) -> Result { + let repo = git::Repository::open(&project.path).map_err(|err| match err { + git::Error::NotFound(_) => anyhow::Error::from(err).context(format!( + "repository not found at \"{}\"", + project.path.display() + )), + other => other.into(), + })?; + + // XXX(qix-): This is a temporary measure to disable GC on the project repository. + // XXX(qix-): We do this because the internal repository we use to store the "virtual" + // XXX(qix-): refs and information use Git's alternative-objects mechanism to refer + // XXX(qix-): to the project repository's objects. However, the project repository + // XXX(qix-): has no knowledge of these refs, and will GC them away (usually after + // XXX(qix-): about 2 weeks) which will corrupt the internal repository. + // XXX(qix-): + // XXX(qix-): We will ultimately move away from an internal repository for a variety + // XXX(qix-): of reasons, but for now, this is a simple, short-term solution that we + // XXX(qix-): can clean up later on. We're aware this isn't ideal. + if let Ok(config) = repo.config().as_mut() { + let should_set = match config.get_bool("gitbutler.didSetPrune") { + Ok(None | Some(false)) => true, + Ok(Some(true)) => false, + Err(err) => { + tracing::warn!( "failed to get gitbutler.didSetPrune for repository at {}; cannot disable gc: {}", project.path.display(), - error + err ); - false - } - }; + false + } + }; - if should_set { - if let Err(error) = config - .set_str("gc.pruneExpire", "never") - .and_then(|()| config.set_bool("gitbutler.didSetPrune", true)) - { - tracing::warn!( + if should_set { + if let Err(error) = config + .set_str("gc.pruneExpire", "never") + .and_then(|()| config.set_bool("gitbutler.didSetPrune", true)) + { + tracing::warn!( "failed to set gc.auto to false for repository at {}; cannot disable gc: {}", project.path.display(), error ); - } - } - } else { - tracing::warn!( - "failed to get config for repository at {}; cannot disable gc", - project.path.display() - ); } + } + } else { + tracing::warn!( + "failed to get config for repository at {}; cannot disable gc", + project.path.display() + ); + } - git_repository - }) - .map(|git_repository| Self { - git_repository, - project: project.clone(), - }) + Ok(Self { + git_repository: repo, + project: project.clone(), + }) } pub fn is_resolving(&self) -> bool { conflicts::is_resolving(self) } + pub fn assure_resolved(&self) -> Result<()> { + if self.is_resolving() { + Err(anyhow!("project has active conflicts")).context(Marker::ProjectConflict) + } else { + Ok(()) + } + } + + pub fn assure_unconflicted(&self) -> Result<()> { + if conflicts::is_conflicting(self, None)? { + Err(anyhow!("project has active conflicts")).context(Marker::ProjectConflict) + } else { + Ok(()) + } + } + pub fn path(&self) -> &path::Path { path::Path::new(&self.project.path) } @@ -351,18 +346,17 @@ impl Repository { &self, user: Option<&users::User>, ref_specs: &[&str], - ) -> Result { + ) -> Result { let url = self .project .api .as_ref() - .ok_or(RemoteError::Other(anyhow::anyhow!("api not set")))? + .context("api not set")? .code_git_url .as_ref() - .ok_or(RemoteError::Other(anyhow::anyhow!("code_git_url not set")))? + .context("code_git_url not set")? .as_str() - .parse::() - .map_err(|e| RemoteError::Other(e.into()))?; + .parse::()?; tracing::debug!( project_id = %self.project.id, @@ -372,7 +366,8 @@ impl Repository { let access_token = user .map(|user| user.access_token.clone()) - .ok_or(RemoteError::Auth)?; + .context("access token is missing") + .context(Code::ProjectGitAuth)?; let mut callbacks = git2::RemoteCallbacks::new(); if self.project.omit_certificate_check.unwrap_or(false) { @@ -395,23 +390,16 @@ impl Repository { let headers = &[auth_header.as_str()]; push_options.custom_headers(headers); - let mut remote = self - .git_repository - .remote_anonymous(&url) - .map_err(|e| RemoteError::Other(e.into()))?; + let mut remote = self.git_repository.remote_anonymous(&url)?; remote .push(ref_specs, Some(&mut push_options)) - .map_err(|error| match error { - git::Error::Network(error) => { - tracing::warn!(project_id = %self.project.id, ?error, "git push failed",); - RemoteError::Network - } - git::Error::Auth(error) => { - tracing::warn!(project_id = %self.project.id, ?error, "git push failed",); - RemoteError::Auth - } - error => RemoteError::Other(error.into()), + .map_err(|err| match err { + git::Error::Network(err) => anyhow!("network failed").context(err), + git::Error::Auth(err) => anyhow!("authentication failed") + .context(Code::ProjectGitAuth) + .context(err), + err => anyhow::Error::from(err), })?; let bytes_pushed = bytes_pushed.load(std::sync::atomic::Ordering::Relaxed); @@ -436,7 +424,7 @@ impl Repository { credentials: &git::credentials::Helper, refspec: Option, askpass_broker: Option>, - ) -> Result<(), RemoteError> { + ) -> Result<()> { let refspec = refspec.unwrap_or_else(|| { if with_force { format!("+{}:refs/heads/{}", head, branch.branch()) @@ -468,7 +456,7 @@ impl Repository { }) .join() .unwrap() - .map_err(|e| RemoteError::Other(e.into())); + .map_err(Into::into); } let auth_flows = credentials.help(self, branch.remote())?; @@ -507,27 +495,24 @@ impl Repository { ); return Ok(()); } - Err(git::Error::Auth(error) | git::Error::Http(error)) => { - tracing::warn!(project_id = %self.project.id, ?error, "git push failed"); + Err(git::Error::Auth(err) | git::Error::Http(err)) => { + tracing::warn!(project_id = %self.project.id, ?err, "git push failed"); continue; } - Err(git::Error::Network(error)) => { - tracing::warn!(project_id = %self.project.id, ?error, "git push failed"); - return Err(RemoteError::Network); + Err(git::Error::Network(err)) => { + return Err(anyhow!("network failed")).context(err); } Err(err) => { - if let Some(err) = update_refs_error.as_ref() { - return Err(RemoteError::Other( - anyhow::anyhow!(err.to_string()).context(Code::ProjectGitPush), - )); + if let Some(update_refs_err) = update_refs_error { + return Err(update_refs_err).context(err); } - return Err(RemoteError::Other(err.into())); + return Err(err.into()); } } } } - Err(RemoteError::Auth) + Err(anyhow!("authentication failed").context(Code::ProjectGitAuth)) } pub fn fetch( @@ -535,7 +520,7 @@ impl Repository { remote_name: &str, credentials: &git::credentials::Helper, askpass: Option, - ) -> Result<(), RemoteError> { + ) -> Result<()> { let refspec = format!("+refs/heads/*:refs/remotes/{}/*", remote_name); // NOTE(qix-): This is a nasty hack, however the codebase isn't structured @@ -560,7 +545,7 @@ impl Repository { }) .join() .unwrap() - .map_err(|e| RemoteError::Other(e.into())); + .map_err(Into::into); } let auth_flows = credentials.help(self, remote_name)?; @@ -584,20 +569,20 @@ impl Repository { tracing::info!(project_id = %self.project.id, %refspec, "git fetched"); return Ok(()); } - Err(git::Error::Auth(error) | git::Error::Http(error)) => { - tracing::warn!(project_id = %self.project.id, ?error, "fetch failed"); + Err(git::Error::Auth(err) | git::Error::Http(err)) => { + tracing::warn!(project_id = %self.project.id, ?err, "fetch failed"); continue; } - Err(git::Error::Network(error)) => { - tracing::warn!(project_id = %self.project.id, ?error, "fetch failed"); - return Err(RemoteError::Network); + Err(git::Error::Network(err)) => { + tracing::warn!(project_id = %self.project.id, ?err, "fetch failed"); + return Err(anyhow!("network failed")).context(err); } - Err(error) => return Err(RemoteError::Other(error.into())), + Err(err) => return Err(err.into()), } } } - Err(RemoteError::Auth) + Err(anyhow!("authentication failed")).context(Code::ProjectGitAuth) } pub fn remotes(&self) -> Result> { @@ -613,41 +598,6 @@ impl Repository { } } -#[derive(Debug, thiserror::Error)] -pub enum RemoteError { - #[error(transparent)] - Help(#[from] HelpError), - #[error("network failed")] - Network, - #[error("authentication failed")] - Auth, - #[error("Git failed")] - Git(#[from] git::Error), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for RemoteError { - fn context(&self) -> Option { - Some(match self { - RemoteError::Help(error) => return error.context(), - RemoteError::Network => { - error::Context::new_static(Code::ProjectGitRemote, "Network error occurred") - } - RemoteError::Auth => error::Context::new_static( - Code::ProjectGitAuth, - "Project remote authentication error", - ), - RemoteError::Git(_) => { - error::Context::new_static(Code::ProjectGitRemote, "Git command failed") - } - RemoteError::Other(error) => { - return error.custom_context_or_root_cause().into(); - } - }) - } -} - type OidFilter = dyn Fn(&git2::Commit) -> Result; pub enum LogUntil { diff --git a/crates/gitbutler-core/src/project_repository/signatures.rs b/crates/gitbutler-core/src/project_repository/signatures.rs index 7a41955330..b31fbfe54c 100644 --- a/crates/gitbutler-core/src/project_repository/signatures.rs +++ b/crates/gitbutler-core/src/project_repository/signatures.rs @@ -21,18 +21,11 @@ pub fn signatures<'a>( Ok((author, comitter)) } -fn try_from<'a>(value: &users::User) -> Result, git::Error> { - if let Some(name) = &value.name { - git2::Signature::now(name, &value.email) - .map(Into::into) - .map_err(Into::into) - } else if let Some(name) = &value.given_name { - git2::Signature::now(name, &value.email) - .map(Into::into) - .map_err(Into::into) - } else { - git2::Signature::now(&value.email, &value.email) - .map(Into::into) - .map_err(Into::into) - } +fn try_from(value: &users::User) -> Result, git::Error> { + let name = value + .name + .as_deref() + .or(value.given_name.as_deref()) + .unwrap_or(&value.email); + Ok(git2::Signature::now(name, &value.email)?) } diff --git a/crates/gitbutler-core/src/projects/controller.rs b/crates/gitbutler-core/src/projects/controller.rs index b35f93c07d..b0f7c90e88 100644 --- a/crates/gitbutler-core/src/projects/controller.rs +++ b/crates/gitbutler-core/src/projects/controller.rs @@ -3,15 +3,12 @@ use std::{ sync::Arc, }; -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; use async_trait::async_trait; use super::{storage, storage::UpdateRequest, Project, ProjectId}; +use crate::projects::AuthKey; use crate::{error, project_repository}; -use crate::{ - error::{AnyhowContextExt, Code, Error, ErrorWithContext}, - projects::AuthKey, -}; #[async_trait] pub trait Watchers { @@ -50,36 +47,37 @@ impl Controller { } } - pub fn add>(&self, path: P) -> Result { + pub fn add>(&self, path: P) -> Result { let path = path.as_ref(); let all_projects = self .projects_storage .list() .context("failed to list projects from storage")?; if all_projects.iter().any(|project| project.path == path) { - return Err(AddError::AlreadyExists); + bail!("project already exists"); } if !path.exists() { - return Err(AddError::PathNotFound); + bail!("path not found"); } if !path.is_dir() { - return Err(AddError::NotADirectory); + bail!("not a directory"); } match gix::open_opts(path, gix::open::Options::isolated()) { Ok(repo) if repo.is_bare() => { - return Err(AddError::BareUnsupported); + bail!("bare repositories are unsupported"); } Ok(repo) if repo.worktree().map_or(false, |wt| !wt.is_main()) => { if path.join(".git").is_file() { - return Err(AddError::WorktreeNotSupported); + bail!("can only work in main worktrees"); }; } Ok(repo) if repo.submodules().map_or(false, |sm| sm.is_some()) => { - return Err(AddError::SubmodulesNotSupported); + bail!("repositories with git submodules are not supported"); } Ok(_repo) => {} Err(err) => { - return Err(AddError::NotAGitRepository(Box::new(err))); + return Err(anyhow::Error::from(err)) + .context(error::Context::new("must be a Git repository")); } } @@ -115,7 +113,7 @@ impl Controller { Ok(project) } - pub async fn update(&self, project: &UpdateRequest) -> Result { + pub async fn update(&self, project: &UpdateRequest) -> Result { #[cfg(not(windows))] if let Some(AuthKey::Local { private_key_path, .. @@ -125,15 +123,17 @@ impl Controller { let private_key_path = private_key_path.resolve(); if !private_key_path.exists() { - return Err(UpdateError::Validation(UpdateValidationError::KeyNotFound( - private_key_path.to_path_buf(), - ))); + bail!( + "private key at \"{}\" not found", + private_key_path.display() + ); } if !private_key_path.is_file() { - return Err(UpdateError::Validation(UpdateValidationError::KeyNotFile( - private_key_path.to_path_buf(), - ))); + bail!( + "private key at \"{}\" is not a file", + private_key_path.display() + ); } } @@ -149,63 +149,42 @@ impl Controller { #[cfg(windows)] let project = &project_owned; - let updated = self - .projects_storage - .update(project) - .map_err(|error| match error { - super::storage::Error::NotFound => UpdateError::NotFound, - error => UpdateError::Other(error.into()), - })?; - - Ok(updated) + self.projects_storage.update(project) } - pub fn get(&self, id: ProjectId) -> Result { - let project = self.projects_storage.get(id).map_err(|error| match error { - super::storage::Error::NotFound => GetError::NotFound, - error => GetError::Other(error.into()), - }); - if let Ok(project) = &project { - if !project.gb_dir().exists() { - if let Err(error) = std::fs::create_dir_all(project.gb_dir()) { - tracing::error!(project_id = %project.id, ?error, "failed to create {:?} on project get", project.gb_dir()); - } + pub fn get(&self, id: ProjectId) -> Result { + #[cfg_attr(not(windows), allow(unused_mut))] + let mut project = self.projects_storage.get(id)?; + if !project.gb_dir().exists() { + if let Err(error) = std::fs::create_dir_all(project.gb_dir()) { + tracing::error!(project_id = %project.id, ?error, "failed to create \"{}\" on project get", project.gb_dir().display()); } - // Clean up old virtual_branches.toml that was never used - if project - .path - .join(".git") - .join("virtual_branches.toml") - .exists() - { - if let Err(error) = - std::fs::remove_file(project.path.join(".git").join("virtual_branches.toml")) - { - tracing::error!(project_id = %project.id, ?error, "failed to remove old virtual_branches.toml"); - } + } + // Clean up old virtual_branches.toml that was never used + let old_virtual_branches_path = project.path.join(".git").join("virtual_branches.toml"); + if old_virtual_branches_path.exists() { + if let Err(error) = std::fs::remove_file(old_virtual_branches_path) { + tracing::error!(project_id = %project.id, ?error, "failed to remove old virtual_branches.toml"); } } // FIXME(qix-): On windows, we have to force to system executable #[cfg(windows)] - let project = project.map(|mut p| { - p.preferred_key = AuthKey::SystemExecutable; - p - }); + { + project.preferred_key = AuthKey::SystemExecutable; + } - project + Ok(project) } pub fn list(&self) -> Result> { self.projects_storage.list().map_err(Into::into) } - pub async fn delete(&self, id: ProjectId) -> Result<(), Error> { - let project = match self.projects_storage.get(id) { - Ok(project) => Ok(project), - Err(super::storage::Error::NotFound) => return Ok(()), - Err(error) => Err(Error::from_err(error)), - }?; + pub async fn delete(&self, id: ProjectId) -> Result<()> { + let Some(project) = self.projects_storage.try_get(id)? else { + return Ok(()); + }; if let Some(watchers) = &self.watchers { watchers.stop(id).await; @@ -236,154 +215,17 @@ impl Controller { Ok(()) } - pub fn get_local_config( - &self, - id: ProjectId, - key: &str, - ) -> Result, ConfigError> { - let project = self.projects_storage.get(id).map_err(|error| match error { - super::storage::Error::NotFound => ConfigError::NotFound, - error => ConfigError::Other(error.into()), - })?; - - let repo = project_repository::Repository::open(&project) - .map_err(|e| ConfigError::Other(e.into()))?; - repo.config() - .get_local(key) - .map_err(|e| ConfigError::Other(e.into())) - } - - pub fn set_local_config( - &self, - id: ProjectId, - key: &str, - value: &str, - ) -> Result<(), ConfigError> { - let project = self.projects_storage.get(id).map_err(|error| match error { - super::storage::Error::NotFound => ConfigError::NotFound, - error => ConfigError::Other(error.into()), - })?; - - let repo = project_repository::Repository::open(&project) - .map_err(|e| ConfigError::Other(e.into()))?; - repo.config() - .set_local(key, value) - .map_err(|e| ConfigError::Other(e.into()))?; - - Ok(()) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ConfigError { - #[error("project not found")] - NotFound, - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -#[derive(Debug, thiserror::Error)] -pub enum GetError { - #[error("project not found")] - NotFound, - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl error::ErrorWithContext for GetError { - fn context(&self) -> Option { - match self { - GetError::NotFound => { - error::Context::new_static(Code::Projects, "Project not found").into() - } - GetError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UpdateError { - #[error("project not found")] - NotFound, - #[error(transparent)] - Validation(UpdateValidationError), - #[error(transparent)] - Other(#[from] anyhow::Error), -} + pub fn get_local_config(&self, id: ProjectId, key: &str) -> Result> { + let project = self.projects_storage.get(id)?; -impl ErrorWithContext for UpdateError { - fn context(&self) -> Option { - Some(match self { - UpdateError::Validation(UpdateValidationError::KeyNotFound(path)) => { - error::Context::new(Code::Projects, format!("'{}' not found", path.display())) - } - UpdateError::Validation(UpdateValidationError::KeyNotFile(path)) => { - error::Context::new( - Code::Projects, - format!("'{}' is not a file", path.display()), - ) - } - UpdateError::NotFound => { - error::Context::new_static(Code::Projects, "Project not found") - } - UpdateError::Other(error) => return error.custom_context_or_root_cause().into(), - }) + let repo = project_repository::Repository::open(&project)?; + Ok(repo.config().get_local(key)?) } -} -#[derive(Debug, thiserror::Error)] -pub enum UpdateValidationError { - #[error("{0} not found")] - KeyNotFound(PathBuf), - #[error("{0} is not a file")] - KeyNotFile(PathBuf), -} - -#[derive(Debug, thiserror::Error)] -pub enum AddError { - #[error("not a directory")] - NotADirectory, - #[error("not a git repository")] - NotAGitRepository(#[from] Box), - #[error("bare repositories are not supported")] - BareUnsupported, - #[error("worktrees unsupported")] - WorktreeNotSupported, - #[error("path not found")] - PathNotFound, - #[error("project already exists")] - AlreadyExists, - #[error("submodules not supported")] - SubmodulesNotSupported, - #[error(transparent)] - OpenProjectRepository(#[from] project_repository::OpenError), - #[error(transparent)] - Other(#[from] anyhow::Error), -} + pub fn set_local_config(&self, id: ProjectId, key: &str, value: &str) -> Result<()> { + let project = self.projects_storage.get(id)?; -impl ErrorWithContext for AddError { - fn context(&self) -> Option { - Some(match self { - AddError::NotAGitRepository(_) => { - error::Context::new_static(Code::Projects, "Must be a git directory") - } - AddError::BareUnsupported => { - error::Context::new_static(Code::Projects, "Bare repositories are unsupported") - } - AddError::AlreadyExists => { - error::Context::new_static(Code::Projects, "Project already exists") - } - AddError::OpenProjectRepository(error) => return error.context(), - AddError::NotADirectory => error::Context::new(Code::Projects, "Not a directory"), - AddError::WorktreeNotSupported => { - error::Context::new(Code::Projects, "Can only work in main worktrees") - } - AddError::PathNotFound => error::Context::new(Code::Projects, "Path not found"), - AddError::SubmodulesNotSupported => error::Context::new_static( - Code::Projects, - "Repositories with git submodules are not supported", - ), - AddError::Other(error) => return error.custom_context_or_root_cause().into(), - }) + let repo = project_repository::Repository::open(&project)?; + Ok(repo.config().set_local(key, value)?) } } diff --git a/crates/gitbutler-core/src/projects/storage.rs b/crates/gitbutler-core/src/projects/storage.rs index a0e41cf8f3..36271a65f1 100644 --- a/crates/gitbutler-core/src/projects/storage.rs +++ b/crates/gitbutler-core/src/projects/storage.rs @@ -1,3 +1,4 @@ +use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; @@ -29,16 +30,6 @@ pub struct UpdateRequest { pub snapshot_lines_threshold: Option, } -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error(transparent)] - Storage(#[from] std::io::Error), - #[error(transparent)] - Json(#[from] serde_json::Error), - #[error("project not found")] - NotFound, -} - impl Storage { pub fn new(storage: storage::Storage) -> Self { Self { inner: storage } @@ -48,11 +39,11 @@ impl Storage { Self::new(storage::Storage::new(path)) } - pub fn list(&self) -> Result, Error> { + pub fn list(&self) -> Result> { match self.inner.read(PROJECTS_FILE)? { Some(projects) => { let all_projects: Vec = serde_json::from_str(&projects)?; - let mut all_projects: Vec = all_projects + let mut all_projects: Vec<_> = all_projects .into_iter() .map(|mut p| { // backwards compatibility for description field @@ -66,27 +57,28 @@ impl Storage { .collect(); all_projects.sort_by(|a, b| a.title.cmp(&b.title)); - Ok(all_projects) } None => Ok(vec![]), } } - pub fn get(&self, id: ProjectId) -> Result { + pub fn get(&self, id: ProjectId) -> Result { + self.try_get(id)? + .with_context(|| format!("project {id} not found")) + } + + pub fn try_get(&self, id: ProjectId) -> Result> { let projects = self.list()?; - match projects.into_iter().find(|p| p.id == id) { - Some(project) => Ok(project), - None => Err(Error::NotFound), - } + Ok(projects.into_iter().find(|p| p.id == id)) } - pub fn update(&self, update_request: &UpdateRequest) -> Result { + pub fn update(&self, update_request: &UpdateRequest) -> Result { let mut projects = self.list()?; let project = projects .iter_mut() .find(|p| p.id == update_request.id) - .ok_or(Error::NotFound)?; + .with_context(|| "project {id} not found for update")?; if let Some(title) = &update_request.title { project.title.clone_from(title); @@ -140,7 +132,7 @@ impl Storage { .clone()) } - pub fn purge(&self, id: ProjectId) -> Result<(), Error> { + pub fn purge(&self, id: ProjectId) -> Result<()> { let mut projects = self.list()?; if let Some(index) = projects.iter().position(|p| p.id == id) { projects.remove(index); @@ -150,7 +142,7 @@ impl Storage { Ok(()) } - pub fn add(&self, project: &project::Project) -> Result<(), Error> { + pub fn add(&self, project: &project::Project) -> Result<()> { let mut projects = self.list()?; projects.push(project.clone()); let projects = serde_json::to_string_pretty(&projects)?; diff --git a/crates/gitbutler-core/src/reader.rs b/crates/gitbutler-core/src/reader.rs index 80f72965ec..0ce1e71174 100644 --- a/crates/gitbutler-core/src/reader.rs +++ b/crates/gitbutler-core/src/reader.rs @@ -1,51 +1,7 @@ -use std::{ - fs, io, num, - path::{Path, PathBuf}, - str, - sync::Arc, -}; - -use anyhow::Result; -use serde::{ser::SerializeStruct, Serialize}; - -#[derive(Debug, Clone, thiserror::Error)] -pub enum Error { - #[error("file not found")] - NotFound, - #[error("io error: {0}")] - Io(Arc), - #[error(transparent)] - From(FromError), - #[error("failed to parse {}", path.display())] - ParseError { - path: PathBuf, - source: toml::de::Error, - }, -} +use std::{fs, io, path::Path, str}; -impl From for Error { - fn from(error: io::Error) -> Self { - Error::Io(Arc::new(error)) - } -} - -impl From for Error { - fn from(error: FromError) -> Self { - Error::From(error) - } -} - -#[derive(Debug, Clone, thiserror::Error)] -pub enum FromError { - #[error(transparent)] - ParseInt(#[from] num::ParseIntError), - #[error(transparent)] - ParseBool(#[from] str::ParseBoolError), - #[error("file is binary")] - Binary, - #[error("file too large")] - Large, -} +use anyhow::{bail, Result}; +use serde::{ser::SerializeStruct, Serialize}; #[derive(Debug, Clone, PartialEq)] pub enum Content { @@ -118,19 +74,19 @@ impl From<&[u8]> for Content { } impl TryFrom<&Content> for usize { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { match content { - Content::UTF8(text) => text.parse().map_err(FromError::ParseInt), - Content::Binary => Err(FromError::Binary), - Content::Large => Err(FromError::Large), + Content::UTF8(text) => text.parse().map_err(Into::into), + Content::Binary => bail!("file is binary"), + Content::Large => bail!("file too large"), } } } impl TryFrom for usize { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -138,19 +94,19 @@ impl TryFrom for usize { } impl TryFrom<&Content> for String { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { match content { Content::UTF8(text) => Ok(text.clone()), - Content::Binary => Err(FromError::Binary), - Content::Large => Err(FromError::Large), + Content::Binary => bail!("file is binary"), + Content::Large => bail!("file too large"), } } } impl TryFrom for String { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -158,7 +114,7 @@ impl TryFrom for String { } impl TryFrom for i64 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -166,16 +122,16 @@ impl TryFrom for i64 { } impl TryFrom<&Content> for i64 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { let text: String = content.try_into()?; - text.parse().map_err(FromError::ParseInt) + text.parse().map_err(Into::into) } } impl TryFrom for u64 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -183,16 +139,16 @@ impl TryFrom for u64 { } impl TryFrom<&Content> for u64 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { let text: String = content.try_into()?; - text.parse().map_err(FromError::ParseInt) + text.parse().map_err(Into::into) } } impl TryFrom for u128 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -200,16 +156,16 @@ impl TryFrom for u128 { } impl TryFrom<&Content> for u128 { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { let text: String = content.try_into()?; - text.parse().map_err(FromError::ParseInt) + text.parse().map_err(Into::into) } } impl TryFrom for bool { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: Content) -> Result { Self::try_from(&content) @@ -217,10 +173,10 @@ impl TryFrom for bool { } impl TryFrom<&Content> for bool { - type Error = FromError; + type Error = anyhow::Error; fn try_from(content: &Content) -> Result { let text: String = content.try_into()?; - text.parse().map_err(FromError::ParseBool) + text.parse().map_err(Into::into) } } diff --git a/crates/gitbutler-core/src/remotes/controller.rs b/crates/gitbutler-core/src/remotes/controller.rs index b0abaa79da..9f51df3ac3 100644 --- a/crates/gitbutler-core/src/remotes/controller.rs +++ b/crates/gitbutler-core/src/remotes/controller.rs @@ -1,5 +1,6 @@ +use anyhow::Result; + use crate::{ - error::Error, project_repository, projects::{self, ProjectId}, }; @@ -14,22 +15,17 @@ impl Controller { Self { projects } } - pub async fn remotes(&self, project_id: ProjectId) -> Result, Error> { + pub async fn remotes(&self, project_id: ProjectId) -> Result> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(project_repository.remotes()?) + project_repository.remotes() } - pub async fn add_remote( - &self, - project_id: ProjectId, - name: &str, - url: &str, - ) -> Result<(), Error> { + pub async fn add_remote(&self, project_id: ProjectId, name: &str, url: &str) -> Result<()> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(project_repository.add_remote(name, url)?) + project_repository.add_remote(name, url) } } diff --git a/crates/gitbutler-core/src/synchronize/mod.rs b/crates/gitbutler-core/src/synchronize/mod.rs index ba79e84caf..03105a2d7a 100644 --- a/crates/gitbutler-core/src/synchronize/mod.rs +++ b/crates/gitbutler-core/src/synchronize/mod.rs @@ -61,7 +61,7 @@ async fn push_target( project_id: Id, user: &users::User, batch_size: usize, -) -> Result<(), project_repository::RemoteError> { +) -> Result<()> { let ids = batch_rev_walk( &project_repository.git_repository, batch_size, @@ -147,7 +147,7 @@ fn push_all_refs( project_repository: &project_repository::Repository, user: &users::User, project_id: Id, -) -> Result<(), project_repository::RemoteError> { +) -> Result<()> { let gb_references = collect_refs(project_repository)?; let all_refs: Vec<_> = gb_references .iter() @@ -175,7 +175,7 @@ async fn update_project( projects: &projects::Controller, project_id: Id, id: Oid, -) -> Result<(), project_repository::RemoteError> { +) -> Result<()> { projects .update(&projects::UpdateRequest { id: project_id, diff --git a/crates/gitbutler-core/src/users/storage.rs b/crates/gitbutler-core/src/users/storage.rs index 8e6b51feb9..1e61bd5f2b 100644 --- a/crates/gitbutler-core/src/users/storage.rs +++ b/crates/gitbutler-core/src/users/storage.rs @@ -10,14 +10,6 @@ pub struct Storage { inner: storage::Storage, } -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error(transparent)] - Storage(#[from] std::io::Error), - #[error(transparent)] - Json(#[from] serde_json::Error), -} - impl Storage { pub fn new(storage: storage::Storage) -> Storage { Storage { inner: storage } @@ -27,21 +19,19 @@ impl Storage { Storage::new(storage::Storage::new(path)) } - pub fn get(&self) -> Result, Error> { + pub fn get(&self) -> Result> { match self.inner.read(USER_FILE)? { Some(data) => Ok(Some(serde_json::from_str(&data)?)), None => Ok(None), } } - pub fn set(&self, user: &user::User) -> Result<(), Error> { + pub fn set(&self, user: &user::User) -> Result<()> { let data = serde_json::to_string(user)?; - self.inner.write(USER_FILE, &data)?; - Ok(()) + Ok(self.inner.write(USER_FILE, &data)?) } - pub fn delete(&self) -> Result<(), Error> { - self.inner.delete(USER_FILE)?; - Ok(()) + pub fn delete(&self) -> Result<()> { + Ok(self.inner.delete(USER_FILE)?) } } diff --git a/crates/gitbutler-core/src/virtual_branches/base.rs b/crates/gitbutler-core/src/virtual_branches/base.rs index d8375012a5..a17fdcf217 100644 --- a/crates/gitbutler-core/src/virtual_branches/base.rs +++ b/crates/gitbutler-core/src/virtual_branches/base.rs @@ -1,16 +1,17 @@ use std::{path::Path, time}; -use anyhow::{Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use git2::Index; use serde::Serialize; use super::{ - branch, errors, + branch, integration::{ get_workspace_head, update_gitbutler_integration, GITBUTLER_INTEGRATION_REFERENCE, }, target, BranchId, RemoteCommit, VirtualBranchHunk, VirtualBranchesHandle, }; +use crate::virtual_branches::errors::Marker; use crate::{ git::{self, diff}, project_repository::{self, LogUntil}, @@ -46,7 +47,7 @@ pub fn get_base_branch_data( fn go_back_to_integration( project_repository: &project_repository::Repository, default_target: &target::Target, -) -> Result { +) -> Result { let statuses = project_repository .git_repository .statuses(Some( @@ -56,7 +57,7 @@ fn go_back_to_integration( )) .context("failed to get status")?; if !statuses.is_empty() { - return Err(errors::SetBaseBranchError::DirtyWorkingDirectory); + return Err(anyhow!("current HEAD is dirty")).context(Marker::ProjectConflict); } let vb_state = project_repository.project().virtual_branches(); @@ -117,7 +118,7 @@ fn go_back_to_integration( pub fn set_base_branch( project_repository: &project_repository::Repository, target_branch_ref: &git::RemoteRefname, -) -> Result { +) -> Result { let repo = &project_repository.git_repository; // if target exists, and it is the same as the requested branch, we should go back @@ -129,12 +130,10 @@ pub fn set_base_branch( // lookup a branch by name let target_branch = match repo.find_branch(&target_branch_ref.clone().into()) { - Ok(branch) => Ok(branch), - Err(git::Error::NotFound(_)) => Err(errors::SetBaseBranchError::BranchNotFound( - target_branch_ref.clone(), - )), - Err(error) => Err(errors::SetBaseBranchError::Other(error.into())), - }?; + Ok(branch) => branch, + Err(git::Error::NotFound(_)) => bail!("remote branch '{}' not found", target_branch_ref), + Err(err) => return Err(err.into()), + }; let remote = repo .find_remote(target_branch_ref.remote()) @@ -276,24 +275,21 @@ pub fn set_base_branch( pub fn set_target_push_remote( project_repository: &project_repository::Repository, push_remote_name: &str, -) -> Result<(), errors::SetBaseBranchError> { - let repo = &project_repository.git_repository; - - let remote = repo +) -> Result<()> { + let remote = project_repository + .git_repository .find_remote(push_remote_name) .context(format!("failed to find remote {}", push_remote_name))?; // if target exists, and it is the same as the requested branch, we should go back let mut target = default_target(&project_repository.project().gb_dir())?; - - target.push_remote_name = Some( - remote - .name() - .context("failed to get remote name")? - .to_string(), - ); + target.push_remote_name = remote + .name() + .context("failed to get remote name")? + .to_string() + .into(); let vb_state = project_repository.project().virtual_branches(); - vb_state.set_default_target(target.clone())?; + vb_state.set_default_target(target)?; Ok(()) } @@ -337,13 +333,7 @@ pub fn update_base_branch( project_repository: &project_repository::Repository, user: Option<&users::User>, ) -> anyhow::Result> { - if project_repository.is_resolving() { - anyhow::bail!(errors::UpdateBaseBranchError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } + project_repository.assure_resolved()?; // look up the target and see if there is a new oid let target = default_target(&project_repository.project().gb_dir())?; diff --git a/crates/gitbutler-core/src/virtual_branches/controller.rs b/crates/gitbutler-core/src/virtual_branches/controller.rs index fcf5bd78f1..e26acbe3df 100644 --- a/crates/gitbutler-core/src/virtual_branches/controller.rs +++ b/crates/gitbutler-core/src/virtual_branches/controller.rs @@ -1,7 +1,5 @@ -use crate::{ - error::Error, - ops::entry::{OperationKind, SnapshotDetails}, -}; +use crate::ops::entry::{OperationKind, SnapshotDetails}; +use anyhow::Result; use std::{collections::HashMap, path::Path, sync::Arc}; use anyhow::Context; @@ -9,8 +7,7 @@ use tokio::{sync::Semaphore, task::JoinHandle}; use super::{ branch::{BranchId, BranchOwnershipClaims}, - errors, target, target_to_base_branch, BaseBranch, Branch, RemoteBranchFile, - VirtualBranchesHandle, + target, target_to_base_branch, BaseBranch, Branch, RemoteBranchFile, VirtualBranchesHandle, }; use crate::{ git, project_repository, @@ -58,7 +55,7 @@ impl Controller { message: &str, ownership: Option<&BranchOwnershipClaims>, run_hooks: bool, - ) -> Result { + ) -> Result { self.inner(project_id) .await .create_commit(project_id, branch_id, message, ownership, run_hooks) @@ -69,7 +66,7 @@ impl Controller { &self, project_id: ProjectId, branch_name: &git::RemoteRefname, - ) -> Result { + ) -> Result { self.inner(project_id) .await .can_apply_remote_branch(project_id, branch_name) @@ -79,7 +76,7 @@ impl Controller { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result { + ) -> Result { self.inner(project_id) .await .can_apply_virtual_branch(project_id, branch_id) @@ -88,7 +85,7 @@ impl Controller { pub async fn list_virtual_branches( &self, project_id: ProjectId, - ) -> Result<(Vec, Vec), Error> { + ) -> Result<(Vec, Vec)> { self.inner(project_id) .await .list_virtual_branches(project_id) @@ -99,7 +96,7 @@ impl Controller { &self, project_id: ProjectId, create: &super::branch::BranchCreateRequest, - ) -> Result { + ) -> Result { self.inner(project_id) .await .create_virtual_branch(project_id, create) @@ -110,14 +107,14 @@ impl Controller { &self, project_id: ProjectId, branch: &git::Refname, - ) -> Result { + ) -> Result { self.inner(project_id) .await .create_virtual_branch_from_branch(project_id, branch) .await } - pub async fn get_base_branch_data(&self, project_id: ProjectId) -> Result { + pub async fn get_base_branch_data(&self, project_id: ProjectId) -> Result { self.inner(project_id) .await .get_base_branch_data(project_id) @@ -127,7 +124,7 @@ impl Controller { &self, project_id: ProjectId, commit_oid: git::Oid, - ) -> Result, Error> { + ) -> Result> { self.inner(project_id) .await .list_remote_commit_files(project_id, commit_oid) @@ -137,7 +134,7 @@ impl Controller { &self, project_id: ProjectId, target_branch: &git::RemoteRefname, - ) -> Result { + ) -> Result { self.inner(project_id) .await .set_base_branch(project_id, target_branch) @@ -147,7 +144,7 @@ impl Controller { &self, project_id: ProjectId, push_remote: &str, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .set_target_push_remote(project_id, push_remote) @@ -157,14 +154,14 @@ impl Controller { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .integrate_upstream_commits(project_id, branch_id) .await } - pub async fn update_base_branch(&self, project_id: ProjectId) -> Result, Error> { + pub async fn update_base_branch(&self, project_id: ProjectId) -> Result> { self.inner(project_id) .await .update_base_branch(project_id) @@ -175,7 +172,7 @@ impl Controller { &self, project_id: ProjectId, branch_update: super::branch::BranchUpdateRequest, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .update_virtual_branch(project_id, branch_update) @@ -185,7 +182,7 @@ impl Controller { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .delete_virtual_branch(project_id, branch_id) @@ -196,7 +193,7 @@ impl Controller { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .apply_virtual_branch(project_id, branch_id) @@ -207,18 +204,14 @@ impl Controller { &self, project_id: ProjectId, ownership: &BranchOwnershipClaims, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .unapply_ownership(project_id, ownership) .await } - pub async fn reset_files( - &self, - project_id: ProjectId, - files: &Vec, - ) -> Result<(), Error> { + pub async fn reset_files(&self, project_id: ProjectId, files: &Vec) -> Result<()> { self.inner(project_id) .await .reset_files(project_id, files) @@ -231,7 +224,7 @@ impl Controller { branch_id: BranchId, commit_oid: git::Oid, ownership: &BranchOwnershipClaims, - ) -> Result { + ) -> Result { self.inner(project_id) .await .amend(project_id, branch_id, commit_oid, ownership) @@ -245,7 +238,7 @@ impl Controller { from_commit_oid: git::Oid, to_commit_oid: git::Oid, ownership: &BranchOwnershipClaims, - ) -> Result { + ) -> Result { self.inner(project_id) .await .move_commit_file( @@ -263,7 +256,7 @@ impl Controller { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .undo_commit(project_id, branch_id, commit_oid) @@ -276,7 +269,7 @@ impl Controller { branch_id: BranchId, commit_oid: git::Oid, offset: i32, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .insert_blank_commit(project_id, branch_id, commit_oid, offset) @@ -289,7 +282,7 @@ impl Controller { branch_id: BranchId, commit_oid: git::Oid, offset: i32, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .reorder_commit(project_id, branch_id, commit_oid, offset) @@ -301,7 +294,7 @@ impl Controller { project_id: ProjectId, branch_id: BranchId, target_commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .reset_virtual_branch(project_id, branch_id, target_commit_oid) @@ -312,7 +305,7 @@ impl Controller { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .unapply_virtual_branch(project_id, branch_id) @@ -325,7 +318,7 @@ impl Controller { branch_id: BranchId, with_force: bool, askpass: Option>, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .push_virtual_branch(project_id, branch_id, with_force, askpass) @@ -337,7 +330,7 @@ impl Controller { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result, Error> { + ) -> Result> { self.inner(project_id) .await .cherry_pick(project_id, branch_id, commit_oid) @@ -347,7 +340,7 @@ impl Controller { pub async fn list_remote_branches( &self, project_id: ProjectId, - ) -> Result, Error> { + ) -> Result> { self.inner(project_id) .await .list_remote_branches(project_id) @@ -357,7 +350,7 @@ impl Controller { &self, project_id: ProjectId, refname: &git::Refname, - ) -> Result { + ) -> Result { self.inner(project_id) .await .get_remote_branch_data(project_id, refname) @@ -368,7 +361,7 @@ impl Controller { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .squash(project_id, branch_id, commit_oid) @@ -381,7 +374,7 @@ impl Controller { branch_id: BranchId, commit_oid: git::Oid, message: &str, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .update_commit_message(project_id, branch_id, commit_oid, message) @@ -392,7 +385,7 @@ impl Controller { &self, project_id: ProjectId, askpass: Option, - ) -> Result { + ) -> Result { self.inner(project_id) .await .fetch_from_remotes(project_id, askpass) @@ -404,7 +397,7 @@ impl Controller { project_id: ProjectId, target_branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { self.inner(project_id) .await .move_commit(project_id, target_branch_id, commit_oid) @@ -442,7 +435,7 @@ impl ControllerInner { message: &str, ownership: Option<&BranchOwnershipClaims>, run_hooks: bool, - ) -> Result { + ) -> Result { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -472,20 +465,17 @@ impl ControllerInner { &self, project_id: ProjectId, branch_name: &git::RemoteRefname, - ) -> Result { + ) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(super::is_remote_branch_mergeable( - &project_repository, - branch_name, - )?) + super::is_remote_branch_mergeable(&project_repository, branch_name).map_err(Into::into) } pub fn can_apply_virtual_branch( &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result { + ) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; super::is_virtual_branch_mergeable(&project_repository, branch_id).map_err(Into::into) @@ -494,7 +484,7 @@ impl ControllerInner { pub async fn list_virtual_branches( &self, project_id: ProjectId, - ) -> Result<(Vec, Vec), Error> { + ) -> Result<(Vec, Vec)> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -506,7 +496,7 @@ impl ControllerInner { &self, project_id: ProjectId, create: &super::branch::BranchCreateRequest, - ) -> Result { + ) -> Result { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -519,27 +509,26 @@ impl ControllerInner { &self, project_id: ProjectId, branch: &git::Refname, - ) -> Result { + ) -> Result { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { - let result = - super::create_virtual_branch_from_branch(project_repository, branch, user)?; - Ok(result) + super::create_virtual_branch_from_branch(project_repository, branch, user) + .map_err(Into::into) }) } - pub fn get_base_branch_data(&self, project_id: ProjectId) -> Result { + pub fn get_base_branch_data(&self, project_id: ProjectId) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(super::get_base_branch_data(&project_repository)?) + super::get_base_branch_data(&project_repository) } pub fn list_remote_commit_files( &self, project_id: ProjectId, commit_oid: git::Oid, - ) -> Result, Error> { + ) -> Result> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; super::list_remote_commit_files(&project_repository.git_repository, commit_oid) @@ -550,32 +539,26 @@ impl ControllerInner { &self, project_id: ProjectId, target_branch: &git::RemoteRefname, - ) -> Result { + ) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; let _ = project_repository .project() .create_snapshot(SnapshotDetails::new(OperationKind::SetBaseBranch)); - let result = super::set_base_branch(&project_repository, target_branch)?; - Ok(result) + super::set_base_branch(&project_repository, target_branch) } - pub fn set_target_push_remote( - &self, - project_id: ProjectId, - push_remote: &str, - ) -> Result<(), Error> { + pub fn set_target_push_remote(&self, project_id: ProjectId, push_remote: &str) -> Result<()> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - super::set_target_push_remote(&project_repository, push_remote)?; - Ok(()) + super::set_target_push_remote(&project_repository, push_remote) } pub async fn integrate_upstream_commits( &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -587,7 +570,7 @@ impl ControllerInner { }) } - pub async fn update_base_branch(&self, project_id: ProjectId) -> Result, Error> { + pub async fn update_base_branch(&self, project_id: ProjectId) -> Result> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -602,7 +585,7 @@ impl ControllerInner { &self, project_id: ProjectId, branch_update: super::branch::BranchUpdateRequest, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -615,7 +598,7 @@ impl ControllerInner { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -627,7 +610,7 @@ impl ControllerInner { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -648,7 +631,7 @@ impl ControllerInner { &self, project_id: ProjectId, ownership: &BranchOwnershipClaims, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -659,11 +642,7 @@ impl ControllerInner { }) } - pub async fn reset_files( - &self, - project_id: ProjectId, - ownership: &Vec, - ) -> Result<(), Error> { + pub async fn reset_files(&self, project_id: ProjectId, ownership: &Vec) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -680,14 +659,14 @@ impl ControllerInner { branch_id: BranchId, commit_oid: git::Oid, ownership: &BranchOwnershipClaims, - ) -> Result { + ) -> Result { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { let _ = project_repository .project() .create_snapshot(SnapshotDetails::new(OperationKind::AmendCommit)); - super::amend(project_repository, branch_id, commit_oid, ownership).map_err(Into::into) + super::amend(project_repository, branch_id, commit_oid, ownership) }) } @@ -698,7 +677,7 @@ impl ControllerInner { from_commit_oid: git::Oid, to_commit_oid: git::Oid, ownership: &BranchOwnershipClaims, - ) -> Result { + ) -> Result { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -721,12 +700,12 @@ impl ControllerInner { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { let snapshot_tree = project_repository.project().prepare_snapshot(); - let result: Result<(), Error> = + let result: Result<()> = super::undo_commit(project_repository, branch_id, commit_oid).map_err(Into::into); let _ = snapshot_tree.and_then(|snapshot_tree| { project_repository.project().snapshot_commit_undo( @@ -745,7 +724,7 @@ impl ControllerInner { branch_id: BranchId, commit_oid: git::Oid, offset: i32, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -763,7 +742,7 @@ impl ControllerInner { branch_id: BranchId, commit_oid: git::Oid, offset: i32, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -780,7 +759,7 @@ impl ControllerInner { project_id: ProjectId, branch_id: BranchId, target_commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -796,7 +775,7 @@ impl ControllerInner { &self, project_id: ProjectId, branch_id: BranchId, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -817,20 +796,13 @@ impl ControllerInner { branch_id: BranchId, with_force: bool, askpass: Option>, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; let helper = self.helper.clone(); self.with_verify_branch_async(project_id, move |project_repository, _| { - Ok(super::push( - project_repository, - branch_id, - with_force, - &helper, - askpass, - )?) + super::push(project_repository, branch_id, with_force, &helper, askpass) })? - .await - .map_err(Error::from_err)? + .await? } pub async fn cherry_pick( @@ -838,7 +810,7 @@ impl ControllerInner { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result, Error> { + ) -> Result> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -849,23 +821,20 @@ impl ControllerInner { }) } - pub fn list_remote_branches( - &self, - project_id: ProjectId, - ) -> Result, Error> { + pub fn list_remote_branches(&self, project_id: ProjectId) -> Result> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(super::list_remote_branches(&project_repository)?) + super::list_remote_branches(&project_repository) } pub fn get_remote_branch_data( &self, project_id: ProjectId, refname: &git::Refname, - ) -> Result { + ) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(super::get_branch_data(&project_repository, refname)?) + super::get_branch_data(&project_repository, refname) } pub async fn squash( @@ -873,7 +842,7 @@ impl ControllerInner { project_id: ProjectId, branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { @@ -890,7 +859,7 @@ impl ControllerInner { branch_id: BranchId, commit_oid: git::Oid, message: &str, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, _| { let _ = project_repository @@ -905,18 +874,14 @@ impl ControllerInner { &self, project_id: ProjectId, askpass: Option, - ) -> Result { + ) -> Result { let project = self.projects.get(project_id)?; let mut project_repository = project_repository::Repository::open(&project)?; let remotes = project_repository.remotes()?; - let fetch_results: Vec> = remotes + let fetch_results: Vec> = remotes .iter() - .map(|remote| { - project_repository - .fetch(remote, &self.helper, askpass.clone()) - .map_err(errors::FetchFromTargetError::Remote) - }) + .map(|remote| project_repository.fetch(remote, &self.helper, askpass.clone())) .collect(); let project_data_last_fetched = if fetch_results.iter().any(Result::is_err) { @@ -941,9 +906,9 @@ impl ControllerInner { // if we have a push remote, let's fetch from this too if let Some(push_remote) = &default_target.push_remote_name { - let _ = project_repository - .fetch(push_remote, &self.helper, askpass.clone()) - .map_err(errors::FetchFromTargetError::Remote); + if let Err(err) = project_repository.fetch(push_remote, &self.helper, askpass.clone()) { + tracing::warn!(?err, "fetch from push-remote failed"); + } } let updated_project = self @@ -969,7 +934,7 @@ impl ControllerInner { project_id: ProjectId, target_branch_id: BranchId, commit_oid: git::Oid, - ) -> Result<(), Error> { + ) -> Result<()> { let _permit = self.semaphore.acquire().await; self.with_verify_branch(project_id, |project_repository, user| { @@ -986,8 +951,8 @@ impl ControllerInner { fn with_verify_branch( &self, project_id: ProjectId, - action: impl FnOnce(&project_repository::Repository, Option<&users::User>) -> Result, - ) -> Result { + action: impl FnOnce(&project_repository::Repository, Option<&users::User>) -> Result, + ) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; let user = self.users.get_user()?; @@ -998,10 +963,10 @@ impl ControllerInner { fn with_verify_branch_async( &self, project_id: ProjectId, - action: impl FnOnce(&project_repository::Repository, Option<&users::User>) -> Result + action: impl FnOnce(&project_repository::Repository, Option<&users::User>) -> Result + Send + 'static, - ) -> Result>, Error> { + ) -> Result>> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; let user = self.users.get_user()?; diff --git a/crates/gitbutler-core/src/virtual_branches/errors.rs b/crates/gitbutler-core/src/virtual_branches/errors.rs index 9de74a6193..0b467f1f53 100644 --- a/crates/gitbutler-core/src/virtual_branches/errors.rs +++ b/crates/gitbutler-core/src/virtual_branches/errors.rs @@ -1,718 +1,23 @@ -use super::{branch::BranchOwnershipClaims, BranchId, GITBUTLER_INTEGRATION_REFERENCE}; -use crate::error::{AnyhowContextExt, Code, Context, ErrorWithContext}; -use crate::{ - error, git, - project_repository::{self, RemoteError}, - projects::ProjectId, -}; - -// Generic error enum for use in the virtual branches module. -#[derive(Debug, thiserror::Error)] -pub enum VirtualBranchError { - #[error("project")] - Conflict(ProjectConflict), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("target ownership not found")] - TargetOwnerhshipNotFound(BranchOwnershipClaims), - #[error("git object {0} not found")] - GitObjectNotFound(git::Oid), - #[error("commit failed")] - CommitFailed, - #[error("rebase failed")] - RebaseFailed, - #[error("force push not allowed")] - ForcePushNotAllowed(ForcePushNotAllowed), - #[error("branch has no commits")] - BranchHasNoCommits, - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for VirtualBranchError { - fn context(&self) -> Option { - Some(match self { - VirtualBranchError::Conflict(ctx) => ctx.to_context(), - VirtualBranchError::BranchNotFound(ctx) => ctx.to_context(), - VirtualBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - VirtualBranchError::TargetOwnerhshipNotFound(_) => { - error::Context::new_static(Code::Branches, "target ownership not found") - } - VirtualBranchError::GitObjectNotFound(oid) => { - error::Context::new(Code::Branches, format!("git object {oid} not found")) - } - VirtualBranchError::CommitFailed => { - error::Context::new_static(Code::Branches, "commit failed") - } - VirtualBranchError::RebaseFailed => { - error::Context::new_static(Code::Branches, "rebase failed") - } - VirtualBranchError::BranchHasNoCommits => error::Context::new_static( - Code::Branches, - "Branch has no commits - there is nothing to amend to", - ), - VirtualBranchError::ForcePushNotAllowed(ctx) => ctx.to_context(), - VirtualBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum VerifyError { - #[error("head is detached")] - DetachedHead, - #[error("head is {0}")] - InvalidHead(String), - #[error("head not found")] - HeadNotFound, - #[error("integration commit not found")] - NoIntegrationCommit, - #[error(transparent)] - GitError(#[from] git::Error), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for VerifyError { - fn context(&self) -> Option { - Some(match self { - VerifyError::DetachedHead => error::Context::new( - Code::ProjectHead, - format!( - "Project in detached head state. Please checkout {0} to continue.", - GITBUTLER_INTEGRATION_REFERENCE.branch() - ), - ), - VerifyError::InvalidHead(head) => error::Context::new( - Code::ProjectHead, - format!( - "Project is on {}. Please checkout {} to continue.", - head, - GITBUTLER_INTEGRATION_REFERENCE.branch() - ), - ), - VerifyError::NoIntegrationCommit => error::Context::new_static( - Code::ProjectHead, - "GibButler's integration commit not found on head.", - ), - VerifyError::HeadNotFound => { - error::Context::new_static(Code::Validation, "Repo HEAD is unavailable") - } - VerifyError::GitError(error) => { - error::Context::new(Code::Validation, error.to_string()) - } - VerifyError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ResetBranchError { - #[error("commit {0} not in the branch")] - CommitNotFoundInBranch(git::Oid), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), - #[error(transparent)] - Git(#[from] git::Error), -} - -impl ErrorWithContext for ResetBranchError { - fn context(&self) -> Option { - Some(match self { - ResetBranchError::BranchNotFound(ctx) => ctx.to_context(), - ResetBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - ResetBranchError::CommitNotFoundInBranch(oid) => { - error::Context::new(Code::Branches, format!("commit {} not found", oid)) - } - ResetBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - ResetBranchError::Git(_err) => return None, - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ApplyBranchError { - #[error("project")] - Conflict(ProjectConflict), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("branch being applied conflicts with other branch: {0}")] - BranchConflicts(BranchId), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - GitError(#[from] git::Error), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for ApplyBranchError { - fn context(&self) -> Option { - Some(match self { - ApplyBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - ApplyBranchError::Conflict(ctx) => ctx.to_context(), - ApplyBranchError::BranchNotFound(ctx) => ctx.to_context(), - ApplyBranchError::BranchConflicts(id) => error::Context::new( - Code::Branches, - format!("Branch {} is in a conflicting state", id), - ), - ApplyBranchError::GitError(_) => return None, - ApplyBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UnapplyOwnershipError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("project is in conflict state")] - Conflict(ProjectConflict), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for UnapplyOwnershipError { - fn context(&self) -> Option { - Some(match self { - UnapplyOwnershipError::DefaultTargetNotSet(error) => error.to_context(), - UnapplyOwnershipError::Conflict(error) => error.to_context(), - UnapplyOwnershipError::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UnapplyBranchError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for UnapplyBranchError { - fn context(&self) -> Option { - Some(match self { - UnapplyBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - UnapplyBranchError::BranchNotFound(ctx) => ctx.to_context(), - UnapplyBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ListVirtualBranchesError { - #[error("project")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for ListVirtualBranchesError { - fn context(&self) -> Option { - match self { - ListVirtualBranchesError::DefaultTargetNotSet(ctx) => ctx.to_context().into(), - ListVirtualBranchesError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum CreateVirtualBranchError { - #[error("project")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for CreateVirtualBranchError { - fn context(&self) -> Option { - match self { - CreateVirtualBranchError::DefaultTargetNotSet(ctx) => ctx.to_context().into(), - CreateVirtualBranchError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum CommitError { - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("will not commit conflicted files")] - Conflicted(ProjectConflict), - #[error("commit hook rejected")] - CommitHookRejected(String), - #[error("commit msg hook rejected")] - CommitMsgHookRejected(String), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for CommitError { - fn context(&self) -> Option { - Some(match self { - CommitError::BranchNotFound(ctx) => ctx.to_context(), - CommitError::DefaultTargetNotSet(ctx) => ctx.to_context(), - CommitError::Conflicted(ctx) => ctx.to_context(), - CommitError::CommitHookRejected(error) => { - error::Context::new(Code::PreCommitHook, error) - } - CommitError::CommitMsgHookRejected(error) => { - error::Context::new(Code::CommitMsgHook, error) - } - CommitError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum PushError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error(transparent)] - Remote(#[from] project_repository::RemoteError), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for PushError { - fn context(&self) -> Option { - Some(match self { - PushError::DefaultTargetNotSet(ctx) => ctx.to_context(), - PushError::BranchNotFound(ctx) => ctx.to_context(), - PushError::Remote(error) => return error.context(), - PushError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum IsRemoteBranchMergableError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(git::RemoteRefname), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for IsRemoteBranchMergableError { - fn context(&self) -> Option { - Some(match self { - IsRemoteBranchMergableError::BranchNotFound(name) => { - error::Context::new(Code::Branches, format!("Remote branch {} not found", name)) - } - IsRemoteBranchMergableError::DefaultTargetNotSet(ctx) => ctx.to_context(), - IsRemoteBranchMergableError::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum IsVirtualBranchMergeable { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for IsVirtualBranchMergeable { - fn context(&self) -> Option { - Some(match self { - IsVirtualBranchMergeable::BranchNotFound(ctx) => ctx.to_context(), - IsVirtualBranchMergeable::DefaultTargetNotSet(ctx) => ctx.to_context(), - IsVirtualBranchMergeable::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug)] -pub struct ForcePushNotAllowed { - pub project_id: ProjectId, -} - -impl ForcePushNotAllowed { - fn to_context(&self) -> error::Context { - error::Context::new_static( - Code::Branches, - "Action will lead to force pushing, which is not allowed for this", - ) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum CherryPickError { - #[error("target commit {0} not found ")] - CommitNotFound(git::Oid), - #[error("can not cherry pick not applied branch")] - NotApplied, - #[error("project is in conflict state")] - Conflict(ProjectConflict), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for CherryPickError { - fn context(&self) -> Option { - Some(match self { - CherryPickError::NotApplied => { - error::Context::new_static(Code::Branches, "can not cherry pick non applied branch") - } - CherryPickError::Conflict(ctx) => ctx.to_context(), - CherryPickError::CommitNotFound(oid) => { - error::Context::new(Code::Branches, format!("commit {oid} not found")) - } - CherryPickError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum SquashError { - #[error("force push not allowed")] - ForcePushNotAllowed(ForcePushNotAllowed), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("commit {0} not in the branch")] - CommitNotFound(git::Oid), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("project is in conflict state")] - Conflict(ProjectConflict), - #[error("can not squash root commit")] - CantSquashRootCommit, - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for SquashError { - fn context(&self) -> Option { - Some(match self { - SquashError::ForcePushNotAllowed(ctx) => ctx.to_context(), - SquashError::DefaultTargetNotSet(ctx) => ctx.to_context(), - SquashError::BranchNotFound(ctx) => ctx.to_context(), - SquashError::Conflict(ctx) => ctx.to_context(), - SquashError::CantSquashRootCommit => { - error::Context::new_static(Code::Branches, "can not squash root branch commit") - } - SquashError::CommitNotFound(oid) => error::Context::new( - crate::error::Code::Branches, - format!("commit {oid} not found"), - ), - SquashError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum FetchFromTargetError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("failed to fetch")] - Remote(RemoteError), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for FetchFromTargetError { - fn context(&self) -> Option { - match self { - FetchFromTargetError::DefaultTargetNotSet(ctx) => ctx.to_context().into(), - FetchFromTargetError::Remote(error) => error.context(), - FetchFromTargetError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UpdateCommitMessageError { - #[error("force push not allowed")] - ForcePushNotAllowed(ForcePushNotAllowed), - #[error("empty message")] - EmptyMessage, - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("commit {0} not in the branch")] - CommitNotFound(git::Oid), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("project is in conflict state")] - Conflict(ProjectConflict), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for UpdateCommitMessageError { - fn context(&self) -> Option { - Some(match self { - UpdateCommitMessageError::ForcePushNotAllowed(ctx) => ctx.to_context(), - UpdateCommitMessageError::EmptyMessage => { - error::Context::new_static(Code::Branches, "Commit message can not be empty") - } - UpdateCommitMessageError::DefaultTargetNotSet(ctx) => ctx.to_context(), - UpdateCommitMessageError::CommitNotFound(oid) => { - error::Context::new(Code::Branches, format!("Commit {} not found", oid)) - } - UpdateCommitMessageError::BranchNotFound(ctx) => ctx.to_context(), - UpdateCommitMessageError::Conflict(ctx) => ctx.to_context(), - UpdateCommitMessageError::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum SetBaseBranchError { - #[error("wd is dirty")] - DirtyWorkingDirectory, - #[error("branch {0} not found")] - BranchNotFound(git::RemoteRefname), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for SetBaseBranchError { - fn context(&self) -> Option { - Some(match self { - SetBaseBranchError::DirtyWorkingDirectory => { - error::Context::new(Code::ProjectConflict, "Current HEAD is dirty.") - } - SetBaseBranchError::BranchNotFound(name) => error::Context::new( - Code::Branches, - format!("remote branch '{}' not found", name), - ), - SetBaseBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UpdateBaseBranchError { - #[error("project is in conflicting state")] - Conflict(ProjectConflict), - #[error("no default target set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for UpdateBaseBranchError { - fn context(&self) -> Option { - Some(match self { - UpdateBaseBranchError::Conflict(ctx) => ctx.to_context(), - UpdateBaseBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - UpdateBaseBranchError::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum MoveCommitError { - #[error("source branch contains hunks locked to the target commit")] - SourceLocked, - #[error("project is in conflicted state")] - Conflicted(ProjectConflict), - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error("commit not found")] - CommitNotFound(git::Oid), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for MoveCommitError { - fn context(&self) -> Option { - Some(match self { - MoveCommitError::SourceLocked => error::Context::new_static( - Code::Branches, - "Source branch contains hunks locked to the target commit", - ), - MoveCommitError::Conflicted(ctx) => ctx.to_context(), - MoveCommitError::DefaultTargetNotSet(ctx) => ctx.to_context(), - MoveCommitError::BranchNotFound(ctx) => ctx.to_context(), - MoveCommitError::CommitNotFound(oid) => { - error::Context::new(Code::Branches, format!("Commit {} not found", oid)) - } - MoveCommitError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum CreateVirtualBranchFromBranchError { - #[error("failed to apply")] - ApplyBranch(ApplyBranchError), - #[error("can't make branch from default target")] - CantMakeBranchFromDefaultTarget, - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("{0} not found")] - BranchNotFound(git::Refname), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for CreateVirtualBranchFromBranchError { - fn context(&self) -> Option { - Some(match self { - CreateVirtualBranchFromBranchError::ApplyBranch(err) => return err.context(), - CreateVirtualBranchFromBranchError::CantMakeBranchFromDefaultTarget => { - error::Context::new_static( - Code::Branches, - "Can not create a branch from default target", - ) - } - CreateVirtualBranchFromBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - CreateVirtualBranchFromBranchError::BranchNotFound(name) => { - error::Context::new(Code::Branches, format!("Branch {} not found", name)) - } - CreateVirtualBranchFromBranchError::Other(error) => { - return error.custom_context_or_root_cause().into() - } - }) - } -} - -#[derive(Debug)] -pub struct ProjectConflict { - pub project_id: ProjectId, -} - -impl ProjectConflict { - fn to_context(&self) -> error::Context { - error::Context::new( - Code::ProjectConflict, - format!("project {} is in a conflicted state", self.project_id), - ) - } -} - -#[derive(Debug)] -pub struct DefaultTargetNotSet { - pub project_id: ProjectId, -} - -impl DefaultTargetNotSet { - fn to_context(&self) -> error::Context { - error::Context::new( - Code::ProjectConflict, - format!( - "project {} does not have a default target set", - self.project_id - ), - ) - } -} - -#[derive(Debug)] -pub struct BranchNotFound { - pub project_id: ProjectId, - pub branch_id: BranchId, -} - -impl BranchNotFound { - fn to_context(&self) -> error::Context { - error::Context::new( - Code::Branches, - format!("branch {} not found", self.branch_id), - ) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum UpdateBranchError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error("branch not found")] - BranchNotFound(BranchNotFound), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for UpdateBranchError { - fn context(&self) -> Option { - Some(match self { - UpdateBranchError::DefaultTargetNotSet(ctx) => ctx.to_context(), - UpdateBranchError::BranchNotFound(ctx) => ctx.to_context(), - UpdateBranchError::Other(error) => return error.custom_context_or_root_cause().into(), - }) - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ListRemoteCommitFilesError { - #[error("failed to find commit {0}")] - CommitNotFound(git::Oid), - #[error("failed to find commit")] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for ListRemoteCommitFilesError { - fn context(&self) -> Option { - match self { - ListRemoteCommitFilesError::CommitNotFound(oid) => { - error::Context::new(Code::Branches, format!("Commit {} not found", oid)).into() - } - ListRemoteCommitFilesError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum ListRemoteBranchesError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for ListRemoteBranchesError { - fn context(&self) -> Option { - match self { - ListRemoteBranchesError::DefaultTargetNotSet(ctx) => ctx.to_context().into(), - ListRemoteBranchesError::Other(error) => error.custom_context_or_root_cause().into(), - } - } -} - -#[derive(Debug, thiserror::Error)] -pub enum GetRemoteBranchDataError { - #[error("default target not set")] - DefaultTargetNotSet(DefaultTargetNotSet), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl ErrorWithContext for GetRemoteBranchDataError { - fn context(&self) -> Option { +/// A way to mark errors using `[anyhow::Context::context]` for later retrieval, e.g. to know +/// that a certain even happened. +/// +/// Note that the display implementation is visible to users in logs, so it's a bit 'special' +/// to signify its marker status. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Marker { + /// Invalid state was detected, making the repository invalid for operation. + VerificationFailure, + /// An indicator for a conflict in the project. + /// + /// See usages for details on what these conflicts can be. + ProjectConflict, +} + +impl std::fmt::Display for Marker { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - GetRemoteBranchDataError::DefaultTargetNotSet(ctx) => ctx.to_context().into(), - GetRemoteBranchDataError::Other(error) => error.custom_context_or_root_cause().into(), + Marker::VerificationFailure => f.write_str(""), + Marker::ProjectConflict => f.write_str(""), } } } diff --git a/crates/gitbutler-core/src/virtual_branches/files.rs b/crates/gitbutler-core/src/virtual_branches/files.rs index 4149aa190e..c89128d9aa 100644 --- a/crates/gitbutler-core/src/virtual_branches/files.rs +++ b/crates/gitbutler-core/src/virtual_branches/files.rs @@ -1,9 +1,8 @@ use std::path; -use anyhow::{Context, Result}; +use anyhow::{anyhow, Context, Result}; use serde::Serialize; -use super::errors; use crate::git::{self, diff}; #[derive(Debug, PartialEq, Clone, Serialize)] @@ -16,15 +15,12 @@ pub struct RemoteBranchFile { pub fn list_remote_commit_files( repository: &git::Repository, - commit_oid: git::Oid, -) -> Result, errors::ListRemoteCommitFilesError> { - let commit = match repository.find_commit(commit_oid) { - Ok(commit) => Ok(commit), - Err(git::Error::NotFound(_)) => Err(errors::ListRemoteCommitFilesError::CommitNotFound( - commit_oid, - )), - Err(error) => Err(errors::ListRemoteCommitFilesError::Other(error.into())), - }?; + commit_id: git::Oid, +) -> Result> { + let commit = repository.find_commit(commit_id).map_err(|err| match err { + git::Error::NotFound(_) => anyhow!("commit {commit_id} not found"), + err => err.into(), + })?; // If it's a merge commit, we list nothing. In the future we could to a fork exec of `git diff-tree --cc` if commit.parent_count() != 1 { diff --git a/crates/gitbutler-core/src/virtual_branches/integration.rs b/crates/gitbutler-core/src/virtual_branches/integration.rs index d42b92fcf0..d4492a0b1b 100644 --- a/crates/gitbutler-core/src/virtual_branches/integration.rs +++ b/crates/gitbutler-core/src/virtual_branches/integration.rs @@ -1,10 +1,11 @@ use std::{path::PathBuf, vec}; -use anyhow::{anyhow, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use bstr::ByteSlice; use lazy_static::lazy_static; -use super::{errors::VerifyError, VirtualBranchesHandle}; +use super::VirtualBranchesHandle; +use crate::virtual_branches::errors::Marker; use crate::{ git::{self, CommitExt}, project_repository::{self, conflicts, LogUntil}, @@ -50,7 +51,7 @@ pub fn get_workspace_head( let target_commit = repo.find_commit(target.sha.into())?; let mut workspace_tree = target_commit.tree()?; - if conflicts::is_conflicting::(project_repo, None)? { + if conflicts::is_conflicting(project_repo, None)? { let merge_parent = conflicts::merge_parent(project_repo)?.ok_or(anyhow!("No merge parent"))?; let first_branch = applied_branches.first().ok_or(anyhow!("No branches"))?; @@ -291,142 +292,143 @@ pub fn update_gitbutler_integration( Ok(final_commit.into()) } -pub fn verify_branch( - project_repository: &project_repository::Repository, -) -> Result<(), VerifyError> { - verify_current_branch_name(project_repository)?; - verify_head_is_set(project_repository)?; - verify_head_is_clean(project_repository)?; +pub fn verify_branch(project_repository: &project_repository::Repository) -> Result<()> { + project_repository + .verify_current_branch_name() + .and_then(|me| me.verify_head_is_set()) + .and_then(|me| me.verify_head_is_clean()) + .context(Marker::VerificationFailure)?; Ok(()) } -fn verify_head_is_clean( - project_repository: &project_repository::Repository, -) -> Result<(), VerifyError> { - let head_commit = project_repository - .git_repository - .head() - .context("failed to get head")? - .peel_to_commit() - .context("failed to peel to commit")?; - - let vb_handle = VirtualBranchesHandle::new(project_repository.project().gb_dir()); - let default_target = vb_handle - .get_default_target() - .context("failed to get default target")?; - - let mut extra_commits = project_repository - .log( - head_commit.id().into(), - LogUntil::Commit(default_target.sha), - ) - .context("failed to get log")?; - - let integration_commit = extra_commits.pop(); - - if integration_commit.is_none() { - // no integration commit found - return Err(VerifyError::NoIntegrationCommit); +impl project_repository::Repository { + fn verify_head_is_set(&self) -> Result<&Self> { + match self.get_head().context("failed to get head")?.name() { + Some(refname) if refname.to_string() == GITBUTLER_INTEGRATION_REFERENCE.to_string() => { + Ok(self) + } + Some(head_name) => Err(invalid_head_err(&head_name.to_string())), + None => Err(anyhow!( + "project in detached head state. Please checkout {} to continue", + GITBUTLER_INTEGRATION_REFERENCE.branch() + )), + } } - if extra_commits.is_empty() { - // no extra commits found, so we're good - return Ok(()); + // Returns an error if repo head is not pointing to the integration branch. + fn verify_current_branch_name(&self) -> Result<&Self> { + match self.get_head()?.name() { + Some(head) => { + let head_name = head.to_string(); + if head_name != GITBUTLER_INTEGRATION_REFERENCE.to_string() { + return Err(invalid_head_err(&head_name)); + } + Ok(self) + } + None => Err(anyhow!("Repo HEAD is unavailable")), + } } - project_repository - .git_repository - .reset( - integration_commit.as_ref().unwrap(), - git2::ResetType::Soft, - None, - ) - .context("failed to reset to integration commit")?; - - let mut new_branch = super::create_virtual_branch( - project_repository, - &BranchCreateRequest { - name: extra_commits - .last() - .map(|commit| commit.message_bstr().to_string()), - ..Default::default() - }, - ) - .context("failed to create virtual branch")?; - - // rebasing the extra commits onto the new branch - let vb_state = project_repository.project().virtual_branches(); - extra_commits.reverse(); - let mut head = new_branch.head; - for commit in extra_commits { - let new_branch_head = project_repository - .git_repository - .find_commit(head) - .context("failed to find new branch head")?; - - let rebased_commit_oid = project_repository + fn verify_head_is_clean(&self) -> Result<&Self> { + let head_commit = self .git_repository - .commit( - None, - &commit.author(), - &commit.committer(), - &commit.message_bstr().to_str_lossy(), - &commit.tree().unwrap(), - &[&new_branch_head], - None, + .head() + .context("failed to get head")? + .peel_to_commit() + .context("failed to peel to commit")?; + + let vb_handle = VirtualBranchesHandle::new(self.project().gb_dir()); + let default_target = vb_handle + .get_default_target() + .context("failed to get default target")?; + + let mut extra_commits = self + .log( + head_commit.id().into(), + LogUntil::Commit(default_target.sha), ) - .context(format!( - "failed to rebase commit {} onto new branch", - commit.id() - ))?; + .context("failed to get log")?; - let rebased_commit = project_repository - .git_repository - .find_commit(rebased_commit_oid) - .context(format!( - "failed to find rebased commit {}", - rebased_commit_oid - ))?; - - new_branch.head = rebased_commit.id().into(); - new_branch.tree = rebased_commit.tree_id().into(); - vb_state - .set_branch(new_branch.clone()) - .context("failed to write branch")?; - - head = rebased_commit.id().into(); - } - Ok(()) -} + let integration_commit = extra_commits.pop(); -fn verify_head_is_set( - project_repository: &project_repository::Repository, -) -> Result<(), VerifyError> { - match project_repository - .get_head() - .context("failed to get head") - .map_err(VerifyError::Other)? - .name() - { - Some(refname) if refname.to_string() == GITBUTLER_INTEGRATION_REFERENCE.to_string() => { - Ok(()) + if integration_commit.is_none() { + // no integration commit found + bail!("gibButler's integration commit not found on head"); } - None => Err(VerifyError::DetachedHead), - Some(head_name) => Err(VerifyError::InvalidHead(head_name.to_string())), - } -} -// Returns an error if repo head is not pointing to the integration branch. -pub fn verify_current_branch_name( - project_repository: &project_repository::Repository, -) -> Result { - match project_repository.get_head()?.name() { - Some(head) => { - if head.to_string() != GITBUTLER_INTEGRATION_REFERENCE.to_string() { - return Err(VerifyError::InvalidHead(head.to_string())); - } - Ok(true) + if extra_commits.is_empty() { + // no extra commits found, so we're good + return Ok(self); } - None => Err(VerifyError::HeadNotFound), + + self.git_repository + .reset( + integration_commit.as_ref().unwrap(), + git2::ResetType::Soft, + None, + ) + .context("failed to reset to integration commit")?; + + let mut new_branch = super::create_virtual_branch( + self, + &BranchCreateRequest { + name: extra_commits + .last() + .map(|commit| commit.message_bstr().to_string()), + ..Default::default() + }, + ) + .context("failed to create virtual branch")?; + + // rebasing the extra commits onto the new branch + let vb_state = self.project().virtual_branches(); + extra_commits.reverse(); + let mut head = new_branch.head; + for commit in extra_commits { + let new_branch_head = self + .git_repository + .find_commit(head) + .context("failed to find new branch head")?; + + let rebased_commit_oid = self + .git_repository + .commit( + None, + &commit.author(), + &commit.committer(), + &commit.message_bstr().to_str_lossy(), + &commit.tree().unwrap(), + &[&new_branch_head], + None, + ) + .context(format!( + "failed to rebase commit {} onto new branch", + commit.id() + ))?; + + let rebased_commit = self + .git_repository + .find_commit(rebased_commit_oid) + .context(format!( + "failed to find rebased commit {}", + rebased_commit_oid + ))?; + + new_branch.head = rebased_commit.id().into(); + new_branch.tree = rebased_commit.tree_id().into(); + vb_state + .set_branch(new_branch.clone()) + .context("failed to write branch")?; + + head = rebased_commit.id().into(); + } + Ok(self) } } + +fn invalid_head_err(head_name: &str) -> anyhow::Error { + anyhow!( + "project is on {head_name}. Please checkout {} to continue", + GITBUTLER_INTEGRATION_REFERENCE.branch() + ) +} diff --git a/crates/gitbutler-core/src/virtual_branches/remote.rs b/crates/gitbutler-core/src/virtual_branches/remote.rs index a8655c3339..5838f3b8e5 100644 --- a/crates/gitbutler-core/src/virtual_branches/remote.rs +++ b/crates/gitbutler-core/src/virtual_branches/remote.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result}; use bstr::BString; use serde::Serialize; -use super::{errors, target, Author, VirtualBranchesHandle}; +use super::{target, Author, VirtualBranchesHandle}; use crate::{ git::{self, CommitExt}, project_repository::{self, LogUntil}, @@ -55,7 +55,7 @@ pub struct RemoteCommit { // a list of all the normal (non-gitbutler) git branches. pub fn list_remote_branches( project_repository: &project_repository::Repository, -) -> Result, errors::ListRemoteBranchesError> { +) -> Result> { let default_target = default_target(&project_repository.project().gb_dir())?; let mut remote_branches = vec![]; @@ -85,7 +85,7 @@ pub fn list_remote_branches( pub fn get_branch_data( project_repository: &project_repository::Repository, refname: &git::Refname, -) -> Result { +) -> Result { let default_target = default_target(&project_repository.project().gb_dir())?; let branch = project_repository @@ -97,9 +97,7 @@ pub fn get_branch_data( .context("failed to get branch data")?; branch_data - .ok_or_else(|| { - errors::GetRemoteBranchDataError::Other(anyhow::anyhow!("no data found for branch")) - }) + .context("no data found for branch") .map(|branch_data| RemoteBranchData { sha: branch_data.sha, name: branch_data.name, diff --git a/crates/gitbutler-core/src/virtual_branches/state.rs b/crates/gitbutler-core/src/virtual_branches/state.rs index ba0048a128..dd954fbe66 100644 --- a/crates/gitbutler-core/src/virtual_branches/state.rs +++ b/crates/gitbutler-core/src/virtual_branches/state.rs @@ -4,7 +4,7 @@ use std::{ }; use crate::fs::read_toml_file_or_default; -use anyhow::anyhow; +use anyhow::{anyhow, Result}; use serde::{Deserialize, Serialize}; use super::{target::Target, Branch}; @@ -38,7 +38,7 @@ impl VirtualBranchesHandle { /// Persists the default target for the given repository. /// /// Errors if the file cannot be read or written. - pub fn set_default_target(&self, target: Target) -> anyhow::Result<()> { + pub fn set_default_target(&self, target: Target) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.default_target = Some(target); self.write_file(&virtual_branches)?; @@ -48,17 +48,17 @@ impl VirtualBranchesHandle { /// Gets the default target for the given repository. /// /// Errors if the file cannot be read or written. - pub fn get_default_target(&self) -> anyhow::Result { + pub fn get_default_target(&self) -> Result { let virtual_branches = self.read_file(); virtual_branches? .default_target - .ok_or(anyhow!("No default target")) + .ok_or(anyhow!("there is no default target")) } /// Sets the target for the given virtual branch. /// /// Errors if the file cannot be read or written. - pub fn set_branch_target(&self, id: BranchId, target: Target) -> anyhow::Result<()> { + pub fn set_branch_target(&self, id: BranchId, target: Target) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.branch_targets.insert(id, target); self.write_file(&virtual_branches)?; @@ -68,7 +68,7 @@ impl VirtualBranchesHandle { /// Sets the state of the given virtual branch. /// /// Errors if the file cannot be read or written. - pub fn set_branch(&self, branch: Branch) -> anyhow::Result<()> { + pub fn set_branch(&self, branch: Branch) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.branches.insert(branch.id, branch); self.write_file(&virtual_branches)?; @@ -78,7 +78,7 @@ impl VirtualBranchesHandle { /// Removes the given virtual branch. /// /// Errors if the file cannot be read or written. - pub fn remove_branch(&self, id: BranchId) -> anyhow::Result<()> { + pub fn remove_branch(&self, id: BranchId) -> Result<()> { let mut virtual_branches = self.read_file()?; virtual_branches.branches.remove(&id); self.write_file(&virtual_branches)?; @@ -88,19 +88,22 @@ impl VirtualBranchesHandle { /// Gets the state of the given virtual branch. /// /// Errors if the file cannot be read or written. - pub fn get_branch(&self, id: BranchId) -> Result { + pub fn get_branch(&self, id: BranchId) -> Result { + self.try_branch(id)? + .ok_or_else(|| anyhow!("branch with ID {id} not found")) + } + + /// Gets the state of the given virtual branch returning `Some(branch)` or `None` + /// if that branch doesn't exist. + pub fn try_branch(&self, id: BranchId) -> Result> { let virtual_branches = self.read_file()?; - virtual_branches - .branches - .get(&id) - .cloned() - .ok_or(crate::reader::Error::NotFound) + Ok(virtual_branches.branches.get(&id).cloned()) } /// Lists all virtual branches. /// /// Errors if the file cannot be read or written. - pub fn list_branches(&self) -> anyhow::Result> { + pub fn list_branches(&self) -> Result> { let virtual_branches = self.read_file()?; let branches: Vec = virtual_branches.branches.values().cloned().collect(); Ok(branches) @@ -116,15 +119,15 @@ impl VirtualBranchesHandle { /// Reads and parses the state file. /// /// If the file does not exist, it will be created. - fn read_file(&self) -> Result { + fn read_file(&self) -> Result { read_toml_file_or_default(&self.file_path) } - fn write_file(&self, virtual_branches: &VirtualBranches) -> anyhow::Result<()> { + fn write_file(&self, virtual_branches: &VirtualBranches) -> Result<()> { write(self.file_path.as_path(), virtual_branches) } } -fn write>(file_path: P, virtual_branches: &VirtualBranches) -> anyhow::Result<()> { +fn write>(file_path: P, virtual_branches: &VirtualBranches) -> Result<()> { crate::fs::write(file_path, toml::to_string(&virtual_branches)?) } diff --git a/crates/gitbutler-core/src/virtual_branches/virtual.rs b/crates/gitbutler-core/src/virtual_branches/virtual.rs index 7f4fdd47c3..5199a4ebf3 100644 --- a/crates/gitbutler-core/src/virtual_branches/virtual.rs +++ b/crates/gitbutler-core/src/virtual_branches/virtual.rs @@ -23,13 +23,15 @@ use super::{ branch::{ self, Branch, BranchCreateRequest, BranchId, BranchOwnershipClaims, Hunk, OwnershipClaim, }, - branch_to_remote_branch, errors, target, RemoteBranch, VirtualBranchesHandle, + branch_to_remote_branch, target, RemoteBranch, VirtualBranchesHandle, }; -use crate::error::{self, AnyhowContextExt, Code}; +use crate::error::Code; +use crate::git::diff::GitHunk; use crate::git::diff::{diff_files_into_hunks, trees, FileDiff}; use crate::git::{CommitExt, RepositoryExt}; use crate::time::now_since_unix_epoch_ms; use crate::virtual_branches::branch::HunkHash; +use crate::virtual_branches::errors::Marker; use crate::{ dedup::{dedup, dedup_fmt}, git::{ @@ -38,9 +40,8 @@ use crate::{ Refname, RemoteRefname, }, project_repository::{self, conflicts, LogUntil}, - reader, users, + users, }; -use crate::{error::Error, git::diff::GitHunk}; type AppliedStatuses = Vec<(branch::Branch, BranchStatus)>; @@ -220,29 +221,14 @@ pub fn apply_branch( project_repository: &project_repository::Repository, branch_id: BranchId, user: Option<&users::User>, -) -> Result { - if project_repository.is_resolving() { - return Err(errors::ApplyBranchError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } +) -> Result { + project_repository.assure_resolved()?; let repo = &project_repository.git_repository; let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; - let mut branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::ApplyBranchError::BranchNotFound( - errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }, - )), - Err(error) => Err(errors::ApplyBranchError::Other(error.into())), - }?; + let mut branch = vb_state.get_branch(branch_id)?; if branch.applied { return Ok(branch.name); @@ -281,7 +267,7 @@ pub fn apply_branch( if merge_index.has_conflicts() { // currently we can only deal with the merge problem branch for mut branch in - super::get_status_by_branch(project_repository, Some(&target_commit.id().into()))? + get_status_by_branch(project_repository, Some(&target_commit.id().into()))? .0 .into_iter() .map(|(branch, _)| branch) @@ -450,7 +436,8 @@ pub fn apply_branch( .context("failed to merge trees")?; if merge_index.has_conflicts() { - return Err(errors::ApplyBranchError::BranchConflicts(branch_id)); + return Err(anyhow!("branch {branch_id} is in a conflicting state")) + .context(Marker::ProjectConflict); } // apply the branch @@ -472,14 +459,8 @@ pub fn apply_branch( pub fn unapply_ownership( project_repository: &project_repository::Repository, ownership: &BranchOwnershipClaims, -) -> Result<(), errors::UnapplyOwnershipError> { - if conflicts::is_resolving(project_repository) { - return Err(errors::UnapplyOwnershipError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } +) -> Result<()> { + project_repository.assure_resolved()?; let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; @@ -491,8 +472,7 @@ pub fn unapply_ownership( .filter(|b| b.applied) .collect::>(); - let integration_commit_id = - super::integration::get_workspace_head(&vb_state, project_repository)?; + let integration_commit_id = get_workspace_head(&vb_state, project_repository)?; let (applied_statuses, _) = get_applied_status( project_repository, @@ -536,9 +516,7 @@ pub fn unapply_ownership( if let Some(reversed_hunk) = diff::reverse_hunk(h.1) { diff.entry(h.0).or_insert_with(Vec::new).push(reversed_hunk); } else { - return Err(errors::UnapplyOwnershipError::Other(anyhow::anyhow!( - "failed to reverse hunk" - ))); + bail!("failed to reverse hunk") } } @@ -582,14 +560,8 @@ pub fn unapply_ownership( pub fn reset_files( project_repository: &project_repository::Repository, files: &Vec, -) -> Result<(), errors::UnapplyOwnershipError> { - if conflicts::is_resolving(project_repository) { - return Err(errors::UnapplyOwnershipError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } +) -> Result<()> { + project_repository.assure_resolved()?; // for each tree, we need to checkout the entry from the index at that path // or if it doesn't exist, remove the file from the working directory @@ -617,21 +589,10 @@ pub fn reset_files( pub fn unapply_branch( project_repository: &project_repository::Repository, branch_id: BranchId, -) -> Result, errors::UnapplyBranchError> { +) -> Result> { let vb_state = project_repository.project().virtual_branches(); - let mut target_branch = vb_state - .get_branch(branch_id) - .map_err(|error| match error { - reader::Error::NotFound => { - errors::UnapplyBranchError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }) - } - error => errors::UnapplyBranchError::Other(error.into()), - })?; - + let mut target_branch = vb_state.get_branch(branch_id)?; if !target_branch.applied { return Ok(Some(target_branch)); } @@ -760,7 +721,7 @@ fn find_base_tree<'a>( pub fn list_virtual_branches( project_repository: &project_repository::Repository, -) -> Result<(Vec, Vec), errors::ListVirtualBranchesError> { +) -> Result<(Vec, Vec)> { let mut branches: Vec = Vec::new(); let vb_state = project_repository.project().virtual_branches(); @@ -1034,7 +995,7 @@ fn commit_to_vbranch_commit( pub fn create_virtual_branch( project_repository: &project_repository::Repository, create: &BranchCreateRequest, -) -> Result { +) -> Result { let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; @@ -1166,14 +1127,14 @@ pub fn integrate_upstream_commits( project_repository: &project_repository::Repository, branch_id: BranchId, user: Option<&users::User>, -) -> Result<(), anyhow::Error> { - conflicts::is_conflicting::<&Path>(project_repository, None)?; +) -> Result<()> { + conflicts::is_conflicting(project_repository, None)?; let repo = &project_repository.git_repository; let project = project_repository.project(); let vb_state = project.virtual_branches(); - let mut branch = vb_state.get_branch(branch_id).map_err(Error::from_err)?; + let mut branch = vb_state.get_branch(branch_id)?; let default_target = vb_state.get_default_target()?; let upstream_branch = branch.upstream.as_ref().context("upstream not found")?; @@ -1231,19 +1192,20 @@ pub fn integrate_upstream_commits( // scenario we would need to "cherry rebase" new upstream commits onto the last rebased // local commit. if has_rebased_commits && !can_use_force { - let message = "Aborted because force push is disallowed and commits have been rebased."; return Err(anyhow!("Cannot merge rebased commits without force push") - .context(error::Context::new(Code::ProjectConflict, message))); + .context("Aborted because force push is disallowed and commits have been rebased") + .context(Marker::ProjectConflict)); } let integration_result = match can_use_force { true => integrate_with_rebase(project_repository, &mut branch, &mut unknown_commits), false => { if has_rebased_commits { - let message = - "Aborted because force push is disallowed and commits have been rebased."; return Err(anyhow!("Cannot merge rebased commits without force push") - .context(error::Context::new(Code::ProjectConflict, message))); + .context( + "Aborted because force push is disallowed and commits have been rebased", + ) + .context(Marker::ProjectConflict)); } integrate_with_merge( project_repository, @@ -1255,14 +1217,11 @@ pub fn integrate_upstream_commits( } }; - // TODO: Use thiserror for the two integrate_with functions instead of this? - if let Err(err) = &integration_result { - if err - .custom_context() - .is_some_and(|c| c.code == Code::ProjectConflict) - { - return Ok(()); - }; + if integration_result.as_ref().err().map_or(false, |err| { + err.downcast_ref() + .is_some_and(|marker: &Marker| *marker == Marker::ProjectConflict) + }) { + return Ok(()); }; let new_head = integration_result?; @@ -1339,10 +1298,7 @@ pub fn integrate_with_merge( .conflict_style_merge() .force() .checkout()?; - return Err(anyhow!("Merging")).context(error::Context::new_static( - Code::ProjectConflict, - "Merge problem", - )); + return Err(anyhow!("merge problem")).context(Marker::ProjectConflict); } let merge_tree_oid = merge_index.write_tree_to(repo)?; @@ -1367,19 +1323,9 @@ pub fn integrate_with_merge( pub fn update_branch( project_repository: &project_repository::Repository, branch_update: branch::BranchUpdateRequest, -) -> Result { +) -> Result { let vb_state = project_repository.project().virtual_branches(); - let mut branch = vb_state - .get_branch(branch_update.id) - .map_err(|error| match error { - reader::Error::NotFound => { - errors::UpdateBranchError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id: branch_update.id, - }) - } - _ => errors::UpdateBranchError::Other(error.into()), - })?; + let mut branch = vb_state.get_branch(branch_update.id)?; _ = project_repository .project() .snapshot_branch_update(&branch, &branch_update); @@ -1457,14 +1403,11 @@ pub fn update_branch( pub fn delete_branch( project_repository: &project_repository::Repository, branch_id: BranchId, -) -> Result<(), Error> { +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); - let branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => return Ok(()), - Err(error) => Err(error), - } - .context("failed to read branch")?; + let Some(branch) = vb_state.try_branch(branch_id)? else { + return Ok(()); + }; _ = project_repository .project() .snapshot_branch_deletion(branch.name.clone()); @@ -1979,7 +1922,7 @@ fn virtual_hunks_into_virtual_files( .map(|(path, hunks)| { let id = path.display().to_string(); let conflicted = - conflicts::is_conflicting(project_repository, Some(&id)).unwrap_or(false); + conflicts::is_conflicting(project_repository, Some(id.as_ref())).unwrap_or(false); let binary = hunks.iter().any(|h| h.binary); let modified_at = hunks.iter().map(|h| h.modified_at).max().unwrap_or(0); debug_assert!(hunks.iter().all(|hunk| hunk.file_path == path)); @@ -2000,43 +1943,31 @@ fn virtual_hunks_into_virtual_files( pub fn reset_branch( project_repository: &project_repository::Repository, branch_id: BranchId, - target_commit_oid: git::Oid, -) -> Result<(), errors::ResetBranchError> { + target_commit_id: git::Oid, +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; - let mut branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::ResetBranchError::BranchNotFound( - errors::BranchNotFound { - branch_id, - project_id: project_repository.project().id, - }, - )), - Err(error) => Err(errors::ResetBranchError::Other(error.into())), - }?; - - if branch.head == target_commit_oid { + let mut branch = vb_state.get_branch(branch_id)?; + if branch.head == target_commit_id { // nothing to do return Ok(()); } - if default_target.sha != target_commit_oid + if default_target.sha != target_commit_id && !project_repository .l(branch.head, LogUntil::Commit(default_target.sha))? - .contains(&target_commit_oid) + .contains(&target_commit_id) { - return Err(errors::ResetBranchError::CommitNotFoundInBranch( - target_commit_oid, - )); + bail!("commit {target_commit_id} not in the branch"); } - // Compute the old workspace before resetting so we can can figure out + // Compute the old workspace before resetting, so we can figure out // what hunks were released by this reset, and assign them to this branch. let old_head = get_workspace_head(&vb_state, project_repository)?; - branch.head = target_commit_oid; + branch.head = target_commit_id; vb_state .set_branch(branch.clone()) .context("failed to write branch")?; @@ -2288,7 +2219,7 @@ pub fn commit( ownership: Option<&branch::BranchOwnershipClaims>, user: Option<&users::User>, run_hooks: bool, -) -> Result { +) -> Result { let mut message_buffer = message.to_owned(); let vb_state = project_repository.project().virtual_branches(); @@ -2299,7 +2230,7 @@ pub fn commit( .context("failed to run hook")?; if let HookResult::RunNotSuccessful { stdout, .. } = hook_result { - return Err(errors::CommitError::CommitMsgHookRejected(stdout)); + bail!("commit-msg hook rejected: {}", stdout.trim()); } let hook_result = project_repository @@ -2308,14 +2239,13 @@ pub fn commit( .context("failed to run hook")?; if let HookResult::RunNotSuccessful { stdout, .. } = hook_result { - return Err(errors::CommitError::CommitHookRejected(stdout)); + bail!("commit hook rejected: {}", stdout.trim()); } } let message = &message_buffer; - let integration_commit_id = - super::integration::get_workspace_head(&vb_state, project_repository)?; + let integration_commit_id = get_workspace_head(&vb_state, project_repository)?; // get the files to commit let (statuses, _) = get_status_by_branch(project_repository, Some(&integration_commit_id)) .context("failed to get status by branch")?; @@ -2323,20 +2253,11 @@ pub fn commit( let (ref mut branch, files) = statuses .into_iter() .find(|(branch, _)| branch.id == branch_id) - .ok_or_else(|| { - errors::CommitError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }) - })?; + .with_context(|| format!("branch {branch_id} not found"))?; update_conflict_markers(project_repository, &files)?; - if conflicts::is_conflicting::<&Path>(project_repository, None)? { - return Err(errors::CommitError::Conflicted(errors::ProjectConflict { - project_id: project_repository.project().id, - })); - } + project_repository.assure_unconflicted()?; let tree_oid = if let Some(ownership) = ownership { let files = files.into_iter().filter_map(|(filepath, hunks)| { @@ -2422,19 +2343,10 @@ pub fn push( with_force: bool, credentials: &git::credentials::Helper, askpass: Option>, -) -> Result<(), errors::PushError> { +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); - let mut vbranch = vb_state - .get_branch(branch_id) - .map_err(|error| match error { - reader::Error::NotFound => errors::PushError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }), - error => errors::PushError::Other(error.into()), - })?; - + let mut vbranch = vb_state.get_branch(branch_id)?; let remote_branch = if let Some(upstream_branch) = &vbranch.upstream { upstream_branch.clone() } else { @@ -2565,7 +2477,7 @@ fn is_commit_integrated( pub fn is_remote_branch_mergeable( project_repository: &project_repository::Repository, branch_name: &git::RemoteRefname, -) -> Result { +) -> Result { let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; @@ -2574,16 +2486,15 @@ pub fn is_remote_branch_mergeable( .find_commit(default_target.sha) .context("failed to find target commit")?; - let branch = match project_repository + let branch = project_repository .git_repository .find_branch(&branch_name.into()) - { - Ok(branch) => Ok(branch), - Err(git::Error::NotFound(_)) => Err(errors::IsRemoteBranchMergableError::BranchNotFound( - branch_name.clone(), - )), - Err(error) => Err(errors::IsRemoteBranchMergableError::Other(error.into())), - }?; + .map_err(|err| match err { + git::Error::NotFound(_) => { + anyhow!("Remote branch {} not found", branch_name.clone()) + } + err => err.into(), + })?; let branch_oid = branch.target().context("detatched head")?; let branch_commit = project_repository .git_repository @@ -2611,19 +2522,9 @@ pub fn is_remote_branch_mergeable( pub fn is_virtual_branch_mergeable( project_repository: &project_repository::Repository, branch_id: BranchId, -) -> Result { +) -> Result { let vb_state = project_repository.project().virtual_branches(); - let branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::IsVirtualBranchMergeable::BranchNotFound( - errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }, - )), - Err(error) => Err(errors::IsVirtualBranchMergeable::Other(error.into())), - }?; - + let branch = vb_state.get_branch(branch_id)?; if branch.applied { return Ok(true); } @@ -2682,22 +2583,19 @@ pub fn is_virtual_branch_mergeable( pub fn move_commit_file( project_repository: &project_repository::Repository, branch_id: BranchId, - from_commit_oid: git::Oid, - to_commit_oid: git::Oid, + from_commit_id: git::Oid, + to_commit_id: git::Oid, target_ownership: &BranchOwnershipClaims, -) -> Result { +) -> Result { let vb_state = project_repository.project().virtual_branches(); - let mut target_branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => return Ok(to_commit_oid), // this is wrong - Err(error) => Err(error), - } - .context("failed to read branch")?; + let Some(mut target_branch) = vb_state.try_branch(branch_id)? else { + return Ok(to_commit_id); // this is wrong + }; let default_target = vb_state.get_default_target()?; - let mut to_amend_oid = to_commit_oid; + let mut to_amend_oid = to_commit_id; let mut amend_commit = project_repository .git_repository .find_commit(to_amend_oid) @@ -2745,13 +2643,11 @@ pub fn move_commit_file( // if we're not moving anything, return an error if diffs_to_amend.is_empty() { - return Err(errors::VirtualBranchError::TargetOwnerhshipNotFound( - target_ownership.clone(), - )); + bail!("target ownership not found"); } // is from_commit_oid in upstream_commits? - if !upstream_commits.contains(&from_commit_oid) { + if !upstream_commits.contains(&from_commit_id) { // this means that the "from" commit is _below_ the "to" commit in the history // which makes things a little more complicated because in this case we need to // remove the changes from the lower "from" commit, rebase everything, then add the changes @@ -2760,7 +2656,7 @@ pub fn move_commit_file( // first, let's get the from commit data and it's parent data let from_commit = project_repository .git_repository - .find_commit(from_commit_oid) + .find_commit(from_commit_id) .context("failed to find commit")?; let from_tree = from_commit.tree().context("failed to find tree")?; let from_parent = from_commit.parent(0).context("failed to find parent")?; @@ -2807,11 +2703,11 @@ pub fn move_commit_file( let repo = &project_repository.git_repository; // write our new tree and commit for the new "from" commit without the moved changes - let new_from_tree_oid = + let new_from_tree_id = write_tree_onto_commit(project_repository, from_parent.id().into(), &diffs_to_keep)?; let new_from_tree = &repo - .find_tree(new_from_tree_oid) - .map_err(|_error| errors::VirtualBranchError::GitObjectNotFound(new_from_tree_oid))?; + .find_tree(new_from_tree_id) + .with_context(|| "tree {new_from_tree_oid} not found")?; let change_id = from_commit.change_id(); let new_from_commit_oid = repo .commit( @@ -2823,19 +2719,18 @@ pub fn move_commit_file( &[&from_parent], change_id.as_deref(), ) - .map_err(|_error| errors::VirtualBranchError::CommitFailed)?; + .context("commit failed")?; // rebase everything above the new "from" commit that has the moved changes removed let new_head = match cherry_rebase( project_repository, new_from_commit_oid, - from_commit_oid, + from_commit_id, target_branch.head, ) { Ok(Some(new_head)) => new_head, - _ => { - return Err(errors::VirtualBranchError::RebaseFailed); - } + Ok(None) => bail!("no rebase was performed"), + Err(err) => return Err(err).context("rebase failed"), }; // ok, now we need to identify which the new "to" commit is in the rebased history @@ -2855,14 +2750,12 @@ pub fn move_commit_file( let to_commit_offset = old_upstream_commit_oids .iter() .position(|c| *c == to_amend_oid) - .ok_or(errors::VirtualBranchError::Other(anyhow!( - "failed to find commit in old commits" - )))?; + .context("failed to find commit in old commits")?; // find the new "to" commit in our new rebased upstream commits - to_amend_oid = *new_upstream_commit_oids.get(to_commit_offset).ok_or( - errors::VirtualBranchError::Other(anyhow!("failed to find commit in new commits")), - )?; + to_amend_oid = *new_upstream_commit_oids + .get(to_commit_offset) + .context("failed to find commit in new commits")?; // reset the "to" commit variable for writing the changes back to amend_commit = project_repository @@ -2930,7 +2823,7 @@ pub fn move_commit_file( super::integration::update_gitbutler_integration(&vb_state, project_repository)?; Ok(commit_oid) } else { - Err(errors::VirtualBranchError::RebaseFailed) + Err(anyhow!("rebase failed")) } } @@ -2942,15 +2835,8 @@ pub fn amend( branch_id: BranchId, commit_oid: git::Oid, target_ownership: &BranchOwnershipClaims, -) -> Result { - if conflicts::is_conflicting::<&Path>(project_repository, None)? { - return Err(errors::VirtualBranchError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } - +) -> Result { + project_repository.assure_resolved()?; let vb_state = project_repository.project().virtual_branches(); let all_branches = vb_state @@ -2958,12 +2844,7 @@ pub fn amend( .context("failed to read virtual branches")?; if !all_branches.iter().any(|b| b.id == branch_id) { - return Err(errors::VirtualBranchError::BranchNotFound( - errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }, - )); + bail!("could not find any branch with id {branch_id} to amend to"); } let applied_branches = all_branches @@ -2972,12 +2853,7 @@ pub fn amend( .collect::>(); if !applied_branches.iter().any(|b| b.id == branch_id) { - return Err(errors::VirtualBranchError::BranchNotFound( - errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }, - )); + bail!("could not find applied branch with id {branch_id} to amend to"); } let default_target = vb_state.get_default_target()?; @@ -2995,20 +2871,11 @@ pub fn amend( let (ref mut target_branch, target_status) = applied_statuses .iter_mut() .find(|(b, _)| b.id == branch_id) - .ok_or_else(|| { - errors::VirtualBranchError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }) - })?; + .ok_or_else(|| anyhow!("could not find branch {branch_id} in status list"))?; if target_branch.upstream.is_some() && !project_repository.project().ok_with_force_push { // amending to a pushed head commit will cause a force push that is not allowed - return Err(errors::VirtualBranchError::ForcePushNotAllowed( - errors::ForcePushNotAllowed { - project_id: project_repository.project().id, - }, - )); + bail!("force-push is not allowed"); } if project_repository @@ -3018,7 +2885,7 @@ pub fn amend( )? .is_empty() { - return Err(errors::VirtualBranchError::BranchHasNoCommits); + bail!("branch has no commits - there is nothing to amend to"); } // find commit oid @@ -3055,9 +2922,7 @@ pub fn amend( .collect::>(); if diffs_to_amend.is_empty() { - return Err(errors::VirtualBranchError::TargetOwnerhshipNotFound( - target_ownership.clone(), - )); + bail!("target ownership not found"); } // apply diffs_to_amend to the commit tree @@ -3109,7 +2974,7 @@ pub fn amend( super::integration::update_gitbutler_integration(&vb_state, project_repository)?; Ok(commit_oid) } else { - Err(errors::VirtualBranchError::RebaseFailed) + Err(anyhow!("rebase failed")) } } @@ -3122,22 +2987,12 @@ pub fn reorder_commit( branch_id: BranchId, commit_oid: git::Oid, offset: i32, -) -> Result<(), errors::VirtualBranchError> { +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; - let mut branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::VirtualBranchError::BranchNotFound( - errors::BranchNotFound { - branch_id, - project_id: project_repository.project().id, - }, - )), - Err(error) => Err(errors::VirtualBranchError::Other(error.into())), - }?; - + let mut branch = vb_state.get_branch(branch_id)?; // find the commit to offset from let commit = project_repository .git_repository @@ -3163,20 +3018,16 @@ pub fn reorder_commit( ids_to_rebase.push(commit_oid); ids_to_rebase.push(last_oid); - match cherry_rebase_group(project_repository, parent_oid.into(), &mut ids_to_rebase) { - Ok(new_head) => { - branch.head = new_head; - vb_state - .set_branch(branch.clone()) - .context("failed to write branch")?; + let new_head = + cherry_rebase_group(project_repository, parent_oid.into(), &mut ids_to_rebase) + .context("rebase failed")?; + branch.head = new_head; + vb_state + .set_branch(branch.clone()) + .context("failed to write branch")?; - super::integration::update_gitbutler_integration(&vb_state, project_repository) - .context("failed to update gitbutler integration")?; - } - _ => { - return Err(errors::VirtualBranchError::RebaseFailed); - } - } + super::integration::update_gitbutler_integration(&vb_state, project_repository) + .context("failed to update gitbutler integration")?; } else { // move commit down if default_target.sha == parent_oid.into() { @@ -3195,20 +3046,17 @@ pub fn reorder_commit( ids_to_rebase.push(parent_oid.into()); ids_to_rebase.push(commit_oid); - match cherry_rebase_group(project_repository, target_oid.into(), &mut ids_to_rebase) { - Ok(new_head) => { - branch.head = new_head; - vb_state - .set_branch(branch.clone()) - .context("failed to write branch")?; + let new_head = + cherry_rebase_group(project_repository, target_oid.into(), &mut ids_to_rebase) + .context("rebase failed")?; - super::integration::update_gitbutler_integration(&vb_state, project_repository) - .context("failed to update gitbutler integration")?; - } - _ => { - return Err(errors::VirtualBranchError::RebaseFailed); - } - } + branch.head = new_head; + vb_state + .set_branch(branch.clone()) + .context("failed to write branch")?; + + super::integration::update_gitbutler_integration(&vb_state, project_repository) + .context("failed to update gitbutler integration")?; } Ok(()) @@ -3223,20 +3071,10 @@ pub fn insert_blank_commit( commit_oid: git::Oid, user: Option<&users::User>, offset: i32, -) -> Result<(), errors::VirtualBranchError> { +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); - let mut branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::VirtualBranchError::BranchNotFound( - errors::BranchNotFound { - branch_id, - project_id: project_repository.project().id, - }, - )), - Err(error) => Err(errors::VirtualBranchError::Other(error.into())), - }?; - + let mut branch = vb_state.get_branch(branch_id)?; // find the commit to offset from let mut commit = project_repository .git_repository @@ -3275,8 +3113,9 @@ pub fn insert_blank_commit( super::integration::update_gitbutler_integration(&vb_state, project_repository) .context("failed to update gitbutler integration")?; } - _ => { - return Err(errors::VirtualBranchError::RebaseFailed); + Ok(None) => bail!("no rebase happened"), + Err(err) => { + return Err(err).context("rebase failed"); } } } @@ -3290,20 +3129,10 @@ pub fn undo_commit( project_repository: &project_repository::Repository, branch_id: BranchId, commit_oid: git::Oid, -) -> Result<(), errors::VirtualBranchError> { +) -> Result<()> { let vb_state = project_repository.project().virtual_branches(); - let mut branch = match vb_state.get_branch(branch_id) { - Ok(branch) => Ok(branch), - Err(reader::Error::NotFound) => Err(errors::VirtualBranchError::BranchNotFound( - errors::BranchNotFound { - branch_id, - project_id: project_repository.project().id, - }, - )), - Err(error) => Err(errors::VirtualBranchError::Other(error.into())), - }?; - + let mut branch = vb_state.get_branch(branch_id)?; let commit = project_repository .git_repository .find_commit(commit_oid) @@ -3327,8 +3156,9 @@ pub fn undo_commit( Ok(Some(new_head)) => { new_commit_oid = new_head.into(); } - _ => { - return Err(errors::VirtualBranchError::RebaseFailed); + Ok(None) => bail!("no rebase happened"), + Err(err) => { + return Err(err).context("rebase failed"); } } } @@ -3354,7 +3184,7 @@ pub fn cherry_rebase( target_commit_oid: git::Oid, start_commit_oid: git::Oid, end_commit_oid: git::Oid, -) -> Result, anyhow::Error> { +) -> Result> { // get a list of the commits to rebase let mut ids_to_rebase = project_repository.l( end_commit_oid, @@ -3379,7 +3209,7 @@ fn cherry_rebase_group( project_repository: &project_repository::Repository, target_commit_oid: git::Oid, ids_to_rebase: &mut [git::Oid], -) -> Result { +) -> Result { ids_to_rebase.reverse(); // now, rebase unchanged commits onto the new commit let commits_to_rebase = ids_to_rebase @@ -3445,13 +3275,9 @@ fn cherry_rebase_group( pub fn cherry_pick( project_repository: &project_repository::Repository, branch_id: BranchId, - target_commit_oid: git::Oid, -) -> Result, errors::CherryPickError> { - if conflicts::is_conflicting::<&Path>(project_repository, None)? { - return Err(errors::CherryPickError::Conflict(errors::ProjectConflict { - project_id: project_repository.project().id, - })); - } + target_commit_id: git::Oid, +) -> Result> { + project_repository.assure_unconflicted()?; let vb_state = project_repository.project().virtual_branches(); @@ -3461,15 +3287,15 @@ pub fn cherry_pick( if !branch.applied { // todo? - return Err(errors::CherryPickError::NotApplied); + bail!("can not cherry pick a branch that is not applied") } let target_commit = project_repository .git_repository - .find_commit(target_commit_oid) + .find_commit(target_commit_id) .map_err(|error| match error { - git::Error::NotFound(_) => errors::CherryPickError::CommitNotFound(target_commit_oid), - error => errors::CherryPickError::Other(error.into()), + git::Error::NotFound(_) => anyhow!("commit {target_commit_id} not found "), + err => err.into(), })?; let branch_head_commit = project_repository @@ -3487,8 +3313,7 @@ pub fn cherry_pick( .filter(|b| b.applied) .collect::>(); - let integration_commit_id = - super::integration::get_workspace_head(&vb_state, project_repository)?; + let integration_commit_id = get_workspace_head(&vb_state, project_repository)?; let (applied_statuses, _) = get_applied_status( project_repository, @@ -3624,46 +3449,29 @@ pub fn cherry_pick( Ok(commit_oid) } -/// squashes a commit from a virtual branch into it's parent. +/// squashes a commit from a virtual branch into its parent. pub fn squash( project_repository: &project_repository::Repository, branch_id: BranchId, - commit_oid: git::Oid, -) -> Result<(), errors::SquashError> { - if conflicts::is_conflicting::<&Path>(project_repository, None)? { - return Err(errors::SquashError::Conflict(errors::ProjectConflict { - project_id: project_repository.project().id, - })); - } + commit_id: git::Oid, +) -> Result<()> { + project_repository.assure_resolved()?; let vb_state = project_repository.project().virtual_branches(); - + let mut branch = vb_state.get_branch(branch_id)?; let default_target = vb_state.get_default_target()?; - - let mut branch = vb_state - .get_branch(branch_id) - .map_err(|error| match error { - reader::Error::NotFound => { - errors::SquashError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }) - } - error => errors::SquashError::Other(error.into()), - })?; - let branch_commit_oids = project_repository.l( branch.head, project_repository::LogUntil::Commit(default_target.sha), )?; - if !branch_commit_oids.contains(&commit_oid) { - return Err(errors::SquashError::CommitNotFound(commit_oid)); + if !branch_commit_oids.contains(&commit_id) { + bail!("commit {commit_id} not in the branch") } let commit_to_squash = project_repository .git_repository - .find_commit(commit_oid) + .find_commit(commit_id) .context("failed to find commit")?; let parent_commit = commit_to_squash @@ -3684,15 +3492,11 @@ pub fn squash( && !project_repository.project().ok_with_force_push { // squashing into a pushed commit will cause a force push that is not allowed - return Err(errors::SquashError::ForcePushNotAllowed( - errors::ForcePushNotAllowed { - project_id: project_repository.project().id, - }, - )); + bail!("force push not allowed"); } if !branch_commit_oids.contains(&parent_commit.id().into()) { - return Err(errors::SquashError::CantSquashRootCommit); + bail!("can not squash root commit"); } // create a commit that: @@ -3723,11 +3527,11 @@ pub fn squash( let ids_to_rebase = { let ids = branch_commit_oids - .split(|oid| oid.eq(&commit_oid)) + .split(|oid| oid.eq(&commit_id)) .collect::>(); ids.first().copied() } - .ok_or(errors::SquashError::CommitNotFound(commit_oid))?; + .with_context(|| format!("commit {commit_id} not in the branch"))?; let mut ids_to_rebase = ids_to_rebase.to_vec(); match cherry_rebase_group(project_repository, new_commit_oid, &mut ids_to_rebase) { @@ -3742,7 +3546,7 @@ pub fn squash( .context("failed to update gitbutler integration")?; Ok(()) } - _ => Err(errors::SquashError::Other(anyhow!("rebase error"))), + Err(err) => Err(err.context("rebase error").context(Code::Unknown)), } } @@ -3750,43 +3554,25 @@ pub fn squash( pub fn update_commit_message( project_repository: &project_repository::Repository, branch_id: BranchId, - commit_oid: git::Oid, + commit_id: git::Oid, message: &str, -) -> Result<(), errors::UpdateCommitMessageError> { +) -> Result<()> { if message.is_empty() { - return Err(errors::UpdateCommitMessageError::EmptyMessage); - } - - if conflicts::is_conflicting::<&Path>(project_repository, None)? { - return Err(errors::UpdateCommitMessageError::Conflict( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); + bail!("commit message can not be empty"); } + project_repository.assure_unconflicted()?; let vb_state = project_repository.project().virtual_branches(); let default_target = vb_state.get_default_target()?; - let mut branch = vb_state - .get_branch(branch_id) - .map_err(|error| match error { - reader::Error::NotFound => { - errors::UpdateCommitMessageError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id, - }) - } - error => errors::UpdateCommitMessageError::Other(error.into()), - })?; - + let mut branch = vb_state.get_branch(branch_id)?; let branch_commit_oids = project_repository.l( branch.head, project_repository::LogUntil::Commit(default_target.sha), )?; - if !branch_commit_oids.contains(&commit_oid) { - return Err(errors::UpdateCommitMessageError::CommitNotFound(commit_oid)); + if !branch_commit_oids.contains(&commit_id) { + bail!("commit {commit_id} not in the branch"); } let pushed_commit_oids = branch.upstream_head.map_or_else( @@ -3799,19 +3585,14 @@ pub fn update_commit_message( }, )?; - if pushed_commit_oids.contains(&commit_oid) && !project_repository.project().ok_with_force_push - { + if pushed_commit_oids.contains(&commit_id) && !project_repository.project().ok_with_force_push { // updating the message of a pushed commit will cause a force push that is not allowed - return Err(errors::UpdateCommitMessageError::ForcePushNotAllowed( - errors::ForcePushNotAllowed { - project_id: project_repository.project().id, - }, - )); + bail!("force push not allowed"); } let target_commit = project_repository .git_repository - .find_commit(commit_oid) + .find_commit(commit_id) .context("failed to find commit")?; let parents: Vec<_> = target_commit.parents().collect(); @@ -3833,46 +3614,34 @@ pub fn update_commit_message( let ids_to_rebase = { let ids = branch_commit_oids - .split(|oid| oid.eq(&commit_oid)) + .split(|oid| oid.eq(&commit_id)) .collect::>(); ids.first().copied() } - .ok_or(errors::UpdateCommitMessageError::CommitNotFound(commit_oid))?; + .with_context(|| format!("commit {commit_id} not in the branch"))?; let mut ids_to_rebase = ids_to_rebase.to_vec(); - match cherry_rebase_group(project_repository, new_commit_oid, &mut ids_to_rebase) { - Ok(new_head_id) => { - // save new branch head - branch.head = new_head_id; - vb_state - .set_branch(branch.clone()) - .context("failed to write branch")?; + let new_head_id = cherry_rebase_group(project_repository, new_commit_oid, &mut ids_to_rebase) + .map_err(|err| err.context("rebase error"))?; + // save new branch head + branch.head = new_head_id; + vb_state + .set_branch(branch.clone()) + .context("failed to write branch")?; - super::integration::update_gitbutler_integration(&vb_state, project_repository) - .context("failed to update gitbutler integration")?; - Ok(()) - } - _ => Err(errors::UpdateCommitMessageError::Other(anyhow!( - "rebase error" - ))), - } + super::integration::update_gitbutler_integration(&vb_state, project_repository) + .context("failed to update gitbutler integration")?; + Ok(()) } /// moves commit from the branch it's in to the top of the target branch pub fn move_commit( project_repository: &project_repository::Repository, target_branch_id: BranchId, - commit_oid: git::Oid, + commit_id: git::Oid, user: Option<&users::User>, -) -> Result<(), errors::MoveCommitError> { - if project_repository.is_resolving() { - return Err(errors::MoveCommitError::Conflicted( - errors::ProjectConflict { - project_id: project_repository.project().id, - }, - )); - } - +) -> Result<()> { + project_repository.assure_resolved()?; let vb_state = project_repository.project().virtual_branches(); let applied_branches = vb_state @@ -3883,12 +3652,7 @@ pub fn move_commit( .collect::>(); if !applied_branches.iter().any(|b| b.id == target_branch_id) { - return Err(errors::MoveCommitError::BranchNotFound( - errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id: target_branch_id, - }, - )); + bail!("branch {target_branch_id} is not among applied branches") } let default_target = vb_state.get_default_target()?; @@ -3905,14 +3669,14 @@ pub fn move_commit( let (ref mut source_branch, source_status) = applied_statuses .iter_mut() - .find(|(b, _)| b.head == commit_oid) - .ok_or_else(|| errors::MoveCommitError::CommitNotFound(commit_oid))?; + .find(|(b, _)| b.head == commit_id) + .ok_or_else(|| anyhow!("commit {commit_id} to be moved could not be found"))?; let source_branch_non_comitted_files = source_status; let source_branch_head = project_repository .git_repository - .find_commit(commit_oid) + .find_commit(commit_id) .context("failed to find commit")?; let source_branch_head_parent = source_branch_head .parent(0) @@ -3948,7 +3712,7 @@ pub fn move_commit( }); if is_source_locked { - return Err(errors::MoveCommitError::SourceLocked); + bail!("the source branch contains hunks locked to the target commit") } // move files ownerships from source branch to the destination branch @@ -3973,18 +3737,7 @@ pub fn move_commit( // move the commit to destination branch target branch { - let mut destination_branch = - vb_state - .get_branch(target_branch_id) - .map_err(|error| match error { - reader::Error::NotFound => { - errors::MoveCommitError::BranchNotFound(errors::BranchNotFound { - project_id: project_repository.project().id, - branch_id: target_branch_id, - }) - } - error => errors::MoveCommitError::Other(error.into()), - })?; + let mut destination_branch = vb_state.get_branch(target_branch_id)?; for ownership in ownerships_to_transfer { destination_branch.ownership.put(ownership); @@ -4029,12 +3782,16 @@ pub fn create_virtual_branch_from_branch( project_repository: &project_repository::Repository, upstream: &git::Refname, user: Option<&users::User>, -) -> Result { - if !matches!(upstream, git::Refname::Local(_) | git::Refname::Remote(_)) { - return Err(errors::CreateVirtualBranchFromBranchError::BranchNotFound( - upstream.clone(), - )); - } +) -> Result { + // only set upstream if it's not the default target + let upstream_branch = match upstream { + git::Refname::Other(_) | git::Refname::Virtual(_) => { + // we only support local or remote branches + bail!("branch {upstream} must be a local or remote branch"); + } + git::Refname::Remote(remote) => Some(remote.clone()), + git::Refname::Local(local) => local.remote().cloned(), + }; let branch_name = upstream .branch() @@ -4050,23 +3807,16 @@ pub fn create_virtual_branch_from_branch( let default_target = vb_state.get_default_target()?; if let git::Refname::Remote(remote_upstream) = upstream { - if default_target.branch.eq(remote_upstream) { - return Err( - errors::CreateVirtualBranchFromBranchError::CantMakeBranchFromDefaultTarget, - ); + if default_target.branch == *remote_upstream { + bail!("cannot create a branch from default target") } } let repo = &project_repository.git_repository; - let head_reference = match repo.find_reference(upstream) { - Ok(head) => Ok(head), - Err(git::Error::NotFound(_)) => Err( - errors::CreateVirtualBranchFromBranchError::BranchNotFound(upstream.clone()), - ), - Err(error) => Err(errors::CreateVirtualBranchFromBranchError::Other( - error.into(), - )), - }?; + let head_reference = repo.find_reference(upstream).map_err(|err| match err { + git::Error::NotFound(_) => anyhow!("branch {upstream} was not found"), + err => err.into(), + })?; let head_commit = head_reference .peel_to_commit() .context("failed to peel to commit")?; @@ -4087,30 +3837,10 @@ pub fn create_virtual_branch_from_branch( let now = crate::time::now_ms(); - // only set upstream if it's not the default target - let upstream_branch = match upstream { - git::Refname::Other(_) | git::Refname::Virtual(_) => { - // we only support local or remote branches - return Err(errors::CreateVirtualBranchFromBranchError::BranchNotFound( - upstream.clone(), - )); - } - git::Refname::Remote(remote) => Some(remote.clone()), - git::Refname::Local(local) => local.remote().cloned(), - }; - // add file ownership based off the diff - let target_commit = repo - .find_commit(default_target.sha) - .map_err(|error| errors::CreateVirtualBranchFromBranchError::Other(error.into()))?; - let merge_base_oid = repo - .merge_base(target_commit.id().into(), head_commit.id().into()) - .map_err(|error| errors::CreateVirtualBranchFromBranchError::Other(error.into()))?; - let merge_base_tree = repo - .find_commit(merge_base_oid) - .map_err(|error| errors::CreateVirtualBranchFromBranchError::Other(error.into()))? - .tree() - .map_err(|error| errors::CreateVirtualBranchFromBranchError::Other(error.into()))?; + let target_commit = repo.find_commit(default_target.sha)?; + let merge_base_oid = repo.merge_base(target_commit.id().into(), head_commit.id().into())?; + let merge_base_tree = repo.find_commit(merge_base_oid)?.tree()?; // do a diff between the head of this branch and the target base let diff = diff::trees( @@ -4162,13 +3892,15 @@ pub fn create_virtual_branch_from_branch( match apply_branch(project_repository, branch.id, user) { Ok(_) => Ok(branch.id), - Err(errors::ApplyBranchError::BranchConflicts(_)) => { + Err(err) + if err + .downcast_ref() + .map_or(false, |marker: &Marker| *marker == Marker::ProjectConflict) => + { // if branch conflicts with the workspace, it's ok. keep it unapplied Ok(branch.id) } - Err(error) => Err(errors::CreateVirtualBranchFromBranchError::ApplyBranch( - error, - )), + Err(err) => Err(err).context("failed to apply"), } } @@ -4295,7 +4027,7 @@ mod tests { // let conflicts = merge_index.conflicts()?; // let conflict_message = conflicts_to_string(conflicts)?; // return Err(anyhow!("Merge failed") -// .context(error::Context::new(Code::ProjectConflict, conflict_message)) +// .context(error::Context::new(Marker::ProjectConflict, conflict_message)) // .); // fn conflicts_to_string(conflicts: IndexConflicts) -> Result { diff --git a/crates/gitbutler-core/src/zip/controller.rs b/crates/gitbutler-core/src/zip/controller.rs index eb44a4f984..1fe2b74ae1 100644 --- a/crates/gitbutler-core/src/zip/controller.rs +++ b/crates/gitbutler-core/src/zip/controller.rs @@ -1,4 +1,4 @@ -use crate::error::Error; +use anyhow::Result; use std::path; use super::Zipper; @@ -28,12 +28,12 @@ impl Controller { } } - pub fn archive(&self, project_id: ProjectId) -> Result { + pub fn archive(&self, project_id: ProjectId) -> Result { let project = self.projects_controller.get(project_id)?; self.zipper.zip(project.path).map_err(Into::into) } - pub fn data_archive(&self, project_id: ProjectId) -> Result { + pub fn data_archive(&self, project_id: ProjectId) -> Result { let project = self.projects_controller.get(project_id)?; self.zipper .zip( @@ -44,7 +44,7 @@ impl Controller { .map_err(Into::into) } - pub fn logs_archive(&self) -> anyhow::Result { + pub fn logs_archive(&self) -> Result { self.zipper.zip(&self.logs_dir).map_err(Into::into) } } diff --git a/crates/gitbutler-core/tests/core.rs b/crates/gitbutler-core/tests/core.rs index 45353094fd..22e13a0fb8 100644 --- a/crates/gitbutler-core/tests/core.rs +++ b/crates/gitbutler-core/tests/core.rs @@ -3,7 +3,6 @@ mod suite { mod virtual_branches; } -mod error; mod git; mod keys; mod lock; diff --git a/crates/gitbutler-core/tests/error/mod.rs b/crates/gitbutler-core/tests/error/mod.rs deleted file mode 100644 index 5b7e64d541..0000000000 --- a/crates/gitbutler-core/tests/error/mod.rs +++ /dev/null @@ -1,49 +0,0 @@ -mod into_anyhow { - use gitbutler_core::error::{into_anyhow, Code, Context, ErrorWithContext}; - - #[test] - fn code_as_context() { - #[derive(thiserror::Error, Debug)] - #[error("err")] - struct Error(Code); - - impl ErrorWithContext for Error { - fn context(&self) -> Option { - Context::from(self.0).into() - } - } - - let err = into_anyhow(Error(Code::Projects)); - let ctx = err.downcast_ref::().unwrap(); - assert_eq!(ctx.code, Code::Projects, "the context is attached"); - assert_eq!( - ctx.message, None, - "there is no message when context was created from bare code" - ); - } - - #[test] - fn nested_code_as_context() { - #[derive(thiserror::Error, Debug)] - #[error("Err")] - struct Inner(Code); - - #[derive(thiserror::Error, Debug)] - #[error(transparent)] - struct Outer(#[from] Inner); - - impl ErrorWithContext for Outer { - fn context(&self) -> Option { - Context::from(self.0 .0).into() - } - } - - let err = into_anyhow(Outer::from(Inner(Code::Projects))); - let ctx = err.downcast_ref::().unwrap(); - assert_eq!( - ctx.code, - Code::Projects, - "there is no magic here, it's all about manually implementing the nesting :/" - ); - } -} diff --git a/crates/gitbutler-core/tests/suite/projects.rs b/crates/gitbutler-core/tests/suite/projects.rs index dd8d98e5e3..4f25e03ece 100644 --- a/crates/gitbutler-core/tests/suite/projects.rs +++ b/crates/gitbutler-core/tests/suite/projects.rs @@ -23,40 +23,39 @@ mod add { } mod error { - use gitbutler_core::projects::AddError; - use super::*; #[test] fn missing() { let (controller, _tmp) = new(); let tmp = tempfile::tempdir().unwrap(); - assert!(matches!( - controller.add(tmp.path().join("missing")), - Err(AddError::PathNotFound) - )); + assert_eq!( + controller + .add(tmp.path().join("missing")) + .unwrap_err() + .to_string(), + "path not found" + ); } #[test] - fn not_git() { + fn directory_without_git() { let (controller, _tmp) = new(); let tmp = tempfile::tempdir().unwrap(); let path = tmp.path(); std::fs::write(path.join("file.txt"), "hello world").unwrap(); - assert!(matches!( - controller.add(path), - Err(AddError::NotAGitRepository(_)) - )); + assert_eq!( + controller.add(path).unwrap_err().to_string(), + "must be a Git repository" + ); } #[test] fn empty() { let (controller, _tmp) = new(); let tmp = tempfile::tempdir().unwrap(); - assert!(matches!( - controller.add(tmp.path()), - Err(AddError::NotAGitRepository(_)) - )); + let err = controller.add(tmp.path()).unwrap_err(); + assert_eq!(err.to_string(), "must be a Git repository"); } #[test] @@ -65,7 +64,10 @@ mod add { let repository = gitbutler_testsupport::TestProject::default(); let path = repository.path(); controller.add(path).unwrap(); - assert!(matches!(controller.add(path), Err(AddError::AlreadyExists))); + assert_eq!( + controller.add(path).unwrap_err().to_string(), + "project already exists" + ); } #[test] @@ -78,7 +80,7 @@ mod add { create_initial_commit(&repo); let err = controller.add(repo_dir).unwrap_err(); - assert!(matches!(err, AddError::BareUnsupported)); + assert_eq!(err.to_string(), "bare repositories are unsupported"); } #[test] @@ -93,7 +95,7 @@ mod add { let worktree = repo.worktree("feature", &worktree_dir, None).unwrap(); let err = controller.add(worktree.path()).unwrap_err(); - assert_eq!(err.to_string(), "worktrees unsupported"); + assert_eq!(err.to_string(), "can only work in main worktrees"); } fn create_initial_commit(repo: &git2::Repository) -> git2::Oid { diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/amend.rs b/crates/gitbutler-core/tests/suite/virtual_branches/amend.rs index 88067cd0b6..ab31ced238 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/amend.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/amend.rs @@ -118,14 +118,14 @@ async fn forcepush_forbidden() { { fs::write(repository.path().join("file2.txt"), "content2").unwrap(); let to_amend: branch::BranchOwnershipClaims = "file2.txt:1-2".parse().unwrap(); - assert!(matches!( + assert_eq!( controller .amend(*project_id, branch_id, commit_oid, &to_amend) .await .unwrap_err() - .downcast_ref(), - Some(errors::VirtualBranchError::ForcePushNotAllowed(_)) - )); + .to_string(), + "force-push is not allowed" + ); } } @@ -301,13 +301,13 @@ async fn non_existing_ownership() { { // amend non existing hunk let to_amend: branch::BranchOwnershipClaims = "file2.txt:1-2".parse().unwrap(); - assert!(matches!( + assert_eq!( controller .amend(*project_id, branch_id, commit_oid, &to_amend) .await .unwrap_err() - .downcast_ref(), - Some(errors::VirtualBranchError::TargetOwnerhshipNotFound(_)) - )); + .to_string(), + "target ownership not found" + ); } } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/apply_virtual_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/apply_virtual_branch.rs index 6f85b98d27..3b9bf6db00 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/apply_virtual_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/apply_virtual_branch.rs @@ -52,7 +52,7 @@ async fn deltect_conflict() { .await .unwrap_err() .downcast_ref(), - Some(errors::ApplyBranchError::BranchConflicts(_)) + Some(Marker::ProjectConflict) )); } } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/cherry_pick.rs b/crates/gitbutler-core/tests/suite/virtual_branches/cherry_pick.rs index 30d72220fc..24769965ec 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/cherry_pick.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/cherry_pick.rs @@ -212,14 +212,14 @@ mod cleanly { .await .unwrap(); - assert!(matches!( + assert_eq!( controller .cherry_pick(*project_id, branch_id, commit_three_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::CherryPickError::NotApplied) - )); + .to_string(), + "can not cherry pick a branch that is not applied" + ); } } @@ -374,13 +374,13 @@ mod with_conflicts { .await .unwrap(); - assert!(matches!( + assert_eq!( controller .cherry_pick(*project_id, branch_id, commit_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::CherryPickError::NotApplied) - )); + .to_string(), + "can not cherry pick a branch that is not applied" + ); } } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/create_virtual_branch_from_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/create_virtual_branch_from_branch.rs index 1c17e7c871..1051c43d35 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/create_virtual_branch_from_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/create_virtual_branch_from_branch.rs @@ -293,7 +293,7 @@ async fn from_default_target() { // branch should be created unapplied, because of the conflict - assert!(matches!( + assert_eq!( controller .create_virtual_branch_from_branch( *project_id, @@ -301,9 +301,9 @@ async fn from_default_target() { ) .await .unwrap_err() - .downcast_ref(), - Some(errors::CreateVirtualBranchFromBranchError::CantMakeBranchFromDefaultTarget) - )); + .to_string(), + "cannot create a branch from default target" + ); } #[tokio::test] @@ -321,7 +321,7 @@ async fn from_non_existent_branch() { // branch should be created unapplied, because of the conflict - assert!(matches!( + assert_eq!( controller .create_virtual_branch_from_branch( *project_id, @@ -329,11 +329,9 @@ async fn from_non_existent_branch() { ) .await .unwrap_err() - .downcast_ref(), - Some(errors::CreateVirtualBranchFromBranchError::BranchNotFound( - _ - )) - )); + .to_string(), + "branch refs/remotes/origin/branch was not found" + ); } #[tokio::test] diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs index d54152a151..0391fbd053 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs @@ -1,11 +1,12 @@ use std::path::PathBuf; use std::{fs, path, str::FromStr}; +use gitbutler_core::virtual_branches::errors::Marker; use gitbutler_core::{ git, projects::{self, Project, ProjectId}, users, - virtual_branches::{branch, errors, Controller}, + virtual_branches::{branch, Controller}, }; use tempfile::TempDir; @@ -169,7 +170,7 @@ async fn resolve_conflict_flow() { .await .unwrap_err() .downcast_ref(), - Some(errors::CommitError::Conflicted(_)) + Some(Marker::ProjectConflict) )); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_to_vbranch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_to_vbranch.rs index 3a8723c393..010b81a84e 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_to_vbranch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/move_commit_to_vbranch.rs @@ -2,7 +2,7 @@ use std::str::FromStr; use gitbutler_core::{ git, - virtual_branches::{branch, errors, BranchId}, + virtual_branches::{branch, BranchId}, }; use crate::suite::virtual_branches::Test; @@ -238,14 +238,14 @@ async fn locked_hunks_on_source_branch() { .await .unwrap(); - assert!(matches!( + assert_eq!( controller .move_commit(*project_id, target_branch_id, commit_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::MoveCommitError::SourceLocked) - )); + .to_string(), + "the source branch contains hunks locked to the target commit" + ); } #[tokio::test] @@ -279,18 +279,19 @@ async fn no_commit() { .await .unwrap(); - assert!(matches!( + let commit_id_hex = "a99c95cca7a60f1a2180c2f86fb18af97333c192"; + assert_eq!( controller .move_commit( *project_id, target_branch_id, - git::Oid::from_str("a99c95cca7a60f1a2180c2f86fb18af97333c192").unwrap() + git::Oid::from_str(commit_id_hex).unwrap() ) .await .unwrap_err() - .downcast_ref(), - Some(errors::MoveCommitError::CommitNotFound(_)) - )); + .to_string(), + format!("commit {commit_id_hex} to be moved could not be found") + ); } #[tokio::test] @@ -319,12 +320,13 @@ async fn no_branch() { .await .unwrap(); - assert!(matches!( + let id = BranchId::generate(); + assert_eq!( controller - .move_commit(*project_id, BranchId::generate(), commit_oid) + .move_commit(*project_id, id, commit_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::MoveCommitError::BranchNotFound(_)) - )); + .to_string(), + format!("branch {id} is not among applied branches") + ); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/reset_virtual_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/reset_virtual_branch.rs index d2ecf276cb..8625c9afba 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/reset_virtual_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/reset_virtual_branch.rs @@ -1,6 +1,6 @@ use std::fs; -use gitbutler_core::virtual_branches::{branch, errors::ResetBranchError}; +use gitbutler_core::virtual_branches::branch; use crate::suite::virtual_branches::Test; @@ -252,7 +252,7 @@ async fn to_non_existing() { oid }; - assert!(matches!( + assert_eq!( controller .reset_virtual_branch( *project_id, @@ -261,7 +261,7 @@ async fn to_non_existing() { ) .await .unwrap_err() - .downcast_ref(), - Some(ResetBranchError::CommitNotFoundInBranch(_)) - )); + .to_string(), + "commit fe14df8c66b73c6276f7bb26102ad91da680afcb not in the branch" + ); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/set_base_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/set_base_branch.rs index 94fd4f7896..6b7bd2c824 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/set_base_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/set_base_branch.rs @@ -25,7 +25,7 @@ mod error { .. } = &Test::default(); - assert!(matches!( + assert_eq!( controller .set_base_branch( *project_id, @@ -33,9 +33,9 @@ mod error { ) .await .unwrap_err() - .downcast_ref(), - Some(errors::SetBaseBranchError::BranchNotFound(_)) - )); + .to_string(), + "remote branch 'refs/remotes/origin/missing' not found" + ); } } @@ -123,7 +123,7 @@ mod go_back_to_integration { .await .unwrap_err() .downcast_ref(), - Some(errors::SetBaseBranchError::DirtyWorkingDirectory) + Some(Marker::ProjectConflict) )); } @@ -159,7 +159,7 @@ mod go_back_to_integration { .await .unwrap_err() .downcast_ref(), - Some(errors::SetBaseBranchError::DirtyWorkingDirectory) + Some(Marker::ProjectConflict) )); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/squash.rs b/crates/gitbutler-core/tests/suite/virtual_branches/squash.rs index 7f331da5ee..b9590c1848 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/squash.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/squash.rs @@ -310,18 +310,18 @@ async fn forcepush_forbidden() { .unwrap() }; - assert!(matches!( + assert_eq!( controller .squash(*project_id, branch_id, commit_two_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::SquashError::ForcePushNotAllowed(_)) - )); + .to_string(), + "force push not allowed" + ); } #[tokio::test] -async fn root() { +async fn root_forbidden() { let Test { repository, project_id, @@ -347,12 +347,12 @@ async fn root() { .unwrap() }; - assert!(matches!( + assert_eq!( controller .squash(*project_id, branch_id, commit_one_oid) .await .unwrap_err() - .downcast_ref(), - Some(errors::SquashError::CantSquashRootCommit) - )); + .to_string(), + "can not squash root commit" + ); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs b/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs index b833ca48fe..55af136efe 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/update_commit_message.rs @@ -260,14 +260,14 @@ async fn forcepush_forbidden() { .await .unwrap(); - assert!(matches!( + assert_eq!( controller .update_commit_message(*project_id, branch_id, commit_one_oid, "commit one updated",) .await .unwrap_err() - .downcast_ref(), - Some(errors::UpdateCommitMessageError::ForcePushNotAllowed(_)) - )); + .to_string(), + "force push not allowed" + ); } #[tokio::test] @@ -365,12 +365,12 @@ async fn empty() { .unwrap() }; - assert!(matches!( + assert_eq!( controller .update_commit_message(*project_id, branch_id, commit_one_oid, "",) .await .unwrap_err() - .downcast_ref(), - Some(errors::UpdateCommitMessageError::EmptyMessage) - )); + .to_string(), + "commit message can not be empty" + ); } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/verify_branch.rs b/crates/gitbutler-core/tests/suite/virtual_branches/verify_branch.rs index 4f21514384..1c29f70b9d 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/verify_branch.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/verify_branch.rs @@ -1,5 +1,3 @@ -use gitbutler_core::virtual_branches::errors::VerifyError; - use super::*; // Ensures that `verify_branch` returns an error when not on the integration branch. @@ -16,10 +14,9 @@ async fn should_fail_on_incorrect_branch() { repository.checkout(&branch_name); let result = controller.list_virtual_branches(*project_id).await; - let error = result.err(); - assert!(&error.is_some()); - - let error = error.unwrap(); - let error = error.downcast_ref::(); - assert!(matches!(error, Some(VerifyError::InvalidHead(_)))); + let err = result.unwrap_err(); + assert_eq!( + format!("{err:#}"), + ": project is on refs/heads/somebranch. Please checkout gitbutler/integration to continue" + ); } diff --git a/crates/gitbutler-core/tests/virtual_branches/mod.rs b/crates/gitbutler-core/tests/virtual_branches/mod.rs index 1761151643..992c4b54d8 100644 --- a/crates/gitbutler-core/tests/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/virtual_branches/mod.rs @@ -19,9 +19,7 @@ use gitbutler_core::{ virtual_branches::{ self, apply_branch, branch::{BranchCreateRequest, BranchOwnershipClaims}, - commit, create_virtual_branch, - errors::CommitError, - integrate_upstream_commits, + commit, create_virtual_branch, integrate_upstream_commits, integration::verify_branch, is_remote_branch_mergeable, is_virtual_branch_mergeable, list_remote_branches, unapply_ownership, update_branch, @@ -2094,8 +2092,8 @@ fn verify_branch_not_integration() -> Result<()> { let verify_result = verify_branch(project_repository); assert!(verify_result.is_err()); assert_eq!( - verify_result.unwrap_err().to_string(), - "head is refs/heads/master" + format!("{:#}", verify_result.unwrap_err()), + ": project is on refs/heads/master. Please checkout gitbutler/integration to continue" ); Ok(()) @@ -2144,15 +2142,8 @@ fn pre_commit_hook_rejection() -> Result<()> { true, ); - let error = res.unwrap_err(); - - assert!(matches!(error, CommitError::CommitHookRejected(_))); - - let CommitError::CommitHookRejected(output) = error else { - unreachable!() - }; - - assert_eq!(&output, "rejected\n"); + let err = res.unwrap_err(); + assert_eq!(err.to_string(), "commit hook rejected: rejected"); Ok(()) } @@ -2256,15 +2247,8 @@ fn commit_msg_hook_rejection() -> Result<()> { true, ); - let error = res.unwrap_err(); - - assert!(matches!(error, CommitError::CommitMsgHookRejected(_))); - - let CommitError::CommitMsgHookRejected(output) = error else { - unreachable!() - }; - - assert_eq!(&output, "rejected\n"); + let err = res.unwrap_err(); + assert_eq!(err.to_string(), "commit-msg hook rejected: rejected"); Ok(()) } @@ -2276,8 +2260,6 @@ where tree.walk(git2::TreeWalkMode::PreOrder, |root, entry| { match callback(root, &entry.clone()) { TreeWalkResult::Continue => git2::TreeWalkResult::Ok, - // TreeWalkResult::Skip => git2::TreeWalkResult::Skip, - // TreeWalkResult::Stop => git2::TreeWalkResult::Abort, } }) .map_err(Into::into) @@ -2285,6 +2267,4 @@ where enum TreeWalkResult { Continue, - // Skip, - // Stop, } diff --git a/crates/gitbutler-tauri/src/app.rs b/crates/gitbutler-tauri/src/app.rs index b36ac3e980..09121a0913 100644 --- a/crates/gitbutler-tauri/src/app.rs +++ b/crates/gitbutler-tauri/src/app.rs @@ -1,5 +1,4 @@ use anyhow::{Context, Result}; -use gitbutler_core::error::Error as CoreError; use gitbutler_core::{ git, project_repository::{self, conflicts}, @@ -17,7 +16,7 @@ impl App { Self { projects } } - pub fn mark_resolved(&self, project_id: ProjectId, path: &str) -> Result<(), CoreError> { + pub fn mark_resolved(&self, project_id: ProjectId, path: &str) -> Result<()> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; // mark file as resolved @@ -25,13 +24,10 @@ impl App { Ok(()) } - pub fn git_remote_branches( - &self, - project_id: ProjectId, - ) -> Result, CoreError> { + pub fn git_remote_branches(&self, project_id: ProjectId) -> Result> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(project_repository.git_remote_branches()?) + project_repository.git_remote_branches() } pub fn git_test_push( @@ -41,10 +37,10 @@ impl App { branch_name: &str, credentials: &git::credentials::Helper, askpass: Option>, - ) -> Result<(), CoreError> { + ) -> Result<()> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(project_repository.git_test_push(credentials, remote_name, branch_name, askpass)?) + project_repository.git_test_push(credentials, remote_name, branch_name, askpass) } pub fn git_test_fetch( @@ -53,13 +49,13 @@ impl App { remote_name: &str, credentials: &git::credentials::Helper, askpass: Option, - ) -> Result<(), CoreError> { + ) -> Result<()> { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; - Ok(project_repository.fetch(remote_name, credentials, askpass)?) + project_repository.fetch(remote_name, credentials, askpass) } - pub fn git_index_size(&self, project_id: ProjectId) -> Result { + pub fn git_index_size(&self, project_id: ProjectId) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; let size = project_repository @@ -68,7 +64,7 @@ impl App { Ok(size) } - pub fn git_head(&self, project_id: ProjectId) -> Result { + pub fn git_head(&self, project_id: ProjectId) -> Result { let project = self.projects.get(project_id)?; let project_repository = project_repository::Repository::open(&project)?; let head = project_repository @@ -98,7 +94,7 @@ impl App { } } - pub async fn delete_all_data(&self) -> Result<(), CoreError> { + pub async fn delete_all_data(&self) -> Result<()> { for project in self.projects.list().context("failed to list projects")? { self.projects .delete(project.id) diff --git a/crates/gitbutler-tauri/src/error.rs b/crates/gitbutler-tauri/src/error.rs index a8d13d5b05..ca8b6814a8 100644 --- a/crates/gitbutler-tauri/src/error.rs +++ b/crates/gitbutler-tauri/src/error.rs @@ -1,6 +1,12 @@ -//! ## How-To +//! Utilities to control which errors show in the frontend. //! -//! This is a primer on how to use the [`Error`] provided here. +//! ## How to use this +//! +//! Just make sure this [`Error`] type is used for each provided `tauri` command. The rest happens automatically +//! such that: +//! +//! * The frontend shows the root error as string by default… +//! * …or it shows the provided [`Context`](gitbutler_core::error::Context) as controlled by the `core` crate. //! //! ### Interfacing with `tauri` using [`Error`] //! @@ -10,17 +16,10 @@ //! //! The values in these fields are controlled by attaching context, please [see the `core` docs](gitbutler_core::error)) //! on how to do this. -//! -//! To assure context is picked up correctly to be made available to the UI if present, use -//! [`Error`] in the `tauri` commands. Due to technical limitations, it will only auto-convert -//! from `anyhow::Error`, or [core::Error](gitbutler_core::error::Error). -//! Errors that are known to have context can be converted using [`Error::from_error_with_context`]. -//! If there is an error without context, one would have to convert to `anyhow::Error` as intermediate step, -//! typically by adding `.context()`. pub(crate) use frontend::Error; mod frontend { - use gitbutler_core::error::{into_anyhow, AnyhowContextExt, ErrorWithContext}; + use gitbutler_core::error::AnyhowContextExt; use serde::{ser::SerializeMap, Serialize}; use std::borrow::Cow; @@ -35,27 +34,12 @@ mod frontend { } } - impl From for Error { - fn from(value: gitbutler_core::error::Error) -> Self { - Self(value.into()) - } - } - - impl Error { - /// Convert an error with context to our type. - /// - /// Note that this is only needed as trait specialization isn't working well enough yet. - pub fn from_error_with_context(err: impl ErrorWithContext + Send + Sync + 'static) -> Self { - Self(into_anyhow(err)) - } - } - impl Serialize for Error { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, { - let ctx = self.0.custom_context().unwrap_or_default(); + let ctx = self.0.custom_context_or_root_cause(); let mut map = serializer.serialize_map(Some(2))?; map.serialize_entry("code", &ctx.code.to_string())?; @@ -81,41 +65,76 @@ mod frontend { } #[test] - fn no_context_or_code() { + fn no_context_or_code_shows_root_error() { let err = anyhow!("err msg"); + assert_eq!( + format!("{:#}", err), + "err msg", + "just one error on display here" + ); assert_eq!( json(err), - "{\"code\":\"errors.unknown\",\"message\":\"Something went wrong\"}", - "if there is no explicit error code or context, the original error message isn't shown" + "{\"code\":\"errors.unknown\",\"message\":\"err msg\"}", + "if there is no explicit error code or context, the original error message is shown" ); } #[test] fn find_code() { - let err = anyhow!("err msg").context(Code::Projects); + let err = anyhow!("err msg").context(Code::Validation); + assert_eq!( + format!("{:#}", err), + "errors.validation: err msg", + "note how the context becomes an error, in front of the original one" + ); assert_eq!( json(err), - "{\"code\":\"errors.projects\",\"message\":\"err msg\"}", + "{\"code\":\"errors.validation\",\"message\":\"err msg\"}", "the 'code' is available as string, but the message is taken from the source error" ); } + #[test] + fn find_code_after_cause() { + let original_err = std::io::Error::new(std::io::ErrorKind::Other, "actual cause"); + let err = anyhow::Error::from(original_err) + .context("err msg") + .context(Code::Validation); + + assert_eq!( + format!("{:#}", err), + "errors.validation: err msg: actual cause", + "an even longer chain, with the cause as root as one might expect" + ); + assert_eq!( + json(err), + "{\"code\":\"errors.validation\",\"message\":\"err msg\"}", + "in order to attach a custom message to an original cause, our messaging (and Code) is the tail" + ); + } + #[test] fn find_context() { - let err = anyhow!("err msg").context(Context::new_static(Code::Projects, "ctx msg")); + let err = anyhow!("err msg").context(Context::new_static(Code::Validation, "ctx msg")); + assert_eq!(format!("{:#}", err), "ctx msg: err msg"); assert_eq!( json(err), - "{\"code\":\"errors.projects\",\"message\":\"ctx msg\"}", + "{\"code\":\"errors.validation\",\"message\":\"ctx msg\"}", "Contexts often provide their own message, so the error message is ignored" ); } #[test] fn find_context_without_message() { - let err = anyhow!("err msg").context(Context::from(Code::Projects)); + let err = anyhow!("err msg").context(Context::from(Code::Validation)); + assert_eq!( + format!("{:#}", err), + "Something went wrong: err msg", + "on display, `Context` does just insert a generic message" + ); assert_eq!( json(err), - "{\"code\":\"errors.projects\",\"message\":\"err msg\"}", + "{\"code\":\"errors.validation\",\"message\":\"err msg\"}", "Contexts without a message show the error's message as well" ); } @@ -124,10 +143,15 @@ mod frontend { fn find_nested_code() { let err = anyhow!("bottom msg") .context("top msg") - .context(Code::Projects); + .context(Code::Validation); + assert_eq!( + format!("{:#}", err), + "errors.validation: top msg: bottom msg", + "now it's clear why bottom is bottom" + ); assert_eq!( json(err), - "{\"code\":\"errors.projects\",\"message\":\"top msg\"}", + "{\"code\":\"errors.validation\",\"message\":\"top msg\"}", "the 'code' gets the message of the error that it provides context to, and it finds it down the chain" ); } @@ -135,12 +159,17 @@ mod frontend { #[test] fn multiple_codes() { let err = anyhow!("bottom msg") - .context(Code::Menu) + .context(Code::ProjectGitAuth) .context("top msg") - .context(Code::Projects); + .context(Code::Validation); + assert_eq!( + format!("{:#}", err), + "errors.validation: top msg: errors.projects.git.auth: bottom msg", + "each code is treated like its own error in the chain" + ); assert_eq!( json(err), - "{\"code\":\"errors.projects\",\"message\":\"top msg\"}", + "{\"code\":\"errors.validation\",\"message\":\"top msg\"}", "it finds the most recent 'code' (and the same would be true for contexts, of course)" ); } diff --git a/crates/gitbutler-tauri/src/menu.rs b/crates/gitbutler-tauri/src/menu.rs index 3bc484cff7..11c12c8f0f 100644 --- a/crates/gitbutler-tauri/src/menu.rs +++ b/crates/gitbutler-tauri/src/menu.rs @@ -27,9 +27,7 @@ pub async fn menu_item_set_enabled( let menu_item = window .menu_handle() .try_get_item(menu_item_id) - .with_context(|| { - error::Context::new(Code::Menu, format!("menu item not found: {}", menu_item_id)) - })?; + .with_context(|| error::Context::new(format!("menu item not found: {}", menu_item_id)))?; menu_item.set_enabled(enabled).context(Code::Unknown)?; diff --git a/crates/gitbutler-tauri/src/projects.rs b/crates/gitbutler-tauri/src/projects.rs index bfe89734b7..d7656fea1c 100644 --- a/crates/gitbutler-tauri/src/projects.rs +++ b/crates/gitbutler-tauri/src/projects.rs @@ -2,7 +2,6 @@ pub mod commands { use anyhow::Context; use std::path; - use gitbutler_core::error::Code; use gitbutler_core::projects::{self, controller::Controller, ProjectId}; use tauri::Manager; use tracing::instrument; @@ -16,11 +15,7 @@ pub mod commands { handle: tauri::AppHandle, project: projects::UpdateRequest, ) -> Result { - handle - .state::() - .update(&project) - .await - .map_err(Error::from_error_with_context) + Ok(handle.state::().update(&project).await?) } #[tauri::command(async)] @@ -29,10 +24,7 @@ pub mod commands { handle: tauri::AppHandle, path: &path::Path, ) -> Result { - handle - .state::() - .add(path) - .map_err(Error::from_error_with_context) + Ok(handle.state::().add(path)?) } #[tauri::command(async)] @@ -41,10 +33,7 @@ pub mod commands { handle: tauri::AppHandle, id: ProjectId, ) -> Result { - handle - .state::() - .get(id) - .map_err(Error::from_error_with_context) + Ok(handle.state::().get(id)?) } #[tauri::command(async)] @@ -83,10 +72,7 @@ pub mod commands { id: ProjectId, key: &str, ) -> Result, Error> { - Ok(handle - .state::() - .get_local_config(id, key) - .context(Code::Projects)?) + Ok(handle.state::().get_local_config(id, key)?) } #[tauri::command(async)] @@ -97,9 +83,9 @@ pub mod commands { key: &str, value: &str, ) -> Result<(), Error> { - Ok(handle + handle .state::() .set_local_config(id, key, value) - .context(Code::Projects)?) + .map_err(Into::into) } } diff --git a/crates/gitbutler-testsupport/src/test_project.rs b/crates/gitbutler-testsupport/src/test_project.rs index 925dd9672c..b4c36292ae 100644 --- a/crates/gitbutler-testsupport/src/test_project.rs +++ b/crates/gitbutler-testsupport/src/test_project.rs @@ -107,8 +107,10 @@ impl TestProject { .unwrap(); } + /// ```text /// git add -A /// git reset --hard + /// ``` pub fn reset_hard(&self, oid: Option) { let mut index = self.local_repository.index().expect("failed to get index"); index diff --git a/crates/gitbutler-watcher/src/handler.rs b/crates/gitbutler-watcher/src/handler.rs index 3b84377b19..d234f33666 100644 --- a/crates/gitbutler-watcher/src/handler.rs +++ b/crates/gitbutler-watcher/src/handler.rs @@ -99,8 +99,15 @@ impl Handler { }, }) } - Err(err) if err.is::() => Ok(()), - Err(err) => Err(err.context("failed to list virtual branches").into()), + Err(err) + if matches!( + err.downcast_ref::(), + Some(virtual_branches::errors::Marker::VerificationFailure) + ) => + { + Ok(()) + } + Err(err) => Err(err.context("failed to list virtual branches")), } }