From fc9a791655b95f492724a9ca867b68950e3310fd Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 18 Jun 2023 18:43:57 -0700 Subject: [PATCH] git.rs: Fix bug where a forgotten branch couldn't be fetched The fix is slightly messy, but I don't know a better way without reworking the logic of `jj git fetch` and `jj git push` in-depth. This bug was originally reported by @arxanas in a [Discord discussion]. [Discord discussion]: https://discord.com/channels/968932220549103686/969829516539228222/1114997445949136936 Summary: > How do I proceed here to check out the remote branch that I know exists? > > ``` > $ jj git fetch --branch 'arxanas/fsmonitor' --remote origin > Nothing changed. > $ jj checkout arxanas/fsmonitor > Error: Revision "arxanas/fsmonitor" doesn't exist > $ jj checkout arxanas/fsmonitor@origin > Error: Revision "arxanas/fsmonitor@origin" doesn't exist > $ git show origin/arxanas/fsmonitor > commit bac43af9fbaade36a2374f4b2a7457ca4caa05ee (origin/arxanas/fsmonitor, refs/jj/keep/bac43af9fbaade36a2374f4b2a7457ca4caa05ee) > Author: Waleed Khan > Date: Fri Jun 10 18:58:21 2022 -0700 > > fsmonitor: add `.watchmanconfig` to repo > > This identifies the directory as Watchman-enabled. Additional config settings can go in this file. > > diff --git a/.watchmanconfig b/.watchmanconfig > new file mode 100644 > index 00000000..0967ef42 > --- /dev/null > +++ b/.watchmanconfig > @@ -0,0 +1 @@ > +{} > ``` --- CHANGELOG.md | 3 ++ lib/src/git.rs | 83 +++++++++++++++++++++++++++++++----- lib/tests/test_git.rs | 57 ++++++++++++++++++++++--- tests/test_branch_command.rs | 13 +++--- 4 files changed, 133 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ba3d8fda10..f2ca8fe6579 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -140,6 +140,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 remote, just like in a non-colocated repo. [#864](https://github.com/martinvonz/jj/issues/864) +* `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 c4548bbcd8a..dd57d587b8e 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -86,6 +86,36 @@ 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 branches, 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. + /// + /// Another long-term solution 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 @@ -95,7 +125,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. @@ -106,7 +136,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(); @@ -144,9 +174,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; } } @@ -159,13 +190,35 @@ pub fn import_some_refs( }; let id = CommitId::from_bytes(git_commit.id().as_bytes()); 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 + && new_target == old_target + && 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==old_target`, merging + // these three will result in the jj local ref staying nonexistent. + // + // 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())); @@ -175,7 +228,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 { @@ -488,10 +541,18 @@ 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, |_| { + ImportMethod::MergeOrResurrect + }) } .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 c40cf7afb2e..17c0a110eba 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 37907399c77..ee252905f92 100644 --- a/tests/test_branch_command.rs +++ b/tests/test_branch_command.rs @@ -233,13 +233,16 @@ 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), @""); } // TODO: Test `jj branch list` with a remote named `git`