Message ID | b8a055cb196dd971ac21611c1957be319557b4d3.1730775908.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | pack-objects: Create an alternative name hash algorithm (recreated) | expand |
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. Thanks, Taylor
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
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 7cb6f0e0942..f68fc30c9b9 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4589,6 +4589,12 @@ int cmd_pack_objects(int argc, if (use_full_name_hash < 0) use_full_name_hash = git_env_bool("GIT_TEST_FULL_NAME_HASH", 0); + if (shallow && use_full_name_hash > 0 && + !git_env_bool("GIT_TEST_USE_FULL_NAME_HASH_WITH_SHALLOW", 0)) { + use_full_name_hash = 0; + warning("the --full-name-hash option is disabled with the --shallow option"); + } + if (write_bitmap_index && use_full_name_hash > 0) { warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index")); use_full_name_hash = 0; diff --git a/t/perf/p5313-pack-objects.sh b/t/perf/p5313-pack-objects.sh index dfa29695315..a7f4e0bf8d8 100755 --- a/t/perf/p5313-pack-objects.sh +++ b/t/perf/p5313-pack-objects.sh @@ -66,6 +66,7 @@ test_size 'shallow pack size' ' ' test_perf 'shallow pack with --full-name-hash' ' + GIT_TEST_USE_FULL_NAME_HASH_WITH_SHALLOW=1 \ git pack-objects --stdout --revs --sparse --shallow --full-name-hash <in-shallow >out '