git: propagate errors for missing commits when importing refs

cli/src/cli_util.rs

@@ -259,9 +259,22 @@ impl From<git2::Error> for CommandError {
 
 impl From<GitImportError> for CommandError {
     fn from(err: GitImportError) -> Self {
-        CommandError::InternalError(format!(
-            "Failed to import refs from underlying Git repo: {err}"
-        ))
+        let message = format!("Failed to import refs from underlying Git repo: {err}");
+        let missing_object = matches!(
+            err,
+            GitImportError::MissingHeadTarget { .. } | GitImportError::MissingRefAncestor { .. }
+        );
+        let hint = missing_object.then(|| {
+            "\
+Is this Git repository a shallow or partial clone (cloned with the --depth or --filter \
+             argument)?
+jj currently does not support shallow/partial clones. To use jj with this repository, \
+             try
+unshallowing the repository (https://stackoverflow.com/q/6802145) or re-cloning with the full
+repository contents."
+                .to_string()
+        });
+        CommandError::UserError { message, hint }
     }
 }
 

cli/src/commands/git.rs

@@ -492,6 +492,7 @@ fn do_git_clone(
         GitFetchError::NoSuchRemote(_) => {
             panic!("shouldn't happen as we just created the git remote")
         }
+        GitFetchError::GitImportError(err) => CommandError::from(err),
         GitFetchError::InternalGitError(err) => map_git_error(err),
         GitFetchError::InvalidGlob => {
             unreachable!("we didn't provide any globs")

lib/src/git.rs

@@ -25,7 +25,7 @@ use itertools::Itertools;
 use tempfile::NamedTempFile;
 use thiserror::Error;
 
-use crate::backend::{CommitId, ObjectId};
+use crate::backend::{BackendError, CommitId, ObjectId};
 use crate::git_backend::NO_GC_REF_NAMESPACE;
 use crate::op_store::{BranchTarget, RefTarget, RefTargetOptionExt};
 use crate::repo::{MutableRepo, Repo};
@@ -33,8 +33,20 @@ use crate::revset;
 use crate::settings::GitSettings;
 use crate::view::{RefName, View};
 
-#[derive(Error, Debug, PartialEq)]
+#[derive(Error, Debug)]
 pub enum GitImportError {
+    #[error("Failed to read Git HEAD target commit {id}: {err}", id=id.hex())]
+    MissingHeadTarget {
+        id: CommitId,
+        #[source]
+        err: BackendError,
+    },
+    #[error("Ancestor of Git ref {ref_name} is missing: {err}")]
+    MissingRefAncestor {
+        ref_name: String,
+        #[source]
+        err: BackendError,
+    },
     #[error("Unexpected git error when importing refs: {0}")]
     InternalGitError(#[from] git2::Error),
 }
@@ -196,7 +208,12 @@ pub fn import_some_refs(
         // doesn't automatically mean the old HEAD branch has been rewritten.
         let head_commit_id = CommitId::from_bytes(head_git_commit.id().as_bytes());
         if !matches!(mut_repo.git_head().as_normal(), Some(id) if id == &head_commit_id) {
-            let head_commit = store.get_commit(&head_commit_id).unwrap();
+            let head_commit = store.get_commit(&head_commit_id).map_err(|err| {
+                GitImportError::MissingHeadTarget {
+                    id: head_commit_id.clone(),
+                    err,
+                }
+            })?;
             prevent_gc(git_repo, &head_commit_id)?;
             mut_repo.add_head(&head_commit);
             mut_repo.set_git_head_target(RefTarget::normal(head_commit_id));
@@ -208,13 +225,18 @@ pub fn import_some_refs(
     let changed_git_refs = diff_refs_to_import(mut_repo.view(), git_repo, git_ref_filter)?;
 
     // Import new heads
-    for id in changed_git_refs
-        .values()
-        .flat_map(|(_, new_git_target)| new_git_target.added_ids())
-    {
-        prevent_gc(git_repo, id)?;
-        let commit = store.get_commit(id).unwrap();
-        mut_repo.add_head(&commit);
+    for (ref_name, (_, new_git_target)) in &changed_git_refs {
+        for id in new_git_target.added_ids() {
+            let commit =
+                store
+                    .get_commit(id)
+                    .map_err(|err| GitImportError::MissingRefAncestor {
+                        ref_name: ref_name.to_string(),
+                        err,
+                    })?;
+            prevent_gc(git_repo, id)?;
+            mut_repo.add_head(&commit);
+        }
     }
 
     for (ref_name, (old_git_target, new_git_target)) in &changed_git_refs {
@@ -582,12 +604,14 @@ pub fn rename_remote(
     Ok(())
 }
 
-#[derive(Error, Debug, PartialEq)]
+#[derive(Error, Debug)]
 pub enum GitFetchError {
     #[error("No git remote named '{0}'")]
     NoSuchRemote(String),
     #[error("Invalid glob provided. Globs may not contain the characters `:` or `^`.")]
     InvalidGlob,
+    #[error("Failec to import Git refs: {0}")]
+    GitImportError(#[from] GitImportError),
     // TODO: I'm sure there are other errors possible, such as transport-level errors.
     #[error("Unexpected git error when fetching: {0}")]
     InternalGitError(#[from] git2::Error),
@@ -718,9 +742,6 @@ pub fn fetch(
         to_remote_branch(ref_name, remote_name)
             .map(&branch_name_filter)
             .unwrap_or(false)
-    })
-    .map_err(|err| match err {
-        GitImportError::InternalGitError(source) => GitFetchError::InternalGitError(source),
     })?;
     Ok(default_branch)
 }

lib/tests/test_git.rs

@@ -15,8 +15,9 @@
 use std::collections::{BTreeMap, HashSet};
 use std::path::PathBuf;
 use std::sync::{mpsc, Arc, Barrier};
-use std::thread;
+use std::{fs, thread};
 
+use assert_matches::assert_matches;
 use git2::Oid;
 use itertools::Itertools;
 use jj_lib::backend::{
@@ -25,7 +26,7 @@ use jj_lib::backend::{
 use jj_lib::commit::Commit;
 use jj_lib::commit_builder::CommitBuilder;
 use jj_lib::git;
-use jj_lib::git::{GitFetchError, GitPushError, GitRefUpdate, SubmoduleConfig};
+use jj_lib::git::{GitFetchError, GitImportError, GitPushError, GitRefUpdate, SubmoduleConfig};
 use jj_lib::git_backend::GitBackend;
 use jj_lib::op_store::{BranchTarget, RefTarget};
 use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo};
@@ -938,6 +939,66 @@ fn test_import_refs_empty_git_repo() {
     assert_eq!(repo.view().git_head(), RefTarget::absent_ref());
 }
 
+#[test]
+fn test_import_refs_missing_git_commit() {
+    let settings = testutils::user_settings();
+    let git_settings = GitSettings::default();
+    let test_workspace = TestRepo::init(true);
+    let repo = &test_workspace.repo;
+    let git_repo = get_git_repo(repo);
+
+    // Missing commit is ancestor of ref
+    let commit1 = empty_git_commit(&git_repo, "refs/heads/main", &[]);
+    empty_git_commit(&git_repo, "refs/heads/main", &[&commit1]);
+    let shard = hex::encode(&commit1.id().as_bytes()[..1]);
+    let object_basename = hex::encode(&commit1.id().as_bytes()[1..]);
+    let object_store_path = git_repo.path().join("objects");
+    let object_file = object_store_path.join(&shard).join(object_basename);
+    let backup_object_file = object_store_path.join(&shard).join("backup");
+    assert!(object_file.exists());
+    fs::rename(&object_file, &backup_object_file).unwrap();
+
+    let mut tx = repo.start_transaction(&settings, "test");
+    let result = git::import_refs(tx.mut_repo(), &git_repo, &git_settings);
+    assert_matches!(
+        result,
+        Err(GitImportError::MissingRefAncestor {
+            ref_name,
+            err: BackendError::ObjectNotFound { .. }
+        }) if &ref_name == "main"
+    );
+
+    // Missing commit is pointed to by ref
+    fs::rename(&backup_object_file, &object_file).unwrap();
+    git_repo
+        .reference("refs/heads/main", commit1.id(), true, "test")
+        .unwrap();
+    fs::rename(&object_file, &backup_object_file).unwrap();
+    let mut tx = repo.start_transaction(&settings, "test");
+    let result = git::import_refs(tx.mut_repo(), &git_repo, &git_settings);
+    assert_matches!(
+        result,
+        Err(GitImportError::MissingRefAncestor {
+            ref_name,
+            err: BackendError::ObjectNotFound { .. }
+        }) if &ref_name == "main"
+    );
+
+    // Missing commit is pointed to by HEAD
+    fs::rename(&backup_object_file, &object_file).unwrap();
+    git_repo.set_head_detached(commit1.id()).unwrap();
+    fs::rename(&object_file, &backup_object_file).unwrap();
+    let mut tx = repo.start_transaction(&settings, "test");
+    let result = git::import_refs(tx.mut_repo(), &git_repo, &git_settings);
+    assert_matches!(
+        result,
+        Err(GitImportError::MissingHeadTarget {
+            id,
+            err: BackendError::ObjectNotFound { .. }
+        }) if id == jj_id(&commit1)
+    );
+}
+
 #[test]
 fn test_import_refs_detached_head() {
     let test_data = GitRepoData::create();