From 1b658ea80ecc1a2fe8671642f817430869cb0e0a Mon Sep 17 00:00:00 2001
From: Ilya Grigoriev <ilyagr@users.noreply.github.com>
Date: Fri, 28 Jun 2024 15:35:13 -0700
Subject: [PATCH] lib id_prefix: look for divergent changes outside short
 prefix set

Fixes #2476.

Previously, if there was a change id match within the short prefix
lookup set, `jj` would not look for commits with that same change id
outside the short prefix set. So, it wouldn't find the conflicted
commits for a commit with a divergent (AKA conflicted) change id.
---
 CHANGELOG.md                |  2 ++
 lib/src/id_prefix.rs        | 15 ++++++++-------
 lib/tests/test_id_prefix.rs | 15 +++++++++++----
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 329d347ccc..75e5c63a70 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -82,6 +82,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   to-be-pushed commit has conflicts, or has no description / committer / author
   set. [#3029](https://github.com/martinvonz/jj/issues/3029)
 
+* `jj` will look for divergent changes outside the short prefix set even if it finds the change id inside the short prefix set. [#2476](https://github.com/martinvonz/jj/issues/2476)
+
 ## [0.18.0] - 2024-06-05
 
 ### Breaking changes
diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs
index f3a47e706d..5f8fd2ea36 100644
--- a/lib/src/id_prefix.rs
+++ b/lib/src/id_prefix.rs
@@ -168,13 +168,14 @@ impl IdPrefixContext {
         prefix: &HexPrefix,
     ) -> PrefixResolution<Vec<CommitId>> {
         if let Some(indexes) = self.disambiguation_indexes(repo) {
-            let resolution = indexes.change_index.resolve_prefix_with(
-                &*indexes.commit_change_ids,
-                prefix,
-                |(commit_id, _)| commit_id.clone(),
-            );
-            if let PrefixResolution::SingleMatch((_, ids)) = resolution {
-                return PrefixResolution::SingleMatch(ids);
+            let resolution = indexes
+                .change_index
+                .resolve_prefix_to_key(&*indexes.commit_change_ids, prefix);
+            if let PrefixResolution::SingleMatch(change_id) = resolution {
+                // There may be more commits with this change id outside the narrower sets.
+                return PrefixResolution::SingleMatch(repo.resolve_change_id(&change_id).expect(
+                    "Change ids present in narrower search set should be present globally.",
+                ));
             }
         }
         repo.resolve_change_id_prefix(prefix)
diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs
index a98e2e2f2a..204e12f4c9 100644
--- a/lib/tests/test_id_prefix.rs
+++ b/lib/tests/test_id_prefix.rs
@@ -365,12 +365,19 @@ fn test_id_prefix_divergent() {
         c.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()),
         1
     );
-    // Short prefix does not find the first commit (as intended).
-    // TODO(#2476): BUG: Looking up the divergent commits by their change id prefix
-    // only finds the id within the lookup set.
+    // This tests two issues, both important:
+    // - We find both commits with the same change id, even though
+    // `third_commit_divergent_with_second` is not in the short prefix set
+    // (#2476).
+    // - The short prefix set still works: we do *not* find the first commit and the
+    //   match is not ambiguous, even though the first commit's change id would also
+    //   match the prefix.
     assert_eq!(
         c.resolve_change_prefix(repo.as_ref(), &prefix("a")),
-        SingleMatch(vec![second_commit.id().clone()])
+        SingleMatch(vec![
+            second_commit.id().clone(),
+            third_commit_divergent_with_second.id().clone()
+        ])
     );
 
     // We can still resolve commits outside the set