Re: [PATCH 06/17] t/helper: add 'pack-mtimes' test-tool
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Feb 23, 2022, 22:24 UTC
- Message-ID
- <Yha0M1bKxCb1/MrJ@nand.local>
- In-Reply-To
- <15f1bbc6-7ae6-0ed3-872a-51feebd1296c@gmail.com>
On Mon, Dec 06, 2021 at 04:16:04PM -0500, Derrick Stolee wrote:
Show 29 quoted lines
> 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