Re: [PATCH 06/17] t/helper: add 'pack-mtimes' test-tool

On Mon, Dec 06, 2021 at 04:16:04PM -0500, Derrick Stolee wrote:

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

quoted

+static int dump_mtimes(struct packed_git *p)

nit: you return an int here so you can use it as an error code...

quoted

+{
+	uint32_t i;
+	if (load_pack_mtimes(p) < 0)
+		die("could not load pack .mtimes");
+
+	for (i = 0; i < p->num_objects; i++) {
+		struct object_id oid;
+		if (nth_packed_object_id(&oid, p, i) < 0)
+			die("could not load object id at position %"PRIu32, i);
+
+		printf("%s %"PRIu32"\n",
+		       oid_to_hex(&oid), nth_packed_mtime(p, i));
+	}
+
+	return 0;

But always return 0 unless you die().

quoted

+	return p ? dump_mtimes(p) : 1;

It makes this line concise, I suppose.

Perhaps just use "return dump_mtimes(p)" and have dump_mtimes()
return 1 if the given pack is NULL?

I think just dying in the case we have a NULL pack is fine, and it
should be OK to lump it in the same case as "could not load pack .mtimes".

But we may want to catch the case a little earlier while we still have
the pack name handy. Perhaps something like this on top:

--- 8< ---
diff --git a/t/helper/test-pack-mtimes.c b/t/helper/test-pack-mtimes.c
index b143f62520..f7b79daf4c 100644
--- a/t/helper/test-pack-mtimes.c
+++ b/t/helper/test-pack-mtimes.c

@@ -5,7 +5,7 @@
 #include "packfile.h"
 #include "pack-mtimes.h"

-static int dump_mtimes(struct packed_git *p)
+static void dump_mtimes(struct packed_git *p)
 {
 	uint32_t i;
 	if (load_pack_mtimes(p) < 0)

@@ -19,8 +19,6 @@ static int dump_mtimes(struct packed_git *p)
 		printf("%s %"PRIu32"\n",
 		       oid_to_hex(&oid), nth_packed_mtime(p, i));
 	}
-
-	return 0;
 }

 static const char *pack_mtimes_usage = "\n"

@@ -49,5 +47,10 @@ int cmd__pack_mtimes(int argc, const char **argv)

 	strbuf_release(&buf);

-	return p ? dump_mtimes(p) : 1;
+	if (!p)
+		die("could not find pack '%s'", argv[1]);
+
+	dump_mtimes(p);
+
+	return 0;
 }
--- >8 ---

Thanks,
Taylor