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 bac43af (origin/arxanas/fsmonitor, refs/jj/keep/bac43af9fbaade36a2374f4b2a7457ca4caa05ee)
> Author: Waleed Khan <[email protected]>
> 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

@@ -145,6 +145,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

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),

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<ReadonlyRepo>) -> 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) {

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
     "###);
 }