Re: [PATCH 6/7] pack-objects: disable --full-name-hash when shallow

[Derrick Stolee <stolee@gmail.com>]

On 11/21/24 3:33 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:06AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> As demonstrated in the previous change, the --full-name-hash option of
>> 'git pack-objects' is less effective in a trunctated history. Thus, even
>> when the option is selected via a command-line option or config, disable
>> this option when the '--shallow' option is specified. This will help
>> performance in servers that choose to enable the --full-name-hash option
>> by default for a repository while not regressing their ability to serve
>> shallow clones.
>>
>> This will not present a compatibility issue in the future when the full
>> name hash values are stored in the reachability bitmaps, since shallow
>> clones disable bitmaps.
>>
>> Signed-off-by: Derrick Stolee <stolee@gmail.com>
>> ---
>>   builtin/pack-objects.c       | 6 ++++++
>>   t/perf/p5313-pack-objects.sh | 1 +
>>   2 files changed, 7 insertions(+)
> 
> I appreciate demonstrating the value of declaring --shallow and
> --full-name-hash incompatible by showing the performance numbers in the
> previous patch.
> 
> But TBH I think that it would be equally fine or slightly better to say
> up front "when combined with --shallow, this option produces larger
> packs during testing, so the two are incompatible for now". You could
> include some performance numbers there to illustrate that difference in
> the commit log too if you wanted.
> 
> But I don't think it's worth introducing the pair as compatible only to
> mark them incompatible later on in the same series.
I disagree and here's why: they are not functionally incompatible. This
performance-focused change is worth justifying with performance test data
_and_ isolating from the initial implementation with its own reasoning
for future history spelunkers. Having these warning lines blame to this
patch instead of the initial implementation will make it much easier to
understand the justification of this change.

But maybe this patch can be removed if we use Jonathan's function. I'll
check the performance tests to see if this continues to be justified.

Thanks,
-Stolee