mirage / ocaml-tar

Pure OCaml library to read and write tar files
ISC License
54 stars 34 forks source link

remove ppx_cstruct dependency #117

Closed hannesm closed 1 year ago

hannesm commented 1 year ago

I'm pretty sure we can further simplify the module by embedding the marshal/unmarshal-string/int/int64 into the get and set functions -- the commit presented here is a first step.

I'm a bit worried that the "get_hdr_version" is not used at all in the unmarshal code.

reynir commented 1 year ago

In #106 the version is checked. https://github.com/mirage/ocaml-tar/pull/106/files#diff-437e75b698404667f7caa6812dc0d41bc2aad6098bfb5afe7957ab1f95d26db5R377-R384

In the same PR I fix as well how the version is serialized: it is serialized as 0\000 for Posix (ustar? I'm confused about the versions) while it should be 00. GNU tar and others don't seem to mind. I'm unsure how much we should check given that.

hannesm commented 1 year ago

In #106 the version is checked. https://github.com/mirage/ocaml-tar/pull/106/files#diff-437e75b698404667f7caa6812dc0d41bc2aad6098bfb5afe7957ab1f95d26db5R377-R384

ah, cool. well, in this PR, there is a get_hdr_vesion, but it is commented out (as _get_hdr_version) to avoid unused binding warning.

In the same PR I fix as well how the version is serialized: it is serialized as 0\000 for Posix (ustar? I'm confused about the versions) while it should be 00. GNU tar and others don't seem to mind. I'm unsure how much we should check given that.

I think this is a great idea. What is needed to move that PR forward?

hannesm commented 1 year ago

I rebased on main, and moved the marshal/unmarshal into the accessors. From my perspective, this is now fine to be reviewed and merged. The offset and sizeof are only used in the accessors (apart from the old gnu magic stuff which uses "magic" and "version" with "ustar ".

WDYT?

reynir commented 1 year ago

Thanks!