Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2023, 20:22 UTC
Message-ID: <xmqq8r8giztv.fsf@gitster.g>
In-Reply-To: <ZR7yOgvk3PCtYIR2@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 21 quoted lines

> On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote:
>> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote:
>> > The repack builtin takes a `--max-pack-size` command-line argument which
>> > it uses to feed into any of the pack-objects children that it may spawn
>> > when generating a new pack.
>> >
>> > This option is parsed with OPT_STRING, meaning that we'll accept
>> > anything as input, punting on more fine-grained validation until we get
>> > down into pack-objects.
>> >
>> > This is fine, but it's wasteful to spend an entire sub-process just to
>> > figure out that one of its option is bogus. Instead, parse the value of
>> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the
>> > knonw-good result down to pack-objects.
>>
>> Tiny nit: s/knonw/known.
>>
>> Other than that this patch looks good to me.
>
> Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he
> doesn't, I don't think it'd be the end of the world...

I cannot do so "when queuing" anymore, but let me see if it is still outside 'next' and then run "rebase -i" on it.

This is a bit different from what I would have written (I would have passed the original string to underlying subprocess for it to sanity check the value to its own liking, after we checked if it makes sense when interpreted as magnitude, so that the underlying subprocess, if it has stricter limit than we know of, can complain with the string the end-user gave it, not the result of us turning into a large integer string. But that is minor.

Thanks, both.