Re: [PATCH 1/3] test-tool: add pack-deltas helper
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Apr 25, 2025, 09:34 UTC
- Message-ID
- <090ef16f-42a7-8de6-a79e-5a1958e2c103@gmx.de>
- In-Reply-To
- <aAsQwSfr-YvS2Mvh@pks.im>
Hi Patrick,
On Fri, 25 Apr 2025, Patrick Steinhardt wrote:
Show 25 quoted lines
> On Wed, Apr 23, 2025 at 05:40:02PM +0000, Derrick Stolee via GitGitGadget wrote:
> > diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c
> > new file mode 100644
> > index 00000000000..db7d1c3cd1f
> > --- /dev/null
> > +++ b/t/helper/test-pack-deltas.c
> > @@ -0,0 +1,140 @@
> [snip]
> > +int cmd__pack_deltas(int argc, const char **argv)
> > +{
> > + int N;
> > + struct hashfile *f;
> > + struct strbuf line = STRBUF_INIT;
> > +
> > + if (argc != 2) {
> > + usage(usage_str);
> > + return -1;
> > + }
> > +
> > + N = atoi(argv[1]);
>
> Is there a reason why we don't use `parse_options()` here? It might make
> this tool easier to use and extend going forward, and we wouldn't have
> to care about invalid arguments. Right now, we silently accept a
> non-integer argument and do the wrong thing.I think that `parse_options()` would be overkill here because:
- This is a _mandatory_ argument, not an optional one.
- The required data type is `uint32_t`, and `parse_options()` has no support for that.
But you do have a good point in that we may want to validate the data type (even if technically, this is not a user-facing program, it's a test helper that is used under tight control by Git's own test suite).
Consequently, I would suggest this fixup instead:
-- snipsnap -- Subject: [PATCH] fixup! test-tool: add pack-deltas helper
Let's make the command-line parsing a bit more stringent. We _could_ use `parse_options()`, but that would be overkill for a single, non-optional argument. Besides, it would not bring any benefit, as the parsed value needs to fit in the `uint32_t` type, and `parse_options()` has no provision to ensure that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/helper/test-pack-deltas.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c index 4af69bdc05d3..f95d8ee16768 100644 --- a/t/helper/test-pack-deltas.c +++ b/t/helper/test-pack-deltas.c @@ -8,11 +8,12 @@ #include "hex.h" #include "pack.h" #include "pack-objects.h" +#include "parse.h" #include "setup.h" #include "strbuf.h" #include "string-list.h" -static const char usage_str[] = "test-tool pack-deltas <n>"; +static const char usage_str[] = "test-tool pack-deltas <nr_entries>"; static unsigned long do_compress(void **pptr, unsigned long size) { @@ -79,7 +80,7 @@ static void write_ref_delta(struct hashfile *f, int cmd__pack_deltas(int argc, const char **argv) { - int N; + unsigned long n; struct hashfile *f; struct strbuf line = STRBUF_INIT; @@ -88,12 +89,13 @@ int cmd__pack_deltas(int argc, const char **argv) return -1; } - N = atoi(argv[1]); + if (!git_parse_ulong(argv[1], &n) || n != (uint32_t)n) + die("invalid number of objects: %s", argv[1]); setup_git_directory(); f = hashfd(1, "<stdout>"); - write_pack_header(f, N); + write_pack_header(f, n); /* Read each line from stdin into 'line' */ while (strbuf_getline_lf(&line, stdin) != EOF) {
-- 2.49.0.windows.1