diff --git a/CHANGELOG.md b/CHANGELOG.md index c4a8cc99987..6a271011a2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,10 @@ should not be broken. date. [#8007](https://github.com/jj-vcs/jj/issues/8007) +* `jj git clone` now succeeds if configuration for the current remote exists + in a global config file. + [#7820](https://github.com/jj-vcs/jj/issues/7820) + ## [0.35.0] - 2025-11-05 ### Release highlights diff --git a/cli/tests/test_git_remotes.rs b/cli/tests/test_git_remotes.rs index f43d84e8dba..08af25de460 100644 --- a/cli/tests/test_git_remotes.rs +++ b/cli/tests/test_git_remotes.rs @@ -553,41 +553,23 @@ fn test_git_remote_with_global_git_remote_config() { // Complete remotes from the global configuration are listed. // // `git remote -v` lists all remotes from the global configuration, - // even incomplete ones like `origin`. This is inconsistent with - // the other `git remote` commands, which ignore the global - // configuration (even `git remote get-url`). + // even incomplete ones like `origin`. Confusingly, these remotes + // are ignored by other `git remote` commands (even `git remote get-url`). + // + // This behavior is replicated by `jj git remote`. `jj git remote list` + // will list remotes from both global and local configuration, but other + // commands like `rename` will ignore the global configuration. insta::assert_snapshot!(output, @r" foo htps://example.com/repo/foo [EOF] "); let output = work_dir.run_jj(["git", "remote", "rename", "foo", "bar"]); - // Divergence from Git: we read the remote from the global - // configuration and write it back out. Git will use the global - // configuration for commands like `git remote -v`, `git fetch`, - // and `git push`, but `git remote rename`, `git remote remove`, - // `git remote set-url`, etc., will ignore it. - // - // This behavior applies to `jj git remote remove` and - // `jj git remote set-url` as well. It would be hard to change due - // to gitoxide’s model, but hopefully it’s relatively harmless. - insta::assert_snapshot!(output, @""); - insta::assert_snapshot!(read_git_config(work_dir.root()), @r#" - [core] - repositoryformatversion = 0 - bare = true - logallrefupdates = false - [remote "bar"] - url = htps://example.com/repo/foo - fetch = +refs/heads/*:refs/remotes/bar/* - "#); - // This has the unfortunate consequence that the original remote - // still exists after renaming. - let output = work_dir.run_jj(["git", "remote", "list"]); insta::assert_snapshot!(output, @r" - bar htps://example.com/repo/foo - foo htps://example.com/repo/foo + ------- stderr ------- + Error: No git remote named 'foo' [EOF] + [exit status: 1] "); let output = work_dir.run_jj([ @@ -610,7 +592,6 @@ fn test_git_remote_with_global_git_remote_config() { let output = work_dir.run_jj(["git", "remote", "list"]); insta::assert_snapshot!(output, @r" - bar htps://example.com/repo/foo foo htps://example.com/repo/foo origin https://example.com/repo/origin/2 [EOF] @@ -620,9 +601,6 @@ fn test_git_remote_with_global_git_remote_config() { repositoryformatversion = 0 bare = true logallrefupdates = false - [remote "bar"] - url = htps://example.com/repo/foo - fetch = +refs/heads/*:refs/remotes/bar/* [remote "origin"] url = https://example.com/repo/origin/2 fetch = +refs/heads/*:refs/remotes/origin/* diff --git a/lib/src/git.rs b/lib/src/git.rs index 28f0e06ebf7..34afa0465dd 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1922,6 +1922,19 @@ fn remove_remote_git_config_sections( Ok(()) } +/// Checks if non-empty configuration for a remote exists in the local +/// .git/config file +fn remote_exists_in_local_config(git_repo: &gix::Repository, remote_name: &RemoteName) -> bool { + let config = git_repo.config_snapshot(); + let subsection_name = Some(BStr::new(remote_name.as_str())); + match config.section("remote", subsection_name) { + Ok(section) => { + section.num_values() > 0 && section.meta().source == gix::config::Source::Local + } + Err(_) => false, + } +} + /// Returns a sorted list of configured remote names. pub fn get_all_remote_names( store: &Store, @@ -1952,7 +1965,7 @@ pub fn add_remote( validate_remote_name(remote_name)?; - if git_repo.try_find_remote(remote_name.as_str()).is_some() { + if remote_exists_in_local_config(&git_repo, remote_name) { return Err(GitRemoteManagementError::RemoteAlreadyExists( remote_name.to_owned(), )); @@ -2046,6 +2059,12 @@ pub fn rename_remote( validate_remote_name(new_remote_name)?; + if !remote_exists_in_local_config(&git_repo, old_remote_name) { + return Err(GitRemoteManagementError::NoSuchRemote( + old_remote_name.to_owned(), + )); + } + let Some(result) = git_repo.try_find_remote(old_remote_name.as_str()) else { return Err(GitRemoteManagementError::NoSuchRemote( old_remote_name.to_owned(), @@ -2053,7 +2072,7 @@ pub fn rename_remote( }; let mut remote = result.map_err(GitRemoteManagementError::from_git)?; - if git_repo.try_find_remote(new_remote_name.as_str()).is_some() { + if remote_exists_in_local_config(&git_repo, new_remote_name) { return Err(GitRemoteManagementError::RemoteAlreadyExists( new_remote_name.to_owned(), )); diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index a64d1128332..2f6326385a7 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -48,6 +48,7 @@ use jj_lib::git::GitPushError; use jj_lib::git::GitPushStats; use jj_lib::git::GitRefKind; use jj_lib::git::GitRefUpdate; +use jj_lib::git::GitRemoteManagementError; use jj_lib::git::GitResetHeadError; use jj_lib::git::GitSettings; use jj_lib::git::IgnoredRefspec; @@ -5028,3 +5029,80 @@ fn auto_track_all() -> HashMap { .map(|name| (name.into(), settings.clone())) .collect() } + +#[test] +fn test_add_remote_rejects_duplicate_in_local_config() { + let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git); + + let mut tx = test_repo.repo.start_transaction(); + git::add_remote( + tx.repo_mut(), + "foo".as_ref(), + "https://example.com/", + Default::default(), + None, + ) + .unwrap(); + let _repo = tx.commit("test").unwrap(); + // Reload after Git configuration change. + let repo = &test_repo + .env + .load_repo_at_head(&testutils::user_settings(), test_repo.repo_path()); + + let mut tx = repo.start_transaction(); + let result = git::add_remote( + tx.repo_mut(), + "foo".as_ref(), + "https://example.com/other", + Default::default(), + None, + ); + + assert_matches!( + result, + Err(GitRemoteManagementError::RemoteAlreadyExists(name)) if name.as_str() == "foo" + ); +} + +#[test] +fn test_rename_remote_rejects_when_new_exists_in_local_config() { + let test_repo = TestRepo::init_with_backend(TestRepoBackend::Git); + + let mut tx = test_repo.repo.start_transaction(); + git::add_remote( + tx.repo_mut(), + "foo".as_ref(), + "https://example.com/foo", + Default::default(), + None, + ) + .unwrap(); + let _repo = tx.commit("test").unwrap(); + // Reload after Git configuration change. + let repo = &test_repo + .env + .load_repo_at_head(&testutils::user_settings(), test_repo.repo_path()); + + let mut tx = repo.start_transaction(); + git::add_remote( + tx.repo_mut(), + "bar".as_ref(), + "https://example.com/bar", + Default::default(), + None, + ) + .unwrap(); + let _repo = tx.commit("test").unwrap(); + // Reload after Git configuration change. + let repo = &test_repo + .env + .load_repo_at_head(&testutils::user_settings(), test_repo.repo_path()); + + let mut tx = repo.start_transaction(); + let result = git::rename_remote(tx.repo_mut(), "foo".as_ref(), "bar".as_ref()); + + assert_matches!( + result, + Err(GitRemoteManagementError::RemoteAlreadyExists(name)) if name.as_str() == "bar" + ); +}