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.

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

simple_op_store: hash view/operation data directly #734

Merged
merged 2 commits into from
Nov 13, 2022

Conversation

Ralith
Copy link
Contributor

@Ralith Ralith commented Nov 12, 2022

Product of discussion in #700. First commit could be replaced with a manual impl for the enum, saving a fair bit of churn at the cost of being slightly more error prone. No strong opinion about which would be preferable.

@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch 2 times, most recently from 2404c2a to c7ed2e8 Compare November 12, 2022 05:28
@Ralith
Copy link
Contributor Author

Ralith commented Nov 12, 2022

On second thought I've about run out of patience updating callsites. WIll probably reprise this with a custom impl tomorrow.

@Ralith Ralith marked this pull request as draft November 12, 2022 05:54
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.

Thanks! I see now what you mean about how derive macros would have looked better. Oh well, we can clean that up later.

Cargo.lock Show resolved Hide resolved
lib/src/content_hash.rs Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
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.

Oh, it would be nice with a longer description for "simple_op_store: hash view/operation data directly". It would be good to answer at least these questions:

Why are you doing this?
Because depending on the serialized format means that hashes change if we change the format, and also that it's possible that an upgrade to the protobuf library results in a different serialized form and thus a different hash.

What happens to existing repos?
Since the hashes are only used for identity (not for integrity checking), it should not break existing repo.

@yuja
Copy link
Contributor

yuja commented Nov 12, 2022

Just a random comment. I thought Hasher could be implemented as a serde Serializer, but that turned out to be not good idea. impl Serializer requires lengthy boilerplate code, and maybe it would be a bit trickier to ensure HashMap has stable order.

https://serde.rs/impl-serializer.html

@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch 5 times, most recently from 5b72287 to 6ed6953 Compare November 12, 2022 19:30
@Ralith Ralith marked this pull request as ready for review November 12, 2022 19:30
@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from 6ed6953 to 5b5d7f1 Compare November 12, 2022 19:31
@Ralith
Copy link
Contributor Author

Ralith commented Nov 12, 2022

Dropped the churn, added support for newtype structs to the macro, added a helper function, and expanded usage to local_backend. I also documented a protocol for enums so hopefully we can keep implementations consistent with eachother and with future improvements to the macro.

I thought Hasher could be implemented as a serde Serializer, but that turned out to be not good idea.

Yeah, see also RustCrypto/utils#2 for prior discussion in the ecosystem.

@Ralith
Copy link
Contributor Author

Ralith commented Nov 12, 2022

Aw, need rustc 1.63 for the convenient hash helper.

@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from 5b5d7f1 to 23bdddf Compare November 12, 2022 19:46
lib/src/content_hash.rs Outdated Show resolved Hide resolved
lib/src/content_hash.rs Show resolved Hide resolved
lib/src/simple_op_store.rs Outdated Show resolved Hide resolved
@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from 23bdddf to 71dd10e Compare November 13, 2022 01:33
lib/src/content_hash.rs Outdated Show resolved Hide resolved
@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from 71dd10e to f3ac695 Compare November 13, 2022 01:42
@Ralith Ralith enabled auto-merge (rebase) November 13, 2022 01:42
@Ralith Ralith disabled auto-merge November 13, 2022 01:42
@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from f3ac695 to e8f2db3 Compare November 13, 2022 01:42
@Ralith Ralith enabled auto-merge (rebase) November 13, 2022 01:42
Decouples view/operation IDs from serialized forms, which are not
necessarily stable. Not breaking as these IDs are persistent, never
recomputed or used for integrity checking.
Insulates identifiers from the unstable serialized form.
@Ralith Ralith force-pushed the push-b3e19dc016af471e8c577aeeb5e0269d branch from e8f2db3 to 134c946 Compare November 13, 2022 05:24
@Ralith Ralith merged commit c3bfe72 into jj-vcs:main Nov 13, 2022
@Ralith Ralith deleted the push-b3e19dc016af471e8c577aeeb5e0269d branch November 13, 2022 05:40
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