From e4f83e353efef9fcb382918bfe1c9ef135fb1148 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Thu, 28 Apr 2022 08:53:51 -0700
Subject: [PATCH] errors: use custom error for failed tree merge

Tree merges can currently fail because of a failure to look up an
object, or because of a failure to read its contents. Both results in
`BackendError` because of a `impl From<std::io::Error> for
BackendError`. That's kind of correct in this case, but it wasn't
intentional (that impl was from `local_backend`), and we need to
making errors more specific for better error handling.
---
 lib/src/tree.rs | 36 ++++++++++++++++++++++++++++--------
 src/commands.rs |  8 +++++++-
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/lib/src/tree.rs b/lib/src/tree.rs
index 2dcaf4813..4f1336a9f 100644
--- a/lib/src/tree.rs
+++ b/lib/src/tree.rs
@@ -20,10 +20,11 @@ use std::pin::Pin;
 use std::sync::Arc;
 
 use itertools::Itertools;
+use thiserror::Error;
 
 use crate::backend::{
-    BackendError, Conflict, ConflictId, ConflictPart, TreeEntriesNonRecursiveIterator, TreeEntry,
-    TreeId, TreeValue,
+    BackendError, Conflict, ConflictId, ConflictPart, FileId, TreeEntriesNonRecursiveIterator,
+    TreeEntry, TreeId, TreeValue,
 };
 use crate::files::MergeResult;
 use crate::matchers::{EverythingMatcher, Matcher};
@@ -31,6 +32,17 @@ use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin};
 use crate::store::Store;
 use crate::{backend, files};
 
+#[derive(Debug, Error)]
+pub enum TreeMergeError {
+    #[error("Failed to read file with ID {} ", .file_id.hex())]
+    ReadError {
+        source: std::io::Error,
+        file_id: FileId,
+    },
+    #[error("Backend error: {0}")]
+    BackendError(#[from] BackendError),
+}
+
 #[derive(Clone)]
 pub struct Tree {
     store: Arc<Store>,
@@ -502,7 +514,7 @@ pub fn merge_trees(
     side1_tree: &Tree,
     base_tree: &Tree,
     side2_tree: &Tree,
-) -> Result<TreeId, BackendError> {
+) -> Result<TreeId, TreeMergeError> {
     let store = base_tree.store();
     let dir = base_tree.dir();
     assert_eq!(side1_tree.dir(), dir);
@@ -541,7 +553,7 @@ pub fn merge_trees(
             }
         }
     }
-    store.write_tree(dir, &new_tree)
+    Ok(store.write_tree(dir, &new_tree)?)
 }
 
 /// Returns `Some(TreeId)` if this is a directory or missing. If it's missing,
@@ -564,7 +576,7 @@ fn merge_tree_value(
     maybe_base: Option<&TreeValue>,
     maybe_side1: Option<&TreeValue>,
     maybe_side2: Option<&TreeValue>,
-) -> Result<Option<TreeValue>, BackendError> {
+) -> Result<Option<TreeValue>, TreeMergeError> {
     // Resolve non-trivial conflicts:
     //   * resolve tree conflicts by recursing
     //   * try to resolve file conflicts by merging the file contents
@@ -634,7 +646,7 @@ fn try_resolve_file_conflict(
     store: &Store,
     filename: &RepoPath,
     conflict: &Conflict,
-) -> Result<Option<(Vec<u8>, bool)>, BackendError> {
+) -> Result<Option<(Vec<u8>, bool)>, TreeMergeError> {
     // If there are any non-file parts in the conflict, we can't merge it. We check
     // early so we don't waste time reading file contents if we can't merge them
     // anyway. At the same time we determine whether the resulting file should
@@ -687,14 +699,22 @@ fn try_resolve_file_conflict(
         let mut content = vec![];
         store
             .read_file(filename, &file_id)?
-            .read_to_end(&mut content)?;
+            .read_to_end(&mut content)
+            .map_err(|err| TreeMergeError::ReadError {
+                source: err,
+                file_id,
+            })?;
         removed_contents.push(content);
     }
     for file_id in added_file_ids {
         let mut content = vec![];
         store
             .read_file(filename, &file_id)?
-            .read_to_end(&mut content)?;
+            .read_to_end(&mut content)
+            .map_err(|err| TreeMergeError::ReadError {
+                source: err,
+                file_id,
+            })?;
         added_contents.push(content);
     }
     let merge_result = files::merge(
diff --git a/src/commands.rs b/src/commands.rs
index 2d83b5dcb..08ced2cdf 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -55,7 +55,7 @@ use jujutsu_lib::rewrite::{back_out_commit, merge_commit_trees, rebase_commit, D
 use jujutsu_lib::settings::UserSettings;
 use jujutsu_lib::store::Store;
 use jujutsu_lib::transaction::Transaction;
-use jujutsu_lib::tree::{merge_trees, Tree, TreeDiffIterator};
+use jujutsu_lib::tree::{merge_trees, Tree, TreeDiffIterator, TreeMergeError};
 use jujutsu_lib::working_copy::{
     CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, WorkingCopy,
 };
@@ -119,6 +119,12 @@ impl From<SnapshotError> for CommandError {
     }
 }
 
+impl From<TreeMergeError> for CommandError {
+    fn from(err: TreeMergeError) -> Self {
+        CommandError::InternalError(format!("Merge failed: {}", err))
+    }
+}
+
 impl From<ResetError> for CommandError {
     fn from(_: ResetError) -> Self {
         CommandError::InternalError("Failed to reset the working copy".to_string())