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

[Taylor Blau <me@ttaylorr.com>]

Junio,

On Fri, May 20, 2022 at 07:17:35PM -0400, Taylor Blau wrote:
> To store the individual mtimes of objects in a cruft pack, introduce a
> new `.mtimes` format that can optionally accompany a single pack in the
> repository.

Like I mentioned in this sub-thread, here is a small fixup! to apply on
top of this patch when queueing. I'm hoping this will be easier than
reapplying the dozen+ or so patches in this series (the rest of which
are unchanged). But if it isn't, please let me know and I can send you a
reroll of the whole thing.

In the meantime, here's the fixup...

--- 8< ---
Subject: [PATCH] fixup! pack-mtimes: support reading .mtimes files

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 object-store.h |  5 +++++
 pack-mtimes.c  | 35 +++++++++++++++++++----------------
 pack-mtimes.h  | 11 +++++++++++
 3 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/object-store.h b/object-store.h
index 2c4671ed7a..05cc9a33ed 100644
--- a/object-store.h
+++ b/object-store.h
@@ -122,6 +122,11 @@ struct packed_git {
 	const uint32_t *revindex_data;
 	const uint32_t *revindex_map;
 	size_t revindex_size;
+	/*
+	 * mtimes_map points at the beginning of the memory mapped region of
+	 * this pack's corresponding .mtimes file, and mtimes_size is the size
+	 * of that .mtimes file
+	 */
 	const uint32_t *mtimes_map;
 	size_t mtimes_size;
 	/* something like ".git/objects/pack/xxxxx.pack" */
diff --git a/pack-mtimes.c b/pack-mtimes.c
index 46ad584af1..0e0aafdcb0 100644
--- a/pack-mtimes.c
+++ b/pack-mtimes.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
 #include "pack-mtimes.h"
 #include "object-store.h"
 #include "packfile.h"
@@ -7,12 +8,10 @@ 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);
 }

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

 struct mtimes_header {
 	uint32_t signature;
@@ -26,10 +25,9 @@ static int load_pack_mtimes_file(char *mtimes_file,
 {
 	int fd, ret = 0;
 	struct stat st;
-	void *data = NULL;
-	size_t mtimes_size;
+	uint32_t *data = NULL;
+	size_t mtimes_size, expected_size;
 	struct mtimes_header header;
-	uint32_t *hdr;

 	fd = git_open(mtimes_file);

@@ -44,21 +42,16 @@ static int load_pack_mtimes_file(char *mtimes_file,

 	mtimes_size = xsize_t(st.st_size);

-	if (mtimes_size < MTIMES_MIN_SIZE) {
+	if (mtimes_size < MTIMES_HEADER_SIZE) {
 		ret = error(_("mtimes file %s is too small"), mtimes_file);
 		goto cleanup;
 	}

-	if (mtimes_size - MTIMES_MIN_SIZE != st_mult(sizeof(uint32_t), num_objects)) {
-		ret = error(_("mtimes file %s is corrupt"), mtimes_file);
-		goto cleanup;
-	}
+	data = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);

-	data = hdr = xmmap(NULL, mtimes_size, PROT_READ, MAP_PRIVATE, fd, 0);
-
-	header.signature = ntohl(hdr[0]);
-	header.version = ntohl(hdr[1]);
-	header.hash_id = ntohl(hdr[2]);
+	header.signature = ntohl(data[0]);
+	header.version = ntohl(data[1]);
+	header.hash_id = ntohl(data[2]);

 	if (header.signature != MTIMES_SIGNATURE) {
 		ret = error(_("mtimes file %s has unknown signature"), mtimes_file);
@@ -77,13 +70,23 @@ static int load_pack_mtimes_file(char *mtimes_file,
 		goto cleanup;
 	}

+
+	expected_size = MTIMES_HEADER_SIZE;
+	expected_size = st_add(expected_size, st_mult(sizeof(uint32_t), num_objects));
+	expected_size = st_add(expected_size, 2 * (header.hash_id == 1 ? GIT_SHA1_RAWSZ : GIT_SHA256_RAWSZ));
+
+	if (mtimes_size != expected_size) {
+		ret = error(_("mtimes file %s is corrupt"), mtimes_file);
+		goto cleanup;
+	}
+
 cleanup:
 	if (ret) {
 		if (data)
 			munmap(data, mtimes_size);
 	} else {
 		*len_p = mtimes_size;
-		*data_p = (const uint32_t *)data;
+		*data_p = data;
 	}

 	close(fd);
diff --git a/pack-mtimes.h b/pack-mtimes.h
index 38ddb9f893..cc957b3e85 100644
--- a/pack-mtimes.h
+++ b/pack-mtimes.h
@@ -8,8 +8,19 @@

 struct packed_git;

+/*
+ * Loads the .mtimes file corresponding to "p", if any, returning zero
+ * on success.
+ */
 int load_pack_mtimes(struct packed_git *p);

+/* Returns the mtime associated with the object at position "pos" (in
+ * lexicographic/index order) in pack "p".
+ *
+ * Note that it is a BUG() to call this function if either (a) "p" does
+ * not have a corresponding .mtimes file, or (b) it does, but it hasn't
+ * been loaded
+ */
 uint32_t nth_packed_mtime(struct packed_git *p, uint32_t pos);

 #endif
--
2.36.1.94.gb0d54bedca

--- >8 ---