TESTS CHANGED branch forget: force jj git export of relevant branches

This is a bit unwieldy; see a comment in the code for a detailed explanation.

We'll have to decide whether we prefer this, or we prefer the approach of
jj-vcs#1714, or we have a better idea.

src/commands/branch.rs

@@ -3,7 +3,8 @@ use std::collections::{BTreeSet, HashSet};
 use clap::builder::NonEmptyStringValueParser;
 use itertools::Itertools;
 use jujutsu_lib::backend::{CommitId, ObjectId};
-use jujutsu_lib::git::local_branch_git_tracking_refs;
+use jujutsu_lib::git::{self, local_branch_git_tracking_refs};
+use jujutsu_lib::git_backend::GitBackend;
 use jujutsu_lib::op_store::{BranchTarget, RefTarget};
 use jujutsu_lib::repo::Repo;
 use jujutsu_lib::revset::{self, RevsetExpression};
@@ -74,11 +75,24 @@ pub struct BranchListArgs {
     revisions: Vec<RevisionArg>,
 }
 
-/// Forget everything about a branch, including its local and remote
-/// targets.
+/// Forget everything about a branch, including its local and remote targets.
 ///
 /// A forgotten branch will not impact remotes on future pushes. It will be
 /// recreated on future pulls if it still exists in the remote.
+///
+/// jj will immediately attempt to remove this branch and the corresponding
+/// remote-tracking branches from the local git repo.
+///
+/// NOTE: This happens even if a repo is not colocated and `jj git export` is
+/// not already applied after every command. If you are using a non-colocated
+/// repo and are counting on this command not touching the git repo's branch
+/// refs, please report your use case on our bug tracker.
+//
+// More details on the above note: The issue is that if the remote-tracking branch in the local git
+// repo wasn't immediately deleted, and that branch didn't move on a remote, a subsequent `jj git
+// fetch` would think the branch is unchanged and wouldn't resurrect the branch in jj (and there
+// wouldn't be a good way to resurrect it for the user). If the branch did move on the remote, there
+// would be a conflict. See also `test_branch_forget_fetched_branch`.
 #[derive(clap::Args, Clone, Debug)]
 pub struct BranchForgetArgs {
     /// The branches to forget.
@@ -296,6 +310,16 @@ fn cmd_branch_forget(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let view = workspace_command.repo().view();
+    // This is based on commands::git::get_git_repo, but we don't want to abort with
+    // a user error even if the backend is not git.
+    // TODO: Some abstraction that allows the backend choose what to do here and
+    // removes GitBackend-specific code.
+    let git_repo = workspace_command
+        .repo()
+        .store()
+        .backend_impl()
+        .downcast_ref::<GitBackend>()
+        .map(|backend| backend.git_repo_clone());
     for branch_name in args.names.iter() {
         if view.get_branch(branch_name).is_none() {
             return Err(user_error(format!("No such branch: {branch_name}")));
@@ -308,6 +332,29 @@ fn cmd_branch_forget(
     for branch_name in names.iter() {
         tx.mut_repo().remove_branch(branch_name);
     }
+
+    // Immediate export of the deletion of remote-tracking branches is necessary so
+    // that subsequent `jj git fetch` work in non-colocated repositories. It would
+    // be confusing to export deletion of the remote-tracking branches and not
+    // of the local branch, even though the latter only exists if the user runs
+    // `jj git export` manually in a non-colocated repo.
+    // TODO: Create a command the user can use to reset jj's branch state to the git
+    // repo's state. In this case, `jj branch forget` doesn't work as it tries
+    // to delete the latter. One possible name is `jj git import --reset
+    // BRANCH`.
+    // TODO: Once the command described above exists, it should be mentioned in `jj
+    // help branch forget`.
+    if let Some(git_repo) = git_repo {
+        git::export_some_refs(tx.mut_repo(), &git_repo, |ref_name| match ref_name {
+            jujutsu_lib::view::RefName::LocalBranch(branch) => names.contains(branch),
+            jujutsu_lib::view::RefName::RemoteBranch { branch, remote: _ } => {
+                names.contains(branch)
+            }
+            _ => false,
+        })?;
+    } else {
+        // We don't know how to export to other backends.
+    }
     tx.finish(ui)?;
     if names.len() > 1 {
         writeln!(ui, "Forgot {} branches.", names.len())?;

tests/test_branch_command.rs

@@ -236,46 +236,24 @@ fn test_branch_forget_export() {
     // Exporting the branch to git creates a local-git tracking branch
     let stdout = test_env.jj_cmd_success(&repo_path, &["git", "export"]);
     insta::assert_snapshot!(stdout, @"");
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r=foo@git", "--no-graph"]);
+    insta::assert_snapshot!(stdout, @r###"
+    rlvkpnrzqnoo [email protected] 2001-02-03 04:05:08.000 +07:00 foo 65b6b74e0897
+    (empty) (no description set)
+    "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "forget", "foo"]);
     insta::assert_snapshot!(stdout, @"");
-    // Forgetting a branch does not delete its local-git tracking branch. This is
-    // the opposite of what happens to remote-tracking branches.
-    // TODO: Consider allowing forgetting local-git tracking branches as an option
+    // Forgetting a branch deletes its local-git tracking branch immediately
     let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
-    insta::assert_snapshot!(stdout, @r###"
-    foo (forgotten)
-      @git: 65b6b74e0897 (no description set)
-      (this branch will be deleted from the underlying Git repo on the next `jj git export`)
+    insta::assert_snapshot!(stdout, @"");
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r=foo@git", "--no-graph"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Revision "foo@git" doesn't exist
     "###);
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r=foo", "--no-graph"]);
     insta::assert_snapshot!(stderr, @r###"
     Error: Revision "foo" doesn't exist
     "###);
-    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r=foo@git", "--no-graph"]);
-    insta::assert_snapshot!(stdout, @r###"
-    rlvkpnrzqnoo [email protected] 2001-02-03 04:05:08.000 +07:00 65b6b74e0897
-    (empty) (no description set)
-    "###);
-
-    // The presence of the @git branch means that a `jj git import` is a no-op...
-    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "import"]);
-    insta::assert_snapshot!(stdout, @r###"
-    Nothing changed.
-    "###);
-    // ... and a `jj git export` will delete the branch from git and will delete the
-    // git-tracking branch. In a colocated repo, this will happen automatically
-    // immediately after a `jj branch forget`. This is demonstrated in
-    // `test_git_colocated_branch_forget` in test_git_colocated.rs
-    let stdout = test_env.jj_cmd_success(&repo_path, &["git", "export"]);
-    insta::assert_snapshot!(stdout, @"");
-    let stdout = test_env.jj_cmd_success(&repo_path, &["branch", "list"]);
-    insta::assert_snapshot!(stdout, @"");
-
-    // Note that if `jj branch forget` *did* delete foo@git, a subsequent `jj
-    // git export` would be a no-op and a `jj git import` would resurrect
-    // the branch. In a normal repo, that might be OK. In a colocated repo,
-    // this would automatically happen before the next command, making `jj
-    // branch forget` useless.
 }
 
 #[test]
@@ -333,10 +311,6 @@ fn test_branch_forget_fetched_branch() {
     // the ref in jj view's `git_refs` tracking the local git repo's remote-tracking
     // branch.
     // TODO: Show that jj git push is also a no-op
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "export"]), @"");
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["git", "import"]), @r###"
-    Nothing changed.
-    "###);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
 
     // We can fetch feature1 again.
@@ -354,10 +328,10 @@ fn test_branch_forget_fetched_branch() {
     // Short-term TODO: Fix this BUG. It should be possible to fetch `feature1`
     // again.
     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), @"");
 
     // TEST 3: fetch branch that was moved & forgotten
 
@@ -377,13 +351,11 @@ fn test_branch_forget_fetched_branch() {
     Error: No such branch: feature1
     "###);
 
-    // BUG: fetching a moved branch creates a move-deletion conflict
+    // Fetching a moved branch does not create a conflict
     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
     "###);
 }