From: John Cai <johncai86@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: John Cai via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, John Cai <jcai@gitlab.com>
Subject: Re: [PATCH] pack-refs: teach --exclude option to exclude refs from being packed
Date: Thu, 4 May 2023 17:26:47 -0400 [thread overview]
Message-ID: <20230504212647.hdozs6sxewry2hay@pop-os> (raw)
In-Reply-To: <xmqq4joskpom.fsf@gitster.g>
On 23/05/04 09:48AM, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
Hi Junio,
> > 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.
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
next prev parent reply other threads:[~2023-05-04 21:26 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-04 15:48 [PATCH] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-04 16:48 ` Junio C Hamano
2023-05-04 21:26 ` John Cai [this message]
2023-05-09 19:18 ` [PATCH v2 0/3] pack-refs: Teach " John Cai via GitGitGadget
2023-05-09 19:18 ` [PATCH v2 1/3] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-09 19:18 ` [PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-09 21:04 ` Junio C Hamano
2023-05-09 19:18 ` [PATCH v2 3/3] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
2023-05-09 21:25 ` Junio C Hamano
2023-05-10 19:52 ` John Cai
2023-05-11 18:10 ` [PATCH v3 0/4] pack-refs: allow users control over which refs to pack John Cai via GitGitGadget
2023-05-11 18:10 ` [PATCH v3 1/4] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-11 23:53 ` Taylor Blau
2023-05-11 18:10 ` [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-11 19:34 ` Junio C Hamano
2023-05-12 0:00 ` Taylor Blau
2023-05-12 12:53 ` John Cai
2023-05-12 21:11 ` John Cai
2023-05-11 18:10 ` [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions John Cai via GitGitGadget
2023-05-11 19:54 ` Junio C Hamano
2023-05-12 14:56 ` John Cai
2023-05-11 18:10 ` [PATCH v3 4/4] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
2023-05-11 20:06 ` Junio C Hamano
2023-05-12 14:48 ` John Cai
2023-05-12 19:03 ` John Cai
2023-05-12 21:34 ` [PATCH v4 0/3] pack-refs: allow users control over which refs to pack John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 1/3] docs: clarify git-pack-refs --all will pack all refs John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 2/3] pack-refs: teach --exclude option to exclude refs from being packed John Cai via GitGitGadget
2023-05-12 21:34 ` [PATCH v4 3/3] pack-refs: teach pack-refs --include option John Cai via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230504212647.hdozs6sxewry2hay@pop-os \
--to=johncai86@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jcai@gitlab.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).