Re: [PATCH 1/3] test-tool: add pack-deltas helper

On 4/25/2025 12:27 PM, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

quoted

quoted

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.

Support for that has been merged just this week via 2bc5414c411 (Merge
branch 'ps/parse-options-integers', 2025-04-24).

The thing that confused me even with those changes is that this is a
_positional_ argument and we don't have a way to say "parse the 1st
positional argument into an integer".

quoted

quoted

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:

But in any case, I'd be equally fine with your suggestion.

Yeah, I think we clearly showed our "it's just test helper, whose
callers are supposed to know what they are doing" attitude, but with
proper helpers, it is not too much additional effort to do the right
thing.

But with this philosophy in mind I can change the CLI to be of the form
"--num-objects <n>" to use the parse-options feature. This should make
things more extensible in the future.

Thanks,
-Stolee