From 35ea607abd581cea842748527e92577878d94a1c Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 13 Oct 2023 04:06:10 +0900 Subject: [PATCH] view: make get_remote_branch() return RemoteRef instead of RefTarget I'm going to add tracking state to RemoteRef, and we should compare both target and state in the tests. --- cli/src/commands/git.rs | 12 ++- lib/src/repo.rs | 4 +- lib/src/revset.rs | 2 +- lib/src/view.rs | 15 ++-- lib/tests/test_git.rs | 147 ++++++++++++++++++++++++++++--------- lib/tests/test_mut_repo.rs | 12 +-- lib/tests/test_rewrite.rs | 14 ++-- 7 files changed, 143 insertions(+), 63 deletions(-) diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 9dc4f0739d..110492cde0 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -491,11 +491,11 @@ fn cmd_git_clone( let (mut workspace_command, git_repo, stats) = clone_result?; if let Some(default_branch) = &stats.default_branch { - let default_branch_target = workspace_command + let default_branch_remote_ref = workspace_command .repo() .view() .get_remote_branch(default_branch, "origin"); - if let Some(commit_id) = default_branch_target.as_normal().cloned() { + if let Some(commit_id) = default_branch_remote_ref.target.as_normal().cloned() { let mut checkout_tx = workspace_command.start_transaction("check out git remote's default branch"); if args.colocate { @@ -730,7 +730,7 @@ fn cmd_git_push( } let targets = TrackingRefPair { local_target: repo.view().get_local_branch(branch_name), - remote_target: repo.view().get_remote_branch(branch_name, &remote), + remote_target: &repo.view().get_remote_branch(branch_name, &remote).target, }; if targets.local_target.is_absent() && targets.remote_target.is_absent() { return Err(user_error(format!("Branch {branch_name} doesn't exist"))); @@ -784,7 +784,11 @@ fn cmd_git_push( .set_local_branch_target(&branch_name, RefTarget::normal(commit.id().clone())); let targets = TrackingRefPair { local_target: tx.repo().view().get_local_branch(&branch_name), - remote_target: tx.repo().view().get_remote_branch(&branch_name, &remote), + remote_target: &tx + .repo() + .view() + .get_remote_branch(&branch_name, &remote) + .target, }; match classify_branch_update(&branch_name, &remote, targets) { Ok(Some(update)) => branch_updates.push((branch_name.clone(), update)), diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 8f91af5411..2274b380e7 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -42,7 +42,7 @@ use crate::git_backend::GitBackend; use crate::index::{HexPrefix, Index, IndexStore, MutableIndex, PrefixResolution, ReadonlyIndex}; use crate::local_backend::LocalBackend; use crate::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore}; -use crate::op_store::{OpStore, OpStoreError, OperationId, RefTarget, WorkspaceId}; +use crate::op_store::{OpStore, OpStoreError, OperationId, RefTarget, RemoteRef, WorkspaceId}; use crate::operation::Operation; use crate::refs::{diff_named_refs, merge_ref_targets}; use crate::revset::{self, ChangeIdIndex, Revset, RevsetExpression}; @@ -998,7 +998,7 @@ impl MutableRepo { self.view_mut().set_local_branch_target(name, target); } - pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> RefTarget { + pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> RemoteRef { self.view .with_ref(|v| v.get_remote_branch(name, remote_name).clone()) } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 1ae023e9ec..6531a67397 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -2018,7 +2018,7 @@ fn resolve_remote_branch(repo: &dyn Repo, name: &str, remote: &str) -> Option view.git_head(), - (name, remote) => view.get_remote_branch(name, remote), + (name, remote) => &view.get_remote_branch(name, remote).target, }; target .is_present() diff --git a/lib/src/view.rs b/lib/src/view.rs index 960499b1d1..ae0d5dbf8d 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -130,7 +130,9 @@ impl View { pub fn get_ref(&self, name: &RefName) -> &RefTarget { match &name { RefName::LocalBranch(name) => self.get_local_branch(name), - RefName::RemoteBranch { branch, remote } => self.get_remote_branch(branch, remote), + RefName::RemoteBranch { branch, remote } => { + &self.get_remote_branch(branch, remote).target + } RefName::Tag(name) => self.get_tag(name), RefName::GitRef(name) => self.get_git_ref(name), } @@ -211,16 +213,11 @@ impl View { .flatten() } - pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> &RefTarget { - // TODO: maybe return RemoteRef instead of RefTarget? + pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> &RemoteRef { if let Some(remote_view) = self.data.remote_views.get(remote_name) { - let maybe_target = remote_view - .branches - .get(name) - .map(|remote_ref| &remote_ref.target); - maybe_target.flatten() + remote_view.branches.get(name).flatten() } else { - RefTarget::absent_ref() + RemoteRef::absent_ref() } } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index a5804b5eac..75e0ba6314 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -127,11 +127,15 @@ fn test_import_refs() { ); assert_eq!( view.get_remote_branch("main", "git"), - &RefTarget::normal(jj_id(&commit2)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit2)), + }, ); assert_eq!( view.get_remote_branch("main", "origin"), - &RefTarget::normal(jj_id(&commit1)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit1)), + }, ); assert_eq!( view.get_local_branch("feature1"), @@ -139,7 +143,9 @@ fn test_import_refs() { ); assert_eq!( view.get_remote_branch("feature1", "git"), - &RefTarget::normal(jj_id(&commit3)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit3)), + }, ); assert!(view.get_remote_branch("feature1", "origin").is_absent()); assert_eq!( @@ -148,7 +154,9 @@ fn test_import_refs() { ); assert_eq!( view.get_remote_branch("feature2", "git"), - &RefTarget::normal(jj_id(&commit4)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit4)), + }, ); assert!(view.get_remote_branch("feature2", "origin").is_absent()); assert_eq!( @@ -158,7 +166,9 @@ fn test_import_refs() { assert!(view.get_remote_branch("feature3", "git").is_absent()); assert_eq!( view.get_remote_branch("feature3", "origin"), - &RefTarget::normal(jj_id(&commit6)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit6)), + }, ); assert_eq!(view.get_tag("v1.0"), &RefTarget::normal(jj_id(&commit5))); @@ -266,16 +276,25 @@ fn test_import_refs_reimport() { ); assert_eq!( view.get_remote_branch("main", "git"), - &RefTarget::normal(jj_id(&commit2)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit2)), + }, + ); + assert_eq!( + view.get_remote_branch("main", "origin"), + &RemoteRef { + target: commit1_target.clone(), + }, ); - assert_eq!(view.get_remote_branch("main", "origin"), &commit1_target); assert_eq!( view.get_local_branch("feature2"), &RefTarget::from_legacy_form([jj_id(&commit4)], [commit6.id().clone(), jj_id(&commit5)]) ); assert_eq!( view.get_remote_branch("feature2", "git"), - &RefTarget::normal(jj_id(&commit5)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit5)), + }, ); assert!(view.get_remote_branch("feature2", "origin").is_absent()); @@ -466,7 +485,9 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { .is_absent()); assert_eq!( view.get_remote_branch("feature-remote-only", "origin"), - &RefTarget::normal(jj_id(&commit_remote_only)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_only)), + }, ); assert_eq!( view.get_local_branch("feature-remote-and-local"), @@ -474,11 +495,15 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "git"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "origin"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert!(view.has_branch("main")); // branch #3 of 3 @@ -503,7 +528,9 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { .is_absent()); assert_eq!( view.get_remote_branch("feature-remote-and-local", "git"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert!(view .get_remote_branch("feature-remote-and-local", "origin") @@ -570,7 +597,9 @@ fn test_import_refs_reimport_with_moved_remote_ref() { .is_absent()); assert_eq!( view.get_remote_branch("feature-remote-only", "origin"), - &RefTarget::normal(jj_id(&commit_remote_only)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_only)), + }, ); assert_eq!( view.get_local_branch("feature-remote-and-local"), @@ -578,11 +607,15 @@ fn test_import_refs_reimport_with_moved_remote_ref() { ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "git"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "origin"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert!(view.has_branch("main")); // branch #3 of 3 @@ -619,7 +652,9 @@ fn test_import_refs_reimport_with_moved_remote_ref() { .is_absent()); assert_eq!( view.get_remote_branch("feature-remote-only", "origin"), - &RefTarget::normal(jj_id(&new_commit_remote_only)) + &RemoteRef { + target: RefTarget::normal(jj_id(&new_commit_remote_only)), + }, ); assert_eq!( view.get_local_branch("feature-remote-and-local"), @@ -627,11 +662,15 @@ fn test_import_refs_reimport_with_moved_remote_ref() { ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "git"), - &RefTarget::normal(jj_id(&commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&commit_remote_and_local)), + }, ); assert_eq!( view.get_remote_branch("feature-remote-and-local", "origin"), - &RefTarget::normal(jj_id(&new_commit_remote_and_local)) + &RemoteRef { + target: RefTarget::normal(jj_id(&new_commit_remote_and_local)), + }, ); assert!(view.has_branch("main")); // branch #3 of 3 let expected_heads = hashset! { @@ -733,7 +772,9 @@ fn test_import_refs_reimport_conflicted_remote_branch() { ); assert_eq!( repo.view().get_remote_branch("main", "origin"), - &RefTarget::from_legacy_form([], [jj_id(&commit1), jj_id(&commit2)]), + &RemoteRef { + target: RefTarget::from_legacy_form([], [jj_id(&commit1), jj_id(&commit2)]), + }, ); // The conflict can be resolved by importing the current Git state @@ -746,7 +787,9 @@ fn test_import_refs_reimport_conflicted_remote_branch() { ); assert_eq!( repo.view().get_remote_branch("main", "origin"), - &RefTarget::normal(jj_id(&commit2)), + &RemoteRef { + target: RefTarget::normal(jj_id(&commit2)), + }, ); } @@ -814,17 +857,25 @@ fn test_import_some_refs() { // Check that branches feature[1-4] have been locally imported and are known to // be present on origin as well. assert_eq!(view.branches().count(), 4); - let commit_feat1_target = RefTarget::normal(jj_id(&commit_feat1)); - let commit_feat2_target = RefTarget::normal(jj_id(&commit_feat2)); - let commit_feat3_target = RefTarget::normal(jj_id(&commit_feat3)); - let commit_feat4_target = RefTarget::normal(jj_id(&commit_feat4)); + let commit_feat1_remote_ref = RemoteRef { + target: RefTarget::normal(jj_id(&commit_feat1)), + }; + let commit_feat2_remote_ref = RemoteRef { + target: RefTarget::normal(jj_id(&commit_feat2)), + }; + let commit_feat3_remote_ref = RemoteRef { + target: RefTarget::normal(jj_id(&commit_feat3)), + }; + let commit_feat4_remote_ref = RemoteRef { + target: RefTarget::normal(jj_id(&commit_feat4)), + }; assert_eq!( view.get_local_branch("feature1"), &RefTarget::normal(jj_id(&commit_feat1)) ); assert_eq!( view.get_remote_branch("feature1", "origin"), - &commit_feat1_target + &commit_feat1_remote_ref ); assert_eq!( view.get_local_branch("feature2"), @@ -832,7 +883,7 @@ fn test_import_some_refs() { ); assert_eq!( view.get_remote_branch("feature2", "origin"), - &commit_feat2_target + &commit_feat2_remote_ref ); assert_eq!( view.get_local_branch("feature3"), @@ -840,7 +891,7 @@ fn test_import_some_refs() { ); assert_eq!( view.get_remote_branch("feature3", "origin"), - &commit_feat3_target + &commit_feat3_remote_ref ); assert_eq!( view.get_local_branch("feature4"), @@ -848,7 +899,7 @@ fn test_import_some_refs() { ); assert_eq!( view.get_remote_branch("feature4", "origin"), - &commit_feat4_target + &commit_feat4_remote_ref ); assert!(!view.has_branch("main")); assert!(!view.heads().contains(&jj_id(&commit_main))); @@ -1310,7 +1361,9 @@ fn test_import_export_no_auto_local_branch() { assert!(mut_repo.view().get_local_branch("main").is_absent()); assert_eq!( mut_repo.view().get_remote_branch("main", "origin"), - &RefTarget::normal(jj_id(&git_commit)) + &RemoteRef { + target: RefTarget::normal(jj_id(&git_commit)), + }, ); assert_eq!( mut_repo.get_git_ref("refs/remotes/origin/main"), @@ -1369,11 +1422,15 @@ fn test_export_conflicts() { // Conflicted branches shouldn't be copied to the "git" remote assert_eq!( mut_repo.get_remote_branch("feature", "git"), - RefTarget::normal(commit_a.id().clone()) + RemoteRef { + target: RefTarget::normal(commit_a.id().clone()), + }, ); assert_eq!( mut_repo.get_remote_branch("main", "git"), - RefTarget::normal(commit_b.id().clone()) + RemoteRef { + target: RefTarget::normal(commit_b.id().clone()), + }, ); } @@ -1443,7 +1500,12 @@ fn test_export_partial_failure() { // Failed branches shouldn't be copied to the "git" remote assert!(mut_repo.get_remote_branch("", "git").is_absent()); assert!(mut_repo.get_remote_branch("HEAD", "git").is_absent()); - assert_eq!(mut_repo.get_remote_branch("main", "git"), target); + assert_eq!( + mut_repo.get_remote_branch("main", "git"), + RemoteRef { + target: target.clone(), + }, + ); assert!(mut_repo.get_remote_branch("main/sub", "git").is_absent()); // Now remove the `main` branch and make sure that the `main/sub` gets exported @@ -1471,7 +1533,12 @@ fn test_export_partial_failure() { assert!(mut_repo.get_remote_branch("", "git").is_absent()); assert!(mut_repo.get_remote_branch("HEAD", "git").is_absent()); assert!(mut_repo.get_remote_branch("main", "git").is_absent()); - assert_eq!(mut_repo.get_remote_branch("main/sub", "git"), target); + assert_eq!( + mut_repo.get_remote_branch("main/sub", "git"), + RemoteRef { + target: target.clone(), + }, + ); } #[test] @@ -1634,7 +1701,12 @@ fn test_export_undo_reexport() { Some(git_id(&commit_a)) ); assert_eq!(mut_repo.get_git_ref("refs/heads/main"), target_a); - assert_eq!(mut_repo.get_remote_branch("main", "git"), target_a); + assert_eq!( + mut_repo.get_remote_branch("main", "git"), + RemoteRef { + target: target_a.clone(), + }, + ); // Undo remote changes only mut_repo.set_remote_branch_target("main", "git", RefTarget::absent()); @@ -1646,7 +1718,12 @@ fn test_export_undo_reexport() { Some(git_id(&commit_a)) ); assert_eq!(mut_repo.get_git_ref("refs/heads/main"), target_a); - assert_eq!(mut_repo.get_remote_branch("main", "git"), target_a); + assert_eq!( + mut_repo.get_remote_branch("main", "git"), + RemoteRef { + target: target_a.clone(), + }, + ); } #[test] diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index 069ba672de..a064367e17 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use jj_lib::op_store::{RefTarget, WorkspaceId}; +use jj_lib::op_store::{RefTarget, RemoteRef, WorkspaceId}; use jj_lib::repo::Repo; use maplit::hashset; use testutils::{ @@ -590,12 +590,14 @@ fn test_rename_remote() { let mut tx = repo.start_transaction(&settings, "test"); let mut_repo = tx.mut_repo(); let commit = write_random_commit(mut_repo, &settings); - let target = RefTarget::normal(commit.id().clone()); - mut_repo.set_remote_branch_target("main", "origin", target.clone()); + let remote_ref = RemoteRef { + target: RefTarget::normal(commit.id().clone()), + }; + mut_repo.set_remote_branch_target("main", "origin", remote_ref.target.clone()); mut_repo.rename_remote("origin", "upstream"); - assert_eq!(mut_repo.get_remote_branch("main", "upstream"), target); + assert_eq!(mut_repo.get_remote_branch("main", "upstream"), remote_ref); assert_eq!( mut_repo.get_remote_branch("main", "origin"), - RefTarget::absent() + RemoteRef::absent() ); } diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 50147094a1..12043d60b2 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -13,7 +13,7 @@ // limitations under the License. use itertools::Itertools as _; -use jj_lib::op_store::{RefTarget, WorkspaceId}; +use jj_lib::op_store::{RefTarget, RemoteRef, WorkspaceId}; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; use jj_lib::rewrite::DescendantRebaser; @@ -1000,13 +1000,13 @@ fn test_rebase_descendants_basic_branch_update_with_non_local_branch() { let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); let commit_a = graph_builder.initial_commit(); let commit_b = graph_builder.commit_with_parents(&[&commit_a]); + let commit_b_remote_ref = RemoteRef { + target: RefTarget::normal(commit_b.id().clone()), + }; tx.mut_repo() .set_local_branch_target("main", RefTarget::normal(commit_b.id().clone())); - tx.mut_repo().set_remote_branch_target( - "main", - "origin", - RefTarget::normal(commit_b.id().clone()), - ); + tx.mut_repo() + .set_remote_branch_target("main", "origin", commit_b_remote_ref.target.clone()); tx.mut_repo() .set_tag_target("v1", RefTarget::normal(commit_b.id().clone())); let repo = tx.commit(); @@ -1025,7 +1025,7 @@ fn test_rebase_descendants_basic_branch_update_with_non_local_branch() { // The remote branch and tag should not get updated assert_eq!( tx.mut_repo().get_remote_branch("main", "origin"), - RefTarget::normal(commit_b.id().clone()) + commit_b_remote_ref, ); assert_eq!( tx.mut_repo().get_tag("v1"),