Re: [PATCH] pack-refs: teach --exclude option to exclude refs from being packed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 4, 2023, 16:48 UTC
- Message-ID
- <xmqq4joskpom.fsf@gitster.g>
- In-Reply-To
- <pull.1501.git.git.1683215331910.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <jcai@gitlab.com> > > Currently once refs are packed into a pack-refs file, deleting them can be > slow since it involves a full file scan of the entire pack-refs file. At
"pack-refs" -> "packed-refs".
But performing a full file scan of 100MB file would take how many milliseconds? Having to remove many refs from a single packed-refs file would be costly if it is done one-by-one, though. I wonder how effective our batched update mechanism is these days...
Sorry for straying to a tangent. In any case, I do not think the first sentence is necessary; just start it with "At GitLab, ..." to say that you have a need to be more selective than "is it a tag or not?" to choose refs to be and not to be packed. The reason why we may want to leave a ref loose is not limited to ref deletion performance, and the benefit of this new feature is not limited to it, either.
> GitLab, we have a system that creates ephemeral internal refs that don't > live long before getting deleted. Having an option to not include certain > refs from a pack-refs file allows these internal references to be deleted > much more efficiently.
I think that is a valid use case.
If we step back a bit, we can see "pack-refs" has an option "--all" that was added by b3d4204f (git-pack-refs --all, 2006-10-08), to allow us pack only tags by default, because packing branches that are meant to be updated often and also removed more often than tags were found to be detrimental. We can view this new option a follow-up work for the commit, to allow the users to define what to be and what not to be packed, depending on their workflow.
This observation also makes us realize that we should consider the opposite. It would benefit us to include some refs that we normally do not pack and be more selective than "--all" ("--exclude" proposed here is a way to leave some refs that we normally pack and be more selective than not running pack-refs at all). A set of branches that are only occasionally used may want to be packed while the rest of branches want to be left loose because they are more active, or something.
> Add an --exclude option to the pack-refs builtin, and use the ref > exclusions API to exclude certain refs from being packed into the final > pack-refs file
"pack-refs" appears here, too.
Show 34 quoted lines
> Documentation/git-pack-refs.txt | 8 +++++++- > builtin/pack-refs.c | 17 +++++++++++++++-- > refs.c | 4 ++-- > refs.h | 6 +++++- > refs/debug.c | 4 ++-- > refs/files-backend.c | 13 ++++++++++--- > refs/packed-backend.c | 3 ++- > refs/refs-internal.h | 4 +++- > t/helper/test-ref-store.c | 3 ++- > t/t3210-pack-refs.sh | 18 ++++++++++++++++++ > 10 files changed, 66 insertions(+), 14 deletions(-) > > diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt > index 154081f2de2..d80e0a1562d 100644 > --- a/Documentation/git-pack-refs.txt > +++ b/Documentation/git-pack-refs.txt > @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access > SYNOPSIS > -------- > [verse] > -'git pack-refs' [--all] [--no-prune] > +'git pack-refs' [--all] [--no-prune] [--exclude <pattern>] > > DESCRIPTION > ----------- > @@ -59,6 +59,12 @@ a repository with many branches of historical interests. > The command usually removes loose refs under `$GIT_DIR/refs` > hierarchy after packing them. This option tells it not to. > > +--exclude <pattern>:: > + > +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option > +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of > +patterns.
The interaction of this option with "--all" needs to be described somewhere. If we are to be adding "--include" for completeness, that one also needs to interact with "--all".
Show 13 quoted lines
> diff --git a/refs.c b/refs.c
> index d2a98e1c21f..637810347a0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,
> }
>
> /* backend functions */
> -int refs_pack_refs(struct ref_store *refs, unsigned int flags)
> +int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *pack_opts)
> {
> - return refs->be->pack_refs(refs, flags);
> + return refs->be->pack_refs(refs, flags, pack_opts);That's a curious choice of the API. It is not like backend methods all share the same "flags" bitset (they share "refs" pointer to the ref_store), so I would have expected that it would become part of the pack_refs_opts structure. Do not call the parameter pack_opts; either spell it out as "pack_refs_opts", or just use "opts". The latter is probably more preferrable as I do not expect it to be ambiguous with other kinds of "opts".
The rest of the pack I found nothing unexpected or surprising.