Re: [PATCH v2 3/3] pack-refs: teach pack-refs --include option

> "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.