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

Commit Message

John Cai May 9, 2023, 7:18 p.m. UTC

From: John Cai <[e-mail]>

Allow users to be more selective over which refs to pack by adding an
--include option to git-pack-refs.

The existing options allow some measure of selectivity. By default
git-pack-refs packs all tags. --all can be used to include all refs,
and the previous commit added the ability to exclude certain refs with
--exclude.

While these knobs give the user some selection over which refs to pack,
it could be useful to give more control. For instance, a repository may
have a set of branches that are rarely updated and would benefit from
being packed. --include would allow the user to easily include a set of
branches to be packed while leaving everything else unpacked.

Signed-off-by: John Cai <[e-mail]>
---
 Documentation/git-pack-refs.txt | 15 ++++++++++++++-
 builtin/pack-refs.c             | 10 ++++++++--
 refs.h                          |  1 +
 refs/files-backend.c            | 14 +++++++++++---
 t/t3210-pack-refs.sh            | 10 ++++++++++
 5 files changed, 44 insertions(+), 6 deletions(-)

Comments

Junio C Hamano May 9, 2023, 9:25 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> 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.

John Cai May 10, 2023, 7:52 p.m. UTC | #2

On 23/05/09 02:25PM, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <[e-mail]> 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

Patch

diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt
index c0f7426e519..f187925bdc0 100644
--- a/Documentation/git-pack-refs.txt
+++ b/Documentation/git-pack-refs.txt
@@ -8,7 +8,7 @@  git-pack-refs - Pack heads and tags for efficient repository access
 SYNOPSIS
 --------
 [verse]
-'git pack-refs' [--all] [--no-prune] [--exclude <pattern>]
+'git pack-refs' [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,15 @@  interests.
 The command usually removes loose refs under `$GIT_DIR/refs`
 hierarchy after packing them.  This option tells it not to.
 
+--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.
+
 --exclude <pattern>::
 
 Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option
@@ -70,6 +79,10 @@  unpack it.
 When used with `--all`, it will use the difference between the set of all refs,
 and what is provided to `--exclude`.
 
+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`.
+
 
 BUGS
 ----
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 3dcbd6e2421..293cdd17181 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -7,7 +7,7 @@ 
 #include "revision.h"
 
 static char const * const pack_refs_usage[] = {
-	N_("git pack-refs [--all] [--no-prune] [--exclude <pattern>]"),
+	N_("git pack-refs [--all] [--no-prune] [--include <pattern>] [--exclude <pattern>]"),
 	NULL
 };
 
@@ -15,13 +15,19 @@  int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	unsigned int flags = PACK_REFS_PRUNE;
 	static struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
-	struct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};
+	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
+	struct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,
+						 .flags = flags,
+						 .included_refs = &included_refs };
+
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
 
 	struct option opts[] = {
 		OPT_BIT(0, "all",   &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL),
 		OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
+		OPT_STRING_LIST(0, "include", pack_refs_opts.included_refs, N_("pattern"),
+			N_("references to include")),
 		OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
 			N_("references to exclude")),
 		OPT_END(),
diff --git a/refs.h b/refs.h
index 46020bd335c..2f91f4c4a90 100644
--- a/refs.h
+++ b/refs.h
@@ -66,6 +66,7 @@  struct worktree;
 struct pack_refs_opts {
 	unsigned int flags;
 	struct ref_exclusions *exclusions;
+	struct string_list *included_refs;
 };
 
 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;
+	}
+
 	/* 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;
 }
 
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 31b9f72e84a..86b3ff6a3e0 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -126,6 +126,16 @@  test_expect_success \
      ! test -f .git/refs/heads/dont_pack3 &&
      ! test -f .git/refs/heads/dont_pack4'
 
+test_expect_success \
+    'test only included refs are packed' '
+     git branch pack_this1 &&
+     git branch pack_this2 &&
+     git tag dont_pack5 &&
+     git pack-refs --include refs/tags/pack_this* --exclude refs/tags/dont_pack* &&
+     test -f .git/refs/tags/dont_pack5 &&
+     ! test -f ./git/refs/heads/pack_this1 &&
+     ! test -f ./git/refs/heads/pack_this2'
+
 test_expect_success \
 	'see if up-to-date packed refs are preserved' \
 	'git branch q &&