From 35a596ff66f90caf9961716b86fe1f79f348fd3e Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sat, 19 Aug 2023 14:01:49 +0900
Subject: [PATCH] git: prohibit creation of remote named "git"

#1690
---
 CHANGELOG.md                  |  4 ++++
 cli/tests/test_git_remotes.rs | 32 ++++++++++++++++++++++++++++++++
 lib/src/git.rs                | 11 +++++++++++
 3 files changed, 47 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a04393bf9..47cede4f4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -53,6 +53,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `jj git push` will now push all branches in the range `remote_branches()..@`
   instead of only branches pointing to `@` or `@-`.
 
+* It's no longer allowed to create a Git remote named "git". Use `jj git remote
+  rename` to rename the existing remote.
+  [#1690](https://github.com/martinvonz/jj/issues/1690)
+
 ### New features
 
 * Default template for `jj log` now does not show irrelevant information
diff --git a/cli/tests/test_git_remotes.rs b/cli/tests/test_git_remotes.rs
index f80a146c2..de961532f 100644
--- a/cli/tests/test_git_remotes.rs
+++ b/cli/tests/test_git_remotes.rs
@@ -74,6 +74,13 @@ fn test_git_remote_add() {
     insta::assert_snapshot!(stderr, @r###"
     Error: Git remote named 'foo' already exists
     "###);
+    let stderr = test_env.jj_cmd_failure(
+        &repo_path,
+        &["git", "remote", "add", "git", "http://example.com/repo/git"],
+    );
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Git remote named 'git' is reserved for local Git repository
+    "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
     insta::assert_snapshot!(stdout, @r###"
     foo http://example.com/repo/foo
@@ -102,6 +109,10 @@ fn test_git_remote_rename() {
     insta::assert_snapshot!(stderr, @r###"
     Error: Git remote named 'baz' already exists
     "###);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["git", "remote", "rename", "foo", "git"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Git remote named 'git' is reserved for local Git repository
+    "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "rename", "foo", "bar"]);
     insta::assert_snapshot!(stdout, @"");
     let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
@@ -110,3 +121,24 @@ fn test_git_remote_rename() {
     baz http://example.com/repo/baz
     "###);
 }
+
+#[test]
+fn test_git_remote_named_git() {
+    let test_env = TestEnvironment::default();
+
+    // Existing remote named 'git' shouldn't block the repo initialization.
+    let repo_path = test_env.env_root().join("repo");
+    let git_repo = git2::Repository::init(&repo_path).unwrap();
+    git_repo
+        .remote("git", "http://example.com/repo/repo")
+        .unwrap();
+    test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]);
+
+    // The remote can be renamed.
+    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "rename", "git", "bar"]);
+    insta::assert_snapshot!(stdout, @"");
+    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    bar http://example.com/repo/repo
+    "###);
+}
diff --git a/lib/src/git.rs b/lib/src/git.rs
index 6959b28ec..0ef3916d0 100644
--- a/lib/src/git.rs
+++ b/lib/src/git.rs
@@ -561,6 +561,11 @@ pub enum GitRemoteManagementError {
     NoSuchRemote(String),
     #[error("Git remote named '{0}' already exists")]
     RemoteAlreadyExists(String),
+    #[error(
+        "Git remote named '{name}' is reserved for local Git repository",
+        name = REMOTE_NAME_FOR_LOCAL_GIT_REPO
+    )]
+    RemoteReservedForLocalGitRepo,
     #[error(transparent)]
     InternalGitError(git2::Error),
 }
@@ -587,6 +592,9 @@ pub fn add_remote(
     remote_name: &str,
     url: &str,
 ) -> Result<(), GitRemoteManagementError> {
+    if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
+        return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo);
+    }
     git_repo.remote(remote_name, url).map_err(|err| {
         if is_remote_exists_err(&err) {
             GitRemoteManagementError::RemoteAlreadyExists(remote_name.to_owned())
@@ -638,6 +646,9 @@ pub fn rename_remote(
     old_remote_name: &str,
     new_remote_name: &str,
 ) -> Result<(), GitRemoteManagementError> {
+    if new_remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
+        return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo);
+    }
     git_repo
         .remote_rename(old_remote_name, new_remote_name)
         .map_err(|err| {