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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
