Skip to content

Conversation

@bts
Copy link
Contributor

@bts bts commented Oct 23, 2025

Context

This is part of a series of changes to make most methods on index traits (i.e. ChangeIdIndex, MutableIndex, ReadonlyIndex, Index) fallible. The changes will enable networked implementations of these traits, which can produce I/O errors during operation. See #7825 for more information.

This is a follow-up to #7799 and #7823.

Changes

These two changes convert the necessary methods on ReadonlyIndex and MutableIndex to be fallible:

  • ReadonlyIndex::change_id_index
  • MutableIndex::change_id_index
  • MutableIndex::merge_in

Two things to note:

  • I did not also convert ReadonlyIndex::start_modification to return a Result for the reasons described in FR: Support distributed implementations of index traits #7825.
  • I changed MutableRepo::merge to return a RepoLoaderError instead of a BackendError, but we should verify that we think this makes sense. My line of thinking was as follows:
    • Without a larger change to error organization, the only other error type that seems to make sense in this context would be to smuggle the new IndexError from merge_in into BackendError::Other.
    • MutableRepo::merge is only used in two contexts (cmd_op_revert and Transaction::merge_operation). In both situations we are loading two repos and then merging them, and Transaction::merge_operation is a caller that already returns a RepoLoaderError.
    • ReadonlyRepo has a few methods that are already returning RepoLoaderError.

EDIT: ended up dropping conversion of change_id_index -- for any implementers looking to perform effects in this method, we can recommend they use interior mutation like they'll have to with start_modification.

Thanks!
Brian

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/updated tests to cover my changes

@bts bts requested a review from a team as a code owner October 23, 2025 17:11
Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

lgtm, but someone else will have to give the real green light

@bts
Copy link
Contributor Author

bts commented Oct 23, 2025

Thanks for taking a look! Going to re-request a review so folks see the PR is not approved to land.

@bts bts requested review from thoughtpolice and removed request for thoughtpolice October 23, 2025 19:58
This is part of a series of changes to make most methods on index traits
(i.e. `ChangeIdIndex`, `MutableIndex`, `ReadonlyIndex`, `Index`)
fallible. This will enable networked implementations of these traits,
which may produce I/O errors during operation. See jj-vcs#7825 for more
information.
@bts bts force-pushed the bts/fallible-ro-mut-index branch from e27dd55 to e169945 Compare October 23, 2025 20:42
@bts bts added this pull request to the merge queue Oct 23, 2025
Merged via the queue into jj-vcs:main with commit 37da011 Oct 23, 2025
30 checks passed
@bts bts deleted the bts/fallible-ro-mut-index branch October 23, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants