Hi Patrick,
On Fri, 25 Apr 2025, Patrick Steinhardt wrote:
> 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@xxxxxx>
---
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
