From: Junio C Hamano <gitster@pobox.com>
To: "John Cai via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>,
John Cai <johncai86@gmail.com>
Subject: Re: [PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed
Date: Tue, 09 May 2023 14:04:32 -0700 [thread overview]
Message-ID: <xmqq8rdxw71b.fsf@gitster.g> (raw)
In-Reply-To: <027b3f85a0b9c33c5334d6dad84e99c325ebec10.1683659931.git.gitgitgadget@gmail.com> (John Cai via GitGitGadget's message of "Tue, 09 May 2023 19:18:50 +0000")
"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.
> +--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.
> 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.
> 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.
> + 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 &&
next prev parent reply other threads:[~2023-05-09 21:04 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
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 [this message]
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=xmqq8rdxw71b.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johncai86@gmail.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).