Skip to content

repo: expose change_id_index() - #9956

Open
OlshaMB wants to merge 1 commit into
jj-vcs:mainfrom
r2rn:push-lylwrtuwurqn
Open

repo: expose change_id_index()#9956
OlshaMB wants to merge 1 commit into
jj-vcs:mainfrom
r2rn:push-lylwrtuwurqn

Conversation

@OlshaMB

@OlshaMB OlshaMB commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

I was converting ChangeIdIndex as part of ongoing async index conversion. These methods seemed unnessasary.

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
  • I fully understand the code that I am submitting (what it does,
    how it works, how it's organized), including any code drafted by an LLM.
  • For any prose generated by an LLM, I have proof-read and copy-edited with
    an eye towards deleting anything that is irrelevant, clarifying anything
    that is confusing, and adding details that are relevant. This includes,
    for example, commit descriptions, PR descriptions, and code comments.

@OlshaMB
OlshaMB requested a review from a team as a code owner August 10, 2026 10:33
Comment thread lib/src/repo.rs
loader: RepoLoader,
operation: Operation,
index: Box<dyn ReadonlyIndex>,
change_id_index: OnceCell<Box<dyn ChangeIdIndex>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like you removed the cache and make it recompute for each change id resolution.

@OlshaMB OlshaMB Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops... accident

Comment thread lib/src/repo.rs
&self,
target_id_bytes: &ChangeId,
) -> IndexResult<usize>;
fn change_id_index(&self) -> Box<dyn ChangeIdIndex + '_>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add the motivation for this change to the commit message?

@OlshaMB
OlshaMB force-pushed the push-lylwrtuwurqn branch from fc8e56f to 0f4e5fc Compare August 11, 2026 13:36
ReadonlyRepo change_id prefix methods only forward the calls to ChangeIdIndex, this seems pointless and out of convention compared to rest of the index related methods. So we expose ChangeIdIndex using change_id_index() method simmiliarly to index() method, and convert resolve_change_id to be utility function.
@OlshaMB
OlshaMB force-pushed the push-lylwrtuwurqn branch from 0f4e5fc to 51543cc Compare August 11, 2026 13:39
@OlshaMB

OlshaMB commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

I am kind of stuck trying to move ChangeIdIndex out of repo, since it's wrapped by a Box and that means it has to be uniquely owned by Repo, so that causes borrow checker problems, so i want to ask a question does ChangeIdIndex have to be contained by Box and not Arc, .index() uses Arc, or is there a principle that ChangeIdIndex has to boxed?
I am gonna move #9957 to be independent of this pr

@yuja

yuja commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I am kind of stuck trying to move ChangeIdIndex out of repo, since it's wrapped by a Box and that means it has to be uniquely owned by Repo, so that causes borrow checker problems, so i want to ask a question does ChangeIdIndex have to be contained by Box and not Arc, .index() uses Arc, or is there a principle that ChangeIdIndex has to boxed? I am gonna move #9957 to be independent of this pr

What's your goal? You can clone Arc<ReadonlyRepo> if there are multiple consumers of a (readonly) repo and index.

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.

2 participants