From a07574a2338730a7c8fef13b487c74c2ec41808a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 28 Jun 2023 19:32:49 +0900 Subject: [PATCH] git: pass RefName enum to git_ref_filter callback I think it's slightly better to compare each ref fragment than building "refs/remotes/{remote}/{branch}" pattern to be matched. --- lib/src/git.rs | 19 +++++++++++++------ lib/tests/test_git.rs | 20 +++++++++++++++----- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/lib/src/git.rs b/lib/src/git.rs index 315e7e16d..55f6fd0ba 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -147,7 +147,7 @@ pub fn import_some_refs( mut_repo: &mut MutableRepo, git_repo: &git2::Repository, git_settings: &GitSettings, - git_ref_filter: impl Fn(&str) -> bool, + git_ref_filter: impl Fn(&RefName) -> bool, ) -> Result<(), GitImportError> { let store = mut_repo.store().clone(); let mut jj_view_git_refs = mut_repo.view().git_refs().clone(); @@ -200,7 +200,7 @@ pub fn import_some_refs( continue; }; pinned_git_heads.insert(ref_name.clone(), vec![id.clone()]); - if !git_ref_filter(full_name) { + if !git_ref_filter(&ref_name) { continue; } // TODO: Make it configurable which remotes are publishing and update public @@ -218,7 +218,7 @@ pub fn import_some_refs( for (full_name, target) in jj_view_git_refs { // TODO: or clean up invalid ref in case it was stored due to historical bug? let ref_name = parse_git_ref(&full_name).expect("stored git ref should be parsable"); - if git_ref_filter(&full_name) { + if git_ref_filter(&ref_name) { mut_repo.remove_git_ref(&full_name); changed_git_refs.insert(ref_name, (Some(target), None)); } else { @@ -520,16 +520,23 @@ pub fn fetch( tracing::debug!("import_refs"); if let Some(globs) = branch_name_globs { let pattern = format!( - "^refs/remotes/{remote_name}/({})$", + "^({})$", globs.iter().map(|glob| glob.replace('*', ".*")).join("|") ); tracing::debug!(?globs, ?pattern, "globs as regex"); - let regex = regex::Regex::new(&pattern).map_err(|_| GitFetchError::InvalidGlob)?; + let branch_regex = regex::Regex::new(&pattern).map_err(|_| GitFetchError::InvalidGlob)?; import_some_refs( mut_repo, git_repo, git_settings, - move |git_ref_name: &str| -> bool { regex.is_match(git_ref_name) }, + |ref_name: &RefName| -> bool { + match ref_name { + RefName::RemoteBranch { branch, remote } => { + remote == remote_name && branch_regex.is_match(branch) + } + RefName::LocalBranch(..) | RefName::Tag(..) | RefName::GitRef(..) => false, + } + }, ) } else { import_refs(mut_repo, git_repo, git_settings) diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index a1452a9f5..27bc8134c 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -30,6 +30,7 @@ use jujutsu_lib::git_backend::GitBackend; use jujutsu_lib::op_store::{BranchTarget, RefTarget}; use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; use jujutsu_lib::settings::{GitSettings, UserSettings}; +use jujutsu_lib::view::RefName; use maplit::{btreemap, hashset}; use tempfile::TempDir; use testutils::{create_random_commit, load_repo_at_head, write_random_commit, TestRepo}; @@ -687,10 +688,19 @@ fn test_import_some_refs() { empty_git_commit(&git_repo, "refs/remotes/origin/feature4", &[&commit_feat3]); let commit_ign = empty_git_commit(&git_repo, "refs/remotes/origin/ignored", &[]); + fn get_remote_branch(ref_name: &RefName) -> Option<&str> { + match ref_name { + RefName::RemoteBranch { branch, remote } if remote == "origin" => Some(branch), + _ => None, + } + } + // Import branches feature1, feature2, and feature3. let mut tx = repo.start_transaction(&settings, "test"); git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { - ref_name.starts_with("refs/remotes/origin/feature") + get_remote_branch(ref_name) + .map(|branch| branch.starts_with("feature")) + .unwrap_or(false) }) .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); @@ -755,7 +765,7 @@ fn test_import_some_refs() { delete_git_ref(&git_repo, "refs/remotes/origin/feature4"); let mut tx = repo.start_transaction(&settings, "test"); git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { - ref_name == "refs/remotes/origin/feature2" + get_remote_branch(ref_name) == Some("feature2") }) .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); @@ -771,7 +781,7 @@ fn test_import_some_refs() { // corresponding commit should stay because it is reachable from feature2. let mut tx = repo.start_transaction(&settings, "test"); git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { - ref_name == "refs/remotes/origin/feature1" + get_remote_branch(ref_name) == Some("feature1") }) .unwrap(); // No descendant should be rewritten. @@ -788,7 +798,7 @@ fn test_import_some_refs() { // feature4 should be left alone even though it is no longer in git. let mut tx = repo.start_transaction(&settings, "test"); git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { - ref_name == "refs/remotes/origin/feature3" + get_remote_branch(ref_name) == Some("feature3") }) .unwrap(); // No descendant should be rewritten @@ -804,7 +814,7 @@ fn test_import_some_refs() { // Import feature4: both the head and the branch will disappear. let mut tx = repo.start_transaction(&settings, "test"); git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { - ref_name == "refs/remotes/origin/feature4" + get_remote_branch(ref_name) == Some("feature4") }) .unwrap(); // No descendant should be rewritten