git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Junio C HamanoNext: Junio C Hamano
Message 13 of 29 in “Fix REF_DELTA chain bug in 'git index-pack'”
  1. 0/3 Fix REF_DELTA chain bug in 'git index-pack'Derrick Stolee via GitGitGadget, Apr 23, 2025
  2. 1/3 test-tool: add pack-deltas helperDerrick Stolee via GitGitGadget, Apr 23, 2025
  3. Junio C HamanoApr 23, 2025
  4. Derrick StoleeApr 23, 2025
  5. Junio C HamanoApr 24, 2025
  6. Derrick StoleeApr 24, 2025
  7. Junio C HamanoApr 24, 2025
  8. Patrick SteinhardtApr 25, 2025
  9. Johannes SchindelinApr 25, 2025
  10. Patrick SteinhardtApr 25, 2025
  11. Johannes SchindelinApr 25, 2025
  12. Junio C HamanoApr 25, 2025
  13. Derrick StoleeApr 28, 2025
  14. Junio C HamanoApr 28, 2025
  15. Derrick StoleeApr 28, 2025
  16. Junio C HamanoApr 28, 2025
  17. 2/3 t5309: create failing test for 'git index-pack'Derrick Stolee via GitGitGadget, Apr 23, 2025
  18. Junio C HamanoApr 23, 2025
  19. 3/3 index-pack: allow revisiting REF_DELTA chainsDerrick Stolee via GitGitGadget, Apr 23, 2025
  20. Junio C HamanoApr 24, 2025
  21. Derrick StoleeApr 25, 2025
  22. 0/3 Fix REF_DELTA chain bug in 'git index-pack'Derrick Stolee via GitGitGadget, Apr 28, 2025
  23. 1/3 test-tool: add pack-deltas helperDerrick Stolee via GitGitGadget, Apr 28, 2025
  24. 2/3 t5309: create failing test for 'git index-pack'Derrick Stolee via GitGitGadget, Apr 28, 2025
  25. 3/3 index-pack: allow revisiting REF_DELTA chainsDerrick Stolee via GitGitGadget, Apr 28, 2025
  26. Taylor BlauMay 7, 2025
  27. Derrick StoleeMay 7, 2025
  28. Junio C HamanoApr 28, 2025
  29. Patrick SteinhardtApr 29, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.