Re: [PATCH v2 3/3] pack-refs: teach pack-refs --include option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 9, 2023, 21:25 UTC
- Message-ID
- <xmqqo7mturi1.fsf@gitster.g>
- In-Reply-To
- <03950e8f120e48b7df68f3273bbb2f7bb1e9073d.1683659931.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> +--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.
Show 5 quoted lines
> @@ -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.
Show 21 quoted lines
> };
>
> 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.
Show 12 quoted lines
> /* 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.