lore-revision: Expose size and file mode in TreePath#19
Conversation
This allows a thin client to use ThinClientService.RevisionTree to efficiently retrieve additional information such as file size and mode. Fixes EpicGames#18 Signed-off-by: Javier Blazquez <blaz@blazlabs.com>
| /// Content address for FILE / LINK entries; unused for DIRECTORY. | ||
| #[prost(message, optional, tag = "3")] | ||
| pub address: ::core::option::Option<crate::lore::model::v1::Address>, | ||
| /// Original size in bytes. For DIRECTORY entries, this is the cumulative size of its descendant files. |
There was a problem hiding this comment.
I'm not sure if size for DIRECTORY entries being the sum of all descendant files is a guarantee that Lore fundamentally provides or whether it just happens to be like that in the current implementation. If it's not something Lore wants to commit to, we could explicitly set this to 0 for directories.
There was a problem hiding this comment.
It is definitely something Lore provides - with the caveat that it is the full unfiltered size of all child nodes.
There was a problem hiding this comment.
Maybe it would be possible to math-it-out, like, subtract the sizes of child directory nodes, to get the size of files in the directory?
There was a problem hiding this comment.
I think the current behavior is actually better. Imagine a UI that shows the contents of the repo as a tree view. At each level, it could show a size next to a directory that describes the total size of everything under that directory, recursively, so you can at a glance see where the largest parts of the repo are.
There was a problem hiding this comment.
Well I think that it can do both: show the size of files in the directory and the tree beneath. Just make two fields, makes the API reacher
There was a problem hiding this comment.
Lore usually avoids duplicating info that can be gathered or calculated reasonably easy on the receiving side. Data over the wire should be the minimal unique set of data.
|
Looks good, my only concern is the reduction of the file mode to an executable bit boolean - this is the only mode we currently track, but we could potentially add more in the future. |
Yes, I wasn't sure which way we'd like to go. I'll change the boolean to an integer field, and add an enum that represents the different mode flags. |
This replaces the use of a boolean to indicate the executable flags on files. Signed-off-by: Javier Blazquez <blaz@blazlabs.com>
jblazquez
force-pushed
the
treepath-size-and-mode
branch
from
788409f to
f37a1e8
Compare
| // Original size in bytes. For DIRECTORY entries, this is the cumulative size of its descendant files. | ||
| uint64 size = 4; | ||
| // File mode for this entry. For possible flags and values, see enum FileMode. | ||
| uint64 mode = 5; |
There was a problem hiding this comment.
Not making this of type FileMode directly because it needs to be able to represent multiple mode flags, so it has to be an integer. Also making it uint64 despite mode internally being u16 for future-proofness and because Protobuf varint encoding takes care of making this small on the wire (0 bytes if mode = NONE and 1 byte if mode = EXECUTABLE).
|
@mjansson Thanks for the review, I have updated the ThinClient service to use a new |
3 participants
This allows a thin client to use ThinClientService.RevisionTree to efficiently retrieve additional information such as file size and mode.
If this change looks good, a future change could add similar information to
RevisionDiffResponse.Fixes #18