From 59b354992acb2424dd2860a1f3bd645ce72d5ddb Mon Sep 17 00:00:00 2001
From: Ilya Grigoriev <ilyagr@users.noreply.github.com>
Date: Tue, 27 Jun 2023 22:18:15 +0000
Subject: [PATCH] git export: export deletion of forgotten remote-tracking
 branches

---
 lib/src/git.rs               | 32 +++++++++++++++++++++++---------
 src/cli_util.rs              |  3 ++-
 tests/test_branch_command.rs | 19 ++++++++++++++++---
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/lib/src/git.rs b/lib/src/git.rs
index d55383f03..b3f388ecf 100644
--- a/lib/src/git.rs
+++ b/lib/src/git.rs
@@ -332,23 +332,37 @@ pub fn export_refs(
     let mut branches_to_delete = BTreeMap::new();
     let mut failed_branches = vec![];
     let view = mut_repo.view();
-    // This list includes refs known to jj, namely all git-tracking refs and refs
-    // for local jj branches.
-    // Short-term TODO: use the fact that git refs other than local branches are
-    // included.
+    let jj_repo_iter_all_branches = view.branches().iter().flat_map(|(branch, target)| {
+        itertools::chain(
+            target
+                .local_target
+                .as_ref()
+                .map(|_| RefName::LocalBranch(branch.to_owned())),
+            target
+                .remote_targets
+                .keys()
+                .map(|remote| RefName::RemoteBranch {
+                    branch: branch.to_string(),
+                    remote: remote.to_string(),
+                }),
+        )
+    });
     let jj_known_refs: HashSet<_> = view
         .git_refs()
         .keys()
         .filter_map(|name| parse_git_ref(name))
-        .chain(
-            view.branches()
-                .keys()
-                .map(|branch| RefName::LocalBranch(branch.to_owned())),
-        )
+        .chain(jj_repo_iter_all_branches)
         .collect();
     for jj_known_ref in jj_known_refs {
         let new_branch = match &jj_known_ref {
             RefName::LocalBranch(branch) => view.get_local_branch(branch),
+            RefName::RemoteBranch { remote, branch } => {
+                // Currently, the only situation where this case occurs *and* new_branch !=
+                // old_branch is after a `jj branch forget`. So, in practice, for
+                // remote-tracking branches either `new_branch == old_branch` or
+                // `new_branch == None`.
+                view.get_remote_branch(branch, remote)
+            }
             _ => continue,
         };
         let old_branch = view.get_git_ref(&to_git_ref_name(&jj_known_ref));
diff --git a/src/cli_util.rs b/src/cli_util.rs
index 26becb9f8..435ccb058 100644
--- a/src/cli_util.rs
+++ b/src/cli_util.rs
@@ -1522,7 +1522,8 @@ pub fn print_failed_git_export(
                 "{}",
                 match branch_ref {
                     RefName::LocalBranch(name) => name.to_string(),
-                    // Should never happen, only local branches are exported
+                    RefName::RemoteBranch { branch, remote } => format!("{branch}@{remote}"),
+                    // Should never happen, tags and git_refs should never be exported
                     branch_ref => to_git_ref_name(branch_ref),
                 }
             )?;
diff --git a/tests/test_branch_command.rs b/tests/test_branch_command.rs
index a4db7d36e..003d3a295 100644
--- a/tests/test_branch_command.rs
+++ b/tests/test_branch_command.rs
@@ -318,6 +318,7 @@ fn test_branch_forget_fetched_branch() {
     feature1: 9f01a0e04879 message
     "###);
 
+    // TEST 1: with export-import
     // Forget the branch
     test_env.jj_cmd_success(&repo_path, &["branch", "forget", "feature1"]);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
@@ -330,14 +331,24 @@ 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"]), @r###"
-    Nothing changed.
-    "###);
+    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.
+    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: 9f01a0e04879 message
+    "###);
+
+    // TEST 2: No export/import (otherwise the same as test 1)
+    // Short-term TODO: While it looks like the bug above is fixed, correct behavior
+    // now actually depends on export/import.
+    test_env.jj_cmd_success(&repo_path, &["branch", "forget", "feature1"]);
+    insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
     // 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"]);
@@ -346,6 +357,8 @@ fn test_branch_forget_fetched_branch() {
     "###);
     insta::assert_snapshot!(get_branch_output(&test_env, &repo_path), @"");
 
+    // TEST 3: fetch branch that was moved & forgotten
+
     // Move the branch in the git repo.
     git_repo
         .commit(