Skip to content

Conversation

@kornelski
Copy link
Collaborator

Copy link
Collaborator

@nox nox left a comment

Choose a reason for hiding this comment

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

I don't like that you introduced a panic in cert_store_mut, as it can make currently existing code panic now when combined with set_cert_store. Let's wait for boring 5.

@kornelski
Copy link
Collaborator Author

Existing code won't panic, because I haven't changed the semantics of set_cert_store.

It think it's better to deprecate set_cert_store first in v4, and then make a breaking behavior change in v5. The current API will force users to decide if they want immutable or mutable store by switching to one of the new methods.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025

Existing code won't panic, because I haven't changed the semantics of set_cert_store.

Existing code can do .set_cert_store(store) and then .cert_store_mut().

Never mind.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025

I assumed that the PR also implemented Clone for X509Store hence my confusion, I'm ok with the changes now.

@nox
Copy link
Collaborator

nox commented Jun 5, 2025

This is clever btw, nice of you to find a way without having to do boring 5.

@0x676e67
Copy link
Contributor

0x676e67 commented Jun 5, 2025

It looks good, so it can also be referenced in the external Arc packaging Cert Store.

@kornelski kornelski merged commit 5d57b3a into master Jun 5, 2025
25 checks passed
@kornelski kornelski deleted the store-by-ref branch June 5, 2025 13:45
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