From 717d0d3d6d270b76058e28842d47ec43a372d88a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 25 Sep 2023 20:05:24 +0900 Subject: [PATCH] git: on deserialize/import/export, copy refs/heads/* to remote named "git" I've added a boolean flag to the store to ensure that the migration never runs more than once after the view gets "op restore"-d. I'll probably reorganize the branches structure to support non-tracking branches later, but updating the storage format in a single commit would be too involved. If jj is downgraded, these "git" remote refs would be exported to the Git repo. Users might have to remove them manually. --- CHANGELOG.md | 4 +++ cli/tests/test_branch_command.rs | 21 ++++--------- cli/tests/test_git_import_export.rs | 34 ++++++++------------ cli/tests/test_git_remotes.rs | 35 +++++++++++++++++++++ lib/src/git.rs | 48 ++++++++++++++++++++++++----- lib/src/protos/op_store.proto | 2 ++ lib/src/protos/op_store.rs | 3 ++ lib/src/simple_op_store.rs | 30 ++++++++++++++++-- lib/tests/test_git.rs | 37 ++++++++++++++++------ 9 files changed, 159 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb996ef2ce..abfb157d35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 should be noticeably faster on large repos. You may need to create a new clone to see the full speedup. +* The `remote_branches()` revset now includes branches exported to the Git + repository (so called Git-tracking branches.) Use + `remote_branches(remote=exact:"origin")` to query branches of certain remote. + ### New features ### Fixed bugs diff --git a/cli/tests/test_branch_command.rs b/cli/tests/test_branch_command.rs index f30f466f44..57dcb49b0a 100644 --- a/cli/tests/test_branch_command.rs +++ b/cli/tests/test_branch_command.rs @@ -241,6 +241,8 @@ fn test_branch_forget_export() { insta::assert_snapshot!(stdout, @""); // Forgetting a branch does not delete its local-git tracking branch. The // git-tracking branch is kept. + // TODO: Actually git-tracking branch is forgotten. Update the branch + // resolution code to not shadow the real @git branch. let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]); insta::assert_snapshot!(stdout, @r###" foo (forgotten) @@ -258,25 +260,14 @@ fn test_branch_forget_export() { (empty) (no description set) "###); - // The presence of the @git branch means that a `jj git import` is a no-op... - let stdout = test_env.jj_cmd_success(&repo_path, &["git", "import"]); - insta::assert_snapshot!(stdout, @r###" - Nothing changed. - "###); - // ... and a `jj git export` will delete the branch from git and will delete the - // git-tracking branch. In a colocated repo, this will happen automatically - // immediately after a `jj branch forget`. This is demonstrated in - // `test_git_colocated_branch_forget` in test_git_colocated.rs + // `jj git export` will delete the branch from git. In a colocated repo, + // this will happen automatically immediately after a `jj branch forget`. + // This is demonstrated in `test_git_colocated_branch_forget` in + // test_git_colocated.rs let stdout = test_env.jj_cmd_success(&repo_path, &["git", "export"]); insta::assert_snapshot!(stdout, @""); let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]); insta::assert_snapshot!(stdout, @""); - - // Note that if `jj branch forget` *did* delete foo@git, a subsequent `jj - // git export` would be a no-op and a `jj git import` would resurrect - // the branch. In a normal repo, that might be OK. In a colocated repo, - // this would automatically happen before the next command, making `jj - // branch forget` useless. } #[test] diff --git a/cli/tests/test_git_import_export.rs b/cli/tests/test_git_import_export.rs index 4347a147c7..66cfaf82ad 100644 --- a/cli/tests/test_git_import_export.rs +++ b/cli/tests/test_git_import_export.rs @@ -39,30 +39,20 @@ fn test_resolution_of_git_tracking_branches() { "###); // Test that we can address both revisions - let stdout = test_env.jj_cmd_success( - &repo_path, - &[ - "log", - "-r=main", - "-T", - r#"commit_id ++ " " ++ description"#, - "--no-graph", - ], - ); - insta::assert_snapshot!(stdout, @r###" + let query = |expr| { + let template = r#"commit_id ++ " " ++ description"#; + test_env.jj_cmd_success( + &repo_path, + &["log", "-r", expr, "-T", template, "--no-graph"], + ) + }; + insta::assert_snapshot!(query("main"), @r###" 3af370264cdcbba791762f8ef6bc79b456dcbf3b new_message "###); - let stdout = test_env.jj_cmd_success( - &repo_path, - &[ - "log", - "-r=main@git", - "-T", - r#"commit_id ++ " " ++ description"#, - "--no-graph", - ], - ); - insta::assert_snapshot!(stdout, @r###" + insta::assert_snapshot!(query("main@git"), @r###" + 16d541ca40f42baf2dea41aa61a0b5f1cbf1f91b old_message + "###); + insta::assert_snapshot!(query(r#"remote_branches(exact:"main", exact:"git")"#), @r###" 16d541ca40f42baf2dea41aa61a0b5f1cbf1f91b old_message "###); } diff --git a/cli/tests/test_git_remotes.rs b/cli/tests/test_git_remotes.rs index de961532f7..2777b85a91 100644 --- a/cli/tests/test_git_remotes.rs +++ b/cli/tests/test_git_remotes.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::fs; + use crate::common::TestEnvironment; pub mod common; @@ -133,6 +135,7 @@ fn test_git_remote_named_git() { .remote("git", "http://example.com/repo/repo") .unwrap(); test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]); + test_env.jj_cmd_success(&repo_path, &["branch", "set", "main"]); // The remote can be renamed. let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "rename", "git", "bar"]); @@ -141,4 +144,36 @@ fn test_git_remote_named_git() { insta::assert_snapshot!(stdout, @r###" bar http://example.com/repo/repo "###); + // @git branch shouldn't be renamed. + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rmain@git", "-Tbranches"]); + insta::assert_snapshot!(stdout, @r###" + @ main + │ + ~ + "###); + + // The remote cannot be renamed back by jj. + let stderr = test_env.jj_cmd_failure(&repo_path, &["git", "remote", "rename", "bar", "git"]); + insta::assert_snapshot!(stderr, @r###" + Error: Git remote named 'git' is reserved for local Git repository + "###); + + // Reinitialize the repo with remote named 'git'. + fs::remove_dir_all(repo_path.join(".jj")).unwrap(); + git_repo.remote_rename("bar", "git").unwrap(); + test_env.jj_cmd_success(&repo_path, &["init", "--git-repo=."]); + + // The remote can also be removed. + let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "remove", "git"]); + insta::assert_snapshot!(stdout, @""); + let stdout = test_env.jj_cmd_success(&repo_path, &["git", "remote", "list"]); + insta::assert_snapshot!(stdout, @r###" + "###); + // @git branch shouldn't be removed. + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rmain@git", "-Tbranches"]); + insta::assert_snapshot!(stdout, @r###" + ◉ main + │ + ~ + "###); } diff --git a/lib/src/git.rs b/lib/src/git.rs index a729e30e29..3149d4dce7 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -284,6 +284,14 @@ pub fn import_some_refs( mut_repo.merge_single_ref(&local_ref_name, old_git_target, new_git_target); } } else { + if let RefName::LocalBranch(branch) = ref_name { + // Update Git-tracking branch like the other remote branches. + mut_repo.set_remote_branch_target( + branch, + REMOTE_NAME_FOR_LOCAL_GIT_REPO, + new_git_target.clone(), + ); + } mut_repo.merge_single_ref(ref_name, old_git_target, new_git_target); } } @@ -493,7 +501,15 @@ pub fn export_some_refs( reason, }); } else { - mut_repo.set_git_ref_target(&git_ref_name, RefTarget::absent()); + let new_target = RefTarget::absent(); + if let RefName::LocalBranch(branch) = &parsed_ref_name { + mut_repo.set_remote_branch_target( + branch, + REMOTE_NAME_FOR_LOCAL_GIT_REPO, + new_target.clone(), + ); + } + mut_repo.set_git_ref_target(&git_ref_name, new_target); } } for (parsed_ref_name, (old_oid, new_oid)) in branches_to_update { @@ -504,10 +520,15 @@ pub fn export_some_refs( reason, }); } else { - mut_repo.set_git_ref_target( - &git_ref_name, - RefTarget::normal(CommitId::from_bytes(new_oid.as_bytes())), - ); + let new_target = RefTarget::normal(CommitId::from_bytes(new_oid.as_bytes())); + if let RefName::LocalBranch(branch) = &parsed_ref_name { + mut_repo.set_remote_branch_target( + branch, + REMOTE_NAME_FOR_LOCAL_GIT_REPO, + new_target.clone(), + ); + } + mut_repo.set_git_ref_target(&git_ref_name, new_target); } } failed_branches.sort_by_key(|failed| failed.name.clone()); @@ -533,6 +554,7 @@ fn diff_refs_to_export( target .remote_targets .keys() + .filter(|&remote| remote != REMOTE_NAME_FOR_LOCAL_GIT_REPO) .map(|remote| RefName::RemoteBranch { branch: branch.to_string(), remote: remote.to_string(), @@ -777,6 +799,13 @@ pub fn remove_remote( GitRemoteManagementError::InternalGitError(err) } })?; + if remote_name != REMOTE_NAME_FOR_LOCAL_GIT_REPO { + remove_remote_refs(mut_repo, remote_name); + } + Ok(()) +} + +fn remove_remote_refs(mut_repo: &mut MutableRepo, remote_name: &str) { let mut branches_to_delete = vec![]; for (branch, target) in mut_repo.view().branches() { if target.remote_targets.contains_key(remote_name) { @@ -797,7 +826,6 @@ pub fn remove_remote( for git_ref in git_refs_to_delete { mut_repo.set_git_ref_target(&git_ref, RefTarget::absent()); } - Ok(()) } pub fn rename_remote( @@ -820,6 +848,13 @@ pub fn rename_remote( GitRemoteManagementError::InternalGitError(err) } })?; + if old_remote_name != REMOTE_NAME_FOR_LOCAL_GIT_REPO { + rename_remote_refs(mut_repo, old_remote_name, new_remote_name); + } + Ok(()) +} + +fn rename_remote_refs(mut_repo: &mut MutableRepo, old_remote_name: &str, new_remote_name: &str) { mut_repo.rename_remote(old_remote_name, new_remote_name); let prefix = format!("refs/remotes/{old_remote_name}/"); let git_refs = mut_repo @@ -840,7 +875,6 @@ pub fn rename_remote( mut_repo.set_git_ref_target(&old, RefTarget::absent()); mut_repo.set_git_ref_target(&new, target); } - Ok(()) } #[derive(Error, Debug)] diff --git a/lib/src/protos/op_store.proto b/lib/src/protos/op_store.proto index 87de2db7e8..f7b4799656 100644 --- a/lib/src/protos/op_store.proto +++ b/lib/src/protos/op_store.proto @@ -82,6 +82,8 @@ message View { // TODO: Delete support for the old format. bytes git_head_legacy = 7 [deprecated = true]; RefTarget git_head = 9; + // Whether "@git" branches have been migrated to remote_targets. + bool has_git_refs_migrated_to_remote = 10; } message Operation { diff --git a/lib/src/protos/op_store.rs b/lib/src/protos/op_store.rs index 58180d0ea7..fdd8671697 100644 --- a/lib/src/protos/op_store.rs +++ b/lib/src/protos/op_store.rs @@ -120,6 +120,9 @@ pub struct View { pub git_head_legacy: ::prost::alloc::vec::Vec, #[prost(message, optional, tag = "9")] pub git_head: ::core::option::Option, + /// Whether "@git" branches have been migrated to remote_targets. + #[prost(bool, tag = "10")] + pub has_git_refs_migrated_to_remote: bool, } #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index 39fac510b8..e175aabc96 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -242,7 +242,11 @@ fn operation_from_proto(proto: crate::protos::op_store::Operation) -> Operation } fn view_to_proto(view: &View) -> crate::protos::op_store::View { - let mut proto = crate::protos::op_store::View::default(); + let mut proto = crate::protos::op_store::View { + // New/loaded view should have been migrated to the latest format + has_git_refs_migrated_to_remote: true, + ..Default::default() + }; for (workspace_id, commit_id) in &view.wc_commit_ids { proto .wc_commit_ids @@ -355,7 +359,10 @@ fn view_from_proto(proto: crate::protos::op_store::View) -> View { view.git_head = RefTarget::normal(CommitId::new(proto.git_head_legacy)); } - migrate_git_refs_to_remote(&mut view); + if !proto.has_git_refs_migrated_to_remote { + migrate_git_refs_to_remote(&mut view); + } + view } @@ -371,6 +378,16 @@ fn migrate_git_refs_to_remote(view: &mut View) { .remote_targets .remove(git::REMOTE_NAME_FOR_LOCAL_GIT_REPO); } + for (full_name, target) in &view.git_refs { + if let Some(name) = full_name.strip_prefix("refs/heads/") { + assert!(!name.is_empty()); + let branch_target = view.branches.entry(name.to_owned()).or_default(); + branch_target.remote_targets.insert( + git::REMOTE_NAME_FOR_LOCAL_GIT_REPO.to_owned(), + target.clone(), + ); + } + } // jj < 0.9 might have imported refs from remote named "git" let reserved_git_ref_prefix = format!("refs/remotes/{}/", git::REMOTE_NAME_FOR_LOCAL_GIT_REPO); @@ -609,6 +626,7 @@ mod tests { git_ref_to_proto("refs/remotes/git/main", &normal_ref_target("555555")), git_ref_to_proto("refs/remotes/gita/main", &normal_ref_target("666666")), ], + has_git_refs_migrated_to_remote: false, ..Default::default() }; @@ -619,6 +637,7 @@ mod tests { "main".to_owned() => BranchTarget { local_target: normal_ref_target("111111"), remote_targets: btreemap! { + "git".to_owned() => normal_ref_target("444444"), // refs/heads/main "gita".to_owned() => normal_ref_target("333333"), }, }, @@ -631,6 +650,13 @@ mod tests { "refs/remotes/gita/main".to_owned() => normal_ref_target("666666"), }, ); + + // Once migrated, "git" remote branches shouldn't be populated again. + let mut proto = view_to_proto(&view); + assert!(proto.has_git_refs_migrated_to_remote); + proto.branches.clear(); + let view = view_from_proto(proto); + assert!(view.branches.is_empty()); } #[test] diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 95fa2b4726..763f16600f 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -123,13 +123,16 @@ fn test_import_refs() { let expected_main_branch = BranchTarget { local_target: RefTarget::normal(jj_id(&commit2)), remote_targets: btreemap! { - "origin".to_string() => RefTarget::normal(jj_id(&commit1)), + "git".to_string() => RefTarget::normal(jj_id(&commit2)), + "origin".to_string() => RefTarget::normal(jj_id(&commit1)), }, }; assert_eq!(view.get_branch("main"), Some(expected_main_branch).as_ref()); let expected_feature1_branch = BranchTarget { local_target: RefTarget::normal(jj_id(&commit3)), - remote_targets: btreemap! {}, + remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit3)), + }, }; assert_eq!( view.get_branch("feature1"), @@ -137,7 +140,9 @@ fn test_import_refs() { ); let expected_feature2_branch = BranchTarget { local_target: RefTarget::normal(jj_id(&commit4)), - remote_targets: btreemap! {}, + remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit4)), + }, }; assert_eq!( view.get_branch("feature2"), @@ -256,7 +261,8 @@ fn test_import_refs_reimport() { let expected_main_branch = BranchTarget { local_target: RefTarget::normal(jj_id(&commit2)), remote_targets: btreemap! { - "origin".to_string() => commit1_target.clone(), + "git".to_string() => RefTarget::normal(jj_id(&commit2)), + "origin".to_string() => commit1_target.clone(), }, }; assert_eq!(view.get_branch("main"), Some(expected_main_branch).as_ref()); @@ -265,7 +271,9 @@ fn test_import_refs_reimport() { [jj_id(&commit4)], [commit6.id().clone(), jj_id(&commit5)], ), - remote_targets: btreemap! {}, + remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit5)), + }, }; assert_eq!( view.get_branch("feature2"), @@ -463,6 +471,7 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { Some(&BranchTarget { local_target: RefTarget::normal(jj_id(&commit_remote_and_local)), remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), }, }), @@ -482,10 +491,18 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { let view = repo.view(); // The local branches were indeed deleted - assert_eq!(view.branches().len(), 1); - view.get_branch("main").unwrap(); // branch #1 of 1 - assert_eq!(view.get_branch("feature-remote-local"), None); - assert_eq!(view.get_branch("feature-remote-and-local"), None); + assert_eq!(view.branches().len(), 2); + assert!(view.get_branch("main").is_some()); + assert!(view.get_branch("feature-remote-only").is_none()); + assert_eq!( + view.get_branch("feature-remote-and-local"), + Some(&BranchTarget { + local_target: RefTarget::absent(), + remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), + }, + }), + ); let expected_heads = hashset! { jj_id(&commit_main), // Neither commit_remote_only nor commit_remote_and_local should be @@ -552,6 +569,7 @@ fn test_import_refs_reimport_with_moved_remote_ref() { Some(&BranchTarget { local_target: RefTarget::normal(jj_id(&commit_remote_and_local)), remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), }, }), @@ -596,6 +614,7 @@ fn test_import_refs_reimport_with_moved_remote_ref() { Some(&BranchTarget { local_target: RefTarget::normal(jj_id(&new_commit_remote_and_local)), remote_targets: btreemap! { + "git".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)), "origin".to_string() => RefTarget::normal(jj_id(&new_commit_remote_and_local)), }, }),