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

index: Return Result from heads #5360

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

index: Return Result from heads #5360

wants to merge 1 commit into from
+60 −17

Conversation

AM5800
Copy link
Contributor

@AM5800 AM5800 commented Jan 14, 2025 •
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

PhilipMetzger
PhilipMetzger approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LG

lib/src/repo.rs Show resolved Hide resolved
martinvonz
martinvonz reviewed Jan 14, 2025
View reviewed changes
lib/src/default_index/composite.rs Outdated Show resolved Hide resolved
lib/src/repo.rs Show resolved Hide resolved
lib/src/rewrite.rs Outdated Show resolved Hide resolved
lib/src/default_index/composite.rs Outdated Show resolved Hide resolved
@AM5800 AM5800 force-pushed the result_for_heads branch 3 times, most recently from 53afaa9 to 381d3e2 Compare February 5, 2025 14:27
yuja
yuja approved these changes Feb 6, 2025
View reviewed changes
cli/src/commands/simplify_parents.rs Show resolved Hide resolved
lib/src/rewrite.rs Show resolved Hide resolved
@AM5800
index: Return Result from heads
f08506b 
there are many ways to implement `heads` for your custom backend:
one involves calling `self.evaluate_revset` with a proper revset. 
However, it can return an error, but `heads` does not allow it.

In our implementation of the index backend we do exactly the above 
and we ended up with several unwraps which we are trying to avoid 
as it could potentially crash our prod jobs.

P.S. The same logic applies to many methods in this trait, but I am doing one at a time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilipMetzger PhilipMetzger PhilipMetzger approved these changes

@martinvonz martinvonz martinvonz 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.
4 participants
@AM5800 @yuja @martinvonz @PhilipMetzger