jj log -r <change id prefix> can miss some commits in divergent change

[martinvonz]

Description

We currently resolve a change id prefix to the matching commits in revsets.short-prefixes. If there are commits with the given change id both inside and outside the configured revset, then those outside will be missed.

Steps to Reproduce the Problem

$ jj init --git
$ jj describe -m foo
$ jj describe -m bar --at-op=@-
$ jj log
◉  p?? [email protected] 1 minute ago a23dab61
│  (empty) bar
│ @  p?? [email protected] 1 minute ago a9f9bf7e
├─╯  (empty) foo
◉  z root() 00000000
$ jj log --config-toml "revsets.short-prefixes= '@'" -r p
@  p?? [email protected] 1 minute ago a9f9bf7e
│  (empty) foo
~

Expected Behavior

It's probably better to first resolve the change id prefix to a change id within revsets.short-prefixes and then resolve that change id among all visible commits.

Note that we currently depend on change id resolution within revsets.short-prefixes at Google. We don't yet have a solution for resolving a change id to commits. I hope we (Google) can find a solution for that soon, so that fixing this bug doesn't completely break the workflow at Google.

Specifications

• Platform: All
• Version: 0.10.0

[ilyagr]

We just saw this bug in the wild: Discord.

There is a workaround: disable the short prefixes feature temporarily. One way to do so is to run commands with --config-toml "revsets.short-prefixes= 'none()'". Then, you resolve the divergence, and after that you can use short prefixes again.

[ilyagr]

I think this could be fixed by adding to the algorithm for looking up change ids: if one change id is found for the prefix in the short prefix set, always try looking for the full change id in the entire repo. I supposed this behavior would have to be disabled at Google, though.

[ilyagr]

I still need to write tests, but let me know if this won't work at Google or is otherwise not a good idea:

diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs
index f3a47e706d...766cbd502d 100644
--- a/lib/src/id_prefix.rs
+++ b/lib/src/id_prefix.rs
@@ -168,13 +168,16 @@
         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: PrefixResolution<(ChangeId, Vec<CommitId>)> = indexes
+                .change_index
+                .resolve_prefix_with(&*indexes.commit_change_ids, prefix, |(commit_id, _)| {
+                    commit_id.clone()
+                });
+            if let PrefixResolution::SingleMatch((change_id, _ids)) = 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)

Aside: This is one example where word-diff shines:

Difftastic does an only slightly better job:

[martinvonz]

I still need to write tests, but let me know if this won't work at Google or is otherwise not a good idea:

The patch looks good to me.

The support for looking up a change id within the full repo at Google that I mentioned at the top is unfortunately still not done :( I don't like the idea of that blocking upstream. We can always patch out this fix at Google until we get proper support for looking up by change id implemented.

Thanks!

[yuja]

nit: .resolve_prefix_to_key() can be used as we're no longer interested in commit ids.