Re: [PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 9, 2023, 21:04 UTC
- Message-ID
- <xmqq8rdxw71b.fsf@gitster.g>
- In-Reply-To
- <027b3f85a0b9c33c5334d6dad84e99c325ebec10.1683659931.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com> > > At GitLab, we have a system that creates ephemeral internal refs that > don't live long before getting deleted. Having an option to not include
"to not include" -> "to exclude", perhaps.
Show 10 quoted lines
> +--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. If a ref is already packed, including it with `--exclude` will not > +unpack it. > + > +When used with `--all`, it will use the difference between the set of all refs, > +and what is provided to `--exclude`. > +
Clearly written.
Show 36 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d0581ee41ac..6a51267f379 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -19,6 +19,7 @@
> #include "../worktree.h"
> #include "../wrapper.h"
> #include "../write-or-die.h"
> +#include "../revision.h"
>
> /*
> * This backend uses the following flags in `ref_update::flags` for
> @@ -1173,7 +1174,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_
> */
> static int should_pack_ref(const char *refname,
> const struct object_id *oid, unsigned int ref_flags,
> - unsigned int pack_flags)
> + struct pack_refs_opts *opts)
> {
> /* Do not pack per-worktree refs: */
> if (parse_worktree_ref(refname, NULL, NULL, NULL) !=
> @@ -1181,7 +1182,7 @@ static int should_pack_ref(const char *refname,
> return 0;
>
> /* Do not pack non-tags unless PACK_REFS_ALL is set: */
> - if (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
> + if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
> return 0;
>
> /* Do not pack symbolic refs: */
> @@ -1192,10 +1193,14 @@ static int should_pack_ref(const char *refname,
> if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
> return 0;
>
> + if (opts->exclusions && ref_excluded(opts->exclusions, refname))
> + return 0;It is _not_ _wrong_ per-se, but I would have expected for this to go between "what do we include" and "symrefs and broken ones are not packed", given that we will tweak the "what to include" logic in the next step.
Other changes to the code in this patch are all naturally expected to pass the new information through the callchain and looked good.
Show 14 quoted lines
> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh > index 07a0ff93def..31b9f72e84a 100755 > --- a/t/t3210-pack-refs.sh > +++ b/t/t3210-pack-refs.sh > @@ -108,6 +108,24 @@ test_expect_success \ > git branch -d n/o/p && > git branch n' > > +test_expect_success \ > + 'test excluded refs are not packed' ' > + git branch dont_pack1 && > + git branch dont_pack2 && > + git branch pack_this && > + git pack-refs --all --exclude refs/heads/dont_pack* &&
Let's quote the value given to "--exclude" to make it clear that we do not expect glob to affect the shell, i.e.
git pack-refs --all --exclude "refs/heads/dont_pack*" &&
Some (early and ancient) parts, but not all parts, of this file use 4-space indentation and have the title of the test on a line that is separate from test_expect_success, which is an ancient deprecated style.
You do not want to imitate them in new tests you add. After the dust from this topic settles, somebody should go in and clean the style of this file. Let's not create more work for them.
Thanks.
Show 15 quoted lines
> + test -f .git/refs/heads/dont_pack1 && > + test -f .git/refs/heads/dont_pack2 && > + ! test -f ./git/refs/heads/pack_this' > + > +test_expect_success \ > + 'test --no-exclude refs clears excluded refs' ' > + git branch dont_pack3 && > + git branch dont_pack4 && > + git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude && > + ! test -f .git/refs/heads/dont_pack3 && > + ! test -f .git/refs/heads/dont_pack4' > + > test_expect_success \ > 'see if up-to-date packed refs are preserved' \ > 'git branch q &&