Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revset: introduce more performant way to check if a commit is in a revset #3258

Merged
merged 1 commit into from
Mar 11, 2024
Merged

revset: introduce more performant way to check if a commit is in a revset #3258

merged 1 commit into from
Mar 11, 2024

Conversation

zummenix
Copy link
Contributor

@zummenix zummenix commented Mar 8, 2024 •
edited
Loading

Suggested in #3234

Checklist

If applicable:

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

@zummenix zummenix mentioned this pull request Mar 8, 2024
Closed
4 tasks
yuja
yuja reviewed Mar 8, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me. Perhaps, lifetime mess can be cleaned up later.

lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
martinvonz
martinvonz reviewed Mar 8, 2024
View reviewed changes
lib/src/revset.rs Outdated Show resolved Hide resolved
@zummenix zummenix self-assigned this Mar 9, 2024
@zummenix zummenix changed the title revset: introduce cached revset allows checking if it contains a given commit revset: introduce more performant way to check if a commit is in a revset Mar 10, 2024
@zummenix zummenix marked this pull request as ready for review March 10, 2024 06:53
@zummenix
Copy link
Contributor Author

This is ready for final review.

yuja
yuja approved these changes Mar 10, 2024
View reviewed changes
lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
lib/src/default_index/revset_engine.rs Show resolved Hide resolved
@zummenix
revset: introduce more performant way to check if a commit is in a re…
b0a12ce 
…vset

Initially we were thinking to have `Revset` return something like
`CachedRevset`:

```
pub trait CachedRevset {
  fn iter(&self) -> Box<dyn Iterator<Item = Commit>>;
  fn contains(&self, &CommitId) -> bool;
}
```

But we weren't sure what use case for `iter` would be, so we dropped the `iter`
method. `CachedRevset` with single `contains` method needed a better name. We
weren't able to come up with one, so we decided instead to have a method on
`Revset` that returns a closure to check if a commit is in a revset.
@zummenix zummenix merged commit cd3d75e into jj-vcs:main Mar 11, 2024
16 checks passed
@zummenix zummenix deleted the cached-revset branch March 11, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gulbanana gulbanana gulbanana left review comments

@martinvonz martinvonz martinvonz left review comments

@yuja yuja yuja approved these changes

Assignees

@zummenix zummenix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zummenix @yuja @martinvonz @gulbanana