Skip to content

Commit

Permalink
lib id_prefix: look for divergent changes outside short prefix set
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ilyagr committed Jun 29, 2024
1 parent 87dcc8e commit 896bf8f
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 11 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,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
Expand Down
15 changes: 8 additions & 7 deletions lib/src/id_prefix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 10 additions & 4 deletions lib/tests/test_id_prefix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,11 +376,17 @@ fn test_id_prefix_divergent() {
c.resolve_change_prefix(repo.as_ref(), &prefix("781")),
SingleMatch(vec![first_commit.id().clone()])
);
// Short prefix does not find the first commit. (This is correct)
// TODO(#2476): 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).
// - We do *not* find the first commit, even though its (different) change id
// would also match the prefix, since it's outside the short prefix set.
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("7")),
SingleMatch(vec![second_commit.id().clone()])
SingleMatch(vec![
second_commit.id().clone(),
third_commit_divergent_with_second.id().clone()
])
);
}

0 comments on commit 896bf8f

Please sign in to comment.