From c5ed3e14776bd66cd3b366ef41360c28ccb87cf5 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 27 Nov 2022 18:39:35 +0900
Subject: [PATCH] revset: for short hash, look up both commit and change ids to
 disambiguate

Because the use of the change id is recommended, any operation should abort
if a valid change id happens to match a commit id. We still try the commit
id lookup first as the change id lookup is more costly.

Ambiguous change/commit id is reported as AmbiguousCommitIdPrefix for now.
Maybe we can merge AmbiguousCommit/ChangeIdPrefix errors into one?

Closes #799
---
 docs/revsets.md          |  3 +--
 lib/src/revset.rs        | 52 +++++++++++++++++++++++++---------------
 lib/tests/test_revset.rs | 19 ++++++++++++++-
 3 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/docs/revsets.md b/docs/revsets.md
index 6983ae516..2de81261a 100644
--- a/docs/revsets.md
+++ b/docs/revsets.md
@@ -46,8 +46,7 @@ Jujutsu attempts to resolve a symbol in the following order:
 3. Tag name
 4. Branch name
 5. Git ref
-6. Commit ID
-7. Change ID
+6. Commit ID or change ID
 
 
 ## Operators
diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 3d7cb7129..13b82628e 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -83,30 +83,37 @@ fn resolve_branch(repo: RepoRef, symbol: &str) -> Option<Vec<CommitId>> {
     None
 }
 
-fn resolve_commit_id(repo: RepoRef, symbol: &str) -> Result<Option<Vec<CommitId>>, RevsetError> {
-    // First check if it's a full commit id.
+fn resolve_full_commit_id(
+    repo: RepoRef,
+    symbol: &str,
+) -> Result<Option<Vec<CommitId>>, RevsetError> {
     if let Ok(binary_commit_id) = hex::decode(symbol) {
         let commit_id = CommitId::new(binary_commit_id);
         match repo.store().get_commit(&commit_id) {
-            Ok(_) => return Ok(Some(vec![commit_id])),
-            Err(BackendError::NotFound) => {} // fall through
-            Err(err) => return Err(RevsetError::StoreError(err)),
+            Ok(_) => Ok(Some(vec![commit_id])),
+            Err(BackendError::NotFound) => Ok(None),
+            Err(err) => Err(RevsetError::StoreError(err)),
         }
+    } else {
+        Ok(None)
     }
+}
 
+fn resolve_short_commit_id(
+    repo: RepoRef,
+    symbol: &str,
+) -> Result<Option<Vec<CommitId>>, RevsetError> {
     if let Some(prefix) = HexPrefix::new(symbol.to_owned()) {
         match repo.index().resolve_prefix(&prefix) {
-            PrefixResolution::NoMatch => {
-                return Ok(None);
-            }
+            PrefixResolution::NoMatch => Ok(None),
             PrefixResolution::AmbiguousMatch => {
-                return Err(RevsetError::AmbiguousCommitIdPrefix(symbol.to_owned()));
+                Err(RevsetError::AmbiguousCommitIdPrefix(symbol.to_owned()))
             }
-            PrefixResolution::SingleMatch(commit_id) => return Ok(Some(vec![commit_id])),
+            PrefixResolution::SingleMatch(commit_id) => Ok(Some(vec![commit_id])),
         }
+    } else {
+        Ok(None)
     }
-
-    Ok(None)
 }
 
 fn resolve_change_id(
@@ -177,17 +184,24 @@ pub fn resolve_symbol(
             return Ok(ids);
         }
 
-        // Try to resolve as a commit id.
-        if let Some(ids) = resolve_commit_id(repo, symbol)? {
+        // Try to resolve as a full commit id. We assume a full commit id is unambiguous
+        // even if it's shorter than change id.
+        if let Some(ids) = resolve_full_commit_id(repo, symbol)? {
             return Ok(ids);
         }
 
-        // Try to resolve as a change id.
-        if let Some(ids) = resolve_change_id(repo, symbol)? {
-            return Ok(ids);
+        // Try to resolve as a commit/change id.
+        match (
+            resolve_short_commit_id(repo, symbol)?,
+            resolve_change_id(repo, symbol)?,
+        ) {
+            // Likely a root_commit_id, but not limited to it.
+            (Some(ids1), Some(ids2)) if ids1 == ids2 => Ok(ids1),
+            // TODO: maybe unify Ambiguous*IdPrefix error variants?
+            (Some(_), Some(_)) => Err(RevsetError::AmbiguousCommitIdPrefix(symbol.to_owned())),
+            (Some(ids), None) | (None, Some(ids)) => Ok(ids),
+            (None, None) => Err(RevsetError::NoSuchRevision(symbol.to_owned())),
         }
-
-        Err(RevsetError::NoSuchRevision(symbol.to_owned()))
     }
 }
 
diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
index a9ffda7e4..dc8e8cee0 100644
--- a/lib/tests/test_revset.rs
+++ b/lib/tests/test_revset.rs
@@ -171,7 +171,7 @@ fn test_resolve_symbol_change_id() {
     .unwrap();
     let git_tree = git_repo.find_tree(empty_tree_id).unwrap();
     let mut git_commit_ids = vec![];
-    for i in &[133, 664, 840] {
+    for i in &[133, 664, 840, 5085] {
         let git_commit_id = git_repo
             .commit(
                 Some(&format!("refs/heads/branch{}", i)),
@@ -205,6 +205,11 @@ fn test_resolve_symbol_change_id() {
         // "04e1c7082e4e34f3f371d8a1a46770b861b9b547" reversed
         "e2ad9d861d0ee625851b8ecfcf2c727410e38720"
     );
+    assert_eq!(
+        hex::encode(git_commit_ids[3]),
+        // "911d7e52fd5ba04b8f289e14c3d30b52d38c0020" reversed
+        "040031cb4ad0cbc3287914f1d205dabf4a7eb889"
+    );
 
     // Test lookup by full change id
     let repo_ref = repo.as_repo_ref();
@@ -254,6 +259,18 @@ fn test_resolve_symbol_change_id() {
         Err(RevsetError::NoSuchRevision("04e13".to_string()))
     );
 
+    // Test commit/changed id conflicts.
+    assert_eq!(
+        resolve_symbol(repo_ref, "040b", None),
+        Ok(vec![CommitId::from_hex(
+            "5339432b8e7b90bd3aa1a323db71b8a5c5dcd020"
+        )])
+    );
+    assert_eq!(
+        resolve_symbol(repo_ref, "040", None),
+        Err(RevsetError::AmbiguousCommitIdPrefix("040".to_string()))
+    );
+
     // Test non-hex string
     assert_eq!(
         resolve_symbol(repo_ref, "foo", None),