On 23/05/04 09:48AM, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
Hi Junio,
> > From: John Cai <jcai@xxxxxxxxxx>
> >
> > 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.
Good point
>
> > 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.
Yeah, that's a good observation. It would be nice to add an --include option to
give the user full flexibility of which refs to include.
>
> > 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.
>
> > 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".
Sounds good
>
> > 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".
I didn't notice this, but it makes sense. We can move flags into the
pack_refs_opts struct.
>
> The rest of the pack I found nothing unexpected or surprising.
thanks
John
