From 3e294ca2d6a853b0cb0618d2c485e3ba8fe60e29 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 2 Jul 2023 13:57:44 +0900
Subject: [PATCH] revset: do not suggest deleted local branches, suggest
 @remote branches

It seemed too verbose to always include @remote branches, so synced remotes
are omitted by default. If the given symbol contained '@', all remote symbols
are populated so that the distance of remote fragment is taken into account.
---
 lib/src/revset.rs            | 22 +++++++++++++++-
 lib/tests/test_revset.rs     | 49 ++++++++++++++++++++++++++----------
 tests/test_branch_command.rs |  2 +-
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 95945a82f..9d7f4d56e 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -1659,6 +1659,24 @@ fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
     None
 }
 
+fn collect_branch_symbols(repo: &dyn Repo, include_synced_remotes: bool) -> Vec<String> {
+    // TODO: include "@git" branches
+    repo.view()
+        .branches()
+        .iter()
+        .flat_map(|(name, branch_target)| {
+            let local_target = branch_target.local_target.as_ref();
+            let local_symbol = local_target.is_some().then(|| name.to_owned());
+            let remote_symbols = branch_target
+                .remote_targets
+                .iter()
+                .filter(move |&(_, target)| include_synced_remotes || Some(target) != local_target)
+                .map(move |(remote_name, _)| format!("{name}@{remote_name}"));
+            local_symbol.into_iter().chain(remote_symbols)
+        })
+        .collect()
+}
+
 fn resolve_full_commit_id(
     repo: &dyn Repo,
     symbol: &str,
@@ -1821,7 +1839,9 @@ impl SymbolResolver for DefaultSymbolResolver<'_> {
             Err(RevsetResolutionError::NoSuchRevision {
                 name: symbol.to_owned(),
                 candidates: {
-                    let branch_names = self.repo.view().branches().keys().collect_vec();
+                    // TODO: include tags?
+                    let mut branch_names = collect_branch_symbols(self.repo, symbol.contains('@'));
+                    branch_names.sort_unstable();
                     collect_similar(symbol, &branch_names)
                 },
             })
diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
index a1f5f3cc9..9454e4d6e 100644
--- a/lib/tests/test_revset.rs
+++ b/lib/tests/test_revset.rs
@@ -473,19 +473,21 @@ fn test_resolve_symbol_branches() {
         name: "local@origin",
         candidates: [
             "local",
+            "local-remote@origin",
+            "remote@origin",
         ],
     }
     "###);
 
     // Remote only (or locally deleted)
-    // TODO: shouldn't suggest deleted local branch
     insta::assert_debug_snapshot!(
         resolve_symbol(mut_repo, "remote", None).unwrap_err(), @r###"
     NoSuchRevision {
         name: "remote",
         candidates: [
-            "remote",
-            "remote-conflicted",
+            "local-remote@origin",
+            "remote-conflicted@origin",
+            "remote@origin",
         ],
     }
     "###);
@@ -518,7 +520,9 @@ fn test_resolve_symbol_branches() {
         vec![commit5.id().clone(), commit4.id().clone()],
     );
 
-    // Typo of local/remote branch name
+    // Typo of local/remote branch name:
+    // For "local-emote" (without @remote part), "local-remote@mirror" isn't
+    // suggested since it points to the same target as "local-remote".
     insta::assert_debug_snapshot!(
         resolve_symbol(mut_repo, "local-emote", None).unwrap_err(), @r###"
     NoSuchRevision {
@@ -527,6 +531,7 @@ fn test_resolve_symbol_branches() {
             "local",
             "local-conflicted",
             "local-remote",
+            "local-remote@origin",
         ],
     }
     "###);
@@ -537,10 +542,13 @@ fn test_resolve_symbol_branches() {
         candidates: [
             "local",
             "local-remote",
+            "local-remote@mirror",
+            "local-remote@origin",
+            "remote-conflicted@origin",
+            "remote@origin",
         ],
     }
     "###);
-    // TODO: shouldn't suggest deleted local branch
     insta::assert_debug_snapshot!(
         resolve_symbol(mut_repo, "local-remote@origine", None).unwrap_err(), @r###"
     NoSuchRevision {
@@ -548,21 +556,34 @@ fn test_resolve_symbol_branches() {
         candidates: [
             "local",
             "local-remote",
-            "remote",
-            "remote-conflicted",
+            "local-remote@mirror",
+            "local-remote@origin",
+            "remote-conflicted@origin",
+            "remote@origin",
+        ],
+    }
+    "###);
+    // "local-remote@mirror" shouldn't be omitted just because it points to the same
+    // target as "local-remote".
+    insta::assert_debug_snapshot!(
+        resolve_symbol(mut_repo, "remote@mirror", None).unwrap_err(), @r###"
+    NoSuchRevision {
+        name: "remote@mirror",
+        candidates: [
+            "local-remote@mirror",
+            "remote@origin",
         ],
     }
     "###);
 
     // Typo of remote-only branch name
-    // TODO: shouldn't suggest deleted local branch
     insta::assert_debug_snapshot!(
         resolve_symbol(mut_repo, "emote", None).unwrap_err(), @r###"
     NoSuchRevision {
         name: "emote",
         candidates: [
-            "remote",
-            "remote-conflicted",
+            "remote-conflicted@origin",
+            "remote@origin",
         ],
     }
     "###);
@@ -571,7 +592,8 @@ fn test_resolve_symbol_branches() {
     NoSuchRevision {
         name: "emote@origin",
         candidates: [
-            "remote",
+            "local-remote@origin",
+            "remote@origin",
         ],
     }
     "###);
@@ -580,8 +602,9 @@ fn test_resolve_symbol_branches() {
     NoSuchRevision {
         name: "remote@origine",
         candidates: [
-            "remote",
-            "remote-conflicted",
+            "local-remote@origin",
+            "remote-conflicted@origin",
+            "remote@origin",
         ],
     }
     "###);
diff --git a/tests/test_branch_command.rs b/tests/test_branch_command.rs
index 1a421414c..259ca91bb 100644
--- a/tests/test_branch_command.rs
+++ b/tests/test_branch_command.rs
@@ -532,7 +532,7 @@ fn test_branch_list_filtered_by_revset() {
     "###);
     insta::assert_snapshot!(query_error("remote-delete"), @r###"
     Error: Revision "remote-delete" doesn't exist
-    Hint: Did you mean "remote-delete", "remote-keep", "remote-rewrite"?
+    Hint: Did you mean "remote-delete@origin", "remote-keep", "remote-rewrite", "remote-rewrite@origin"?
     "###);
 }