Avoid evaluating large revsets just to check if they're immutable #7766
Conversation
|
While I like the idea in general, this seems to be solved at the wrong level to me. Because to it makes more sense to have a custom evaluator for that since it gives downstreams a better extension point. This will also help if the project ever wants to move away from the tree-interpreter/compiler in the revset evaluation path. |
|
I don't understand what you mean. Can you elaborate? I would have explained what Google has a custom revset engine but I know you already know that. |
I mean the current revset evaluation looks like this: this patch modifies the general evaluation mechanism. What I imagine or what I think is better since its just a feeling to have something like: and then downstreams can change parts of the evaluation engine to avoid such issues. I mean the patch can stand on its own without that and in general is good. And having such a way to allow changes in the revset evalutation may make some other ideas possible like using a custom SSA IR for revsets possible without a large cost. I hope that explains it a bit better. |
…mmits I had missed that `jj log` is supposed to render commits as immutable even when `--ignore-immutable` is passed. Thanks to @yuja for pointing this out.
I'm about to add a version of `check_rewritable()` that works on a revset expression instead of an evaluated revset. This patch prepares for that by having the `--ignore-immutable` case also evaluate a revset instead of iterating over the commits looking for the root commit.
This will help us error out on things like `jj abandon ..main` without first evaluating the revset to all the commits in the set.
This should make things like accidental `jj abandon ..main` error out much more quickly on large repos like the one at Google (without first reading hundreds of millions of commits into memory). I think we should update other commands in a similar way.
…cases This updates a somewhat arbitrary set of commands that were easy to update.
martinvonz
force-pushed
the
mz/mzkwqzyvnlqo
branch
from
dea9b8a to
36e6b84
Compare
|
@PhilipMetzger: I still don't follow what you mean, but it sounds like it's quite unrelated to this PR? Or at least this PR doesn't make what you have in mind harder (maybe it even makes it easier)? |
I mean building a somekind of higher abstraction will be worth it at some point, but as I've said it shouldn't really block the PR. Because this currently works for Google and co, doesn't really mean it scales into the future which may benefit from some other abstraction dealing with unevaluated or partially evaluated revsets. |
3 participants
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)