From 4a5060a61854e57702fa7929a00169040ea8d842 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 2 Jul 2023 09:59:34 +0900
Subject: [PATCH] revset: don't resolve deleted branch symbol to empty set

A deleted branch disappears immediately if there's no remote counterpart,
so I don't think a local name should be resolvable like zombie.
---
 lib/src/revset.rs            | 19 ++++++-------------
 lib/tests/test_revset.rs     | 15 +++++++++++----
 tests/test_branch_command.rs |  8 +++++++-
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 68d26f596..95945a82f 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -1641,24 +1641,17 @@ fn resolve_git_ref(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
 }
 
 fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
-    if let Some(branch_target) = repo.view().branches().get(symbol) {
-        return Some(
-            branch_target
-                .local_target
-                .as_ref()
-                .map(|target| target.adds().to_vec())
-                .unwrap_or_default(),
-        );
+    let view = repo.view();
+    if let Some(target) = view.get_local_branch(symbol) {
+        return Some(target.adds().to_vec());
     }
     if let Some((name, remote_name)) = symbol.split_once('@') {
-        if let Some(branch_target) = repo.view().branches().get(name) {
-            if let Some(target) = branch_target.remote_targets.get(remote_name) {
-                return Some(target.adds().to_vec());
-            }
+        if let Some(target) = view.get_remote_branch(name, remote_name) {
+            return Some(target.adds().to_vec());
         }
         // A remote with name "git" will shadow local-git tracking branches
         if remote_name == "git" {
-            if let Some(target) = get_local_git_tracking_branch(repo.view(), name) {
+            if let Some(target) = get_local_git_tracking_branch(view, name) {
                 return Some(target.adds().to_vec());
             }
         }
diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
index 5434ca59e..a1f5f3cc9 100644
--- a/lib/tests/test_revset.rs
+++ b/lib/tests/test_revset.rs
@@ -478,10 +478,17 @@ fn test_resolve_symbol_branches() {
     "###);
 
     // Remote only (or locally deleted)
-    assert_eq!(
-        resolve_symbol(mut_repo, "remote", None).unwrap(),
-        vec![], // TODO: NoSuchRevision
-    );
+    // 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",
+        ],
+    }
+    "###);
     assert_eq!(
         resolve_symbol(mut_repo, "remote@origin", None).unwrap(),
         vec![commit2.id().clone()],
diff --git a/tests/test_branch_command.rs b/tests/test_branch_command.rs
index d38408900..1a421414c 100644
--- a/tests/test_branch_command.rs
+++ b/tests/test_branch_command.rs
@@ -500,6 +500,8 @@ fn test_branch_list_filtered_by_revset() {
     "###);
 
     let query = |revset| test_env.jj_cmd_success(&local_path, &["branch", "list", "-r", revset]);
+    let query_error =
+        |revset| test_env.jj_cmd_failure(&local_path, &["branch", "list", "-r", revset]);
 
     // "all()" doesn't include deleted branches since they have no local targets.
     // So "all()" is identical to "branches()".
@@ -526,7 +528,11 @@ fn test_branch_list_filtered_by_revset() {
     "###);
 
     // Can't select deleted branch.
-    insta::assert_snapshot!(query("remote-delete"), @r###"
+    insta::assert_snapshot!(query("branches(remote-delete)"), @r###"
+    "###);
+    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"?
     "###);
 }