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

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 28, 2025, 18:59 UTC
Message-ID: <6df25bf2-6df1-4c3a-9061-e3297dec38ab@gmail.com>
In-Reply-To: <xmqqmsc0dyyf.fsf@gitster.g>

On 4/28/2025 12:37 PM, Junio C Hamano wrote:

Show 14 quoted lines

> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> 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.
> 
> True.  If we are aiming to deliver this to end-user's hands in some
> future, I agree that we want to make it extensible, make it dtrt
> without being told, and make it harder to give wrong input.

I'm not focused on usability, but instead on faster development cycles if more advanced options are required in the future. I'll happily take some extra time now to help those who come after.

Show 5 quoted lines

> If we
> are going in that direction [*], I suspect this should not be a
> separate and independent input---rather, shouldn't the tool already
> _know_ what objects it placed in the resulting output stream, and
> should be able to _count_ that number by itself?

This makes sense as a feature, except that we need to write the number of objects present in a packfile in its header. If we wanted to avoid the argument, then we'd need to load the data into a list before starting to write the packfile. By taking the count in advance, the implementation is simpler.

Show 5 quoted lines

> One thing it lets
> us do to have this as a separate number is to create an invalid pack
> stream where the header gives a wrong number, and as a test tool,
> that may trump the convenience of not having to give the number
> explicitly.

But also, this allows generating bad packfiles which is a bonus. A very good point.

> Another thing we may want to add to the tool is to give it a mode
> that either (1) refuses to place the same object in a single pack
> stream more than once, or (2) warn when it happens.  The latter
> would be useful to create an invalid pack stream for testing.

Noted for potential future expansion.

Thanks, -Stolee