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: add coalesce(revsets...) #4628

Merged
merged 1 commit into from
Oct 16, 2024
Merged

revset: add coalesce(revsets...) #4628

merged 1 commit into from
Oct 16, 2024

Conversation

bnjmnt4n
Copy link
Member

@bnjmnt4n bnjmnt4n commented Oct 12, 2024 •
edited
Loading

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

@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from 7c88796 to cf084dc Compare October 12, 2024 11:02
yuja
yuja reviewed Oct 13, 2024
View reviewed changes
lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved
lib/src/revset.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from cf084dc to 4d6f75c Compare October 13, 2024 15:26
@bnjmnt4n bnjmnt4n changed the title revset: add if(condition, consequent[, alternate = none()] revset: add if_not_none and coalesce functions Oct 13, 2024
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from 4d6f75c to a80b5a5 Compare October 13, 2024 15:35
@bnjmnt4n
Copy link
Member Author

I've updated to add a coalesce function as well. I'm not very familiar with the revset engine for now, so I'm not sure if I should attempt to create an InternalRevset for these two functions versus evaluating it in the evaluate function.

@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from a80b5a5 to c8fefec Compare October 13, 2024 15:38
yuja
yuja reviewed Oct 14, 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.

I'm not very familiar with the revset engine for now, so I'm not sure if I should attempt to create an InternalRevset for these two functions versus evaluating it in the evaluate function.

I think the current implementation is good. We could add InternalRevset to make iteration lazy, but it doesn't mean the evaluation can always be lazy. For example, coalesce(foo, heads(bar)) would have to "evaluate" bar because heads(candidates) evaluates candidates eagerly. With the current implementation, heads(bar) evaluation can be skipped if foo is not empty.

lib/src/revset.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from c8fefec to 1a8f649 Compare October 14, 2024 10:38
@bnjmnt4n
Copy link
Member Author

Updated the PR to remove if and opt for coalesce only for now.

@bnjmnt4n bnjmnt4n changed the title revset: add if_not_none and coalesce functions revset: add coalesce(revsets...) Oct 14, 2024
yuja
yuja approved these changes Oct 14, 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.

Thanks!

docs/revsets.md Outdated Show resolved Hide resolved
lib/src/revset.rs Outdated Show resolved Hide resolved
docs/revsets.md Show resolved Hide resolved
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch 2 times, most recently from 6bf1386 to 9046c5c Compare October 15, 2024 11:56
yuja
yuja reviewed Oct 16, 2024
View reviewed changes
lib/src/revset.rs Outdated Show resolved Hide resolved
@bnjmnt4n
revset: add coalesce(revsets...)
3f6814a 
The `coalesce` function takes a list of revsets and returns the commits in the
first revset in the list which evalutes to a non-empty set of commits.

It can be used to display fallbacks if a certain commit cannot be found,
e.g. `coalesce(present(user_configured_trunk), builtin_trunk)`.
@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-ntmnmklnouxm branch from 9046c5c to 3f6814a Compare October 16, 2024 02:25
@bnjmnt4n bnjmnt4n enabled auto-merge (rebase) October 16, 2024 02:26
@bnjmnt4n bnjmnt4n merged commit 8e817bc into main Oct 16, 2024
31 checks passed
@bnjmnt4n bnjmnt4n deleted the bnjmnt4n/push-ntmnmklnouxm branch October 16, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy left review comments

@yuja yuja yuja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bnjmnt4n @yuja @emilazy