Merge pull request #3920 from gitbutlerapp/snapshots-save-errors

snapshots save errors

crates/gitbutler-core/src/error.rs

@@ -120,7 +120,7 @@
 //! 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()`].
 use std::borrow::Cow;
-use std::fmt::{Debug, Display};
+use std::fmt::{self, Debug, Display};
 
 /// A unique code that consumers of the API may rely on to identify errors.
 #[derive(Debug, Default, Copy, Clone, PartialOrd, PartialEq)]
@@ -280,6 +280,12 @@ impl From<Error> for anyhow::Error {
     }
 }
 
+impl Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        Display::fmt(&self.0, f)
+    }
+}
+
 impl<E> From<E> for Error
 where
     E: ErrorWithContext + Send + Sync + 'static,

crates/gitbutler-core/src/ops/snapshot.rs

@@ -1,3 +1,4 @@
+use crate::error::Error;
 use std::vec;
 
 use crate::projects::Project;
@@ -11,49 +12,63 @@ use super::entry::Trailer;
 
 /// Snapshot functionality
 impl Project {
-    pub(crate) fn snapshot_branch_applied(&self, branch_name: String) -> anyhow::Result<()> {
-        let details =
-            SnapshotDetails::new(OperationKind::ApplyBranch).with_trailers(vec![Trailer {
-                key: "name".to_string(),
-                value: branch_name,
-            }]);
-        self.create_snapshot(details)?;
+    pub(crate) fn snapshot_branch_applied(
+        &self,
+        snapshot_tree: git::Oid,
+        result: Result<&String, &Error>,
+    ) -> anyhow::Result<()> {
+        let result = result.map(|o| Some(o.clone()));
+        let details = SnapshotDetails::new(OperationKind::ApplyBranch)
+            .with_trailers(result_trailer(result, "name".to_string()));
+        self.commit_snapshot(snapshot_tree, details)?;
         Ok(())
     }
-    pub(crate) fn snapshot_branch_unapplied(&self, branch_name: String) -> anyhow::Result<()> {
-        let details =
-            SnapshotDetails::new(OperationKind::UnapplyBranch).with_trailers(vec![Trailer {
-                key: "name".to_string(),
-                value: branch_name,
-            }]);
-        self.create_snapshot(details)?;
+    pub(crate) fn snapshot_branch_unapplied(
+        &self,
+        snapshot_tree: git::Oid,
+        result: Result<&Option<Branch>, &Error>,
+    ) -> anyhow::Result<()> {
+        let result = result.map(|o| o.clone().map(|b| b.name));
+        let details = SnapshotDetails::new(OperationKind::UnapplyBranch)
+            .with_trailers(result_trailer(result, "name".to_string()));
+        self.commit_snapshot(snapshot_tree, details)?;
         Ok(())
     }
-    pub(crate) fn snapshot_commit_undo(&self, commit_sha: git::Oid) -> anyhow::Result<()> {
-        let details =
-            SnapshotDetails::new(OperationKind::UndoCommit).with_trailers(vec![Trailer {
-                key: "sha".to_string(),
-                value: commit_sha.to_string(),
-            }]);
-        self.create_snapshot(details)?;
+    pub(crate) fn snapshot_commit_undo(
+        &self,
+        snapshot_tree: git::Oid,
+        result: Result<&(), &Error>,
+        commit_sha: git::Oid,
+    ) -> anyhow::Result<()> {
+        let result = result.map(|_| Some(commit_sha.to_string()));
+        let details = SnapshotDetails::new(OperationKind::UndoCommit)
+            .with_trailers(result_trailer(result, "sha".to_string()));
+        self.commit_snapshot(snapshot_tree, details)?;
         Ok(())
     }
     pub(crate) fn snapshot_commit_creation(
         &self,
         snapshot_tree: git::Oid,
+        error: Option<&Error>,
         commit_message: String,
         sha: Option<git::Oid>,
     ) -> anyhow::Result<()> {
-        let details = SnapshotDetails::new(OperationKind::CreateCommit).with_trailers(vec![
-            Trailer {
-                key: "message".to_string(),
-                value: commit_message,
-            },
-            Trailer {
-                key: "sha".to_string(),
-                value: sha.map(|sha| sha.to_string()).unwrap_or_default(),
-            },
-        ]);
+        let details = SnapshotDetails::new(OperationKind::CreateCommit).with_trailers(
+            [
+                vec![
+                    Trailer {
+                        key: "message".to_string(),
+                        value: commit_message,
+                    },
+                    Trailer {
+                        key: "sha".to_string(),
+                        value: sha.map(|sha| sha.to_string()).unwrap_or_default(),
+                    },
+                ],
+                error_trailer(error),
+            ]
+            .concat(),
+        );
         self.commit_snapshot(snapshot_tree, details)?;
         Ok(())
     }
@@ -146,3 +161,33 @@ impl Project {
         Ok(())
     }
 }
+
+fn result_trailer(result: Result<Option<String>, &Error>, key: String) -> Vec<Trailer> {
+    match result {
+        Ok(v) => {
+            if let Some(v) = v {
+                vec![Trailer {
+                    key,
+                    value: v.clone(),
+                }]
+            } else {
+                vec![]
+            }
+        }
+        Err(error) => vec![Trailer {
+            key: "error".to_string(),
+            value: error.to_string(),
+        }],
+    }
+}
+
+fn error_trailer(error: Option<&Error>) -> Vec<Trailer> {
+    error
+        .map(|e| {
+            vec![Trailer {
+                key: "error".to_string(),
+                value: e.to_string(),
+            }]
+        })
+        .unwrap_or_default()
+}

crates/gitbutler-core/src/virtual_branches/controller.rs

@@ -459,6 +459,7 @@ impl ControllerInner {
             let _ = snapshot_tree.and_then(|snapshot_tree| {
                 project_repository.project().snapshot_commit_creation(
                     snapshot_tree,
+                    result.as_ref().err(),
                     message.to_owned(),
                     None,
                 )
@@ -630,7 +631,16 @@ impl ControllerInner {
         let _permit = self.semaphore.acquire().await;
 
         self.with_verify_branch(project_id, |project_repository, user| {
-            super::apply_branch(project_repository, branch_id, user).map_err(Into::into)
+            let snapshot_tree = project_repository.project().prepare_snapshot();
+            let result =
+                super::apply_branch(project_repository, branch_id, user).map_err(Into::into);
+
+            let _ = snapshot_tree.and_then(|snapshot_tree| {
+                project_repository
+                    .project()
+                    .snapshot_branch_applied(snapshot_tree, result.as_ref())
+            });
+            result.map(|_| ())
         })
     }
 
@@ -715,10 +725,17 @@ impl ControllerInner {
         let _permit = self.semaphore.acquire().await;
 
         self.with_verify_branch(project_id, |project_repository, _| {
-            let _ = project_repository
-                .project()
-                .snapshot_commit_undo(commit_oid);
-            super::undo_commit(project_repository, branch_id, commit_oid).map_err(Into::into)
+            let snapshot_tree = project_repository.project().prepare_snapshot();
+            let result: Result<(), Error> =
+                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(
+                    snapshot_tree,
+                    result.as_ref(),
+                    commit_oid,
+                )
+            });
+            result
         })
     }
 
@@ -783,8 +800,14 @@ impl ControllerInner {
         let _permit = self.semaphore.acquire().await;
 
         self.with_verify_branch(project_id, |project_repository, _| {
-            let result = super::unapply_branch(project_repository, branch_id);
-            result.map(|_| ()).map_err(Into::into)
+            let snapshot_tree = project_repository.project().prepare_snapshot();
+            let result = super::unapply_branch(project_repository, branch_id).map_err(Into::into);
+            let _ = snapshot_tree.and_then(|snapshot_tree| {
+                project_repository
+                    .project()
+                    .snapshot_branch_unapplied(snapshot_tree, result.as_ref())
+            });
+            result.map(|_| ())
         })
     }
 

crates/gitbutler-core/src/virtual_branches/virtual.rs

@@ -220,7 +220,7 @@ pub fn apply_branch(
     project_repository: &project_repository::Repository,
     branch_id: BranchId,
     user: Option<&users::User>,
-) -> Result<(), errors::ApplyBranchError> {
+) -> Result<String, errors::ApplyBranchError> {
     if project_repository.is_resolving() {
         return Err(errors::ApplyBranchError::Conflict(
             errors::ProjectConflict {
@@ -244,12 +244,8 @@ pub fn apply_branch(
         Err(error) => Err(errors::ApplyBranchError::Other(error.into())),
     }?;
 
-    _ = project_repository
-        .project()
-        .snapshot_branch_applied(branch.name.clone());
-
     if branch.applied {
-        return Ok(());
+        return Ok(branch.name);
     }
 
     let target_commit = repo
@@ -297,7 +293,7 @@ pub fn apply_branch(
 
             // apply the branch
             branch.applied = true;
-            vb_state.set_branch(branch)?;
+            vb_state.set_branch(branch.clone())?;
 
             // checkout the conflicts
             repo.checkout_index(&mut merge_index)
@@ -326,7 +322,7 @@ pub fn apply_branch(
                 Some(default_target.sha),
             )?;
 
-            return Ok(());
+            return Ok(branch.name);
         }
 
         let head_commit = repo
@@ -470,7 +466,7 @@ pub fn apply_branch(
         .context("failed to checkout index")?;
 
     super::integration::update_gitbutler_integration(&vb_state, project_repository)?;
-    Ok(())
+    Ok(branch.name)
 }
 
 pub fn unapply_ownership(
@@ -639,9 +635,6 @@ pub fn unapply_branch(
     if !target_branch.applied {
         return Ok(Some(target_branch));
     }
-    let _ = project_repository
-        .project()
-        .snapshot_branch_unapplied(target_branch.name.clone());
 
     let default_target = vb_state.get_default_target()?;
     let repo = &project_repository.git_repository;
@@ -4168,7 +4161,7 @@ pub fn create_virtual_branch_from_branch(
     project_repository.add_branch_reference(&branch)?;
 
     match apply_branch(project_repository, branch.id, user) {
-        Ok(()) => Ok(branch.id),
+        Ok(_) => Ok(branch.id),
         Err(errors::ApplyBranchError::BranchConflicts(_)) => {
             // if branch conflicts with the workspace, it's ok. keep it unapplied
             Ok(branch.id)