[v2,2/3] pack-refs: teach --exclude option to exclude refs from being packed

Commit Message

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

From: John Cai <[e-mail]>

At GitLab, we have a system that creates ephemeral internal refs that
don't live long before getting deleted. Having an option to not include
certain refs from a packed-refs file allows these internal references to
be deleted much more efficiently.

Add an --exclude option to the pack-refs builtin, and use the ref
exclusions API to exclude certain refs from being packed into the final
packed-refs file

Signed-off-by: John Cai <[e-mail]>
---
 Documentation/git-pack-refs.txt | 12 +++++++++++-
 builtin/pack-refs.c             | 21 +++++++++++++++++----
 refs.c                          |  4 ++--
 refs.h                          |  7 ++++++-
 refs/debug.c                    |  4 ++--
 refs/files-backend.c            | 16 ++++++++++------
 refs/packed-backend.c           |  2 +-
 refs/refs-internal.h            |  3 ++-
 t/helper/test-ref-store.c       |  3 ++-
 t/t3210-pack-refs.sh            | 18 ++++++++++++++++++
 10 files changed, 71 insertions(+), 19 deletions(-)

Comments

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

"John Cai via GitGitGadget" <[e-mail]> writes:

> From: John Cai <[e-mail]>
>
> At GitLab, we have a system that creates ephemeral internal refs that
> don't live long before getting deleted. Having an option to not include

"to not include" -> "to exclude", perhaps.


> +--exclude <pattern>::
> +
> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option
> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of
> +patterns. If a ref is already packed, including it with `--exclude` will not
> +unpack it.
> +
> +When used with `--all`, it will use the difference between the set of all refs,
> +and what is provided to `--exclude`.
> +

Clearly written.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d0581ee41ac..6a51267f379 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -19,6 +19,7 @@
>  #include "../worktree.h"
>  #include "../wrapper.h"
>  #include "../write-or-die.h"
> +#include "../revision.h"
>  
>  /*
>   * This backend uses the following flags in `ref_update::flags` for
> @@ -1173,7 +1174,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_
>   */
>  static int should_pack_ref(const char *refname,
>  			   const struct object_id *oid, unsigned int ref_flags,
> -			   unsigned int pack_flags)
> +			   struct pack_refs_opts *opts)
>  {
>  	/* Do not pack per-worktree refs: */
>  	if (parse_worktree_ref(refname, NULL, NULL, NULL) !=
> @@ -1181,7 +1182,7 @@ static int should_pack_ref(const char *refname,
>  		return 0;
>  
>  	/* Do not pack non-tags unless PACK_REFS_ALL is set: */
> -	if (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
> +	if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
>  		return 0;
>  
>  	/* Do not pack symbolic refs: */
> @@ -1192,10 +1193,14 @@ 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;

It is _not_ _wrong_ per-se, but I would have expected for this to go
between "what do we include" and "symrefs and broken ones are not
packed", given that we will tweak the "what to include" logic in the
next step.

Other changes to the code in this patch are all naturally expected
to pass the new information through the callchain and looked good.

> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
> index 07a0ff93def..31b9f72e84a 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t3210-pack-refs.sh
> @@ -108,6 +108,24 @@ test_expect_success \
>       git branch -d n/o/p &&
>       git branch n'
>
> +test_expect_success \
> +    'test excluded refs are not packed' '
> +     git branch dont_pack1 &&
> +     git branch dont_pack2 &&
> +     git branch pack_this &&
> +     git pack-refs --all --exclude refs/heads/dont_pack* &&

Let's quote the value given to "--exclude" to make it clear that we
do not expect glob to affect the shell, i.e.

	git pack-refs --all --exclude "refs/heads/dont_pack*" &&

Some (early and ancient) parts, but not all parts, of this file use
4-space indentation and have the title of the test on a line that is
separate from test_expect_success, which is an ancient deprecated
style.

You do not want to imitate them in new tests you add.  After the
dust from this topic settles, somebody should go in and clean the
style of this file.  Let's not create more work for them.

Thanks.

> +     test -f .git/refs/heads/dont_pack1 &&
> +     test -f .git/refs/heads/dont_pack2 &&
> +     ! test -f ./git/refs/heads/pack_this'
> +
> +test_expect_success \
> +    'test --no-exclude refs clears excluded refs' '
> +     git branch dont_pack3 &&
> +     git branch dont_pack4 &&
> +     git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&
> +     ! test -f .git/refs/heads/dont_pack3 &&
> +     ! test -f .git/refs/heads/dont_pack4'
> +
>  test_expect_success \
>  	'see if up-to-date packed refs are preserved' \
>  	'git branch q &&

Patch

diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt
index e011e5fead3..c0f7426e519 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]
+'git pack-refs' [--all] [--no-prune] [--exclude <pattern>]
 
 DESCRIPTION
 -----------
@@ -60,6 +60,16 @@  interests.
 The command usually removes loose refs under `$GIT_DIR/refs`
 hierarchy after packing them.  This option tells it not to.
 
+--exclude <pattern>::
+
+Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option
+accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of
+patterns. If a ref is already packed, including it with `--exclude` will not
+unpack it.
+
+When used with `--all`, it will use the difference between the set of all refs,
+and what is provided to `--exclude`.
+
 
 BUGS
 ----
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 9833815fb30..3dcbd6e2421 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -4,22 +4,35 @@ 
 #include "parse-options.h"
 #include "refs.h"
 #include "repository.h"
+#include "revision.h"
 
 static char const * const pack_refs_usage[] = {
-	N_("git pack-refs [--all] [--no-prune]"),
+	N_("git pack-refs [--all] [--no-prune] [--exclude <pattern>]"),
 	NULL
 };
 
 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 option_excluded_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
+
 	struct option opts[] = {
-		OPT_BIT(0, "all",   &flags, N_("pack everything"), PACK_REFS_ALL),
-		OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
+		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, "exclude", &option_excluded_refs, N_("pattern"),
+			N_("references to exclude")),
 		OPT_END(),
 	};
+
 	git_config(git_default_config, NULL);
 	if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
 		usage_with_options(pack_refs_usage, opts);
-	return refs_pack_refs(get_main_ref_store(the_repository), flags);
+
+	for_each_string_list_item(item, &option_excluded_refs)
+		add_ref_exclusion(pack_refs_opts.exclusions, item->string);
+
+	return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
 }
diff --git a/refs.c b/refs.c
index d2a98e1c21f..881a0da65cf 100644
--- a/refs.c
+++ b/refs.c
@@ -2132,9 +2132,9 @@  void base_ref_store_init(struct ref_store *refs, struct repository *repo,
 }
 
 /* backend functions */
-int refs_pack_refs(struct ref_store *refs, unsigned int flags)
+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)
 {
-	return refs->be->pack_refs(refs, flags);
+	return refs->be->pack_refs(refs, opts);
 }
 
 int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)
diff --git a/refs.h b/refs.h
index 123cfa44244..46020bd335c 100644
--- a/refs.h
+++ b/refs.h
@@ -63,6 +63,11 @@  struct worktree;
 #define RESOLVE_REF_NO_RECURSE 0x02
 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
 
+struct pack_refs_opts {
+	unsigned int flags;
+	struct ref_exclusions *exclusions;
+};
+
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 				    const char *refname,
 				    int resolve_flags,
@@ -405,7 +410,7 @@  void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
  * Write a packed-refs file for the current repository.
  * flags: Combination of the above PACK_REFS_* flags.
  */
-int refs_pack_refs(struct ref_store *refs, unsigned int flags);
+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);
 
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
diff --git a/refs/debug.c b/refs/debug.c
index adc34c836fc..63f434ea26b 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -122,10 +122,10 @@  static int debug_initial_transaction_commit(struct ref_store *refs,
 	return res;
 }
 
-static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)
+static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
-	int res = drefs->refs->be->pack_refs(drefs->refs, flags);
+	int res = drefs->refs->be->pack_refs(drefs->refs, opts);
 	trace_printf_key(&trace_refs, "pack_refs: %d\n", res);
 	return res;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d0581ee41ac..6a51267f379 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -19,6 +19,7 @@ 
 #include "../worktree.h"
 #include "../wrapper.h"
 #include "../write-or-die.h"
+#include "../revision.h"
 
 /*
  * This backend uses the following flags in `ref_update::flags` for
@@ -1173,7 +1174,7 @@  static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_
  */
 static int should_pack_ref(const char *refname,
 			   const struct object_id *oid, unsigned int ref_flags,
-			   unsigned int pack_flags)
+			   struct pack_refs_opts *opts)
 {
 	/* Do not pack per-worktree refs: */
 	if (parse_worktree_ref(refname, NULL, NULL, NULL) !=
@@ -1181,7 +1182,7 @@  static int should_pack_ref(const char *refname,
 		return 0;
 
 	/* Do not pack non-tags unless PACK_REFS_ALL is set: */
-	if (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
+	if (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, "refs/tags/"))
 		return 0;
 
 	/* Do not pack symbolic refs: */
@@ -1192,10 +1193,14 @@  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;
 }
 
-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
+static int files_pack_refs(struct ref_store *ref_store,
+			   struct pack_refs_opts *opts)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
@@ -1220,8 +1225,7 @@  static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 		 * in the packed ref cache. If the reference should be
 		 * pruned, also add it to refs_to_prune.
 		 */
-		if (!should_pack_ref(iter->refname, iter->oid, iter->flags,
-				     flags))
+		if (!should_pack_ref(iter->refname, iter->oid, iter->flags, opts))
 			continue;
 
 		/*
@@ -1235,7 +1239,7 @@  static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 			    iter->refname, err.buf);
 
 		/* Schedule the loose reference for pruning if requested. */
-		if ((flags & PACK_REFS_PRUNE)) {
+		if ((opts->flags & PACK_REFS_PRUNE)) {
 			struct ref_to_prune *n;
 			FLEX_ALLOC_STR(n, name, iter->refname);
 			oidcpy(&n->oid, iter->oid);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 2333ed5a1f7..9a2dd10dba7 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1576,7 +1576,7 @@  static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
 }
 
 static int packed_pack_refs(struct ref_store *ref_store UNUSED,
-			    unsigned int flags UNUSED)
+			    struct pack_refs_opts *pack_opts UNUSED)
 {
 	/*
 	 * Packed refs are already packed. It might be that loose refs
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index a85d113123c..f72b7be8941 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -547,7 +547,8 @@  typedef int ref_transaction_commit_fn(struct ref_store *refs,
 				      struct ref_transaction *transaction,
 				      struct strbuf *err);
 
-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);
+typedef int pack_refs_fn(struct ref_store *ref_store,
+			 struct pack_refs_opts *opts);
 typedef int create_symref_fn(struct ref_store *ref_store,
 			     const char *ref_target,
 			     const char *refs_heads_master,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 6d8f844e9c7..de4197708d9 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -116,8 +116,9 @@  static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),
 static int cmd_pack_refs(struct ref_store *refs, const char **argv)
 {
 	unsigned int flags = arg_flags(*argv++, "flags", pack_flags);
+	struct pack_refs_opts pack_opts = { .flags = flags };
 
-	return refs_pack_refs(refs, flags);
+	return refs_pack_refs(refs, &pack_opts);
 }
 
 static int cmd_create_symref(struct ref_store *refs, const char **argv)
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 07a0ff93def..31b9f72e84a 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -108,6 +108,24 @@  test_expect_success \
      git branch -d n/o/p &&
      git branch n'
 
+test_expect_success \
+    'test excluded refs are not packed' '
+     git branch dont_pack1 &&
+     git branch dont_pack2 &&
+     git branch pack_this &&
+     git pack-refs --all --exclude refs/heads/dont_pack* &&
+     test -f .git/refs/heads/dont_pack1 &&
+     test -f .git/refs/heads/dont_pack2 &&
+     ! test -f ./git/refs/heads/pack_this'
+
+test_expect_success \
+    'test --no-exclude refs clears excluded refs' '
+     git branch dont_pack3 &&
+     git branch dont_pack4 &&
+     git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&
+     ! test -f .git/refs/heads/dont_pack3 &&
+     ! test -f .git/refs/heads/dont_pack4'
+
 test_expect_success \
 	'see if up-to-date packed refs are preserved' \
 	'git branch q &&