Use shorter ID prefixes in configured revset #1603

martinvonz · 2023-05-11T17:04:39Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (src/config-schema.json)
I have added tests to cover my changes

I plan to add `revsets.short-prefixes` and `revsets.immutable` soon, and I think `[revsets]` seems like reasonable place to put them. It seems consistent with our `[templates]` section. However, it also suffers from the same problem as that section, which is that the difference between `[templates]` and `[template-aliases]` is not clear. We can decide about about templates and revsets later.

I'd like to make the symbol resolution more flexible, both so we can support customizing it (in custom `jj` binaries) and so we can use it for resolving short prefixes within a small revset.

PhilipMetzger · 2023-05-11T18:00:14Z

SGTM, besides the typo in the last sentence of the sixth commit message I've update the templater to use it. which should be I've updated the templater to use it. I'll defer to @yuja for all revset related changes though.

I do have a question related to the fourth commit as it will impact my PR(#1200). Is there a good pattern for the default SymbolResolver usage?

martinvonz · 2023-05-11T18:05:52Z

I do have a question related to the fourth commit as it will impact my PR(#1200). Is there a good pattern for the default SymbolResolver usage?

I don't think it should impact your PR. Were you thinking that we'd implement jj prev 5 by resolving "5" as a revset symbol pointing that many commits back? I think it's better to just parse it as an integer.

PhilipMetzger · 2023-05-11T18:28:05Z

No I didn't use the revset symbols, although it could've worked.

But I restructured the PR to use RevsetExpressions recently, using this pattern:

// Similar to cmd_next, on my machine 
let helper = ...;
let current_wc_id = helper.get_wc_id()?;
let parent = current_wc_id.parents().pop();
let all_ancestors = RevsetExpression::ancestors(&parent);
//...

which will need to use the Resolver.

martinvonz · 2023-05-11T18:31:29Z

No I didn't use the revset symbols, although it could've worked.

But I restructured the PR to use RevsetExpressions recently, using this pattern:
// Similar to cmd_next, on my machine 
let helper = ...;
let current_wc_id = helper.get_wc_id()?;
let parent = current_wc_id.parents().pop();
let all_ancestors = RevsetExpression::ancestors(&parent);
//...
which will need to use the Resolver.

Ah, I see. I think that should actually just use all_ancestors.resolve(&repo) (see the third commit in this series).

PhilipMetzger · 2023-05-11T18:49:46Z

Thanks for clearing my concerns.

CHANGELOG.md

yuja

Nice

lib/src/revset.rs

src/cli_util.rs

lib/src/revset.rs

lib/src/id_prefix.rs

src/cli_util.rs

lib/src/id_prefix.rs

I would eventually want the `SymbolResolver` to be customizable (in custom `jj` binaries), so we want to make sure we always use the customized version of it. I left `RevsetExpression::resolve()` unchanged. I consider that to be for programmatically created expressions.

When creating `RevsetExpression` programmatically, I think we should use commit ids instead of symbols in the expression. This commit adds a check for that by using a `SymbolResolver` that always errors out.

I want to store some lazily calculated data associated with a repo. The data will depend on the user's config, which means it shouldn't live in the `ReadonlyRepo` itself. We could store it directly in `WorkspaceCommandHelper` - and I did that at first - but it's annoying and risky to remember to reset the cached data when we update the repo instance (which we do when a transaction finishes). This commit therefore introduces a wrapper type where we can store it. Having a wrapper also means that we can use `OnceCell` instead of more manually initializing it with a `RefCell`.

I would like to copy Mercurial's way of abbreviating ids within a user-configurable revset. We would do it for both commit ids and change ids. For that feature, we need a place to keep the set of commits the revset evaluates to. This commit adds a new `IdPrefixContext` type which will eventually be that place. The new type has functions for going back and forth between full and abbreviated ids. I've updated the templater to use it.

This prepares for adding callbacks to resolve these ids.

This is another step towards resolving abbreviated commit ids within a configured revset.

I'll reuse it there next.

I'll use this in `IdPrefixContext` soon.

For the support for shorter prefixes within a revset, we'll want to be able to check if an id is in the index.

In large repos, the unique prefixes can get somewhat long (~6 hex digits seems typical in the Linux repo), which makes them less useful for manually entering on the CLI. The user typically cares most about a small set of commits, so it would be nice to give shorter unique ids to those. That's what Mercurial enables with its `experimental.revisions.disambiguatewithin` config. This commit provides an implementation of that feature in `IdPrefixContext`. In very large repos, it can also be slow to calculate the unique prefixes, especially if it involves a request to a server. This feature becomes much more important in such repos.

This adds a config called `revsets.short-prefixes`, which lets the user specify a revset in which to disambiguate otherwise ambiguous change/commit ids. It defaults to the value of `revsets.log`. I made it so you can disable the feature by setting `revsets.short-prefixes = ""`. I don't like that the default value (using `revsets.log`) cannot be configured explicitly by the user. That will be addressed if we decide to merge the `[revsets]` and `[revset-aliases]` sections some day.

By passing the repo as argument to the methods instead, we can remove the `repo` field and the associated lifetime. Thanks to Yuya for the suggestion.

martinvonz added 2 commits May 10, 2023 23:49

revset: introduce a trait for resolving symbols

cbbf588

I'd like to make the symbol resolution more flexible, both so we can support customizing it (in custom `jj` binaries) and so we can use it for resolving short prefixes within a small revset.

martinvonz force-pushed the push-kkxkpzssqmnt branch 2 times, most recently from 3d835fc to fc64b78 Compare May 11, 2023 17:19

martinvonz force-pushed the push-kkxkpzssqmnt branch from fc64b78 to 1d5984d Compare May 11, 2023 18:01

ilyagr reviewed May 12, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

yuja approved these changes May 12, 2023

View reviewed changes

martinvonz force-pushed the push-kkxkpzssqmnt branch from 1d5984d to e983ba1 Compare May 12, 2023 05:15

martinvonz added 12 commits May 11, 2023 23:28

revset: don't allow symbols in RevsetExpression::resolve()

1f6fa78

When creating `RevsetExpression` programmatically, I think we should use commit ids instead of symbols in the expression. This commit adds a check for that by using a `SymbolResolver` that always errors out.

revset: inline resolution of change/commit ids

36d1cc0

This prepares for adding callbacks to resolve these ids.

revset: use IdPrefixContext for resolving commit/change ids

593cd8c

This is another step towards resolving abbreviated commit ids within a configured revset.

prefixes: move IdIndex to id_prefix module

8ff5e1c

I'll reuse it there next.

id_prefix: add IdIndex::resolve_prefix()

3b072b2

I'll use this in `IdPrefixContext` soon.

id_prefix: add IdIndex::has_key()

5a63eeb

For the support for shorter prefixes within a revset, we'll want to be able to check if an id is in the index.

id_prefix: remove repo field from IdPrefixContext

6bfee4a

By passing the repo as argument to the methods instead, we can remove the `repo` field and the associated lifetime. Thanks to Yuya for the suggestion.

martinvonz force-pushed the push-kkxkpzssqmnt branch from 26474ba to 6bfee4a Compare May 12, 2023 06:29

martinvonz enabled auto-merge (rebase) May 12, 2023 06:30

martinvonz merged commit 916b00c into main May 12, 2023

martinvonz deleted the push-kkxkpzssqmnt branch May 12, 2023 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use shorter ID prefixes in configured revset #1603

Use shorter ID prefixes in configured revset #1603

martinvonz commented May 11, 2023

PhilipMetzger commented May 11, 2023

martinvonz commented May 11, 2023

PhilipMetzger commented May 11, 2023

martinvonz commented May 11, 2023

PhilipMetzger commented May 11, 2023

yuja left a comment

Use shorter ID prefixes in configured revset #1603

Use shorter ID prefixes in configured revset #1603

Conversation

martinvonz commented May 11, 2023

Checklist

PhilipMetzger commented May 11, 2023

martinvonz commented May 11, 2023

PhilipMetzger commented May 11, 2023

martinvonz commented May 11, 2023

PhilipMetzger commented May 11, 2023

yuja left a comment

Choose a reason for hiding this comment