git: reject remotes with forward slashes

Although this behaviour is accepted by git, it's a degenerate case.
Especially because we implicitely rely on being able to parse out the
remote from the refname (i.e., `refs/remotes/<remote>/<branch>`).

Branches can have forward slashes, but if remotes can also have them,
parsing the refname becomes ambiguous, and a pain. Especially because it
would be totally legal to have a branch "c" on remote "a/b" and a branch
"b" on remote "a".

Fixes #5731

cli/src/command_error.rs

@@ -537,6 +537,10 @@ jj currently does not support partial clones. To use jj with this repository, tr
             }
             match err {
                 GitFetchError::NoSuchRemote(_) => user_error(err),
+                GitFetchError::RemoteWithSlash(_) => user_error_with_hint(
+                    err,
+                    "Run `jj git remote rename` to give a different name.",
+                ),
                 GitFetchError::InvalidBranchPattern(_) => user_error(err),
                 GitFetchError::InternalGitError(err) => map_git2_error(err),
                 GitFetchError::Subprocess(_) => user_error(err),
@@ -557,6 +561,10 @@ jj currently does not support partial clones. To use jj with this repository, tr
         fn from(err: GitPushError) -> Self {
             match err {
                 GitPushError::NoSuchRemote(_) => user_error(err),
+                GitPushError::RemoteWithSlash(_) => user_error_with_hint(
+                    err,
+                    "Run `jj git remote rename` to give a different name.",
+                ),
                 GitPushError::RemoteReservedForLocalGitRepo => user_error(err),
                 GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint(
                     format!(
@@ -580,7 +588,8 @@ jj currently does not support partial clones. To use jj with this repository, tr
             match err {
                 GitRemoteManagementError::NoSuchRemote(_) => user_error(err),
                 GitRemoteManagementError::RemoteAlreadyExists(_) => user_error(err),
-                GitRemoteManagementError::RemoteReservedForLocalGitRepo => user_error_with_hint(
+                GitRemoteManagementError::RemoteReservedForLocalGitRepo
+                | GitRemoteManagementError::RemoteWithSlash(_) => user_error_with_hint(
                     err,
                     "Run `jj git remote rename` to give a different name.",
                 ),

cli/tests/test_git_clone.rs

@@ -768,6 +768,28 @@ fn test_git_clone_with_remote_named_git(subprocess: bool) {
     }
 }
 
+#[test_case(false; "use git2 for remote calls")]
+#[test_case(true; "spawn a git subprocess for remote calls")]
+fn test_git_clone_with_remote_with_slashes(subprocess: bool) {
+    let test_env = TestEnvironment::default();
+    if !subprocess {
+        test_env.add_config("git.subprocess = false");
+    }
+    let git_repo_path = test_env.env_root().join("source");
+    git2::Repository::init(git_repo_path).unwrap();
+
+    let stderr = test_env.jj_cmd_failure(
+        test_env.env_root(),
+        &["git", "clone", "--remote=slash/origin", "source", "dest"],
+    );
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(stderr, @r"
+    Error: Git remotes with slashes are incompatible with jj: slash/origin
+    Hint: Run `jj git remote rename` to give a different name.
+    ");
+    }
+}
+
 #[test_case(false; "use git2 for remote calls")]
 #[test_case(true; "spawn a git subprocess for remote calls")]
 fn test_git_clone_trunk_deleted(subprocess: bool) {

cli/tests/test_git_fetch.rs

@@ -435,6 +435,33 @@ fn test_git_fetch_from_remote_named_git(subprocess: bool) {
     }
 }
 
+#[test_case(false; "use git2 for remote calls")]
+#[test_case(true; "spawn a git subprocess for remote calls")]
+fn test_git_fetch_from_remote_with_slashes(subprocess: bool) {
+    let test_env = TestEnvironment::default();
+    if !subprocess {
+        test_env.add_config("git.subprocess = false");
+    }
+    test_env.add_config("git.auto-local-bookmark = true");
+    let repo_path = test_env.env_root().join("repo");
+    init_git_remote(&test_env, "source");
+
+    let git_repo = git2::Repository::init(&repo_path).unwrap();
+    git_repo.remote("slash/origin", "../source").unwrap();
+
+    // Existing remote with slash shouldn't block the repo initialization.
+    test_env.jj_cmd_ok(&repo_path, &["git", "init", "--git-repo=."]);
+
+    // Try fetching from the remote named 'git'.
+    let stderr = &test_env.jj_cmd_failure(&repo_path, &["git", "fetch", "--remote=slash/origin"]);
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(stderr, @r"
+    Error: Git remotes with slashes are incompatible with jj: slash/origin
+    Hint: Run `jj git remote rename` to give a different name.
+    ");
+    }
+}
+
 #[test_case(false; "use git2 for remote calls")]
 #[test_case(true; "spawn a git subprocess for remote calls")]
 fn test_git_fetch_prune_before_updating_tips(subprocess: bool) {

cli/tests/test_git_push.rs

@@ -1866,6 +1866,35 @@ fn test_git_push_to_remote_named_git(subprocess: bool) {
     }
 }
 
+#[test_case(false; "use git2 for remote calls")]
+#[test_case(true; "spawn a git subprocess for remote calls")]
+fn test_git_push_to_remote_with_slashes(subprocess: bool) {
+    let (test_env, workspace_root) = set_up();
+    if !subprocess {
+        test_env.add_config("git.subprocess = false");
+    }
+    let git_repo = {
+        let mut git_repo_path = workspace_root.clone();
+        git_repo_path.extend([".jj", "repo", "store", "git"]);
+        git2::Repository::open(&git_repo_path).unwrap()
+    };
+    git_repo.remote_rename("origin", "slash/origin").unwrap();
+
+    let stderr = test_env.jj_cmd_failure(
+        &workspace_root,
+        &["git", "push", "--all", "--remote=slash/origin"],
+    );
+    insta::allow_duplicates! {
+    insta::assert_snapshot!(stderr, @r"
+    Changes to push to slash/origin:
+      Add bookmark bookmark1 to d13ecdbda2a2
+      Add bookmark bookmark2 to 8476341eb395
+    Error: Git remotes with slashes are incompatible with jj: slash/origin
+    Hint: Run `jj git remote rename` to give a different name.
+    ");
+    }
+}
+
 #[test]
 fn test_git_push_sign_on_push() {
     let (test_env, workspace_root) = set_up();

cli/tests/test_git_remotes.rs

@@ -266,3 +266,80 @@ fn test_git_remote_named_git() {
     ~
     "###);
 }
+
+#[test]
+fn test_git_remote_with_slashes() {
+    let test_env = TestEnvironment::default();
+
+    // Existing remote with slashes 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("slash/origin", "http://example.com/repo/repo")
+        .unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["git", "init", "--git-repo=."]);
+    test_env.jj_cmd_ok(&repo_path, &["bookmark", "create", "-r@", "main"]);
+
+    // Cannot add remote with a slash via `jj`
+    let stderr = test_env.jj_cmd_failure(
+        &repo_path,
+        &[
+            "git",
+            "remote",
+            "add",
+            "another/origin",
+            "http://examples.org/repo/repo",
+        ],
+    );
+    insta::assert_snapshot!(stderr, @r"
+    Error: Git remotes with slashes are incompatible with jj: another/origin
+    Hint: Run `jj git remote rename` to give a different name.
+    ");
+    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    slash/origin http://example.com/repo/repo
+    "###);
+
+    // The remote can be renamed.
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &["git", "remote", "rename", "slash/origin", "origin"],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @"");
+    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    origin http://example.com/repo/repo
+    "###);
+
+    // The remote cannot be renamed back by jj.
+    let stderr = test_env.jj_cmd_failure(
+        &repo_path,
+        &["git", "remote", "rename", "origin", "slash/origin"],
+    );
+    insta::assert_snapshot!(stderr, @r"
+    Error: Git remotes with slashes are incompatible with jj: slash/origin
+    Hint: Run `jj git remote rename` to give a different name.
+    ");
+
+    // Reinitialize the repo with remote with slashes
+    fs::remove_dir_all(repo_path.join(".jj")).unwrap();
+    git_repo.remote_rename("origin", "slash/origin").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["git", "init", "--git-repo=."]);
+
+    // The remote can also be removed.
+    let (stdout, stderr) =
+        test_env.jj_cmd_ok(&repo_path, &["git", "remote", "remove", "slash/origin"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @"");
+    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    "###);
+    // @git bookmark shouldn't be removed.
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rmain@git", "-Tbookmarks"]);
+    insta::assert_snapshot!(stdout, @r###"
+    ○  main
+    │
+    ~
+    "###);
+}

lib/src/git.rs

@@ -1293,6 +1293,8 @@ pub enum GitRemoteManagementError {
     NoSuchRemote(String),
     #[error("Git remote named '{0}' already exists")]
     RemoteAlreadyExists(String),
+    #[error("Git remotes with slashes are incompatible with jj: {0}")]
+    RemoteWithSlash(String),
     #[error(
         "Git remote named '{name}' is reserved for local Git repository",
         name = REMOTE_NAME_FOR_LOCAL_GIT_REPO
@@ -1349,6 +1351,10 @@ pub fn add_remote(
 ) -> Result<(), GitRemoteManagementError> {
     if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
         return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo);
+    } else if remote_name.contains("/") {
+        return Err(GitRemoteManagementError::RemoteWithSlash(
+            remote_name.to_owned(),
+        ));
     }
     git_repo.remote(remote_name, url).map_err(|err| {
         if is_remote_exists_err(&err) {
@@ -1401,6 +1407,10 @@ pub fn rename_remote(
 ) -> Result<(), GitRemoteManagementError> {
     if new_remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
         return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo);
+    } else if new_remote_name.contains("/") {
+        return Err(GitRemoteManagementError::RemoteWithSlash(
+            new_remote_name.to_owned(),
+        ));
     }
     git_repo
         .remote_rename(old_remote_name, new_remote_name)
@@ -1426,6 +1436,10 @@ pub fn set_remote_url(
 ) -> Result<(), GitRemoteManagementError> {
     if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO {
         return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo);
+    } else if remote_name.contains("/") {
+        return Err(GitRemoteManagementError::RemoteWithSlash(
+            remote_name.to_owned(),
+        ));
     }
 
     // Repository::remote_set_url() doesn't ensure the remote exists, it just
@@ -1479,6 +1493,8 @@ pub enum GitFetchError {
         chars = INVALID_REFSPEC_CHARS.iter().join("`, `")
     )]
     InvalidBranchPattern(StringPattern),
+    #[error("Git remotes with slashes are incompatible with jj: {0}")]
+    RemoteWithSlash(String),
     // TODO: I'm sure there are other errors possible, such as transport-level errors.
     #[error("Unexpected git error when fetching")]
     InternalGitError(#[from] git2::Error),
@@ -1559,6 +1575,9 @@ impl<'a> GitFetch<'a> {
         callbacks: RemoteCallbacks<'_>,
         depth: Option<NonZeroU32>,
     ) -> Result<(), GitFetchError> {
+        if remote_name.contains("/") {
+            return Err(GitFetchError::RemoteWithSlash(remote_name.to_owned()));
+        }
         self.fetch_impl
             .fetch(remote_name, branch_names, callbacks, depth)?;
         self.fetched.push(FetchedBranches {
@@ -1845,6 +1864,8 @@ fn subprocess_get_default_branch(
 pub enum GitPushError {
     #[error("No git remote named '{0}'")]
     NoSuchRemote(String),
+    #[error("Git remotes with slashes are incompatible with jj: {0}")]
+    RemoteWithSlash(String),
     #[error(
         "Git remote named '{name}' is reserved for local Git repository",
         name = REMOTE_NAME_FOR_LOCAL_GIT_REPO
@@ -1887,6 +1908,10 @@ pub fn push_branches(
     targets: &GitBranchPushTargets,
     callbacks: RemoteCallbacks<'_>,
 ) -> Result<(), GitPushError> {
+    if remote_name.contains("/") {
+        return Err(GitPushError::RemoteWithSlash(remote_name.to_owned()));
+    }
+
     let ref_updates = targets
         .branch_updates
         .iter()