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

[Derrick Stolee <stolee@gmail.com>]

On 4/25/2025 12:27 PM, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
>>>> 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".

>>> 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