On Mon, Dec 06, 2021 at 04:16:04PM -0500, Derrick Stolee wrote:
> On 11/29/2021 5:25 PM, Taylor Blau wrote:
> > +static int dump_mtimes(struct packed_git *p)
>
> nit: you return an int here so you can use it as an error code...
>
> > +{
> > + 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().
>
> > + 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
