Re: [PATCH v2 08/13] pack-objects: enable --path-walk via config
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 2, 2025, 23:42 UTC
- Message-ID
- <aBVYa9SzXk7eguIz@nand.local>
- In-Reply-To
- <dd66a5b46f2ef52eedea323df891a2e24fcd48c1.1742829770.git.gitgitgadget@gmail.com>
On Mon, Mar 24, 2025 at 03:22:44PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 56 quoted lines
> From: Derrick Stolee <stolee@gmail.com> > > Users may want to enable the --path-walk option for 'git pack-objects' by > default, especially underneath commands like 'git push' or 'git repack'. > > This should be limited to client repositories, since the --path-walk option > disables bitmap walks, so would be bad to include in Git servers when > serving fetches and clones. There is potential that it may be helpful to > consider when repacking the repository, to take advantage of improved deltas > across historical versions of the same files. > > Much like how "pack.useSparse" was introduced and included in > "feature.experimental" before being enabled by default, use the repository > settings infrastructure to make the new "pack.usePathWalk" config enabled by > "feature.experimental" and "feature.manyFiles". > > Signed-off-by: Derrick Stolee <stolee@gmail.com> > --- > Documentation/config/feature.adoc | 4 ++++ > Documentation/config/pack.adoc | 8 ++++++++ > builtin/pack-objects.c | 3 +++ > repo-settings.c | 3 +++ > repo-settings.h | 1 + > 5 files changed, 19 insertions(+) > > diff --git a/Documentation/config/feature.adoc b/Documentation/config/feature.adoc > index f061b64b748..cb49ff2604a 100644 > --- a/Documentation/config/feature.adoc > +++ b/Documentation/config/feature.adoc > @@ -20,6 +20,10 @@ walking fewer objects. > + > * `pack.allowPackReuse=multi` may improve the time it takes to create a pack by > reusing objects from multiple packs instead of just one. > ++ > +* `pack.usePathWalk` may speed up packfile creation and make the packfiles be > +significantly smaller in the presence of certain filename collisions with Git's > +default name-hash. > > feature.manyFiles:: > Enable config options that optimize for repos with many files in the > diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc > index da527377faf..08d06271177 100644 > --- a/Documentation/config/pack.adoc > +++ b/Documentation/config/pack.adoc > @@ -155,6 +155,14 @@ pack.useSparse:: > commits contain certain types of direct renames. Default is > `true`. > > +pack.usePathWalk:: > + When true, git will default to using the '--path-walk' option in > + 'git pack-objects' when the '--revs' option is present. This > + algorithm groups objects by path to maximize the ability to > + compute delta chains across historical versions of the same > + object. This may disable other options, such as using bitmaps to > + enumerate objects. > +
Same note here as in the previous patch, I think it's fine to refer readers to the git-pack-objects[1] documentation instead of repeating yourself here. (And it removes the risk of these three descriptions falling out of sync with one another).
Show 17 quoted lines
> pack.preferBitmapTips::
> When selecting which commits will receive bitmaps, prefer a
> commit at the tip of any reference that is a suffix of any value
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a6b8a78d42a..0ea85754c52 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4652,6 +4652,9 @@ int cmd_pack_objects(int argc,
> if (use_bitmap_index > 0 ||
> !use_internal_rev_list)
> path_walk = 0;
> + else if (the_repository->gitdir &&
> + the_repository->settings.pack_use_path_walk)
> + path_walk = 1;
> else
> path_walk = git_env_bool("GIT_TEST_PACK_PATH_WALK", 0);
> }The limited diff context makes it hard for me to tell for sure, but this takes place after git_config(), right? If so, I think we can avoid using the repository settings machinery here and just use the config API directly.
(FWIW, I typically think of repository settings as a way to expose config information to some part of the codebase that doesn't otherwise have easy access to, e.g., a static field that was set by a git_config() callback).
Separate from the above: should this have a sanity test to makes sure that we read the config setting correctly?
Thanks, Taylor