clamp: fix clamp handling for xattr/bigfiles repos

[jlebon]

Commit 2869ce7 made xattr and bigfiles components clamp their mtimes to file mtimes instead of the resolved created epoch, which fixed the reproducibility of ancestor directories when file mtimes are canonical (i.e. were explicitly set to a meaningful, fixed, value) but an epoch wasn't explicitly set.

However, this breaks the case where an epoch is explicitly passed. The problem is that the file mtime may be arbitrary (e.g. if it's just created with the current time at image build time, like the rpmdb), which thus renders the image non-reproducible (even though SOURCE_DATE_EPOCH has been passed!).

I think what we want is actually the min() of the file mtime and the resolved epoch. That way, (1) if an SDE is passed in, and no mtime is explicitly set on a file created at build time, it's reasonable to expect it to be more recent than SDE and thus gets capped, but (2) if no SDE is passed, then it defaults to $now, and it's reasonable to expect any explicit mtime set on a bigfiles for reproducibility reasons to be before $now, and thus the file mtime wins. In both cases, we're basically picking the timestamp most likely to be stable.

This was actually part of an earlier version of PR #161, but I mistakenly in review claimed that we could drop it:

#161 (comment)

Fixes: #176
Assisted-by: AI

[jlebon]

@HastD Would appreciate a lookie if you have a chance. Will cut a release after we fix this.

[HastD]

Couldn't we just use the simpler implementation from the earlier version of my PR, where we set the component's mtime clamp to the minimum of the file mtime and the default_mtime_clamp parameter passed to the component repo?

Or, to put it another way, why do the component repos need to "know" about whether the default mtime clamp is from a deterministic source or from the build time? That feels like it's leaking an implementation detail and introduces unnecessary complexity. In terms of the actual outcome, I don't think there should be any difference as long as one isn't setting file mtimes or SOURCE_DATE_EPOCH to a time in the future, which is a rather silly thing to do.

[jlebon]

Or, to put it another way, why do the component repos need to "know" about whether the default mtime clamp is from a deterministic source or from the build time? That feels like it's leaking an implementation detail and introduces unnecessary complexity.

Yeah, I was thinking along those lines as well after letting this stew a bit more. I did initially think about that approach. The reason I then went with this path is that we can know whether an epoch was provided or not, whereas we can't know whether the file mtime is arbitrary or meaningful. So it feels more correct to always pick the file mtime when no epoch was provided.

But yes, I think regardless of whether it's meaningful or not, it would be quite odd for it to be after the defaulted wall clock. (I wouldn't at all be surprised if there's a use case somewhere for setting meaningful future dates, but I'm happy doing the simpler thing until that case becomes apparent.)

[HastD]

In any case, the implementation in this PR looks correct to me. I do still think it's a bit of unnecessary added complexity to inform the component repos of whether the global mtime clamp is deterministically provided or not, but it's up to you whether to stick with that or not.

[jlebon]

OK updated this now to just always do min() and added more comments for posterity.