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, which mainly reorganized error types before converting methods to return Result.

Changes

These changes convert two methods on the trait ChangeIdIndex to be fallible:

  • ChangeIdIndex::resolve_prefix
  • ChangeIdIndex::shortest_unique_prefix_len

Note that rather than widen the error type of rewrite::find_duplicate_divergent_commits from BackendError to some new error enum that "properly" subsumes BackendError and IndexError, I opted to add another instance of the existing pattern:

// TODO: indexing error shouldn't be a "BackendError"
.map_err(|err| BackendError::Other(err.into()))?

I figured that we already need to address some of these existing BackendError::Other(IndexError) cases, and it'll be better to tackle this sort of error reorganization separate from these fallibility changes.

Please let me know if there's anything I can fix up.

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 03:32
Copy link
Member

@martinvonz martinvonz 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 I'll leave it to someone else to approve it since this is quite specific to things that Google and ERSC care about so I sense a bit of a conflict of interest (even though I know Yuya and Philip have already seen basically these changes).

@bts bts force-pushed the bts/fallible-change-id-index branch from 8ad1c68 to eda486c Compare October 23, 2025 14:47
bts added 2 commits October 23, 2025 11:09
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 can produce I/O errors during operation. See jj-vcs#7825 for more
information.
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 can produce I/O errors during operation. See jj-vcs#7825 for more
information.
@bts bts force-pushed the bts/fallible-change-id-index branch from eda486c to b708fa8 Compare October 23, 2025 15:13
@bts bts added this pull request to the merge queue Oct 23, 2025
Merged via the queue into jj-vcs:main with commit c70f9b5 Oct 23, 2025
31 checks passed
@bts bts deleted the bts/fallible-change-id-index branch October 23, 2025 15:48
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.

3 participants