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

On May 24, 2022 3:32 PM, Taylor Blau wrote:

Taylor Blau wrote:

quoted

This patch prepares for cruft packs by defining the `.mtimes` format,
and introducing a basic API that callers can use to read out
individual mtimes.

Makes sense.  Does this intend to produce any functional change?  I'm

guessing

not (and the lack of tests agrees), but the commit message doesn't say so.

By the way, is this something we could cover in tests, e.g. using a test

helper that

exercises the new code?

[...]

quoted

--- a/Documentation/technical/pack-format.txt
+++ b/Documentation/technical/pack-format.txt

@@ -294,6 +294,25 @@ Pack file entry: <+

 All 4-byte numbers are in network order.

+== pack-*.mtimes files have the format:
+
+All 4-byte numbers are in network byte order.
+
+  - 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).
+
+  - A table of 4-byte unsigned integers. The ith value is the
+    modification time (mtime) of the ith object in the corresponding
+    pack by lexicographic (index) order. The mtimes count standard
+    epoch seconds.
+
+  - A trailer, containing a checksum of the corresponding packfile,
+    and a checksum of all of the above (each having length according
+    to the specified hash function).
+

This describes the "syntax" but not the "semantics" of the file.
Should I look to a separate piece of documentation for the semantics?
If so, can this one include a mention of that piece of documentation to

make it

easier to find?

[...]

quoted

--- a/object-store.h
+++ b/object-store.h

@@ -115,12 +115,15 @@ struct packed_git {
 		 freshened:1,
 		 do_not_close:1,
 		 pack_promisor:1,
-		 multi_pack_index:1;
+		 multi_pack_index:1,
+		 is_cruft:1;
 	unsigned char hash[GIT_MAX_RAWSZ];
 	struct revindex_entry *revindex;
 	const uint32_t *revindex_data;
 	const uint32_t *revindex_map;
 	size_t revindex_size;
+	const uint32_t *mtimes_map;
+	size_t mtimes_size;

What does mtimes_map contain?  A comment would help.

quoted

--- /dev/null
+++ b/pack-mtimes.c

@@ -0,0 +1,126 @@
+#include "pack-mtimes.h"
+#include "object-store.h"
+#include "packfile.h"

Missing #include of git-compat-util.h.

quoted

+
+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); }

This seems simple enough that it's not obvious we need more code sharing.

Do

you agree?  If so, I'd suggest just removing the NEEDSWORK comment.

quoted

+
+#define MTIMES_HEADER_SIZE (12)
+#define MTIMES_MIN_SIZE (MTIMES_HEADER_SIZE + (2 *
+the_hash_algo->rawsz))

Hm, the all-caps name makes this feel like a compile-time constant but it

contains

a reference to the_hash_algo.  Could it be an inline function instead?

quoted

+
+struct mtimes_header {
+	uint32_t signature;
+	uint32_t version;
+	uint32_t hash_id;
+};
+
+static int load_pack_mtimes_file(char *mtimes_file,
+				 uint32_t num_objects,
+				 const uint32_t **data_p, size_t *len_p)

What does this function do?  A comment would help.

quoted

+{
+	int fd, ret = 0;
+	struct stat st;
+	void *data = NULL;
+	size_t mtimes_size;
+	struct mtimes_header header;
+	uint32_t *hdr;
+
+	fd = git_open(mtimes_file);
+
+	if (fd < 0) {

nit: this would be more readable without the blank line between setting and
checking fd (likewise for the other examples below).

quoted

+		ret = -1;
+		goto cleanup;
+	}

[...]

quoted

+	if (mtimes_size - MTIMES_MIN_SIZE != st_mult(sizeof(uint32_t),
+num_objects)) {

This presupposes that the hash_id matches the_hash_algo.  Maybe worth a
NEEDSWORK comment.

[...]

quoted

+cleanup:
+	if (ret) {
+		if (data)
+			munmap(data, mtimes_size);
+	} else {
+		*len_p = mtimes_size;
+		*data_p = (const uint32_t *)data;

Do we know that 'data' is uint32_t aligned?  Casting earlier in the

function could

make that more obvious.

[...]

quoted

+int load_pack_mtimes(struct packed_git *p)

This could use a doc comment in the header file.  For example, what

requirements

do we have on what the caller passes as 'p'?

[...]

quoted

+uint32_t nth_packed_mtime(struct packed_git *p, uint32_t pos)

Likewise.

[...]

quoted

--- a/packfile.c
+++ b/packfile.c

[...]

quoted

@@ -363,7 +373,7 @@ void close_object_store(struct raw_object_store

*o)

 void unlink_pack_path(const char *pack_name, int force_delete)  {
-	static const char *exts[] = {".pack", ".idx", ".rev", ".keep",

".bitmap",

".promisor"};

quoted

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

Are these in any particular order?  Should they be?

[...]

quoted

@@ -718,6 +728,10 @@ struct packed_git *add_packed_git(const char *path,

size_t path_len, int local)

quoted

 	if (!access(p->pack_name, F_OK))
 		p->pack_promisor = 1;

+	xsnprintf(p->pack_name + path_len, alloc - path_len, ".mtimes");
+	if (!access(p->pack_name, F_OK))
+		p->is_cruft = 1;
+
 	xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack");
 	if (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {
 		free(p);

@@ -869,7 +883,8 @@ static void prepare_pack(const char *full_name,

size_t

full_name_len,

quoted

 	    ends_with(file_name, ".pack") ||
 	    ends_with(file_name, ".bitmap") ||
 	    ends_with(file_name, ".keep") ||
-	    ends_with(file_name, ".promisor"))
+	    ends_with(file_name, ".promisor") ||
+	    ends_with(file_name, ".mtimes"))

likewise

I am again concerned about 32-bit time_t assumptions. time_t is 32-bit on
some platforms, signed/unsigned, and sometimes 64-bit. We are talking about
potentially long-persistent files, as I understand this series, so we should
not be limiting times to end at 2038. That's only 16 years off and I would
wager that many clones that exist today will exist then.
--Randall