ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

id_prefix: add explicit method that loads disambiguation index

Browse source 
This unblocks reuse of a symbol resolver instance for a different repo view
specified by at_operation() revset. See later commits for details. It's also
easier to handle error if there is a single function that can fail.
This commit is contained in:
Yuya Nishihara 2024-09-30 11:12:31 +09:00
parent 4fe8d89a10
commit bf6620d8d9
4 changed files with 95 additions and 76 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
cli/src/commit_templater.rs
View file

@ -33,6 +33,7 @@ use jj_lib::fileset::FilesetExpression;
use jj_lib::git;
use jj_lib::hex_util::to_reverse_hex;
use jj_lib::id_prefix::IdPrefixContext;
use jj_lib::id_prefix::IdPrefixIndex;
use jj_lib::matchers::Matcher;
use jj_lib::merged_tree::MergedTree;
use jj_lib::object_id::ObjectId as _;
@ -1267,13 +1268,13 @@ impl CommitOrChangeId {
pub fn shortest(
&self,
repo: &dyn Repo,
id_prefix_context: &IdPrefixContext,
index: &IdPrefixIndex,
total_len: usize,
) -> ShortestIdPrefix {
let mut hex = self.hex();
let prefix_len = match self {
CommitOrChangeId::Commit(id) => id_prefix_context.shortest_commit_prefix_len(repo, id),
CommitOrChangeId::Change(id) => id_prefix_context.shortest_change_prefix_len(repo, id),
CommitOrChangeId::Commit(id) => index.shortest_commit_prefix_len(repo, id),
CommitOrChangeId::Change(id) => index.shortest_change_prefix_len(repo, id),
};
hex.truncate(max(prefix_len, total_len));
let rest = hex.split_off(prefix_len);
@ -1330,7 +1331,6 @@ fn builtin_commit_or_change_id_methods<'repo>(
map.insert(
"shortest",
|language, diagnostics, build_ctx, self_property, function| {
let id_prefix_context = &language.id_prefix_context;
let ([], [len_node]) = function.expect_arguments()?;
let len_property = len_node
.map(|node| {
@ -1342,8 +1342,10 @@ fn builtin_commit_or_change_id_methods<'repo>(
)
})
.transpose()?;
let repo = language.repo;
let index = language.id_prefix_context.populate(repo);
let out_property = (self_property, len_property)
.map(|(id, len)| id.shortest(language.repo, id_prefix_context, len.unwrap_or(0)));
.map(move |(id, len)| id.shortest(repo, &index, len.unwrap_or(0)));
Ok(L::wrap_shortest_id_prefix(out_property))
},
);

25
lib/src/id_prefix.rs
View file

@ -103,6 +103,7 @@ impl IdIndexSourceEntry<ChangeId> for &'_ (CommitId, ChangeId) {
}
}
/// Manages configuration and cache of commit/change ID disambiguation index.
#[derive(Default)]
pub struct IdPrefixContext {
disambiguation: Option<DisambiguationData>,
@ -125,22 +126,32 @@ impl IdPrefixContext {
self
}
fn disambiguation_indexes(&self, repo: &dyn Repo) -> Option<&Indexes> {
/// Loads disambiguation index once, returns a borrowed index to
/// disambiguate commit/change IDs.
pub fn populate(&self, repo: &dyn Repo) -> IdPrefixIndex<'_> {
// TODO: propagate errors instead of treating them as if no revset was specified
self.disambiguation.as_ref().and_then(|disambiguation| {
let indexes = self.disambiguation.as_ref().and_then(|disambiguation| {
disambiguation
.indexes(repo, self.extensions.symbol_resolvers())
.ok()
})
});
IdPrefixIndex { indexes }
}
}
/// Loaded index to disambiguate commit/change IDs.
pub struct IdPrefixIndex<'a> {
indexes: Option<&'a Indexes>,
}
impl IdPrefixIndex<'_> {
/// Resolve an unambiguous commit ID prefix.
pub fn resolve_commit_prefix(
&self,
repo: &dyn Repo,
prefix: &HexPrefix,
) -> PrefixResolution<CommitId> {
if let Some(indexes) = self.disambiguation_indexes(repo) {
if let Some(indexes) = self.indexes {
let resolution = indexes
.commit_index
.resolve_prefix_to_key(&*indexes.commit_change_ids, prefix);
@ -154,7 +165,7 @@ impl IdPrefixContext {
/// Returns the shortest length of a prefix of `commit_id` that
/// can still be resolved by `resolve_commit_prefix()`.
pub fn shortest_commit_prefix_len(&self, repo: &dyn Repo, commit_id: &CommitId) -> usize {
if let Some(indexes) = self.disambiguation_indexes(repo) {
if let Some(indexes) = self.indexes {
if let Some(lookup) = indexes
.commit_index
.lookup_exact(&*indexes.commit_change_ids, commit_id)
@ -171,7 +182,7 @@ impl IdPrefixContext {
repo: &dyn Repo,
prefix: &HexPrefix,
) -> PrefixResolution<Vec<CommitId>> {
if let Some(indexes) = self.disambiguation_indexes(repo) {
if let Some(indexes) = self.indexes {
let resolution = indexes
.change_index
.resolve_prefix_to_key(&*indexes.commit_change_ids, prefix);
@ -190,7 +201,7 @@ impl IdPrefixContext {
/// Returns the shortest length of a prefix of `change_id` that
/// can still be resolved by `resolve_change_prefix()`.
pub fn shortest_change_prefix_len(&self, repo: &dyn Repo, change_id: &ChangeId) -> usize {
if let Some(indexes) = self.disambiguation_indexes(repo) {
if let Some(indexes) = self.indexes {
if let Some(lookup) = indexes
.change_index
.lookup_exact(&*indexes.commit_change_ids, change_id)

18
lib/src/revset.rs
View file

@ -1624,11 +1624,10 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> {
symbol: &str,
) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
if let Some(prefix) = HexPrefix::new(symbol) {
let resolution = self
.context
.as_ref()
.map(|ctx| ctx.resolve_commit_prefix(repo, &prefix))
.unwrap_or_else(|| repo.index().resolve_commit_id_prefix(&prefix));
let resolution = match self.context.map(|ctx| ctx.populate(repo)) {
Some(index) => index.resolve_commit_prefix(repo, &prefix),
None => repo.index().resolve_commit_id_prefix(&prefix),
};
match resolution {
PrefixResolution::AmbiguousMatch => Err(
RevsetResolutionError::AmbiguousCommitIdPrefix(symbol.to_owned()),
@ -1654,11 +1653,10 @@ impl PartialSymbolResolver for ChangePrefixResolver<'_> {
symbol: &str,
) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
let resolution = self
.context
.as_ref()
.map(|ctx| ctx.resolve_change_prefix(repo, &prefix))
.unwrap_or_else(|| repo.resolve_change_id_prefix(&prefix));
let resolution = match self.context.map(|ctx| ctx.populate(repo)) {
Some(index) => index.resolve_change_prefix(repo, &prefix),
None => repo.resolve_change_id_prefix(&prefix),
};
match resolution {
PrefixResolution::AmbiguousMatch => Err(
RevsetResolutionError::AmbiguousChangeIdPrefix(symbol.to_owned()),

116
lib/tests/test_id_prefix.rs
View file

@ -138,53 +138,54 @@ fn test_id_prefix() {
// Without a disambiguation revset
// ---------------------------------------------------------------------------------------------
let c = IdPrefixContext::default();
let context = IdPrefixContext::default();
let index = context.populate(repo.as_ref());
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
2
);
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), commits[5].id()),
index.shortest_commit_prefix_len(repo.as_ref(), commits[5].id()),
1
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
AmbiguousMatch
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("2a")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("2a")),
SingleMatch(commits[2].id().clone())
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("20")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("20")),
NoMatch
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("2a0")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("2a0")),
NoMatch
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
2
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[6].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[6].change_id()),
1
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("7")),
index.resolve_change_prefix(repo.as_ref(), &prefix("7")),
AmbiguousMatch
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("78")),
index.resolve_change_prefix(repo.as_ref(), &prefix("78")),
SingleMatch(vec![commits[0].id().clone()])
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("70")),
index.resolve_change_prefix(repo.as_ref(), &prefix("70")),
NoMatch
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("780")),
index.resolve_change_prefix(repo.as_ref(), &prefix("780")),
NoMatch
);
@ -192,28 +193,29 @@ fn test_id_prefix() {
// ---------------------------------------------------------------------------------------------
let expression =
RevsetExpression::commits(vec![commits[0].id().clone(), commits[2].id().clone()]);
let c = c.disambiguate_within(expression);
let context = context.disambiguate_within(expression);
let index = context.populate(repo.as_ref());
// The prefix is now shorter
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
1
);
// Shorter prefix within the set can be used
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
SingleMatch(commits[2].id().clone())
);
// Can still resolve commits outside the set
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("21")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("21")),
SingleMatch(commits[24].id().clone())
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
1
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("7")),
index.resolve_change_prefix(repo.as_ref(), &prefix("7")),
SingleMatch(vec![commits[0].id().clone()])
);
@ -221,29 +223,30 @@ fn test_id_prefix() {
// needed.
// ---------------------------------------------------------------------------------------------
let expression = RevsetExpression::commit(root_commit_id.clone());
let c = c.disambiguate_within(expression);
let context = context.disambiguate_within(expression);
let index = context.populate(repo.as_ref());
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), root_commit_id),
index.shortest_commit_prefix_len(repo.as_ref(), root_commit_id),
1
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("")),
AmbiguousMatch
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix("0")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("0")),
SingleMatch(root_commit_id.clone())
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), root_change_id),
index.shortest_change_prefix_len(repo.as_ref(), root_change_id),
1
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("")),
index.resolve_change_prefix(repo.as_ref(), &prefix("")),
AmbiguousMatch
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("0")),
index.resolve_change_prefix(repo.as_ref(), &prefix("0")),
SingleMatch(vec![root_commit_id.clone()])
);
@ -251,13 +254,14 @@ fn test_id_prefix() {
// ---------------------------------------------------------------------------------------------
// TODO: Should be an error
let expression = RevsetExpression::symbol("nonexistent".to_string());
let context = c.disambiguate_within(expression);
let context = context.disambiguate_within(expression);
let index = context.populate(repo.as_ref());
assert_eq!(
context.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()),
2
);
assert_eq!(
context.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
index.resolve_commit_prefix(repo.as_ref(), &prefix("2")),
AmbiguousMatch
);
}
@ -335,29 +339,30 @@ fn test_id_prefix_divergent() {
// Without a disambiguation revset
// --------------------------------
let c = IdPrefixContext::default();
let context = IdPrefixContext::default();
let index = context.populate(repo.as_ref());
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()),
3
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[1].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[1].change_id()),
3
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()),
index.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()),
3
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("a5")),
index.resolve_change_prefix(repo.as_ref(), &prefix("a5")),
AmbiguousMatch
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("a53")),
index.resolve_change_prefix(repo.as_ref(), &prefix("a53")),
SingleMatch(vec![first_commit.id().clone()])
);
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("a50")),
index.resolve_change_prefix(repo.as_ref(), &prefix("a50")),
SingleMatch(vec![
second_commit.id().clone(),
third_commit_divergent_with_second.id().clone()
@ -367,10 +372,11 @@ fn test_id_prefix_divergent() {
// Now, disambiguate within the revset containing only the second commit
// ----------------------------------------------------------------------
let expression = RevsetExpression::commits(vec![second_commit.id().clone()]);
let c = c.disambiguate_within(expression);
let context = context.disambiguate_within(expression);
let index = context.populate(repo.as_ref());
// The prefix is now shorter
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()),
index.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()),
1
);
// This tests two issues, both important:
@ -381,7 +387,7 @@ fn test_id_prefix_divergent() {
// 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")),
index.resolve_change_prefix(repo.as_ref(), &prefix("a")),
SingleMatch(vec![
second_commit.id().clone(),
third_commit_divergent_with_second.id().clone()
@ -390,11 +396,11 @@ fn test_id_prefix_divergent() {
// We can still resolve commits outside the set
assert_eq!(
c.resolve_change_prefix(repo.as_ref(), &prefix("a53")),
index.resolve_change_prefix(repo.as_ref(), &prefix("a53")),
SingleMatch(vec![first_commit.id().clone()])
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), first_commit.change_id()),
index.shortest_change_prefix_len(repo.as_ref(), first_commit.change_id()),
3
);
}
@ -479,32 +485,33 @@ fn test_id_prefix_hidden() {
// Without a disambiguation revset
// --------------------------------
let c = IdPrefixContext::default();
let context = IdPrefixContext::default();
let index = context.populate(repo.as_ref());
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()),
index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()),
2
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()),
index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()),
3
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])),
index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])),
AmbiguousMatch
);
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..2])),
index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..2])),
SingleMatch(hidden_commit.id().clone())
);
assert_eq!(
c.resolve_change_prefix(
index.resolve_change_prefix(
repo.as_ref(),
&prefix(&hidden_commit.change_id().hex()[..2])
),
AmbiguousMatch
);
assert_eq!(
c.resolve_change_prefix(
index.resolve_change_prefix(
repo.as_ref(),
&prefix(&hidden_commit.change_id().hex()[..3])
),
@ -514,32 +521,33 @@ fn test_id_prefix_hidden() {
// Disambiguate within hidden
// --------------------------
let expression = RevsetExpression::commit(hidden_commit.id().clone());
let c = c.disambiguate_within(expression);
let context = context.disambiguate_within(expression);
let index = context.populate(repo.as_ref());
assert_eq!(
c.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()),
index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()),
1
);
assert_eq!(
c.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()),
index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()),
1
);
// Short commit id can be resolved even if it's hidden.
assert_eq!(
c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])),
index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])),
SingleMatch(hidden_commit.id().clone())
);
// OTOH, hidden change id should never be found. The resolution might be
// ambiguous if hidden commits were excluded from the disambiguation set.
// In that case, shortest_change_prefix_len() shouldn't be 1.
assert_eq!(
c.resolve_change_prefix(
index.resolve_change_prefix(
repo.as_ref(),
&prefix(&hidden_commit.change_id().hex()[..1])
),
NoMatch
);
assert_eq!(
c.resolve_change_prefix(
index.resolve_change_prefix(
repo.as_ref(),
&prefix(&hidden_commit.change_id().hex()[..2])
),