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 3/3] pack-refs: teach pack-refs --include option
Date: Tue, 09 May 2023 14:25:26 -0700 [thread overview]
Message-ID: <xmqqo7mturi1.fsf@gitster.g> (raw)
In-Reply-To: <03950e8f120e48b7df68f3273bbb2f7bb1e9073d.1683659931.git.gitgitgadget@gmail.com> (John Cai via GitGitGadget's message of "Tue, 09 May 2023 19:18:51 +0000")
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +--include <pattern>::
> +
> +Pack refs based on a `glob(7)` pattern. Repetitions of this option
> +accumulate inclusion patterns. If a ref is both included in `--include` and
> +`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude
> +all tags from being included by default. Symbolic refs and broken refs will never
> +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear
> +and reset the list of patterns.
Hmph, that was a bit unexpected. exclude taking precedence over
include is very much in line with how negative pathspec works and
the end-users should be familiar with it, but when the user bothers
to specify with --include what to include, I would have expected
that the "pack tags by default" would be defeated.
In other words, I would have expected that the program acts as if
the machinery works this way (iow, the code does not have to exactly
implement it this way---it just has to behave as if it did):
- it maintains two pattern list, positive and negative,
both start empty.
- "--exclude" are accumulated to the negative list.
- "--include" are accumulated to the positive list.
- "--all" adds "*" to the positive list.
- after parsing command line options, if the positive list is
empty, then "refs/tags/*" is added to the positive list.
- refs that match positive list but does not match negative list
are shown.
> +When used with `--include`, it will use what is provided to `--include` as well
> +as the the default of all tags and already packed refs, minus refs that are
> +provided to `--exclude`.
IOW, I would expect that the use of "--include" alone is enough to
defeat the default; the end user does not have to figure out that
they have to pass "--exclude=refs/tags/*" to do so when they are
specifying a specific hierarchy to include.
> @@ -66,6 +66,7 @@ struct worktree;
> struct pack_refs_opts {
> unsigned int flags;
> struct ref_exclusions *exclusions;
> + struct string_list *included_refs;
It is unfortunate that the struct is called ref_exclusions to imply
as if it is only usable for excluding refs from listing. It has
other members for handling hidden refs, and it would have been very
natural to extend it to also add included_refs pattern next to
excluded_refs string list. After all, the struct is used to tweak
which refs are included and which refs are excluded, and
historically everything was included unless listed on the excluded
pattern. We are now allowing the "everything is included" part to
be customizable with this step. If the struct were named with a
more neutral term, like ref_visibility to hint that it is about
setting visibility, then this patch wouldn't have added a separate
string list to this structure---instead it would have extended the
ref_exclusions (with a better name) struct and placed included_refs
string list there.
> };
>
> const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6a51267f379..3f8974a4a32 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1181,6 +1181,17 @@ static int should_pack_ref(const char *refname,
> REF_WORKTREE_SHARED)
> return 0;
>
> + if (opts->exclusions && ref_excluded(opts->exclusions, refname))
> + return 0;
> +
> + if (opts->included_refs && opts->included_refs->nr) {
> + struct string_list_item *item;
> +
> + for_each_string_list_item(item, opts->included_refs)
> + if (!wildmatch(item->string, refname, 0))
> + return 1;
> + }
We can see why the initial placement of exclusion logic in the
earlier step was suboptimal here.
> /* Do not pack non-tags unless PACK_REFS_ALL is set: */
> if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
> return 0;
> @@ -1193,9 +1204,6 @@ 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;
> -
> return 1;
> }
Other than that, the changes look mostly expected and no surprises.
Thanks.
next prev parent reply other threads:[~2023-05-09 21:34 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
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 [this message]
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=xmqqo7mturi1.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).