chunked: skip setting time if empty

[giuseppe]

do not specify the timestamps fields in the TOC if they are empty.

[cgwalters]

/approve
Looks sane to me, but a single sentence for why would be useful to me. What motivated the change? I can guess: is this an optimization to reduce the size of the ToC JSON? Or something else?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[giuseppe]

Looks sane to me, but a single sentence for why would be useful to me. What motivated the change? I can guess: is this an optimization to reduce the size of the ToC JSON? Or something else?

it improves readability if anyone inspects the TOC file itself and it slightly reduces the TOC size, e.g. with Fedora I get:

• compressed TOC before: 86643308
• compressed TOC after: 86642710

so it is just 598 bytes after compression, not really worth mentioning as an improvement :)

[cgwalters]

Hum, the Go docs say:

The zero value of type Time is January 1, year 1, 00:00:00.000000000 UTC. As this time is unlikely to come up in practice

Which is actually different than what I thought (I thought it'd be the unix epoch).

So...apparently this actually did come up in practice, but...why?

with Fedora

Which Fedora? quay.io/fedora/fedora:40 ?

[giuseppe]

The zero value of type Time is January 1, year 1, 00:00:00.000000000 UTC. As this time is unlikely to come up in practice

Which is actually different than what I thought (I thought it'd be the unix epoch).

So...apparently this actually did come up in practice, but...why?

It could be that the tar stream lacks this value and it automatically gets assigned the "zero" value for time.Time

Which Fedora? quay.io/fedora/fedora:40 ?

I've built a local image with:

FROM registry.fedoraproject.org/fedora:latest
RUN touch /timestamp

and squashed it with podman build --squash-all

[cgwalters]

Sorry I know this is a trivial PR and that makes it more likely to be bike-shedded but...this just keeps generating more questions for me.

Looking at e.g. the PAX format, mtime is a mandatory field.

Presumably it's actually the access time or ctime which are zero?

Actually I start to wonder - why the heck did the people creating eStargz add explicit support for atime and ctime? Even mtime is questionable...yes though, it's hard to escape the concept of files having a time (e.g. Python famously depends on it). But nothing should care if we always made atime == ctime == mtime... (xref containers/composefs#163 too here, ctime is simply not representable in composefs).

Anyways,
/lgtm

[cgwalters]

(As far as atime...well, I agree with a lot of the sentiment behind https://lwn.net/Articles/244829/ ...it should be an opt-in thing for /tmp at most. At least on an image-based system like bootc with / being a read-only mount by default, simply running binaries doesn't result in writes to the underlying filesystem due to atime...but, that is what happens on a traditional apt/dnf system...)

[cgwalters]

And for containers, we're kind of saved by overlayfs:

POSIX mandates updating st_atime for reads. This is currently not done in the case when the file resides on a lower layer.

[cgwalters]

Right, clicking on the composefs link, we already covered this topic earlier in #1646 (comment)

So omitting the times here makes sense.