Re: [PATCH 02/17] pack-mtimes: support reading .mtimes files

[Derrick Stolee <stolee@gmail.com>]

On 11/29/2021 5:25 PM, Taylor Blau wrote:

> +== pack-*.mtimes files have the format:
> +
> +  - A 4-byte magic number '0x4d544d45' ('MTME').
> +
> +  - A 4-byte version identifier (= 1).
> +
> +  - A 4-byte hash function identifier (= 1 for SHA-1, 2 for SHA-256).

I vaguely remember complaints about using a 1-byte identifier in
the commit-graph and multi-pack-index formats because the "standard"
way to refer to these hash functions was a magic number that had a
meaning in ASCII that helped human readers a bit. I cannot find an
example of such 4-byte identifiers, but perhaps brian (CC'd) could
remind us.

You are using a 4-byte identifier, but using the same values as
those 1-byte identifiers.

> +  - A table of mtimes (one per packed object, num_objects in total, each
> +    a 4-byte unsigned integer in network order), in the same order as
> +    objects appear in the index file (e.g., the first entry in the mtime
> +    table corresponds to the object with the lowest lexically-sorted
> +    oid). The mtimes count standard epoch seconds.

This paragraph seemed awkward. Here is a rephrasing that might be
less awkward:

 - A table of 4-byte unsigned integers in network order. The ith value
   is the modified time (mtime) of the ith object of the corresponding
   pack in lexicographic order. The mtime represents standard epoch
   seconds.

Storing these mtimes in 32-bits means we will hit the 2038 problem.
The commit-graph stores commit times with an extra two bits to extend
the lifetime by another hundred years or so.

Could we extend the lifetime of cruft packs by decreasing the granularity
here? Should 'mtime' store a number of _minutes_ instead of seconds? That
should be enough granularity for these purposes.

> +  - A trailer, containing a:
> +
> +    checksum of the corresponding packfile, and
> +
> +    a checksum of all of the above.

Could you specify the checksum as having length according to the
specified hash function?

> +All 4-byte numbers are in network order.
> +

Maybe this could be at the start of the format, since the file
version and hash function are both 4-byte numbers here and we
could remove the mention of network order from the mtime values.

> +static char *pack_mtimes_filename(struct packed_git *p)
> +{
> +	size_t len;
> +	if (!strip_suffix(p->pack_name, ".pack", &len))
> +		BUG("pack_name does not end in .pack");
> +	/* NEEDSWORK: this could reuse code from pack-revindex.c. */
> +	return xstrfmt("%.*s.mtimes", (int)len, p->pack_name);
> +}

I see your NEEDSWORK here and you are probably referring to this:

static char *pack_revindex_filename(struct packed_git *p)
{
	size_t len;
	if (!strip_suffix(p->pack_name, ".pack", &len))
		BUG("pack_name does not end in .pack");
	return xstrfmt("%.*s.rev", (int)len, p->pack_name);
}

and the implementation is identical except for the new trailer
(which exist in the exts[] array in builtin/repack.c, but could
also be pulled out into a header somewhere.

I'm happy to delay any cleanup of these code clones until later,
if at all, because doing it right might mean moving more code
than we like. Such refactorings aren't worth it most of the time.

> +static int load_pack_mtimes_file(char *mtimes_file,
> +				 uint32_t num_objects,
> +				 const uint32_t **data_p, size_t *len_p)
> +{

> +	if (mtimes_size - MTIMES_MIN_SIZE != st_mult(sizeof(uint32_t), num_objects)) {
> +		ret = error(_("mtimes file %s is corrupt"), mtimes_file);

This message could be more informative: "mtimes file %s has the wrong size"?

> +	data = hdr = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +
> +	if (ntohl(*hdr) != MTIMES_SIGNATURE) {
> +		ret = error(_("mtimes file %s has unknown signature"), mtimes_file);
> +		goto cleanup;
> +	}

Interesting that you defined 'struct mtimes_header' before this
method, but don't use it here (in favor of moving a uint32_t
pointer). Perhaps you are avoiding pointing the struct at the
memory map, but you could also do this:

	struct mtimes_header header;

	header.signature = ntohl(hdr[0]);
	header.version = ntohl(hdr[1]);
	header.hash_id = ntohl(hdr[2]);

And then operate on the struct for your validation.

At the very least, 'struct mtimes_header' is defined but not
used in this patch. If you decide to not use it this way, then
maybe delay its definition.

> +
> +	if (ntohl(*++hdr) != 1) {
> +		ret = error(_("mtimes file %s has unsupported version %"PRIu32),
> +			    mtimes_file, ntohl(*hdr));

Unlike the commit-graph, if we don't understand the version we
cannot simply ignore the data. error() is appropriate here.

> +int load_pack_mtimes(struct packed_git *p)
> +{
> +	char *mtimes_name = NULL;
> +	int ret = 0;
> +
> +	if (!p->is_cruft)
> +		return ret; /* not a cruft pack */

Interesting that this indicator is essentially "we have an mtimes
file for this pack", but it makes sense to include that check next
to the .keep and .promisor checks.

> +uint32_t nth_packed_mtime(struct packed_git *p, uint32_t pos)
> +{
> +	if (!p->mtimes_map)
> +		BUG("pack .mtimes file not loaded for %s", p->pack_name);
> +	if (p->num_objects <= pos)
> +		BUG("pack .mtimes out-of-bounds (%"PRIu32" vs %"PRIu32")",
> +		    pos, p->num_objects);
> +
> +	return get_be32(p->mtimes_map + pos + 3);
> +}

A nice safe access method. Good.

> -	static const char *exts[] = {".pack", ".idx", ".rev", ".keep", ".bitmap", ".promisor"};
> +	static const char *exts[] = {".pack", ".idx", ".rev", ".keep", ".bitmap", ".promisor", ".mtimes"};

(Speaking of that refactoring earlier, here is a second definition of
exts[] that would be valuable to unify.)

The hunks I did not comment on look good. Nice standard file format
stuff.

Thanks,
-Stolee