"John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
