Re: [PATCH 1/3] test-tool: add pack-deltas helper
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Apr 28, 2025, 15:22 UTC
- Message-ID
- <275808ae-7126-4a24-b5f3-283ea8023f5f@gmail.com>
- In-Reply-To
- <xmqqbjskurz5.fsf@gitster.g>
On 4/25/2025 12:27 PM, Junio C Hamano wrote:
Show 16 quoted lines
> 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".
Show 12 quoted lines
>>> 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