On 23/05/09 02:25PM, Junio C Hamano wrote:
> "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.
Hm yeah, I think that is a nicer user experience.
>
> > @@ -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.
Thanks for calling this out. I was thinking along very similar lines when
working on this patch, but was too lazy to make the change :)
>
> > };
> >
> > 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.
thanks
John
