diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f50e37975..9c990bdd4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -150,6 +150,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * It is now possible to `jj branch forget` deleted branches. [#1537](https://github.com/martinvonz/jj/issues/1537) +* `jj git fetch` can now restore branches forgotten by `jj branch forget`. + [#1714](https://github.com/martinvonz/jj/pull/1714) + ## [0.7.0] - 2023-02-16 ### Breaking changes diff --git a/lib/src/git.rs b/lib/src/git.rs index bd18e8fb96..8db69141b3 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -124,6 +124,42 @@ fn prevent_gc(git_repo: &git2::Repository, id: &CommitId) -> Result<(), git2::Er Ok(()) } +#[derive(Clone, Debug, PartialEq)] +pub enum ImportMethod { + Skip, + Merge, + /// Used by `jj git fetch` so that it can resurrect forgotten branches, even + /// if they did not move on the remote. + /// + /// See `test_branch_forget_fetched_branch` in test_branch_command.rs for a + /// demonstration. + /// + /// This is needed because: + /// + /// - `jj branch forget` deletes the jj's remote-tracking branches, but not + /// the refs tracking the git repo's remote-tracking branches. This is + /// crucial in colocated repos, where deleting that ref would cause a + /// subsequent `jj git import` to immediately resurrect the branch. + /// + /// - `jj git fetch` needs to fetch such branches, otherwise it is very + /// difficult for the user to recover a forgotten branch that they know + /// exists on the remote. + // + // TODO: This is quite hacky. A better long-term solution is need. + // + // For example, `jj branch forget` could store the list of forgotten + // branches in the repo, which `jj git import` and `jj git fetch` would know + // to ignore. We'd then need a `jj branch remember` command. + // + // Another possible solution (less hacky, but without addressing the + // usability issues) would be for `jj git push/fetch` to neither rely on nor + // modify the local git repo's remote-tracking branches in a normal jj repo. + // Then, 'jj git fetch' would respond correctly when jj's remote-tracking + // branches are removed. In a colocated repo, it would be `jj git export`'s + // job to update the git repo's remote-tracking branches. + MergeOrResurrect, +} + /// Reflect changes made in the underlying Git repo in the Jujutsu repo. /// /// This function detects conflicts (if both Git and JJ modified a branch) and @@ -133,7 +169,7 @@ pub fn import_refs( git_repo: &git2::Repository, git_settings: &GitSettings, ) -> Result<(), GitImportError> { - import_some_refs(mut_repo, git_repo, git_settings, |_| true) + import_some_refs(mut_repo, git_repo, git_settings, |_| ImportMethod::Merge) } /// Reflect changes made in the underlying Git repo in the Jujutsu repo. @@ -144,7 +180,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(&str) -> ImportMethod, ) -> Result<(), GitImportError> { let store = mut_repo.store().clone(); let mut jj_view_git_refs = mut_repo.view().git_refs().clone(); @@ -182,9 +218,10 @@ pub fn import_some_refs( continue; } let full_name = git_repo_ref.name().unwrap().to_string(); - if let Some(RefName::RemoteBranch { branch, remote: _ }) = parse_git_ref(&full_name) { + let ref_name = parse_git_ref(&full_name); + if let Some(RefName::RemoteBranch { branch, remote: _ }) = &ref_name { // "refs/remotes/origin/HEAD" isn't a real remote-tracking branch - if &branch == "HEAD" { + if branch == "HEAD" { continue; } } @@ -197,13 +234,36 @@ pub fn import_some_refs( continue; }; pinned_git_heads.insert(full_name.to_string(), vec![id.clone()]); - if !git_ref_filter(&full_name) { - continue; - } + + let allow_resurrection = match git_ref_filter(&full_name) { + ImportMethod::Skip => continue, + ImportMethod::Merge => false, + ImportMethod::MergeOrResurrect => true, + }; // TODO: Make it configurable which remotes are publishing and update public // heads here. - let old_target = jj_view_git_refs.remove(&full_name); let new_target = Some(RefTarget::Normal(id.clone())); + let old_target = jj_view_git_refs.remove(&full_name); + let old_target = if allow_resurrection + && ref_name + .map(|ref_name| mut_repo.view().get_ref(&ref_name).is_none()) + .unwrap_or(false) + { + // Normally, if the jj local ref is None and `new_target` isn't, the updated + // value of the branch will either still be None (if + // `new_target==old_target`, which is common) or become a conflict + // (if the branch moved on the remote). + // + // Changing the `old_target` to None will cause the jj local ref to get + // resurrected from `new_target`. Since we only do this when the local ref does + // not exist, this will never cause conflicts. + // + // This behavior will occur for remote-tracking branches during a `jj git + // fetch`. For motivation, see the docstring of ImportMethod::MergeOrResurrect. + None + } else { + old_target + }; if new_target != old_target { prevent_gc(git_repo, &id)?; mut_repo.set_git_ref(full_name.clone(), RefTarget::Normal(id.clone())); @@ -213,7 +273,7 @@ pub fn import_some_refs( } } for (full_name, target) in jj_view_git_refs { - if git_ref_filter(&full_name) { + if git_ref_filter(&full_name) != ImportMethod::Skip { mut_repo.remove_git_ref(&full_name); changed_git_refs.insert(full_name, (Some(target), None)); } else { @@ -526,10 +586,27 @@ pub fn fetch( mut_repo, git_repo, git_settings, - move |git_ref_name: &str| -> bool { regex.is_match(git_ref_name) }, + move |git_ref_name: &str| -> ImportMethod { + if regex.is_match(git_ref_name) { + ImportMethod::MergeOrResurrect + } else { + ImportMethod::Skip + } + }, ) } else { - import_refs(mut_repo, git_repo, git_settings) + import_some_refs( + mut_repo, + git_repo, + git_settings, + move |git_ref_name: &str| -> ImportMethod { + if git_ref_name.starts_with(&format!("refs/remotes/{remote_name}")) { + ImportMethod::MergeOrResurrect + } else { + ImportMethod::Merge + } + }, + ) } .map_err(|err| match err { GitImportError::InternalGitError(source) => GitFetchError::InternalGitError(source), diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 11cec346e3..979a6ca95f 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -24,8 +24,7 @@ use jujutsu_lib::backend::{ }; use jujutsu_lib::commit::Commit; use jujutsu_lib::commit_builder::CommitBuilder; -use jujutsu_lib::git; -use jujutsu_lib::git::{GitFetchError, GitPushError, GitRefUpdate}; +use jujutsu_lib::git::{self, GitFetchError, GitPushError, GitRefUpdate, ImportMethod}; use jujutsu_lib::git_backend::GitBackend; use jujutsu_lib::op_store::{BranchTarget, RefTarget}; use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; @@ -71,6 +70,14 @@ fn get_git_repo(repo: &Arc) -> git2::Repository { .git_repo_clone() } +fn into_import_method(select: bool) -> ImportMethod { + if select { + ImportMethod::Merge + } else { + ImportMethod::Skip + } +} + #[test] fn test_import_refs() { let settings = testutils::user_settings(); @@ -690,7 +697,7 @@ fn test_import_some_refs() { // 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") + into_import_method(ref_name.starts_with("refs/remotes/origin/feature")) }) .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); @@ -755,7 +762,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" + into_import_method(ref_name == "refs/remotes/origin/feature2") }) .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); @@ -771,7 +778,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" + into_import_method(ref_name == "refs/remotes/origin/feature1") }) .unwrap(); // No descendant should be rewritten. @@ -788,7 +795,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" + into_import_method(ref_name == "refs/remotes/origin/feature3") }) .unwrap(); // No descendant should be rewritten @@ -804,7 +811,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" + into_import_method(ref_name == "refs/remotes/origin/feature4") }) .unwrap(); // No descendant should be rewritten @@ -818,6 +825,42 @@ fn test_import_some_refs() { jj_id(&commit_feat2), }; assert_eq!(*view.heads(), expected_heads); + + // Now, let's forget feature2 + let mut tx = repo.start_transaction(&settings, "test"); + tx.mut_repo().remove_branch("feature2"); + assert_eq!(tx.mut_repo().rebase_descendants(&settings).unwrap(), 0); + let repo = tx.commit(); + + assert_eq!(repo.view().branches().len(), 0); + + // A normal import does not recreate it since the local git repo didn't change + let mut tx = repo.start_transaction(&settings, "test"); + git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { + if ref_name == "refs/remotes/origin/feature2" { + ImportMethod::Merge + } else { + ImportMethod::Skip + } + }) + .unwrap(); + let repo = tx.commit(); + + assert_eq!(repo.view().branches().len(), 0); + + // A "resurrection" import does recreate it + let mut tx = repo.start_transaction(&settings, "test"); + git::import_some_refs(tx.mut_repo(), &git_repo, &git_settings, |ref_name| { + if ref_name == "refs/remotes/origin/feature2" { + ImportMethod::MergeOrResurrect + } else { + ImportMethod::Skip + } + }) + .unwrap(); + let repo = tx.commit(); + + assert_eq!(repo.view().branches().len(), 1); } fn git_ref(git_repo: &git2::Repository, name: &str, target: Oid) { diff --git a/tests/test_branch_command.rs b/tests/test_branch_command.rs index 0d653450f6..0bc5b52d61 100644 --- a/tests/test_branch_command.rs +++ b/tests/test_branch_command.rs @@ -233,15 +233,18 @@ fn test_branch_forget_fetched_branch() { "###); insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); - // Short-term TODO: Fix this BUG. It should be possible to fetch `feature1` - // again. + // On the other hand, `jj git fetch` does resurrect the branch. This is + // important, as otherwise there wouldn't be a good way to resurrect it in + // non-colocated repos. + // + // See the docstring of `ImportMethod::MergeOrResurrect` for technical details. let stdout = test_env.jj_cmd_success(&repo_path, &["git", "fetch", "--remote=origin"]); - insta::assert_snapshot!(stdout, @r###" - Nothing changed. + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" + feature1: 9f01a0e04879 message "###); - insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); - // Move the branch in the git repo. + // Move the branch in the git repo and forget it again in jj git_repo .commit( Some("refs/heads/feature1"), @@ -252,18 +255,15 @@ fn test_branch_forget_fetched_branch() { &[&git_repo.find_commit(first_git_repo_commit).unwrap()], ) .unwrap(); - let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "forget", "feature1"]); - insta::assert_snapshot!(stderr, @r###" - Error: No such branch: feature1 - "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "forget", "feature1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @""); - // BUG: fetching a moved branch creates a move-deletion conflict + // We can fetch a forgotten branch even if it was moved on the remote. let stdout = test_env.jj_cmd_success(&repo_path, &["git", "fetch", "--remote=origin"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @r###" - feature1 (conflicted): - - 9f01a0e04879 message - + 38aefb173976 another message + feature1: 38aefb173976 another message "###); }