git: do not import refs from remote named "git"

I made it simply fail on explicit fetch/import, and ignored on implicit import.
Since the error mode is predictable and less likely to occur. I don't think it
makes sense to implement warning propagation just for this.

Closes martinvonz#1690.

cli/src/cli_util.rs

@@ -261,20 +261,23 @@ impl From<git2::Error> for CommandError {
 impl From<GitImportError> for CommandError {
     fn from(err: GitImportError) -> Self {
         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(|| {
-            "\
+        let hint = match &err {
+            GitImportError::MissingHeadTarget { .. }
+            | GitImportError::MissingRefAncestor { .. } => Some(
+                "\
 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
+                 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()
-        });
+                    .to_string(),
+            ),
+            GitImportError::RemoteReservedForLocalGitRepo => {
+                Some("Run `jj git remote rename` to give different name.".to_string())
+            }
+            GitImportError::InternalGitError(_) => None,
+        };
         CommandError::UserError { message, hint }
     }
 }
@@ -773,7 +776,13 @@ impl WorkspaceCommandHelper {
         git_repo: &Repository,
     ) -> Result<(), CommandError> {
         let mut tx = self.start_transaction("import git refs").into_inner();
-        git::import_refs(tx.mut_repo(), git_repo, &self.settings.git_settings())?;
+        // Automated import shouldn't fail because of reserved remote name.
+        git::import_some_refs(
+            tx.mut_repo(),
+            git_repo,
+            &self.settings.git_settings(),
+            |ref_name| !git::is_reserved_git_remote_ref(ref_name),
+        )?;
         if tx.mut_repo().has_changes() {
             let old_git_head = self.repo().view().git_head().clone();
             let new_git_head = tx.mut_repo().view().git_head().clone();

cli/src/commands/git.rs

@@ -341,6 +341,7 @@ fn cmd_git_fetch(
             )
         })
         .map_err(|err| match err {
+            GitFetchError::GitImportError(err) => err.into(),
             GitFetchError::InternalGitError(err) => map_git_error(err),
             _ => user_error(err.to_string()),
         })?;

cli/src/commands/mod.rs

@@ -1125,7 +1125,12 @@ fn cmd_init(ui: &mut Ui, command: &CommandHelper, args: &InitArgs) -> Result<(),
             git::add_to_git_exclude(ui, &git_repo)?;
         } else {
             let mut tx = workspace_command.start_transaction("import git refs");
-            jj_lib::git::import_refs(tx.mut_repo(), &git_repo, &command.settings().git_settings())?;
+            jj_lib::git::import_some_refs(
+                tx.mut_repo(),
+                &git_repo,
+                &command.settings().git_settings(),
+                |ref_name| !jj_lib::git::is_reserved_git_remote_ref(ref_name),
+            )?;
             if let Some(git_head_id) = tx.mut_repo().view().git_head().as_normal().cloned() {
                 let git_head_commit = tx.mut_repo().store().get_commit(&git_head_id)?;
                 tx.check_out(&git_head_commit)?;

cli/tests/test_git_fetch.rs

@@ -17,8 +17,8 @@ use crate::common::TestEnvironment;
 
 pub mod common;
 
-/// Add a remote containing a branch with the same name
-fn add_git_remote(test_env: &TestEnvironment, repo_path: &Path, remote: &str) {
+/// Creates a remote Git repo containing a branch with the same name
+fn init_git_remote(test_env: &TestEnvironment, remote: &str) {
     let git_repo_path = test_env.env_root().join(remote);
     let git_repo = git2::Repository::init(git_repo_path).unwrap();
     let signature =
@@ -40,6 +40,11 @@ fn add_git_remote(test_env: &TestEnvironment, repo_path: &Path, remote: &str) {
             &[],
         )
         .unwrap();
+}
+
+/// Add a remote containing a branch with the same name
+fn add_git_remote(test_env: &TestEnvironment, repo_path: &Path, remote: &str) {
+    init_git_remote(test_env, remote);
     test_env.jj_cmd_success(
         repo_path,
         &["git", "remote", "add", remote, &format!("../{remote}")],
@@ -224,6 +229,43 @@ fn test_git_fetch_nonexistent_remote_from_config() {
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
 }
 
+#[test]
+fn test_git_fetch_from_remote_named_git() {
+    let test_env = TestEnvironment::default();
+    let repo_path = test_env.env_root().join("repo");
+    init_git_remote(&test_env, "git");
+    let git_repo = git2::Repository::init(&repo_path).unwrap();
+    git_repo.remote("git", "../git").unwrap();
+
+    // Existing remote named 'git' shouldn't block the repo initialization.
+    test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]);
+
+    // Try fetching from the remote named 'git'.
+    let stderr = &test_env.jj_cmd_failure(&repo_path, &["git", "fetch", "--remote=git"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Failed to import refs from underlying Git repo: Git remote named 'git' is reserved for local Git repository
+    Hint: Run `jj git remote rename` to give different name.
+    "###);
+
+    // Implicit import shouldn't fail because of the remote ref.
+    let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
+    insta::assert_snapshot!(stdout, @"");
+
+    // Explicit import is an error.
+    // (This could be warning if we add mechanism to report ignored refs.)
+    insta::assert_snapshot!(test_env.jj_cmd_failure(&repo_path, &["git", "import"]), @r###"
+    Error: Failed to import refs from underlying Git repo: Git remote named 'git' is reserved for local Git repository
+    Hint: Run `jj git remote rename` to give different name.
+    "###);
+
+    // The remote can be renamed, and the ref can be imported.
+    test_env.jj_cmd_success(&repo_path, &["git", "remote", "rename", "git", "bar"]);
+    let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    git: mrylzrtu 76fc7466 message
+    "###);
+}
+
 #[test]
 fn test_git_fetch_prune_before_updating_tips() {
     let test_env = TestEnvironment::default();

lib/src/git.rs

@@ -50,6 +50,11 @@ pub enum GitImportError {
         #[source]
         err: BackendError,
     },
+    #[error(
+        "Git remote named '{name}' is reserved for local Git repository",
+        name = REMOTE_NAME_FOR_LOCAL_GIT_REPO
+    )]
+    RemoteReservedForLocalGitRepo,
     #[error("Unexpected git error when importing refs: {0}")]
     InternalGitError(#[from] git2::Error),
 }
@@ -92,6 +97,15 @@ fn to_remote_branch<'a>(parsed_ref: &'a RefName, remote_name: &str) -> Option<&'
     }
 }
 
+/// Returns true if the `parsed_ref` won't be imported because its remote name
+/// is reserved.
+///
+/// Use this as a negative `git_ref_filter` to be passed in to
+/// `import_some_refs()`.
+pub fn is_reserved_git_remote_ref(parsed_ref: &RefName) -> bool {
+    to_remote_branch(parsed_ref, REMOTE_NAME_FOR_LOCAL_GIT_REPO).is_some()
+}
+
 /// Checks if `git_ref` points to a Git commit object, and returns its id.
 ///
 /// If the ref points to the previously `known_target` (i.e. unchanged), this
@@ -219,6 +233,9 @@ pub fn import_some_refs(
         old_git_head.is_present().then(RefTarget::absent)
     };
     let changed_git_refs = diff_refs_to_import(mut_repo.view(), git_repo, git_ref_filter)?;
+    if changed_git_refs.keys().any(is_reserved_git_remote_ref) {
+        return Err(GitImportError::RemoteReservedForLocalGitRepo);
+    }
 
     // Import new heads
     let store = mut_repo.store();

lib/tests/test_git.rs

@@ -700,6 +700,21 @@ fn test_import_refs_reimport_conflicted_remote_branch() {
     );
 }
 
+#[test]
+fn test_import_refs_reserved_remote_name() {
+    let settings = testutils::user_settings();
+    let git_settings = GitSettings::default();
+    let test_repo = TestRepo::init(true);
+    let repo = &test_repo.repo;
+    let git_repo = get_git_repo(repo);
+
+    empty_git_commit(&git_repo, "refs/remotes/git/main", &[]);
+
+    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::RemoteReservedForLocalGitRepo));
+}
+
 #[test]
 fn test_import_some_refs() {
     let settings = testutils::user_settings();
@@ -717,6 +732,8 @@ fn test_import_some_refs() {
     let commit_feat4 =
         empty_git_commit(&git_repo, "refs/remotes/origin/feature4", &[&commit_feat3]);
     let commit_ign = empty_git_commit(&git_repo, "refs/remotes/origin/ignored", &[]);
+    // No error should be reported for the refs excluded by git_ref_filter.
+    empty_git_commit(&git_repo, "refs/remotes/git/main", &[]);
 
     fn get_remote_branch(ref_name: &RefName) -> Option<&str> {
         match ref_name {