Preserve File Modes

[peterwaller-arm]

Add a notion of FileMetadata (describing file state), as distinct from
ObjectMetadata (which describes objects in the store) which is stored in
the FileHandle and therefore on disk as part of snapshot_deltas.
FileMetadata is intended to be extensible but in the first instance
describes file modes bits. In the future it may also describe mtime and
I can imagine further uses arising.

[Note, I've edited this body to reflect the new approach, much of it
remains true from before and stays the same]

Users of the main branch, please note I forsee further potential version
bumps for additional functionality in the near future. If you're using
elfshaker in anger to make long term packs you may want to hold off
making those packs so you get the extra benefits, though it should work
aside from the missing as-yet-unimplemented features.

Compatibility notes:

• This elfshaker commit will still be able to read old elfshaker pack
  index files, because the default of an optional is None, which gives
  us exactly the old behaviour of elfshaker prior to this commit.

• elfshaker after this commit will produce packfiles that cannot be read
  by older commits of elfshaker. However, those old versions will report
  to the user that the version they have is too old and point them to
  where they can obtain a newer version.

The biggest risk here is that someone using the main branch makes
packfiles, and distributes those packfiles to a user who is not able to
build elfshaker, and we have not yet gotten around to releasing a new
version of elfshaker. So we should aim for a release of a new version
reasonably soon so that users don't find themselves in this situation
where they can't get an elfshaker which can read the packfiles they have
been provided.

Using an old elfshaker to produce packfiles and ship them to users of
any version of elfshaker is intended to be supported in principle.

Implementation:

We make use of the fact that a default Option is None, and that
Serde deserializers can cope with missing fields at the end of a struct
with the default attribute. Therefore to make elfshaker able to read old
files while implementing the new field, it's only necessary to add
#[serde(default)].

The version bump ensures that users of an old elfshaker trying to read a
pack written with the newer elfshaker see an informative message rather
than a deserialization failure.

Review notes:

Please see the individual commits which should make it reasonably clear how this works.

TODO

• Write additional tests. I intend to do this before merging, so
  please consider this draft until then. Feel free to take an early
  look or defer your look until I declare this ready.
  • Check that file modes changing across snapshots has the correct effect; I recall there being a potential gotcha here that changes in metadata without changes in content might be a problem. I'll need to look into that.

[peterwaller-arm]

There is a significant problem here I've spotted while considering the test for changing the metadata for a path. With the method, we can't represent:

• Changes in file modes.
• Files with the same checksum but differing metadata.

The reason is that ObjectMetadatarefers to the metadata of the object (where it live in the pack and its size), and not the metadata of the file. This is designed to be a 1-1 mapping between a checksum and ObjectMetadata, which is fair so long as it represents information about an object (which is uniquely identified by its hash), but not OK for files, whose identity does not relate to this hash.

It looks like a better place to put the file metadata would be in the FileHandle. We can represent mode changes then in the index snapshot deltas as removing the file with the old metadata and inserting a new one with the new data, just as we do for any other file change. I think we can use the same serde default trick here.

[peterwaller-arm]

I've added tests, switched the implementation and ticked the TODOs and edited the body of the PR so it reflects current reality.

The new implementation functions as described above, storing the metadata as part of the FileHandle.

I'm happy with this implementation at this point aside from the need to manually test it further.

[peterwaller-arm]

I consider this ready for review and testing but please leave the merging to me, I want to test it a bit more first.

[veselink1]

Overall, I like the approach, some comments.

[veselink1]

I think we might have to update PackIdx::compute_snapshot_checksum to consider file_metadata too. We use compute_snapshot_checksum to dedup snapshots and logically, two snapshots where that files have different mode bits should be considered different.

The same is true for Repository::compute_entry_diff which is used to calculate the changes we need to make when we extract a snapshot. I believe we will currently miss changes to file mode bits if the objects haven't changed.

[peterwaller-arm]

This is a great spot. I believe I found and fixed some variants of this problem, I'll dig in here.

[peterwaller-arm]

I believe we will currently miss changes to file mode bits if the objects haven't changed.

I think that's covered already, in compute_entry_diff, the e.file_metadata already forms part of the key in the hash map:

 let from_lookup: HashMap<_, _> = from_entries
            .iter()
            .map(|e| ((&e.path, &e.checksum, &e.file_metadata), e))

[peterwaller-arm]

Hmm, including the file mode in the checksum is making the gc test fail. Any bright ideas? I've run out of time for now to dig deeper but I'll take another look some other time.

[veselink1]

We should probably consider adding another test pack in test-scripts/artifacts for every version bump.

[veselink1]

Change looks good, but the gc test is failing (as you mentioned).

I believe it is because of e4620da as we're now considering file metadata.

The check.sh script tests against the old format verification.pack.idx -- which doesn't have file mode. And the gc test checks that we dedup a loose snapshot against one packed in verification.pack (which doesn't have file mode bits).

To fix it, we could consider no file mode bits in the snapshot to mean 644, which is what elfshaker extract would extract as anyway. This would allow dedup against existing snapshots.

Alternatively, we could change the gc test to create a new .pack against which to dedup.