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

Partly remove the notion of entry sizes for non-blobs #1001

Merged
merged 1 commit into from
May 16, 2024

Conversation

jrray
Copy link
Collaborator

@jrray jrray commented Mar 8, 2024

This kind of change is desired to eliminate a problem of spfs diff reporting changes to directory sizes.

However, the size value associated with non-blob entries is still needed in order to preserve the same on-disk representation and digest calculated for manifests.

@jrray jrray self-assigned this Mar 8, 2024
Copy link
Collaborator

@rydrman rydrman left a comment

Choose a reason for hiding this comment

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

Haiving the EntryKind::Blob hold data feels like it breaks the design pattern here, but I appreciate that any other change would need to be much more involved

@jrray jrray added the pr-chain This PR doesn't target the main branch, don't merge! label Mar 8, 2024
@rydrman rydrman force-pushed the flatbuffer-objects branch 3 times, most recently from ed18c8d to 93d4c55 Compare March 13, 2024 23:29
Base automatically changed from flatbuffer-objects to main March 13, 2024 23:44
@jrray jrray removed the pr-chain This PR doesn't target the main branch, don't merge! label May 15, 2024
This kind of change is desired to eliminate a problem of `spfs diff`
reporting changes to directory sizes.

However, the size value associated with non-blob entries is still needed
in order to preserve the same on-disk representation and digest
calculated for manifests.

Signed-off-by: J Robert Ray <jrray@jrray.org>
@jrray
Copy link
Collaborator Author

jrray commented May 15, 2024

Haiving the EntryKind::Blob hold data feels like it breaks the design pattern here, but I appreciate that any other change would need to be much more involved

A common pattern I see is a combination of a struct and an enum for when you want to have an enum but all arms the enum would share the same field. Those shared fields get pushed down into the struct and the enum itself only has fields that are unique to each arm. This pattern helps prevent bugs by making it impossible to describe something meaningless, like give non-blobs a size.

I pushed an update to resolve conflicts, it was just from some tests moving to a different file. If you want to have a look again. Otherwise I'll merge this in a day or so.

@rydrman
Copy link
Collaborator

rydrman commented May 16, 2024

Maybe it's more about the naming and existing usage being focused on describing exactly the kind because I agree with the pattern that you are describing. In any case, I'm not suggesting further changes at this point - just closing the discussion

@jrray jrray merged commit 3f5fbdb into main May 16, 2024
7 checks passed
@jrray jrray deleted the unsized-directories branch May 16, 2024 00:54
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