{"thread":{"id":"59694","subject":"[PATCH] pack-refs: teach --exclude option to exclude refs from being packed","startedAt":"2023-05-04T15:49:00Z","lastAt":"2023-05-12T21:35:06Z","messageCount":29,"participants":["John Cai via GitGitGadget","Junio C Hamano","John Cai","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"476597","messageId":"pull.1501.git.git.1683215331910.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":null,"subject":"[PATCH] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-04T15:48:51Z","receivedAt":"2023-05-04T15:49:00Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nCurrently once refs are packed into a pack-refs file, deleting them can be\nslow since it involves a full file scan of the entire pack-refs file. At\nGitLab, we have a system that creates ephemeral internal refs that don't\nlive long before getting deleted. Having an option to not include certain\nrefs from a pack-refs file allows these internal references to be deleted\nmuch more efficiently.\n\nAdd an --exclude option to the pack-refs builtin, and use the ref\nexclusions API to exclude certain refs from being packed into the final\npack-refs file\n\nSigned-off-by: John Cai \n---\n pack-refs: Teach --exclude option to exclude refs from being packed\n \n Add an --exclude option to the pack-refs builtin, to prevent certain\n refs from being packed into the final packrefs file. At GitLab we keep\n ephemeral internal references that quickly get deleted. Being able to\n exclude them from the packrefs file will avoid incurring the performance\n hit from ref deletions--especially since it scales with packrefs size.\n \n It's worth noting that [1] discussed a proposal for a pack refs v2\n format that would improve deletion speeds. The ref-table backend would\n also improve performance of deletions. However, both of those proposals\n are still being discussed.\n \n 1. https://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1501%2Fjohn-cai%2Fjc%2Fexclude-refs-from-pack-refs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1501/john-cai/jc/exclude-refs-from-pack-refs-v1\nPull-Request: https://github.com/git/git/pull/1501\n\n Documentation/git-pack-refs.txt | 8 +++++++-\n builtin/pack-refs.c | 17 +++++++++++++++--\n refs.c | 4 ++--\n refs.h | 6 +++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 13 ++++++++++---\n refs/packed-backend.c | 3 ++-\n refs/refs-internal.h | 4 +++-\n t/helper/test-ref-store.c | 3 ++-\n t/t3210-pack-refs.sh | 18 ++++++++++++++++++\n 10 files changed, 66 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 154081f2de2..d80e0a1562d 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune]\n+'git pack-refs' [--all] [--no-prune] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -59,6 +59,12 @@ a repository with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--exclude ::\n+\n+Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n+accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n+patterns.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 9833815fb30..bc2db41c2cb 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -4,22 +4,35 @@\n #include \"parse-options.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n \tNULL\n };\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n+\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct pack_refs_opts pack_opts = {.exclusions = &excludes};\n+\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n \t};\n+\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n+\n+\tfor_each_string_list_item(item, &option_excluded_refs)\n+\t\tadd_ref_exclusion(pack_opts.exclusions, item->string);\n+\n+\treturn refs_pack_refs(get_main_ref_store(the_repository), flags, &pack_opts);\n }\ndiff --git a/refs.c b/refs.c\nindex d2a98e1c21f..637810347a0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n }\n \n /* backend functions */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n+int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *pack_opts)\n {\n-\treturn refs->be->pack_refs(refs, flags);\n+\treturn refs->be->pack_refs(refs, flags, pack_opts);\n }\n \n int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\ndiff --git a/refs.h b/refs.h\nindex 123cfa44244..a76dbb2d3f0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -63,6 +63,10 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+struct pack_refs_opts {\n+\tstruct ref_exclusions *exclusions;\n+};\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -405,7 +409,7 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n+int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *opts);\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/debug.c b/refs/debug.c\nindex adc34c836fc..93f6168052a 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -122,10 +122,10 @@ static int debug_initial_transaction_commit(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->pack_refs(drefs->refs, flags);\n+\tint res = drefs->refs->be->pack_refs(drefs->refs, flags, opts);\n \ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d0581ee41ac..b77c474919f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -19,6 +19,7 @@\n #include \"../worktree.h\"\n #include \"../wrapper.h\"\n #include \"../write-or-die.h\"\n+#include \"../revision.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -1173,7 +1174,8 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n */\n static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n-\t\t\t unsigned int pack_flags)\n+\t\t\t unsigned int pack_flags,\n+\t\t\t struct pack_refs_opts *opts)\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n@@ -1192,10 +1194,15 @@ static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int files_pack_refs(struct ref_store *ref_store,\n+\t\t\t unsigned int flags,\n+\t\t\t struct pack_refs_opts *pack_opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1221,7 +1228,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n \t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n-\t\t\t\t flags))\n+\t\t\t\t flags, pack_opts))\n \t\t\tcontinue;\n \n \t\t/*\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 2333ed5a1f7..8a611dd92b4 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1576,7 +1576,8 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n }\n \n static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n-\t\t\t unsigned int flags UNUSED)\n+\t\t\t unsigned int flags UNUSED,\n+\t\t\t struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex a85d113123c..7b4852104f0 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -547,7 +547,9 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n+typedef int pack_refs_fn(struct ref_store *ref_store,\n+\t\t\t unsigned int flags,\n+\t\t\t struct pack_refs_opts *opts);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 6d8f844e9c7..0405cacc99b 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -116,8 +116,9 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n+\tstruct pack_refs_opts pack_opts = { 0 };\n \n-\treturn refs_pack_refs(refs, flags);\n+\treturn refs_pack_refs(refs, flags, &pack_opts);\n }\n \n static int cmd_create_symref(struct ref_store *refs, const char **argv)\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 07a0ff93def..31b9f72e84a 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -108,6 +108,24 @@ test_expect_success \\\n git branch -d n/o/p &&\n git branch n'\n \n+test_expect_success \\\n+ 'test excluded refs are not packed' '\n+ git branch dont_pack1 &&\n+ git branch dont_pack2 &&\n+ git branch pack_this &&\n+ git pack-refs --all --exclude refs/heads/dont_pack* &&\n+ test -f .git/refs/heads/dont_pack1 &&\n+ test -f .git/refs/heads/dont_pack2 &&\n+ ! test -f ./git/refs/heads/pack_this'\n+\n+test_expect_success \\\n+ 'test --no-exclude refs clears excluded refs' '\n+ git branch dont_pack3 &&\n+ git branch dont_pack4 &&\n+ git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&\n+ ! test -f .git/refs/heads/dont_pack3 &&\n+ ! test -f .git/refs/heads/dont_pack4'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n\nbase-commit: f85cd430b12b0d3e4f1a30ef3239a1b73d5f6331\n-- \ngitgitgadget\n"},{"id":"476602","messageId":"xmqq4joskpom.fsf@gitster.g","threadId":"59694","inReplyTo":"pull.1501.git.git.1683215331910.gitgitgadget@gmail.com","subject":"Re: [PATCH] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-04T16:48:57Z","receivedAt":"2023-05-04T16:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Currently once refs are packed into a pack-refs file, deleting them can be\n> slow since it involves a full file scan of the entire pack-refs file. At\n\n\"pack-refs\" -> \"packed-refs\".\n\nBut performing a full file scan of 100MB file would take how many\nmilliseconds? Having to remove many refs from a single packed-refs\nfile would be costly if it is done one-by-one, though. I wonder how\neffective our batched update mechanism is these days...\n\nSorry for straying to a tangent. In any case, I do not think the\nfirst sentence is necessary; just start it with \"At GitLab, ...\" to\nsay that you have a need to be more selective than \"is it a tag or\nnot?\" to choose refs to be and not to be packed. The reason why we\nmay want to leave a ref loose is not limited to ref deletion\nperformance, and the benefit of this new feature is not limited to\nit, either.\n\n> GitLab, we have a system that creates ephemeral internal refs that don't\n> live long before getting deleted. Having an option to not include certain\n> refs from a pack-refs file allows these internal references to be deleted\n> much more efficiently.\n\nI think that is a valid use case.\n\nIf we step back a bit, we can see \"pack-refs\" has an option \"--all\"\nthat was added by b3d4204f (git-pack-refs --all, 2006-10-08), to\nallow us pack only tags by default, because packing branches that\nare meant to be updated often and also removed more often than tags\nwere found to be detrimental. We can view this new option a\nfollow-up work for the commit, to allow the users to define what to\nbe and what not to be packed, depending on their workflow.\n\nThis observation also makes us realize that we should consider the\nopposite. It would benefit us to include some refs that we normally\ndo not pack and be more selective than \"--all\" (\"--exclude\" proposed\nhere is a way to leave some refs that we normally pack and be more\nselective than not running pack-refs at all). A set of branches\nthat are only occasionally used may want to be packed while the rest\nof branches want to be left loose because they are more active, or\nsomething.\n\n> Add an --exclude option to the pack-refs builtin, and use the ref\n> exclusions API to exclude certain refs from being packed into the final\n> pack-refs file\n\n\"pack-refs\" appears here, too.\n\n> Documentation/git-pack-refs.txt | 8 +++++++-\n> builtin/pack-refs.c | 17 +++++++++++++++--\n> refs.c | 4 ++--\n> refs.h | 6 +++++-\n> refs/debug.c | 4 ++--\n> refs/files-backend.c | 13 ++++++++++---\n> refs/packed-backend.c | 3 ++-\n> refs/refs-internal.h | 4 +++-\n> t/helper/test-ref-store.c | 3 ++-\n> t/t3210-pack-refs.sh | 18 ++++++++++++++++++\n> 10 files changed, 66 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> index 154081f2de2..d80e0a1562d 100644\n> --- a/Documentation/git-pack-refs.txt\n> +++ b/Documentation/git-pack-refs.txt\n> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n> SYNOPSIS\n> --------\n> [verse]\n> -'git pack-refs' [--all] [--no-prune]\n> +'git pack-refs' [--all] [--no-prune] [--exclude ]\n> \n> DESCRIPTION\n> -----------\n> @@ -59,6 +59,12 @@ a repository with many branches of historical interests.\n> The command usually removes loose refs under `$GIT_DIR/refs`\n> hierarchy after packing them. This option tells it not to.\n> \n> +--exclude ::\n> +\n> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n> +patterns.\n\nThe interaction of this option with \"--all\" needs to be described\nsomewhere. If we are to be adding \"--include\" for completeness,\nthat one also needs to interact with \"--all\".\n\n> diff --git a/refs.c b/refs.c\n> index d2a98e1c21f..637810347a0 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n> }\n> \n> /* backend functions */\n> -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n> +int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *pack_opts)\n> {\n> -\treturn refs->be->pack_refs(refs, flags);\n> +\treturn refs->be->pack_refs(refs, flags, pack_opts);\n\nThat's a curious choice of the API. It is not like backend methods\nall share the same \"flags\" bitset (they share \"refs\" pointer to the\nref_store), so I would have expected that it would become part of\nthe pack_refs_opts structure. Do not call the parameter pack_opts;\neither spell it out as \"pack_refs_opts\", or just use \"opts\". The\nlatter is probably more preferrable as I do not expect it to be\nambiguous with other kinds of \"opts\".\n\nThe rest of the pack I found nothing unexpected or surprising.\n"},{"id":"476608","messageId":"20230504212647.hdozs6sxewry2hay@pop-os","threadId":"59694","inReplyTo":"xmqq4joskpom.fsf@gitster.g","subject":"Re: [PATCH] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-04T21:26:47Z","receivedAt":"2023-05-04T21:26:53Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"On 23/05/04 09:48AM, Junio C Hamano wrote:\n> \"John Cai via GitGitGadget\" writes:\n>\n\nHi Junio,\n\n> > From: John Cai \n> >\n> > Currently once refs are packed into a pack-refs file, deleting them can be\n> > slow since it involves a full file scan of the entire pack-refs file. At\n> \n> \"pack-refs\" -> \"packed-refs\".\n> \n> But performing a full file scan of 100MB file would take how many\n> milliseconds? Having to remove many refs from a single packed-refs\n> file would be costly if it is done one-by-one, though. I wonder how\n> effective our batched update mechanism is these days...\n> \n> Sorry for straying to a tangent. In any case, I do not think the\n> first sentence is necessary; just start it with \"At GitLab, ...\" to\n> say that you have a need to be more selective than \"is it a tag or\n> not?\" to choose refs to be and not to be packed. The reason why we\n> may want to leave a ref loose is not limited to ref deletion\n> performance, and the benefit of this new feature is not limited to\n> it, either.\n\nGood point\n\n> \n> > GitLab, we have a system that creates ephemeral internal refs that don't\n> > live long before getting deleted. Having an option to not include certain\n> > refs from a pack-refs file allows these internal references to be deleted\n> > much more efficiently.\n> \n> I think that is a valid use case.\n> \n> If we step back a bit, we can see \"pack-refs\" has an option \"--all\"\n> that was added by b3d4204f (git-pack-refs --all, 2006-10-08), to\n> allow us pack only tags by default, because packing branches that\n> are meant to be updated often and also removed more often than tags\n> were found to be detrimental. We can view this new option a\n> follow-up work for the commit, to allow the users to define what to\n> be and what not to be packed, depending on their workflow.\n> \n> This observation also makes us realize that we should consider the\n> opposite. It would benefit us to include some refs that we normally\n> do not pack and be more selective than \"--all\" (\"--exclude\" proposed\n> here is a way to leave some refs that we normally pack and be more\n> selective than not running pack-refs at all). A set of branches\n> that are only occasionally used may want to be packed while the rest\n> of branches want to be left loose because they are more active, or\n> something.\n\nYeah, that's a good observation. It would be nice to add an --include option to\ngive the user full flexibility of which refs to include.\n\n> \n> > Add an --exclude option to the pack-refs builtin, and use the ref\n> > exclusions API to exclude certain refs from being packed into the final\n> > pack-refs file\n> \n> \"pack-refs\" appears here, too.\n> \n> > Documentation/git-pack-refs.txt | 8 +++++++-\n> > builtin/pack-refs.c | 17 +++++++++++++++--\n> > refs.c | 4 ++--\n> > refs.h | 6 +++++-\n> > refs/debug.c | 4 ++--\n> > refs/files-backend.c | 13 ++++++++++---\n> > refs/packed-backend.c | 3 ++-\n> > refs/refs-internal.h | 4 +++-\n> > t/helper/test-ref-store.c | 3 ++-\n> > t/t3210-pack-refs.sh | 18 ++++++++++++++++++\n> > 10 files changed, 66 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> > index 154081f2de2..d80e0a1562d 100644\n> > --- a/Documentation/git-pack-refs.txt\n> > +++ b/Documentation/git-pack-refs.txt\n> > @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n> > SYNOPSIS\n> > --------\n> > [verse]\n> > -'git pack-refs' [--all] [--no-prune]\n> > +'git pack-refs' [--all] [--no-prune] [--exclude ]\n> > \n> > DESCRIPTION\n> > -----------\n> > @@ -59,6 +59,12 @@ a repository with many branches of historical interests.\n> > The command usually removes loose refs under `$GIT_DIR/refs`\n> > hierarchy after packing them. This option tells it not to.\n> > \n> > +--exclude ::\n> > +\n> > +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> > +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n> > +patterns.\n> \n> The interaction of this option with \"--all\" needs to be described\n> somewhere. If we are to be adding \"--include\" for completeness,\n> that one also needs to interact with \"--all\".\n\nSounds good\n\n> \n> > diff --git a/refs.c b/refs.c\n> > index d2a98e1c21f..637810347a0 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n> > }\n> > \n> > /* backend functions */\n> > -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n> > +int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *pack_opts)\n> > {\n> > -\treturn refs->be->pack_refs(refs, flags);\n> > +\treturn refs->be->pack_refs(refs, flags, pack_opts);\n> \n> That's a curious choice of the API. It is not like backend methods\n> all share the same \"flags\" bitset (they share \"refs\" pointer to the\n> ref_store), so I would have expected that it would become part of\n> the pack_refs_opts structure. Do not call the parameter pack_opts;\n> either spell it out as \"pack_refs_opts\", or just use \"opts\". The\n> latter is probably more preferrable as I do not expect it to be\n> ambiguous with other kinds of \"opts\".\n\nI didn't notice this, but it makes sense. We can move flags into the\npack_refs_opts struct.\n\n> \n> The rest of the pack I found nothing unexpected or surprising.\n\nthanks\nJohn\n"},{"id":"476911","messageId":"pull.1501.v2.git.git.1683659931.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.git.git.1683215331910.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] pack-refs: Teach --exclude option to exclude refs from being packed","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-09T19:18:48Z","receivedAt":"2023-05-09T19:19:01Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-pack-refs does not currently give much control over which refs are\npacked. By default, all tags and already packed refs are packed. --all\nallows the user to pack all refs. Beyond this the user does not have\ncontrol. Introduce a pair of options --include and --exclude that will allow\nusers full control over which refs end up in the packed-refs file.\n\nChanges since v1:\n\n * Clarify that --all packs not just branch tips but all refs under refs/ in\n the documentation in patch 1\n * Add --include in patch 3\n\nIt's worth noting that [1] discussed a proposal for a pack refs v2 format\nthat would improve deletion speeds. The ref-table backend would also improve\nperformance of deletions. However, both of those proposals are still being\ndiscussed.\n\n 1. https://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\nJohn Cai (3):\n docs: clarify git-pack-refs --all will pack all refs\n pack-refs: teach --exclude option to exclude refs from being packed\n pack-refs: teach pack-refs --include option\n\n Documentation/git-pack-refs.txt | 32 ++++++++++++++++++++++++++++----\n builtin/pack-refs.c | 27 +++++++++++++++++++++++----\n refs.c | 4 ++--\n refs.h | 8 +++++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 24 ++++++++++++++++++------\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 ++-\n t/helper/test-ref-store.c | 3 ++-\n t/t3210-pack-refs.sh | 28 ++++++++++++++++++++++++++++\n 10 files changed, 113 insertions(+), 22 deletions(-)\n\n\nbase-commit: f85cd430b12b0d3e4f1a30ef3239a1b73d5f6331\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1501%2Fjohn-cai%2Fjc%2Fexclude-refs-from-pack-refs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1501/john-cai/jc/exclude-refs-from-pack-refs-v2\nPull-Request: https://github.com/git/git/pull/1501\n\nRange-diff vs v1:\n\n -: ----------- > 1: 0b40b24b95d docs: clarify git-pack-refs --all will pack all refs\n 1: fb4d5b911ae ! 2: 027b3f85a0b pack-refs: teach --exclude option to exclude refs from being packed\n @@\n ## Metadata ##\n -Author: John Cai \n +Author: John Cai \n \n ## Commit message ##\n pack-refs: teach --exclude option to exclude refs from being packed\n \n - Currently once refs are packed into a pack-refs file, deleting them can be\n - slow since it involves a full file scan of the entire pack-refs file. At\n - GitLab, we have a system that creates ephemeral internal refs that don't\n - live long before getting deleted. Having an option to not include certain\n - refs from a pack-refs file allows these internal references to be deleted\n - much more efficiently.\n + At GitLab, we have a system that creates ephemeral internal refs that\n + don't live long before getting deleted. Having an option to not include\n + certain refs from a packed-refs file allows these internal references to\n + be deleted much more efficiently.\n \n Add an --exclude option to the pack-refs builtin, and use the ref\n exclusions API to exclude certain refs from being packed into the final\n - pack-refs file\n + packed-refs file\n \n Signed-off-by: John Cai \n \n @@ Documentation/git-pack-refs.txt: git-pack-refs - Pack heads and tags for efficie\n \n DESCRIPTION\n -----------\n -@@ Documentation/git-pack-refs.txt: a repository with many branches of historical interests.\n +@@ Documentation/git-pack-refs.txt: interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n @@ Documentation/git-pack-refs.txt: a repository with many branches of historical i\n +\n +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n -+patterns.\n ++patterns. If a ref is already packed, including it with `--exclude` will not\n ++unpack it.\n ++\n ++When used with `--all`, it will use the difference between the set of all refs,\n ++and what is provided to `--exclude`.\n +\n \n BUGS\n @@ builtin/pack-refs.c\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n +\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n -+\tstruct pack_refs_opts pack_opts = {.exclusions = &excludes};\n ++\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n +\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n +\tstruct string_list_item *item;\n +\n \tstruct option opts[] = {\n - \t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n - \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n +-\t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n +-\t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n ++\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n ++\t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n +\t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n +\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n @@ builtin/pack-refs.c\n -\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n +\n +\tfor_each_string_list_item(item, &option_excluded_refs)\n -+\t\tadd_ref_exclusion(pack_opts.exclusions, item->string);\n ++\t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n +\n -+\treturn refs_pack_refs(get_main_ref_store(the_repository), flags, &pack_opts);\n ++\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\n \n ## refs.c ##\n @@ refs.c: void base_ref_store_init(struct ref_store *refs, struct repository *repo\n \n /* backend functions */\n -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n -+int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *pack_opts)\n ++int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n {\n -\treturn refs->be->pack_refs(refs, flags);\n -+\treturn refs->be->pack_refs(refs, flags, pack_opts);\n ++\treturn refs->be->pack_refs(refs, opts);\n }\n \n int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n @@ refs.h: struct worktree;\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n +struct pack_refs_opts {\n ++\tunsigned int flags;\n +\tstruct ref_exclusions *exclusions;\n +};\n +\n @@ refs.h: void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n * flags: Combination of the above PACK_REFS_* flags.\n */\n -int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n -+int refs_pack_refs(struct ref_store *refs, unsigned int flags, struct pack_refs_opts *opts);\n ++int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\n @@ refs/debug.c: static int debug_initial_transaction_commit(struct ref_store *refs\n }\n \n -static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)\n -+static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags, struct pack_refs_opts *opts)\n ++static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n -\tint res = drefs->refs->be->pack_refs(drefs->refs, flags);\n -+\tint res = drefs->refs->be->pack_refs(drefs->refs, flags, opts);\n ++\tint res = drefs->refs->be->pack_refs(drefs->refs, opts);\n \ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n \treturn res;\n }\n @@ refs/files-backend.c: static void prune_refs(struct files_ref_store *refs, struc\n static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n -\t\t\t unsigned int pack_flags)\n -+\t\t\t unsigned int pack_flags,\n +\t\t\t struct pack_refs_opts *opts)\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n +@@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n + \t\treturn 0;\n + \n + \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n +-\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n ++\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n + \t\treturn 0;\n + \n + \t/* Do not pack symbolic refs: */\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \n -static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n +static int files_pack_refs(struct ref_store *ref_store,\n -+\t\t\t unsigned int flags,\n -+\t\t\t struct pack_refs_opts *pack_opts)\n ++\t\t\t struct pack_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n @@ refs/files-backend.c: static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n + \t\t * in the packed ref cache. If the reference should be\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n - \t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n +-\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n -\t\t\t\t flags))\n -+\t\t\t\t flags, pack_opts))\n ++\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags, opts))\n \t\t\tcontinue;\n \n \t\t/*\n +@@ refs/files-backend.c: static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n + \t\t\t iter->refname, err.buf);\n + \n + \t\t/* Schedule the loose reference for pruning if requested. */\n +-\t\tif ((flags & PACK_REFS_PRUNE)) {\n ++\t\tif ((opts->flags & PACK_REFS_PRUNE)) {\n + \t\t\tstruct ref_to_prune *n;\n + \t\t\tFLEX_ALLOC_STR(n, name, iter->refname);\n + \t\t\toidcpy(&n->oid, iter->oid);\n \n ## refs/packed-backend.c ##\n @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n @@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store\n \n static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n -\t\t\t unsigned int flags UNUSED)\n -+\t\t\t unsigned int flags UNUSED,\n +\t\t\t struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n @@ refs/refs-internal.h: typedef int ref_transaction_commit_fn(struct ref_store *re\n \n -typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n +typedef int pack_refs_fn(struct ref_store *ref_store,\n -+\t\t\t unsigned int flags,\n +\t\t\t struct pack_refs_opts *opts);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n @@ t/helper/test-ref-store.c: static struct flag_definition pack_flags[] = { FLAG_D\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n -+\tstruct pack_refs_opts pack_opts = { 0 };\n ++\tstruct pack_refs_opts pack_opts = { .flags = flags };\n \n -\treturn refs_pack_refs(refs, flags);\n -+\treturn refs_pack_refs(refs, flags, &pack_opts);\n ++\treturn refs_pack_refs(refs, &pack_opts);\n }\n \n static int cmd_create_symref(struct ref_store *refs, const char **argv)\n -: ----------- > 3: 03950e8f120 pack-refs: teach pack-refs --include option\n\n-- \ngitgitgadget\n"},{"id":"476912","messageId":"0b40b24b95d81cbc956eb4c7761e043c16a00be0.1683659931.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v2.git.git.1683659931.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] docs: clarify git-pack-refs --all will pack all refs","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-09T19:18:49Z","receivedAt":"2023-05-09T19:19:02Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n--all packs not just branch tips but anything under refs/ with the\nexception of hidden refs and broken refs. Clarify this in the\ndocumentation.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 154081f2de2..e011e5fead3 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -49,10 +49,11 @@ OPTIONS\n \n The command by default packs all tags and refs that are already\n packed, and leaves other refs\n-alone. This is because branches are expected to be actively\n+alone. This is because branches are expected to be actively\n developed and packing their tips does not help performance.\n-This option causes branch tips to be packed as well. Useful for\n-a repository with many branches of historical interests.\n+This option causes all refs to be packed as well, with the exception of hidden\n+and broken refs. Useful for a repository with many branches of historical\n+interests.\n \n --no-prune::\n \n-- \ngitgitgadget\n\n"},{"id":"476913","messageId":"027b3f85a0b9c33c5334d6dad84e99c325ebec10.1683659931.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v2.git.git.1683659931.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-09T19:18:50Z","receivedAt":"2023-05-09T19:19:05Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAt GitLab, we have a system that creates ephemeral internal refs that\ndon't live long before getting deleted. Having an option to not include\ncertain refs from a packed-refs file allows these internal references to\nbe deleted much more efficiently.\n\nAdd an --exclude option to the pack-refs builtin, and use the ref\nexclusions API to exclude certain refs from being packed into the final\npacked-refs file\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 12 +++++++++++-\n builtin/pack-refs.c | 21 +++++++++++++++++----\n refs.c | 4 ++--\n refs.h | 7 ++++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 16 ++++++++++------\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 ++-\n t/helper/test-ref-store.c | 3 ++-\n t/t3210-pack-refs.sh | 18 ++++++++++++++++++\n 10 files changed, 71 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex e011e5fead3..c0f7426e519 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune]\n+'git pack-refs' [--all] [--no-prune] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,16 @@ interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--exclude ::\n+\n+Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n+accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n+patterns. If a ref is already packed, including it with `--exclude` will not\n+unpack it.\n+\n+When used with `--all`, it will use the difference between the set of all refs,\n+and what is provided to `--exclude`.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 9833815fb30..3dcbd6e2421 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -4,22 +4,35 @@\n #include \"parse-options.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n \tNULL\n };\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n+\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\n \tstruct option opts[] = {\n-\t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n-\t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n+\t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n \t};\n+\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n+\n+\tfor_each_string_list_item(item, &option_excluded_refs)\n+\t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n+\n+\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/refs.c b/refs.c\nindex d2a98e1c21f..881a0da65cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n }\n \n /* backend functions */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n {\n-\treturn refs->be->pack_refs(refs, flags);\n+\treturn refs->be->pack_refs(refs, opts);\n }\n \n int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\ndiff --git a/refs.h b/refs.h\nindex 123cfa44244..46020bd335c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -63,6 +63,11 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+struct pack_refs_opts {\n+\tunsigned int flags;\n+\tstruct ref_exclusions *exclusions;\n+};\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -405,7 +410,7 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/debug.c b/refs/debug.c\nindex adc34c836fc..63f434ea26b 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -122,10 +122,10 @@ static int debug_initial_transaction_commit(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->pack_refs(drefs->refs, flags);\n+\tint res = drefs->refs->be->pack_refs(drefs->refs, opts);\n \ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d0581ee41ac..6a51267f379 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -19,6 +19,7 @@\n #include \"../worktree.h\"\n #include \"../wrapper.h\"\n #include \"../write-or-die.h\"\n+#include \"../revision.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -1173,7 +1174,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n */\n static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n-\t\t\t unsigned int pack_flags)\n+\t\t\t struct pack_refs_opts *opts)\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n@@ -1181,7 +1182,7 @@ static int should_pack_ref(const char *refname,\n \t\treturn 0;\n \n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n-\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n+\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n \t\treturn 0;\n \n \t/* Do not pack symbolic refs: */\n@@ -1192,10 +1193,14 @@ static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int files_pack_refs(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1220,8 +1225,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * in the packed ref cache. If the reference should be\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n-\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n-\t\t\t\t flags))\n+\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags, opts))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -1235,7 +1239,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\t iter->refname, err.buf);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n-\t\tif ((flags & PACK_REFS_PRUNE)) {\n+\t\tif ((opts->flags & PACK_REFS_PRUNE)) {\n \t\t\tstruct ref_to_prune *n;\n \t\t\tFLEX_ALLOC_STR(n, name, iter->refname);\n \t\t\toidcpy(&n->oid, iter->oid);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 2333ed5a1f7..9a2dd10dba7 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1576,7 +1576,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n }\n \n static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n-\t\t\t unsigned int flags UNUSED)\n+\t\t\t struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex a85d113123c..f72b7be8941 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -547,7 +547,8 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n+typedef int pack_refs_fn(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 6d8f844e9c7..de4197708d9 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -116,8 +116,9 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n+\tstruct pack_refs_opts pack_opts = { .flags = flags };\n \n-\treturn refs_pack_refs(refs, flags);\n+\treturn refs_pack_refs(refs, &pack_opts);\n }\n \n static int cmd_create_symref(struct ref_store *refs, const char **argv)\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 07a0ff93def..31b9f72e84a 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -108,6 +108,24 @@ test_expect_success \\\n git branch -d n/o/p &&\n git branch n'\n \n+test_expect_success \\\n+ 'test excluded refs are not packed' '\n+ git branch dont_pack1 &&\n+ git branch dont_pack2 &&\n+ git branch pack_this &&\n+ git pack-refs --all --exclude refs/heads/dont_pack* &&\n+ test -f .git/refs/heads/dont_pack1 &&\n+ test -f .git/refs/heads/dont_pack2 &&\n+ ! test -f ./git/refs/heads/pack_this'\n+\n+test_expect_success \\\n+ 'test --no-exclude refs clears excluded refs' '\n+ git branch dont_pack3 &&\n+ git branch dont_pack4 &&\n+ git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&\n+ ! test -f .git/refs/heads/dont_pack3 &&\n+ ! test -f .git/refs/heads/dont_pack4'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n\n"},{"id":"476914","messageId":"03950e8f120e48b7df68f3273bbb2f7bb1e9073d.1683659931.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v2.git.git.1683659931.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] pack-refs: teach pack-refs --include option","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-09T19:18:51Z","receivedAt":"2023-05-09T19:19:07Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAllow users to be more selective over which refs to pack by adding an\n--include option to git-pack-refs.\n\nThe existing options allow some measure of selectivity. By default\ngit-pack-refs packs all tags. --all can be used to include all refs,\nand the previous commit added the ability to exclude certain refs with\n--exclude.\n\nWhile these knobs give the user some selection over which refs to pack,\nit could be useful to give more control. For instance, a repository may\nhave a set of branches that are rarely updated and would benefit from\nbeing packed. --include would allow the user to easily include a set of\nbranches to be packed while leaving everything else unpacked.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 15 ++++++++++++++-\n builtin/pack-refs.c | 10 ++++++++--\n refs.h | 1 +\n refs/files-backend.c | 14 +++++++++++---\n t/t3210-pack-refs.sh | 10 ++++++++++\n 5 files changed, 44 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex c0f7426e519..f187925bdc0 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune] [--exclude ]\n+'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,15 @@ interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--include ::\n+\n+Pack refs based on a `glob(7)` pattern. Repetitions of this option\n+accumulate inclusion patterns. If a ref is both included in `--include` and\n+`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude\n+all tags from being included by default. Symbolic refs and broken refs will never\n+be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n+and reset the list of patterns.\n+\n --exclude ::\n \n Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n@@ -70,6 +79,10 @@ unpack it.\n When used with `--all`, it will use the difference between the set of all refs,\n and what is provided to `--exclude`.\n \n+When used with `--include`, it will use what is provided to `--include` as well\n+as the the default of all tags and already packed refs, minus refs that are\n+provided to `--exclude`.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 3dcbd6e2421..293cdd17181 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -7,7 +7,7 @@\n #include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n \tNULL\n };\n \n@@ -15,13 +15,19 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n \tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n-\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n+\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n+\t\t\t\t\t\t .flags = flags,\n+\t\t\t\t\t\t .included_refs = &included_refs };\n+\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.included_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n \t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\ndiff --git a/refs.h b/refs.h\nindex 46020bd335c..2f91f4c4a90 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -66,6 +66,7 @@ struct worktree;\n struct pack_refs_opts {\n \tunsigned int flags;\n \tstruct ref_exclusions *exclusions;\n+\tstruct string_list *included_refs;\n };\n \n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6a51267f379..3f8974a4a32 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1181,6 +1181,17 @@ static int should_pack_ref(const char *refname,\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n+\tif (opts->included_refs && opts->included_refs->nr) {\n+\t\tstruct string_list_item *item;\n+\n+\t\tfor_each_string_list_item(item, opts->included_refs)\n+\t\t\tif (!wildmatch(item->string, refname, 0))\n+\t\t\t\treturn 1;\n+\t}\n+\n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n \tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n \t\treturn 0;\n@@ -1193,9 +1204,6 @@ static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n-\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n-\t\treturn 0;\n-\n \treturn 1;\n }\n \ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 31b9f72e84a..86b3ff6a3e0 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -126,6 +126,16 @@ test_expect_success \\\n ! test -f .git/refs/heads/dont_pack3 &&\n ! test -f .git/refs/heads/dont_pack4'\n \n+test_expect_success \\\n+ 'test only included refs are packed' '\n+ git branch pack_this1 &&\n+ git branch pack_this2 &&\n+ git tag dont_pack5 &&\n+ git pack-refs --include refs/tags/pack_this* --exclude refs/tags/dont_pack* &&\n+ test -f .git/refs/tags/dont_pack5 &&\n+ ! test -f ./git/refs/heads/pack_this1 &&\n+ ! test -f ./git/refs/heads/pack_this2'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n"},{"id":"476928","messageId":"xmqq8rdxw71b.fsf@gitster.g","threadId":"59694","inReplyTo":"027b3f85a0b9c33c5334d6dad84e99c325ebec10.1683659931.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-09T21:04:32Z","receivedAt":"2023-05-09T21:04:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> At GitLab, we have a system that creates ephemeral internal refs that\n> don't live long before getting deleted. Having an option to not include\n\n\"to not include\" -> \"to exclude\", perhaps.\n\n\n> +--exclude ::\n> +\n> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n> +patterns. If a ref is already packed, including it with `--exclude` will not\n> +unpack it.\n> +\n> +When used with `--all`, it will use the difference between the set of all refs,\n> +and what is provided to `--exclude`.\n> +\n\nClearly written.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index d0581ee41ac..6a51267f379 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -19,6 +19,7 @@\n> #include \"../worktree.h\"\n> #include \"../wrapper.h\"\n> #include \"../write-or-die.h\"\n> +#include \"../revision.h\"\n> \n> /*\n> * This backend uses the following flags in `ref_update::flags` for\n> @@ -1173,7 +1174,7 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n> */\n> static int should_pack_ref(const char *refname,\n> \t\t\t const struct object_id *oid, unsigned int ref_flags,\n> -\t\t\t unsigned int pack_flags)\n> +\t\t\t struct pack_refs_opts *opts)\n> {\n> \t/* Do not pack per-worktree refs: */\n> \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n> @@ -1181,7 +1182,7 @@ static int should_pack_ref(const char *refname,\n> \t\treturn 0;\n> \n> \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n> -\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n> +\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n> \t\treturn 0;\n> \n> \t/* Do not pack symbolic refs: */\n> @@ -1192,10 +1193,14 @@ static int should_pack_ref(const char *refname,\n> \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n> \t\treturn 0;\n> \n> +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> +\t\treturn 0;\n\nIt is _not_ _wrong_ per-se, but I would have expected for this to go\nbetween \"what do we include\" and \"symrefs and broken ones are not\npacked\", given that we will tweak the \"what to include\" logic in the\nnext step.\n\nOther changes to the code in this patch are all naturally expected\nto pass the new information through the callchain and looked good.\n\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index 07a0ff93def..31b9f72e84a 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -108,6 +108,24 @@ test_expect_success \\\n> git branch -d n/o/p &&\n> git branch n'\n>\n> +test_expect_success \\\n> + 'test excluded refs are not packed' '\n> + git branch dont_pack1 &&\n> + git branch dont_pack2 &&\n> + git branch pack_this &&\n> + git pack-refs --all --exclude refs/heads/dont_pack* &&\n\nLet's quote the value given to \"--exclude\" to make it clear that we\ndo not expect glob to affect the shell, i.e.\n\n\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n\nSome (early and ancient) parts, but not all parts, of this file use\n4-space indentation and have the title of the test on a line that is\nseparate from test_expect_success, which is an ancient deprecated\nstyle.\n\nYou do not want to imitate them in new tests you add. After the\ndust from this topic settles, somebody should go in and clean the\nstyle of this file. Let's not create more work for them.\n\nThanks.\n\n> + test -f .git/refs/heads/dont_pack1 &&\n> + test -f .git/refs/heads/dont_pack2 &&\n> + ! test -f ./git/refs/heads/pack_this'\n> +\n> +test_expect_success \\\n> + 'test --no-exclude refs clears excluded refs' '\n> + git branch dont_pack3 &&\n> + git branch dont_pack4 &&\n> + git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&\n> + ! test -f .git/refs/heads/dont_pack3 &&\n> + ! test -f .git/refs/heads/dont_pack4'\n> +\n> test_expect_success \\\n> \t'see if up-to-date packed refs are preserved' \\\n> \t'git branch q &&\n"},{"id":"476931","messageId":"xmqqo7mturi1.fsf@gitster.g","threadId":"59694","inReplyTo":"03950e8f120e48b7df68f3273bbb2f7bb1e9073d.1683659931.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] pack-refs: teach pack-refs --include option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-09T21:25:26Z","receivedAt":"2023-05-09T21:34:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> +--include ::\n> +\n> +Pack refs based on a `glob(7)` pattern. Repetitions of this option\n> +accumulate inclusion patterns. If a ref is both included in `--include` and\n> +`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude\n> +all tags from being included by default. Symbolic refs and broken refs will never\n> +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n> +and reset the list of patterns.\n\nHmph, that was a bit unexpected. exclude taking precedence over\ninclude is very much in line with how negative pathspec works and\nthe end-users should be familiar with it, but when the user bothers\nto specify with --include what to include, I would have expected\nthat the \"pack tags by default\" would be defeated.\n\nIn other words, I would have expected that the program acts as if\nthe machinery works this way (iow, the code does not have to exactly\nimplement it this way---it just has to behave as if it did):\n\n - it maintains two pattern list, positive and negative,\n both start empty.\n - \"--exclude\" are accumulated to the negative list.\n - \"--include\" are accumulated to the positive list.\n - \"--all\" adds \"*\" to the positive list.\n - after parsing command line options, if the positive list is\n empty, then \"refs/tags/*\" is added to the positive list.\n - refs that match positive list but does not match negative list\n are shown.\n\n> +When used with `--include`, it will use what is provided to `--include` as well\n> +as the the default of all tags and already packed refs, minus refs that are\n> +provided to `--exclude`.\n\nIOW, I would expect that the use of \"--include\" alone is enough to\ndefeat the default; the end user does not have to figure out that\nthey have to pass \"--exclude=refs/tags/*\" to do so when they are\nspecifying a specific hierarchy to include.\n\n> @@ -66,6 +66,7 @@ struct worktree;\n> struct pack_refs_opts {\n> \tunsigned int flags;\n> \tstruct ref_exclusions *exclusions;\n> +\tstruct string_list *included_refs;\n\nIt is unfortunate that the struct is called ref_exclusions to imply\nas if it is only usable for excluding refs from listing. It has\nother members for handling hidden refs, and it would have been very\nnatural to extend it to also add included_refs pattern next to\nexcluded_refs string list. After all, the struct is used to tweak\nwhich refs are included and which refs are excluded, and\nhistorically everything was included unless listed on the excluded\npattern. We are now allowing the \"everything is included\" part to\nbe customizable with this step. If the struct were named with a\nmore neutral term, like ref_visibility to hint that it is about\nsetting visibility, then this patch wouldn't have added a separate\nstring list to this structure---instead it would have extended the\nref_exclusions (with a better name) struct and placed included_refs\nstring list there.\n\n> };\n> \n> const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 6a51267f379..3f8974a4a32 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1181,6 +1181,17 @@ static int should_pack_ref(const char *refname,\n> \t REF_WORKTREE_SHARED)\n> \t\treturn 0;\n> \n> +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> +\t\treturn 0;\n> +\n> +\tif (opts->included_refs && opts->included_refs->nr) {\n> +\t\tstruct string_list_item *item;\n> +\n> +\t\tfor_each_string_list_item(item, opts->included_refs)\n> +\t\t\tif (!wildmatch(item->string, refname, 0))\n> +\t\t\t\treturn 1;\n> +\t}\n\nWe can see why the initial placement of exclusion logic in the\nearlier step was suboptimal here.\n\n> \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n> \tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n> \t\treturn 0;\n> @@ -1193,9 +1204,6 @@ static int should_pack_ref(const char *refname,\n> \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n> \t\treturn 0;\n> \n> -\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> -\t\treturn 0;\n> -\n> \treturn 1;\n> }\n\n\nOther than that, the changes look mostly expected and no surprises.\n\nThanks.\n"},{"id":"476987","messageId":"20230510195247.aj2vpzzxzdkvuzff@pop-os","threadId":"59694","inReplyTo":"xmqqo7mturi1.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] pack-refs: teach pack-refs --include option","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-10T19:52:47Z","receivedAt":"2023-05-10T19:52:53Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"On 23/05/09 02:25PM, Junio C Hamano wrote:\n> \"John Cai via GitGitGadget\" writes:\n> \n> > +--include ::\n> > +\n> > +Pack refs based on a `glob(7)` pattern. Repetitions of this option\n> > +accumulate inclusion patterns. If a ref is both included in `--include` and\n> > +`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude\n> > +all tags from being included by default. Symbolic refs and broken refs will never\n> > +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n> > +and reset the list of patterns.\n> \n> Hmph, that was a bit unexpected. exclude taking precedence over\n> include is very much in line with how negative pathspec works and\n> the end-users should be familiar with it, but when the user bothers\n> to specify with --include what to include, I would have expected\n> that the \"pack tags by default\" would be defeated.\n> \n> In other words, I would have expected that the program acts as if\n> the machinery works this way (iow, the code does not have to exactly\n> implement it this way---it just has to behave as if it did):\n> \n> - it maintains two pattern list, positive and negative,\n> both start empty.\n> - \"--exclude\" are accumulated to the negative list.\n> - \"--include\" are accumulated to the positive list.\n> - \"--all\" adds \"*\" to the positive list.\n> - after parsing command line options, if the positive list is\n> empty, then \"refs/tags/*\" is added to the positive list.\n> - refs that match positive list but does not match negative list\n> are shown.\n> \n> > +When used with `--include`, it will use what is provided to `--include` as well\n> > +as the the default of all tags and already packed refs, minus refs that are\n> > +provided to `--exclude`.\n> \n> IOW, I would expect that the use of \"--include\" alone is enough to\n> defeat the default; the end user does not have to figure out that\n> they have to pass \"--exclude=refs/tags/*\" to do so when they are\n> specifying a specific hierarchy to include.\n\nHm yeah, I think that is a nicer user experience.\n\n> \n> > @@ -66,6 +66,7 @@ struct worktree;\n> > struct pack_refs_opts {\n> > \tunsigned int flags;\n> > \tstruct ref_exclusions *exclusions;\n> > +\tstruct string_list *included_refs;\n> \n> It is unfortunate that the struct is called ref_exclusions to imply\n> as if it is only usable for excluding refs from listing. It has\n> other members for handling hidden refs, and it would have been very\n> natural to extend it to also add included_refs pattern next to\n> excluded_refs string list. After all, the struct is used to tweak\n> which refs are included and which refs are excluded, and\n> historically everything was included unless listed on the excluded\n> pattern. We are now allowing the \"everything is included\" part to\n> be customizable with this step. If the struct were named with a\n> more neutral term, like ref_visibility to hint that it is about\n> setting visibility, then this patch wouldn't have added a separate\n> string list to this structure---instead it would have extended the\n> ref_exclusions (with a better name) struct and placed included_refs\n> string list there.\n\nThanks for calling this out. I was thinking along very similar lines when\nworking on this patch, but was too lazy to make the change :)\n\n> \n> > };\n> > \n> > const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 6a51267f379..3f8974a4a32 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1181,6 +1181,17 @@ static int should_pack_ref(const char *refname,\n> > \t REF_WORKTREE_SHARED)\n> > \t\treturn 0;\n> > \n> > +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> > +\t\treturn 0;\n> > +\n> > +\tif (opts->included_refs && opts->included_refs->nr) {\n> > +\t\tstruct string_list_item *item;\n> > +\n> > +\t\tfor_each_string_list_item(item, opts->included_refs)\n> > +\t\t\tif (!wildmatch(item->string, refname, 0))\n> > +\t\t\t\treturn 1;\n> > +\t}\n> \n> We can see why the initial placement of exclusion logic in the\n> earlier step was suboptimal here.\n> \n> > \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n> > \tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n> > \t\treturn 0;\n> > @@ -1193,9 +1204,6 @@ static int should_pack_ref(const char *refname,\n> > \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n> > \t\treturn 0;\n> > \n> > -\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> > -\t\treturn 0;\n> > -\n> > \treturn 1;\n> > }\n> \n> \n> Other than that, the changes look mostly expected and no surprises.\n> \n> Thanks.\n\nthanks\nJohn\n"},{"id":"477056","messageId":"0d462010b798f834fef55cc7aa767f3f2a177f91.1683828635.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] docs: clarify git-pack-refs --all will pack all refs","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-11T18:10:31Z","receivedAt":"2023-05-11T18:11:07Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n--all packs not just branch tips but anything under refs/ with the\nexception of hidden refs and broken refs. Clarify this in the\ndocumentation.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 154081f2de2..e011e5fead3 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -49,10 +49,11 @@ OPTIONS\n \n The command by default packs all tags and refs that are already\n packed, and leaves other refs\n-alone. This is because branches are expected to be actively\n+alone. This is because branches are expected to be actively\n developed and packing their tips does not help performance.\n-This option causes branch tips to be packed as well. Useful for\n-a repository with many branches of historical interests.\n+This option causes all refs to be packed as well, with the exception of hidden\n+and broken refs. Useful for a repository with many branches of historical\n+interests.\n \n --no-prune::\n \n-- \ngitgitgadget\n\n"},{"id":"477057","messageId":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v2.git.git.1683659931.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] pack-refs: allow users control over which refs to pack","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-11T18:10:30Z","receivedAt":"2023-05-11T18:11:09Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-pack-refs does not currently give much control over which refs are\npacked. By default, all tags and already packed refs are packed. --all\nallows the user to pack all refs. Beyond this the user does not have\ncontrol. Introduce a pair of options --include and --exclude that will allow\nusers full control over which refs end up in the packed-refs file.\n\nChanges since v2:\n\n * repurpose ref_exclusions to be used for ref inclusions\n * fixed test formatting\n * adjusted --include to not include default of all tags\n\nChanges since v1:\n\n * Clarify that --all packs not just branch tips but all refs under refs/ in\n the documentation in patch 1\n * Add --include in patch 3\n\nIt's worth noting that [1] discussed a proposal for a pack refs v2 format\nthat would improve deletion speeds. The ref-table backend would also improve\nperformance of deletions. However, both of those proposals are still being\ndiscussed.\n\n 1. https://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\nJohn Cai (4):\n docs: clarify git-pack-refs --all will pack all refs\n pack-refs: teach --exclude option to exclude refs from being packed\n revision: modify ref_exclusions to handle inclusions\n pack-refs: teach pack-refs --include option\n\n Documentation/git-pack-refs.txt | 31 ++++++++++++--\n builtin/pack-refs.c | 34 ++++++++++++++--\n builtin/rev-parse.c | 18 ++++-----\n refs.c | 4 +-\n refs.h | 7 +++-\n refs/debug.c | 4 +-\n refs/files-backend.c | 23 ++++++-----\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 +-\n revision.c | 71 ++++++++++++++++++++-------------\n revision.h | 28 ++++++++-----\n t/helper/test-ref-store.c | 9 ++++-\n t/t3210-pack-refs.sh | 37 +++++++++++++++++\n 13 files changed, 199 insertions(+), 72 deletions(-)\n\n\nbase-commit: 91428f078b8a4fe6948a4c955af1a693841e3985\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1501%2Fjohn-cai%2Fjc%2Fexclude-refs-from-pack-refs-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1501/john-cai/jc/exclude-refs-from-pack-refs-v3\nPull-Request: https://github.com/git/git/pull/1501\n\nRange-diff vs v2:\n\n 1: 0b40b24b95d = 1: 0d462010b79 docs: clarify git-pack-refs --all will pack all refs\n 2: 027b3f85a0b ! 2: 8c5c66a3050 pack-refs: teach --exclude option to exclude refs from being packed\n @@ Commit message\n pack-refs: teach --exclude option to exclude refs from being packed\n \n At GitLab, we have a system that creates ephemeral internal refs that\n - don't live long before getting deleted. Having an option to not include\n + don't live long before getting deleted. Having an option to exclude\n certain refs from a packed-refs file allows these internal references to\n be deleted much more efficiently.\n \n @@ builtin/pack-refs.c\n +\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n \t};\n -+\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n @@ refs/files-backend.c: static void prune_refs(struct files_ref_store *refs, struc\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n -@@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n + \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n ++\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n ++\t\treturn 0;\n ++\n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n -\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n +\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \n \t/* Do not pack symbolic refs: */\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n - \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n - \t\treturn 0;\n - \n -+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n -+\t\treturn 0;\n -+\n \treturn 1;\n }\n \n @@ refs/refs-internal.h: typedef int ref_transaction_commit_fn(struct ref_store *re\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\n \n + ## revision.h ##\n +@@ revision.h: struct rev_cmdline_info {\n + struct ref_exclusions {\n + \t/*\n + \t * Excluded refs is a list of wildmatch patterns. If any of the\n +-\t * patterns matches, the reference will be excluded.\n ++\t * patterns match, the reference will be excluded.\n + \t */\n + \tstruct string_list excluded_refs;\n + \n +\n ## t/helper/test-ref-store.c ##\n @@ t/helper/test-ref-store.c: static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n @@ t/t3210-pack-refs.sh: test_expect_success \\\n git branch -d n/o/p &&\n git branch n'\n \n -+test_expect_success \\\n -+ 'test excluded refs are not packed' '\n -+ git branch dont_pack1 &&\n -+ git branch dont_pack2 &&\n -+ git branch pack_this &&\n -+ git pack-refs --all --exclude refs/heads/dont_pack* &&\n -+ test -f .git/refs/heads/dont_pack1 &&\n -+ test -f .git/refs/heads/dont_pack2 &&\n -+ ! test -f ./git/refs/heads/pack_this'\n ++test_expect_success 'test excluded refs are not packed' '\n ++\tgit branch dont_pack1 &&\n ++\tgit branch dont_pack2 &&\n ++\tgit branch pack_this &&\n ++\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n ++\ttest -f .git/refs/heads/dont_pack1 &&\n ++\ttest -f .git/refs/heads/dont_pack2 &&\n ++\t! test -f ./git/refs/heads/pack_this'\n +\n -+test_expect_success \\\n -+ 'test --no-exclude refs clears excluded refs' '\n -+ git branch dont_pack3 &&\n -+ git branch dont_pack4 &&\n -+ git pack-refs --all --exclude refs/heads/dont_pack* --no-exclude &&\n -+ ! test -f .git/refs/heads/dont_pack3 &&\n -+ ! test -f .git/refs/heads/dont_pack4'\n ++test_expect_success 'test --no-exclude refs clears excluded refs' '\n ++\tgit branch dont_pack3 &&\n ++\tgit branch dont_pack4 &&\n ++\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n ++\t! test -f .git/refs/heads/dont_pack3 &&\n ++\t! test -f .git/refs/heads/dont_pack4'\n +\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n -: ----------- > 3: 0a0693ad612 revision: modify ref_exclusions to handle inclusions\n 3: 03950e8f120 ! 4: b2f3b98cd24 pack-refs: teach pack-refs --include option\n @@ Documentation/git-pack-refs.txt: interests.\n +\n +Pack refs based on a `glob(7)` pattern. Repetitions of this option\n +accumulate inclusion patterns. If a ref is both included in `--include` and\n -+`--exclude`, `--exclude` takes precedence. Using `--include` does not preclude\n -+all tags from being included by default. Symbolic refs and broken refs will never\n ++`--exclude`, `--exclude` takes precedence. Using `--include` will preclude all\n ++tags from being included by default. Symbolic refs and broken refs will never\n +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n +and reset the list of patterns.\n +\n @@ Documentation/git-pack-refs.txt: unpack it.\n When used with `--all`, it will use the difference between the set of all refs,\n and what is provided to `--exclude`.\n \n -+When used with `--include`, it will use what is provided to `--include` as well\n -+as the the default of all tags and already packed refs, minus refs that are\n -+provided to `--exclude`.\n ++When used with `--include`, refs provided to `--include`, minus refs that are\n ++provided to `--exclude` will be packed.\n +\n \n BUGS\n @@ Documentation/git-pack-refs.txt: unpack it.\n \n ## builtin/pack-refs.c ##\n @@\n + #include \"refs.h\"\n + #include \"repository.h\"\n #include \"revision.h\"\n ++#include \"trace.h\"\n \n static char const * const pack_refs_usage[] = {\n -\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n @@ builtin/pack-refs.c\n @@ builtin/pack-refs.c: int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n - \tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n --\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n -+\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n -+\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n -+\t\t\t\t\t\t .flags = flags,\n -+\t\t\t\t\t\t .included_refs = &included_refs };\n -+\n + \tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n +-\tstruct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};\n ++\tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\n ++\t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n ++\tstatic struct string_list option_included_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n -+\t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.included_refs, N_(\"pattern\"),\n ++\t\tOPT_STRING_LIST(0, \"include\", &option_included_refs, N_(\"pattern\"),\n +\t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n \t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n -\n - ## refs.h ##\n -@@ refs.h: struct worktree;\n - struct pack_refs_opts {\n - \tunsigned int flags;\n - \tstruct ref_exclusions *exclusions;\n -+\tstruct string_list *included_refs;\n - };\n +@@ builtin/pack-refs.c: int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n + \tfor_each_string_list_item(item, &option_excluded_refs)\n + \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n \n - const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n ++\tfor_each_string_list_item(item, &option_included_refs)\n ++\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n ++\n ++\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n ++\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n ++\n ++\tif (!pack_refs_opts.visibility->included_refs.nr)\n ++\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n ++\n + \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n + }\n \n ## refs/files-backend.c ##\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n -+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n -+\t\treturn 0;\n -+\n -+\tif (opts->included_refs && opts->included_refs->nr) {\n -+\t\tstruct string_list_item *item;\n -+\n -+\t\tfor_each_string_list_item(item, opts->included_refs)\n -+\t\t\tif (!wildmatch(item->string, refname, 0))\n -+\t\t\t\treturn 1;\n -+\t}\n -+\n - \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n - \tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n +-\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n +-\t\treturn 0;\n +-\n +-\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n +-\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n +-\t\treturn 0;\n +-\n + \t/* Do not pack symbolic refs: */\n + \tif (ref_flags & REF_ISSYMREF)\n \t\treturn 0;\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n --\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n --\t\treturn 0;\n --\n - \treturn 1;\n +-\treturn 1;\n ++\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n ++\t\treturn 0;\n ++\n ++\tif (opts->visibility && ref_included(opts->visibility, refname))\n ++\t\treturn 1;\n ++\n ++\treturn 0;\n }\n \n + static int files_pack_refs(struct ref_store *ref_store,\n +\n + ## t/helper/test-ref-store.c ##\n +@@\n + #include \"worktree.h\"\n + #include \"object-store.h\"\n + #include \"repository.h\"\n ++#include \"revision.h\"\n + \n + struct flag_definition {\n + \tconst char *name;\n +@@ t/helper/test-ref-store.c: static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n + static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n + {\n + \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n +-\tstruct pack_refs_opts pack_opts = { .flags = flags };\n ++\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n ++\tstruct pack_refs_opts pack_opts = { .flags = flags,\n ++\t\t\t\t\t .visibility = &visibility };\n ++\n ++\tif (pack_opts.flags & PACK_REFS_ALL)\n ++\t\tadd_ref_inclusion(pack_opts.visibility, \"*\");\n + \n + \treturn refs_pack_refs(refs, &pack_opts);\n + }\n \n ## t/t3210-pack-refs.sh ##\n -@@ t/t3210-pack-refs.sh: test_expect_success \\\n - ! test -f .git/refs/heads/dont_pack3 &&\n - ! test -f .git/refs/heads/dont_pack4'\n +@@ t/t3210-pack-refs.sh: test_expect_success 'test --no-exclude refs clears excluded refs' '\n + \t! test -f .git/refs/heads/dont_pack3 &&\n + \t! test -f .git/refs/heads/dont_pack4'\n \n -+test_expect_success \\\n -+ 'test only included refs are packed' '\n -+ git branch pack_this1 &&\n -+ git branch pack_this2 &&\n -+ git tag dont_pack5 &&\n -+ git pack-refs --include refs/tags/pack_this* --exclude refs/tags/dont_pack* &&\n -+ test -f .git/refs/tags/dont_pack5 &&\n -+ ! test -f ./git/refs/heads/pack_this1 &&\n -+ ! test -f ./git/refs/heads/pack_this2'\n ++test_expect_success 'test only included refs are packed' '\n ++\tgit branch pack_this1 &&\n ++\tgit branch pack_this2 &&\n ++\tgit tag dont_pack5 &&\n ++\tgit pack-refs --include \"refs/heads/pack_this*\" &&\n ++\ttest -f .git/refs/tags/dont_pack5 &&\n ++\t! test -f ./git/refs/heads/pack_this1 &&\n ++\t! test -f ./git/refs/heads/pack_this2'\n ++\n ++test_expect_success 'test --no-include refs clears included refs' '\n ++\tgit branch pack1 &&\n ++\tgit branch pack2 &&\n ++\tgit pack-refs --include \"refs/heads/pack*\" --no-include &&\n ++\ttest -f .git/refs/heads/pack1 &&\n ++\ttest -f .git/refs/heads/pack2'\n ++\n ++test_expect_success 'test --exclude takes precedence over --include' '\n ++\tgit branch dont_pack5 &&\n ++\tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n ++\ttest -f .git/refs/heads/dont_pack5'\n +\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n\n-- \ngitgitgadget\n"},{"id":"477058","messageId":"8c5c66a3050ee1c0ce8a48a088f5ecc2df7d1e3a.1683828635.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-11T18:10:32Z","receivedAt":"2023-05-11T18:11:10Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAt GitLab, we have a system that creates ephemeral internal refs that\ndon't live long before getting deleted. Having an option to exclude\ncertain refs from a packed-refs file allows these internal references to\nbe deleted much more efficiently.\n\nAdd an --exclude option to the pack-refs builtin, and use the ref\nexclusions API to exclude certain refs from being packed into the final\npacked-refs file\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 12 +++++++++++-\n builtin/pack-refs.c | 20 ++++++++++++++++----\n refs.c | 4 ++--\n refs.h | 7 ++++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 16 ++++++++++------\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 ++-\n revision.h | 2 +-\n t/helper/test-ref-store.c | 3 ++-\n t/t3210-pack-refs.sh | 16 ++++++++++++++++\n 11 files changed, 69 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex e011e5fead3..c0f7426e519 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune]\n+'git pack-refs' [--all] [--no-prune] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,16 @@ interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--exclude ::\n+\n+Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n+accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n+patterns. If a ref is already packed, including it with `--exclude` will not\n+unpack it.\n+\n+When used with `--all`, it will use the difference between the set of all refs,\n+and what is provided to `--exclude`.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 9833815fb30..1d1a64fe386 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -4,22 +4,34 @@\n #include \"parse-options.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n \tNULL\n };\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n+\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\n \tstruct option opts[] = {\n-\t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n-\t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n+\t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n \t};\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n+\n+\tfor_each_string_list_item(item, &option_excluded_refs)\n+\t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n+\n+\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/refs.c b/refs.c\nindex d2a98e1c21f..881a0da65cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n }\n \n /* backend functions */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n {\n-\treturn refs->be->pack_refs(refs, flags);\n+\treturn refs->be->pack_refs(refs, opts);\n }\n \n int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\ndiff --git a/refs.h b/refs.h\nindex 123cfa44244..46020bd335c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -63,6 +63,11 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+struct pack_refs_opts {\n+\tunsigned int flags;\n+\tstruct ref_exclusions *exclusions;\n+};\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -405,7 +410,7 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 6f11e6de46c..c0fa707a1da 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -123,10 +123,10 @@ static int debug_initial_transaction_commit(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->pack_refs(drefs->refs, flags);\n+\tint res = drefs->refs->be->pack_refs(drefs->refs, opts);\n \ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bca7b851c5a..76aebfde695 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -21,6 +21,7 @@\n #include \"../worktree.h\"\n #include \"../wrapper.h\"\n #include \"../write-or-die.h\"\n+#include \"../revision.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -1175,15 +1176,18 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n */\n static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n-\t\t\t unsigned int pack_flags)\n+\t\t\t struct pack_refs_opts *opts)\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n-\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n+\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n \t\treturn 0;\n \n \t/* Do not pack symbolic refs: */\n@@ -1197,7 +1201,8 @@ static int should_pack_ref(const char *refname,\n \treturn 1;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int files_pack_refs(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1222,8 +1227,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * in the packed ref cache. If the reference should be\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n-\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n-\t\t\t\t flags))\n+\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags, opts))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -1237,7 +1241,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\t iter->refname, err.buf);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n-\t\tif ((flags & PACK_REFS_PRUNE)) {\n+\t\tif ((opts->flags & PACK_REFS_PRUNE)) {\n \t\t\tstruct ref_to_prune *n;\n \t\t\tFLEX_ALLOC_STR(n, name, iter->refname);\n \t\t\toidcpy(&n->oid, iter->oid);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 5b412a133be..291e53f5cfe 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1577,7 +1577,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n }\n \n static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n-\t\t\t unsigned int flags UNUSED)\n+\t\t\t struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex a85d113123c..f72b7be8941 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -547,7 +547,8 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n+typedef int pack_refs_fn(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\ndiff --git a/revision.h b/revision.h\nindex 31828748dc0..25776af3815 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -87,7 +87,7 @@ struct rev_cmdline_info {\n struct ref_exclusions {\n \t/*\n \t * Excluded refs is a list of wildmatch patterns. If any of the\n-\t * patterns matches, the reference will be excluded.\n+\t * patterns match, the reference will be excluded.\n \t */\n \tstruct string_list excluded_refs;\n \ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 6d8f844e9c7..de4197708d9 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -116,8 +116,9 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n+\tstruct pack_refs_opts pack_opts = { .flags = flags };\n \n-\treturn refs_pack_refs(refs, flags);\n+\treturn refs_pack_refs(refs, &pack_opts);\n }\n \n static int cmd_create_symref(struct ref_store *refs, const char **argv)\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 07a0ff93def..ddfc1b6e5f1 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -108,6 +108,22 @@ test_expect_success \\\n git branch -d n/o/p &&\n git branch n'\n \n+test_expect_success 'test excluded refs are not packed' '\n+\tgit branch dont_pack1 &&\n+\tgit branch dont_pack2 &&\n+\tgit branch pack_this &&\n+\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n+\ttest -f .git/refs/heads/dont_pack1 &&\n+\ttest -f .git/refs/heads/dont_pack2 &&\n+\t! test -f ./git/refs/heads/pack_this'\n+\n+test_expect_success 'test --no-exclude refs clears excluded refs' '\n+\tgit branch dont_pack3 &&\n+\tgit branch dont_pack4 &&\n+\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n+\t! test -f .git/refs/heads/dont_pack3 &&\n+\t! test -f .git/refs/heads/dont_pack4'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n\n"},{"id":"477059","messageId":"0a0693ad612755e675861dfa5a660baf8d325ed0.1683828635.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-11T18:10:33Z","receivedAt":"2023-05-11T18:11:12Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe ref_exclusions API provides the ability to check if certain refs are\nto be excluded. We can easily extend this API to check if certain refs\nare included, which a subsequent commit will make use of.\n\nRename ref_exclusions to ref_visibility and add a member to keep track\nof inclusions. Add add_ref_inclusion(), ref_included() to be used for\nrefs to explicitly include.\n\nSigned-off-by: John Cai \n---\n builtin/pack-refs.c | 6 ++--\n builtin/rev-parse.c | 18 +++++------\n refs.h | 2 +-\n refs/files-backend.c | 2 +-\n revision.c | 71 +++++++++++++++++++++++++++-----------------\n revision.h | 26 ++++++++++------\n 6 files changed, 75 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 1d1a64fe386..2464575a665 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -14,8 +14,8 @@ static char const * const pack_refs_usage[] = {\n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n-\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n-\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n+\tstruct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n@@ -31,7 +31,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(pack_refs_usage, opts);\n \n \tfor_each_string_list_item(item, &option_excluded_refs)\n-\t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n+\t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n \n \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e3403..9036d8876fc 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -46,7 +46,7 @@ static int abbrev_ref_strict;\n static int output_sq;\n \n static int stuck_long;\n-static struct ref_exclusions ref_excludes = REF_EXCLUSIONS_INIT;\n+static struct ref_visibility ref_visibility = REF_VISIBILITY_INIT;\n \n /*\n * Some arguments are relevant \"revision\" arguments,\n@@ -208,7 +208,7 @@ static int show_default(void)\n static int show_reference(const char *refname, const struct object_id *oid,\n \t\t\t int flag UNUSED, void *cb_data UNUSED)\n {\n-\tif (ref_excluded(&ref_excludes, refname))\n+\tif (ref_excluded(&ref_visibility, refname))\n \t\treturn 0;\n \tshow_rev(NORMAL, oid, refname);\n \treturn 0;\n@@ -596,7 +596,7 @@ static void handle_ref_opt(const char *pattern, const char *prefix)\n \t\tfor_each_glob_ref_in(show_reference, pattern, prefix, NULL);\n \telse\n \t\tfor_each_ref_in(prefix, show_reference, NULL);\n-\tclear_ref_exclusions(&ref_excludes);\n+\tclear_ref_visibility(&ref_visibility);\n }\n \n enum format_type {\n@@ -874,7 +874,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n \t\t\t\tfor_each_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusions(&ref_excludes);\n+\t\t\t\tclear_ref_visibility(&ref_visibility);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--disambiguate=\", &arg)) {\n@@ -888,13 +888,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--branches\", &arg)) {\n-\t\t\t\tif (ref_excludes.hidden_refs_configured)\n+\t\t\t\tif (ref_visibility.hidden_refs_configured)\n \t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n \t\t\t\thandle_ref_opt(arg, \"refs/heads/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--tags\", &arg)) {\n-\t\t\t\tif (ref_excludes.hidden_refs_configured)\n+\t\t\t\tif (ref_visibility.hidden_refs_configured)\n \t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n \t\t\t\thandle_ref_opt(arg, \"refs/tags/\");\n \t\t\t\tcontinue;\n@@ -904,17 +904,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (opt_with_value(arg, \"--remotes\", &arg)) {\n-\t\t\t\tif (ref_excludes.hidden_refs_configured)\n+\t\t\t\tif (ref_visibility.hidden_refs_configured)\n \t\t\t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n \t\t\t\thandle_ref_opt(arg, \"refs/remotes/\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--exclude=\", &arg)) {\n-\t\t\t\tadd_ref_exclusion(&ref_excludes, arg);\n+\t\t\t\tadd_ref_exclusion(&ref_visibility, arg);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--exclude-hidden=\", &arg)) {\n-\t\t\t\texclude_hidden_refs(&ref_excludes, arg);\n+\t\t\t\texclude_hidden_refs(&ref_visibility, arg);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-toplevel\")) {\ndiff --git a/refs.h b/refs.h\nindex 46020bd335c..57949a1726b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -65,7 +65,7 @@ struct worktree;\n \n struct pack_refs_opts {\n \tunsigned int flags;\n-\tstruct ref_exclusions *exclusions;\n+\tstruct ref_visibility *visibility;\n };\n \n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 76aebfde695..3ef19199788 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1183,7 +1183,7 @@ static int should_pack_ref(const char *refname,\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n-\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n+\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n \t\treturn 0;\n \n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\ndiff --git a/revision.c b/revision.c\nindex b33cc1d106a..d59cd728e9e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1533,54 +1533,71 @@ static void add_rev_cmdline_list(struct rev_info *revs,\n \t}\n }\n \n-int ref_excluded(const struct ref_exclusions *exclusions, const char *path)\n+static int ref_matched(const char *path,\n+\t\t const struct string_list *l,\n+\t\t const struct string_list *hidden_refs)\n {\n \tconst char *stripped_path = strip_namespace(path);\n \tstruct string_list_item *item;\n \n-\tfor_each_string_list_item(item, &exclusions->excluded_refs) {\n+\tfor_each_string_list_item(item, l) {\n \t\tif (!wildmatch(item->string, path, 0))\n \t\t\treturn 1;\n \t}\n \n-\tif (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))\n+\tif (ref_is_hidden(stripped_path, path, hidden_refs))\n \t\treturn 1;\n \n \treturn 0;\n }\n \n-void init_ref_exclusions(struct ref_exclusions *exclusions)\n+int ref_excluded(const struct ref_visibility *visibility, const char *path)\n {\n-\tstruct ref_exclusions blank = REF_EXCLUSIONS_INIT;\n-\tmemcpy(exclusions, &blank, sizeof(*exclusions));\n+\treturn ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);\n }\n \n-void clear_ref_exclusions(struct ref_exclusions *exclusions)\n+int ref_included(const struct ref_visibility *visibility, const char *path)\n {\n-\tstring_list_clear(&exclusions->excluded_refs, 0);\n-\tstring_list_clear(&exclusions->hidden_refs, 0);\n-\texclusions->hidden_refs_configured = 0;\n+\treturn ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);\n }\n \n-void add_ref_exclusion(struct ref_exclusions *exclusions, const char *exclude)\n+void init_ref_visibility(struct ref_visibility *visibility)\n {\n-\tstring_list_append(&exclusions->excluded_refs, exclude);\n+\tstruct ref_visibility blank = REF_VISIBILITY_INIT;\n+\tmemcpy(visibility, &blank, sizeof(*visibility));\n+}\n+\n+void clear_ref_visibility(struct ref_visibility *visibility)\n+{\n+\tstring_list_clear(&visibility->excluded_refs, 0);\n+\tstring_list_clear(&visibility->hidden_refs, 0);\n+\tvisibility->hidden_refs_configured = 0;\n+}\n+\n+void add_ref_exclusion(struct ref_visibility *visibility, const char *exclude)\n+{\n+\tstring_list_append(&visibility->excluded_refs, exclude);\n+}\n+\n+void add_ref_inclusion(struct ref_visibility *visibility, const char *include)\n+{\n+\tstring_list_append(&visibility->included_refs, include);\n }\n \n struct exclude_hidden_refs_cb {\n-\tstruct ref_exclusions *exclusions;\n+\tstruct ref_visibility *visibility;\n \tconst char *section;\n };\n \n static int hide_refs_config(const char *var, const char *value, void *cb_data)\n {\n \tstruct exclude_hidden_refs_cb *cb = cb_data;\n-\tcb->exclusions->hidden_refs_configured = 1;\n+\tcb->visibility->hidden_refs_configured = 1;\n \treturn parse_hide_refs_config(var, value, cb->section,\n-\t\t\t\t &cb->exclusions->hidden_refs);\n+\t\t\t\t &cb->visibility->hidden_refs);\n }\n \n-void exclude_hidden_refs(struct ref_exclusions *exclusions, const char *section)\n+void exclude_hidden_refs(struct ref_visibility *visibility, const char *section)\n {\n \tstruct exclude_hidden_refs_cb cb;\n \n@@ -1588,10 +1605,10 @@ void exclude_hidden_refs(struct ref_exclusions *exclusions, const char *section)\n \t\t\tstrcmp(section, \"uploadpack\"))\n \t\tdie(_(\"unsupported section for hidden refs: %s\"), section);\n \n-\tif (exclusions->hidden_refs_configured)\n+\tif (visibility->hidden_refs_configured)\n \t\tdie(_(\"--exclude-hidden= passed more than once\"));\n \n-\tcb.exclusions = exclusions;\n+\tcb.visibility = visibility;\n \tcb.section = section;\n \n \tgit_config(hide_refs_config, &cb);\n@@ -1935,7 +1952,7 @@ void repo_init_revisions(struct repository *r,\n \n \tinit_display_notes(&revs->notes_opt);\n \tlist_objects_filter_init(&revs->filter);\n-\tinit_ref_exclusions(&revs->ref_excludes);\n+\tinit_ref_visibility(&revs->ref_excludes);\n }\n \n static void add_pending_commit_list(struct rev_info *revs,\n@@ -2724,12 +2741,12 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\t\tother_head_refs(handle_one_ref, &cb);\n \t\t}\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--branches\")) {\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n \t\thandle_refs(refs, revs, *flags, refs_for_each_branch_ref);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--bisect\")) {\n \t\tread_bisect_terms(&term_bad, &term_good);\n \t\thandle_refs(refs, revs, *flags, for_each_bad_bisect_ref);\n@@ -2740,17 +2757,17 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n \t\thandle_refs(refs, revs, *flags, refs_for_each_tag_ref);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--remotes\")) {\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n \t\thandle_refs(refs, revs, *flags, refs_for_each_remote_ref);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if ((argcount = parse_long_opt(\"glob\", argv, &optarg))) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref(handle_one_ref, optarg, &cb);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n@@ -2764,21 +2781,21 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --branches\"));\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/heads/\", &cb);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (skip_prefix(arg, \"--tags=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --tags\"));\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/tags/\", &cb);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (skip_prefix(arg, \"--remotes=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n \t\tif (revs->ref_excludes.hidden_refs_configured)\n \t\t\treturn error(_(\"--exclude-hidden cannot be used together with --remotes\"));\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n \t\tfor_each_glob_ref_in(handle_one_ref, optarg, \"refs/remotes/\", &cb);\n-\t\tclear_ref_exclusions(&revs->ref_excludes);\n+\t\tclear_ref_visibility(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\ndiff --git a/revision.h b/revision.h\nindex 25776af3815..296c520a003 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -84,7 +84,12 @@ struct rev_cmdline_info {\n \t} *rev;\n };\n \n-struct ref_exclusions {\n+struct ref_visibility {\n+\t/*\n+\t * Included refs is a list of wildmatch patterns. If any of the\n+\t * patterns match, the reference will be included.\n+\t */\n+\tstruct string_list included_refs;\n \t/*\n \t * Excluded refs is a list of wildmatch patterns. If any of the\n \t * patterns match, the reference will be excluded.\n@@ -106,9 +111,10 @@ struct ref_exclusions {\n };\n \n /**\n- * Initialize a `struct ref_exclusions` with a macro.\n+ * Initialize a `struct ref_visibility` with a macro.\n */\n-#define REF_EXCLUSIONS_INIT { \\\n+#define REF_VISIBILITY_INIT { \\\n+\t.included_refs = STRING_LIST_INIT_DUP, \\\n \t.excluded_refs = STRING_LIST_INIT_DUP, \\\n \t.hidden_refs = STRING_LIST_INIT_DUP, \\\n }\n@@ -135,7 +141,7 @@ struct rev_info {\n \tstruct list_objects_filter_options filter;\n \n \t/* excluding from --branches, --refs, etc. expansion */\n-\tstruct ref_exclusions ref_excludes;\n+\tstruct ref_visibility ref_excludes;\n \n \t/* Basic information */\n \tconst char *prefix;\n@@ -487,11 +493,13 @@ void show_object_with_name(FILE *, struct object *, const char *);\n * Helpers to check if a reference should be excluded.\n */\n \n-int ref_excluded(const struct ref_exclusions *exclusions, const char *path);\n-void init_ref_exclusions(struct ref_exclusions *);\n-void clear_ref_exclusions(struct ref_exclusions *);\n-void add_ref_exclusion(struct ref_exclusions *, const char *exclude);\n-void exclude_hidden_refs(struct ref_exclusions *, const char *section);\n+int ref_excluded(const struct ref_visibility *exclusions, const char *path);\n+int ref_included(const struct ref_visibility *exclusions, const char *path);\n+void init_ref_visibility(struct ref_visibility *);\n+void clear_ref_visibility(struct ref_visibility *);\n+void add_ref_exclusion(struct ref_visibility *, const char *exclude);\n+void add_ref_inclusion(struct ref_visibility *, const char *exclude);\n+void exclude_hidden_refs(struct ref_visibility *, const char *section);\n \n /**\n * This function can be used if you want to add commit objects as revision\n-- \ngitgitgadget\n\n"},{"id":"477060","messageId":"b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] pack-refs: teach pack-refs --include option","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-11T18:10:34Z","receivedAt":"2023-05-11T18:11:13Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAllow users to be more selective over which refs to pack by adding an\n--include option to git-pack-refs.\n\nThe existing options allow some measure of selectivity. By default\ngit-pack-refs packs all tags. --all can be used to include all refs,\nand the previous commit added the ability to exclude certain refs with\n--exclude.\n\nWhile these knobs give the user some selection over which refs to pack,\nit could be useful to give more control. For instance, a repository may\nhave a set of branches that are rarely updated and would benefit from\nbeing packed. --include would allow the user to easily include a set of\nbranches to be packed while leaving everything else unpacked.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 14 +++++++++++++-\n builtin/pack-refs.c | 18 ++++++++++++++++--\n refs/files-backend.c | 15 +++++++--------\n t/helper/test-ref-store.c | 8 +++++++-\n t/t3210-pack-refs.sh | 21 +++++++++++++++++++++\n 5 files changed, 64 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex c0f7426e519..85874a5f5dc 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune] [--exclude ]\n+'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,15 @@ interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--include ::\n+\n+Pack refs based on a `glob(7)` pattern. Repetitions of this option\n+accumulate inclusion patterns. If a ref is both included in `--include` and\n+`--exclude`, `--exclude` takes precedence. Using `--include` will preclude all\n+tags from being included by default. Symbolic refs and broken refs will never\n+be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n+and reset the list of patterns.\n+\n --exclude ::\n \n Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n@@ -70,6 +79,9 @@ unpack it.\n When used with `--all`, it will use the difference between the set of all refs,\n and what is provided to `--exclude`.\n \n+When used with `--include`, refs provided to `--include`, minus refs that are\n+provided to `--exclude` will be packed.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 2464575a665..5062206f22e 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -5,9 +5,10 @@\n #include \"refs.h\"\n #include \"repository.h\"\n #include \"revision.h\"\n+#include \"trace.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n \tNULL\n };\n \n@@ -15,13 +16,17 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n \tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n-\tstruct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};\n+\tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\n+\t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstatic struct string_list option_included_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"include\", &option_included_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n \t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n@@ -33,5 +38,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &option_excluded_refs)\n \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n \n+\tfor_each_string_list_item(item, &option_included_refs)\n+\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n+\n+\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n+\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n+\n+\tif (!pack_refs_opts.visibility->included_refs.nr)\n+\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n+\n \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3ef19199788..c669cf8001a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1183,13 +1183,6 @@ static int should_pack_ref(const char *refname,\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n-\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n-\t\treturn 0;\n-\n-\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n-\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n-\t\treturn 0;\n-\n \t/* Do not pack symbolic refs: */\n \tif (ref_flags & REF_ISSYMREF)\n \t\treturn 0;\n@@ -1198,7 +1191,13 @@ static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n-\treturn 1;\n+\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n+\t\treturn 0;\n+\n+\tif (opts->visibility && ref_included(opts->visibility, refname))\n+\t\treturn 1;\n+\n+\treturn 0;\n }\n \n static int files_pack_refs(struct ref_store *ref_store,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex de4197708d9..0dec1223362 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -5,6 +5,7 @@\n #include \"worktree.h\"\n #include \"object-store.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n struct flag_definition {\n \tconst char *name;\n@@ -116,7 +117,12 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n-\tstruct pack_refs_opts pack_opts = { .flags = flags };\n+\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n+\tstruct pack_refs_opts pack_opts = { .flags = flags,\n+\t\t\t\t\t .visibility = &visibility };\n+\n+\tif (pack_opts.flags & PACK_REFS_ALL)\n+\t\tadd_ref_inclusion(pack_opts.visibility, \"*\");\n \n \treturn refs_pack_refs(refs, &pack_opts);\n }\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex ddfc1b6e5f1..9ff6326b646 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -124,6 +124,27 @@ test_expect_success 'test --no-exclude refs clears excluded refs' '\n \t! test -f .git/refs/heads/dont_pack3 &&\n \t! test -f .git/refs/heads/dont_pack4'\n \n+test_expect_success 'test only included refs are packed' '\n+\tgit branch pack_this1 &&\n+\tgit branch pack_this2 &&\n+\tgit tag dont_pack5 &&\n+\tgit pack-refs --include \"refs/heads/pack_this*\" &&\n+\ttest -f .git/refs/tags/dont_pack5 &&\n+\t! test -f ./git/refs/heads/pack_this1 &&\n+\t! test -f ./git/refs/heads/pack_this2'\n+\n+test_expect_success 'test --no-include refs clears included refs' '\n+\tgit branch pack1 &&\n+\tgit branch pack2 &&\n+\tgit pack-refs --include \"refs/heads/pack*\" --no-include &&\n+\ttest -f .git/refs/heads/pack1 &&\n+\ttest -f .git/refs/heads/pack2'\n+\n+test_expect_success 'test --exclude takes precedence over --include' '\n+\tgit branch dont_pack5 &&\n+\tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n+\ttest -f .git/refs/heads/dont_pack5'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n"},{"id":"477069","messageId":"xmqqzg6avezm.fsf@gitster.g","threadId":"59694","inReplyTo":"8c5c66a3050ee1c0ce8a48a088f5ecc2df7d1e3a.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T19:34:53Z","receivedAt":"2023-05-11T19:35:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> At GitLab, we have a system that creates ephemeral internal refs that\n> don't live long before getting deleted. Having an option to exclude\n> certain refs from a packed-refs file allows these internal references to\n> be deleted much more efficiently.\n>\n> Add an --exclude option to the pack-refs builtin, and use the ref\n> exclusions API to exclude certain refs from being packed into the final\n> packed-refs file\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/git-pack-refs.txt | 12 +++++++++++-\n> builtin/pack-refs.c | 20 ++++++++++++++++----\n> refs.c | 4 ++--\n> refs.h | 7 ++++++-\n> refs/debug.c | 4 ++--\n> refs/files-backend.c | 16 ++++++++++------\n> refs/packed-backend.c | 2 +-\n> refs/refs-internal.h | 3 ++-\n> revision.h | 2 +-\n> t/helper/test-ref-store.c | 3 ++-\n> t/t3210-pack-refs.sh | 16 ++++++++++++++++\n> 11 files changed, 69 insertions(+), 20 deletions(-)\n\nNice.\n\n> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> index e011e5fead3..c0f7426e519 100644\n> --- a/Documentation/git-pack-refs.txt\n> +++ b/Documentation/git-pack-refs.txt\n> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n> SYNOPSIS\n> --------\n> [verse]\n> -'git pack-refs' [--all] [--no-prune]\n> +'git pack-refs' [--all] [--no-prune] [--exclude ]\n> \n> DESCRIPTION\n> -----------\n> @@ -60,6 +60,16 @@ interests.\n> The command usually removes loose refs under `$GIT_DIR/refs`\n> hierarchy after packing them. This option tells it not to.\n> \n> +--exclude ::\n> +\n> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n> +patterns. If a ref is already packed, including it with `--exclude` will not\n> +unpack it.\n> +\n> +When used with `--all`, it will use the difference between the set of all refs,\n> +and what is provided to `--exclude`.\n> +\n\nJust one question. Does the above get formatted correctly, or does\nthe lack of a line with a sole '+' on it between the paragraphs make\nthe second paragraph look as if it is unrelated to the description\nof the \"--exclude\" option?\n\nOther than that I saw nothing surprising or unexpected in the\npatch. Looking good.\n\nThanks.\n"},{"id":"477078","messageId":"xmqqfs82ve34.fsf@gitster.g","threadId":"59694","inReplyTo":"0a0693ad612755e675861dfa5a660baf8d325ed0.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T19:54:23Z","receivedAt":"2023-05-11T19:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> +static int ref_matched(const char *path,\n> +\t\t const struct string_list *l,\n> +\t\t const struct string_list *hidden_refs)\n> {\n> \tconst char *stripped_path = strip_namespace(path);\n> \tstruct string_list_item *item;\n> \n> -\tfor_each_string_list_item(item, &exclusions->excluded_refs) {\n> +\tfor_each_string_list_item(item, l) {\n> \t\tif (!wildmatch(item->string, path, 0))\n> \t\t\treturn 1;\n> \t}\n> \n> -\tif (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))\n> +\tif (ref_is_hidden(stripped_path, path, hidden_refs))\n> \t\treturn 1;\n> \n> \treturn 0;\n> }\n>\n> -void init_ref_exclusions(struct ref_exclusions *exclusions)\n> +int ref_excluded(const struct ref_visibility *visibility, const char *path)\n> {\n> -\tstruct ref_exclusions blank = REF_EXCLUSIONS_INIT;\n> -\tmemcpy(exclusions, &blank, sizeof(*exclusions));\n> +\treturn ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);\n> }\n> \n> -void clear_ref_exclusions(struct ref_exclusions *exclusions)\n> +int ref_included(const struct ref_visibility *visibility, const char *path)\n> {\n> -\tstring_list_clear(&exclusions->excluded_refs, 0);\n> -\tstring_list_clear(&exclusions->hidden_refs, 0);\n> -\texclusions->hidden_refs_configured = 0;\n> +\treturn ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);\n> }\n\nIt is unexpected that we do \"hidden\" inside ref_matched(). I would\nhave expected that no matter what exclusion or inclusion patterns\nsay, hidden things are to be kept hidden. I.e. I expected\n\n - ref_matched(), which takes a path and a list of patterns, checks\n if the path matches any of the given patterns;\n\n - ref_excluded(), whcih takes a path and a visibility, asks\n ref_matched() if the path matches visibility->excluded_refs and\n relays its answer to the caller.\n\n - ref_included(), which takes a path and a visibility, asks\n ref_matched() if the path matches visibility->included_refs and\n relays its answer to the caller.\n\n - ref_visibility(), which takes a path and a visibility, goes\n through the following sequence:\n\n - if ref_is_hidden() says that the path is hidden, it is not\n visible;\n\n - if ref_excluded() says the path is excluded, it is not visible;\n\n - if ref_included() says the path is included, it is visible;\n\n - if none of the above triggers, the fate of the path is\n determined by some default logic.\n\nor something along that line. That would also allow us to avoid\nhaving to call ref_is_hidden() more than once when we need to check\nboth inclusion and exclusion list.\n\nBut perhaps I am missing some requirements to be taken into\naccount, so let me read on.\n\nTo be honest, I didn't expect the \"exclusions can be extended\",\nwhich I alluded to as a future, possible, follow-on work, to be\nincluded as a part of this topic. I appreciate your going an extra\nmile, but I am not sure if it was a good change in the context of\nthis series. With this patch, it is not trivial to even validate\nthat there is no behaviour change to any users of \"struct\nref_exclusions\" when the included_refs list is empty, which is an\nabsolute must to avoid regressions.\n\n"},{"id":"477083","messageId":"xmqq8rduvdj0.fsf@gitster.g","threadId":"59694","inReplyTo":"b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/4] pack-refs: teach pack-refs --include option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T20:06:27Z","receivedAt":"2023-05-11T20:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> @@ -1198,7 +1191,13 @@ static int should_pack_ref(const char *refname,\n> \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n> \t\treturn 0;\n> \n> -\treturn 1;\n> +\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n> +\t\treturn 0;\n> +\n> +\tif (opts->visibility && ref_included(opts->visibility, refname))\n> +\t\treturn 1;\n> +\n> +\treturn 0;\n> }\n\nWhen the user did not say --exclude or --include, can we not have\nopts->visibility? IOW, can opts->visiblity be NULL?\n\nEven if it can be NULL, shouldn't we be defaulting to \"pack\", as we\nrejected refs to be excluded already?\n\n> @@ -33,5 +38,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> \tfor_each_string_list_item(item, &option_excluded_refs)\n> \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n> \n> +\tfor_each_string_list_item(item, &option_included_refs)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n> +\n> +\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n> +\n> +\tif (!pack_refs_opts.visibility->included_refs.nr)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n\nGiven the above code, I think visibility is always non-NULL, and the\ninclusion list has at least one element. So the above \"what should\nbe the default?\" question is theoretical. But it would be nicer if\nwe do not check opts->visibility there. By writing\n\n\tif (opts->visibility && ref_included(opts->visibility, refname))\n\t\treturn 1;\n\nyou are saying \"if visibility is defined and it is included, say\nYES\", and logically it follows that, if we did not return true from\nhere, we do not know if the end-user supplied inclusion list did not\nmatch (i.e. ref_included() said no), or we skipped the check because\nthe end-user did not supply the visibility rule. It is easy to\nmistake that the next statement after this, i.e. \"return 0;\", is the\ndefault action when the end-user did not give any rule.\n\nBut that is not what is going on. Because visibility is always\ngiven,\n\nThe last part would become\n\n\tif (ref_included(opts->visibility, refname))\n\t\treturn 1;\n\treturn 0;\n\nand the \"return 0\" is no longer confusing. If inclusion check says\nyes, the result is \"to include\", otherwise the result is \"not to\ninclude\". Even shorter, we could say\n\n\treturn !ref_excluded(opts->visibility, refname) &&\n\t\tref_included(opts->visibility, refname) &&\n\nwhich we can trivially read the design decision: exclude list has\npriority, and include list is consulted only after exclude list does\nnot ban it.\n\nThanks.\n"},{"id":"477113","messageId":"ZF1/8w9DBNNrghPN@nand.local","threadId":"59694","inReplyTo":"0d462010b798f834fef55cc7aa767f3f2a177f91.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/4] docs: clarify git-pack-refs --all will pack all refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:53:23Z","receivedAt":"2023-05-11T23:53:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 11, 2023 at 06:10:31PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai \n>\n> --all packs not just branch tips but anything under refs/ with the\n> exception of hidden refs and broken refs. Clarify this in the\n> documentation.\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/git-pack-refs.txt | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> index 154081f2de2..e011e5fead3 100644\n> --- a/Documentation/git-pack-refs.txt\n> +++ b/Documentation/git-pack-refs.txt\n> @@ -49,10 +49,11 @@ OPTIONS\n>\n> The command by default packs all tags and refs that are already\n> packed, and leaves other refs\n> -alone. This is because branches are expected to be actively\n> +alone. This is because branches are expected to be actively\n\nThis looks like a stray change.\n\n> developed and packing their tips does not help performance.\n> -This option causes branch tips to be packed as well. Useful for\n> -a repository with many branches of historical interests.\n> +This option causes all refs to be packed as well, with the exception of hidden\n> +and broken refs. Useful for a repository with many branches of historical\n> +interests.\n\nThe new wording looks good, though you may want to mention that `--all`\ndoes not pack symrefs, either. That's not the fault of your patch,\nthough, since it was the case in the pre-image, as well.\n\nThe wrapping on the new text looks a little off, though.\n\nThanks,\nTaylor\n"},{"id":"477115","messageId":"ZF2BmI/SnhfEDB2K@nand.local","threadId":"59694","inReplyTo":"8c5c66a3050ee1c0ce8a48a088f5ecc2df7d1e3a.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-12T00:00:24Z","receivedAt":"2023-05-12T00:01:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 11, 2023 at 06:10:32PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai \n>\n> At GitLab, we have a system that creates ephemeral internal refs that\n> don't live long before getting deleted. Having an option to exclude\n> certain refs from a packed-refs file allows these internal references to\n> be deleted much more efficiently.\n>\n> Add an --exclude option to the pack-refs builtin, and use the ref\n> exclusions API to exclude certain refs from being packed into the final\n> packed-refs file\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/git-pack-refs.txt | 12 +++++++++++-\n> builtin/pack-refs.c | 20 ++++++++++++++++----\n> refs.c | 4 ++--\n> refs.h | 7 ++++++-\n> refs/debug.c | 4 ++--\n> refs/files-backend.c | 16 ++++++++++------\n> refs/packed-backend.c | 2 +-\n> refs/refs-internal.h | 3 ++-\n> revision.h | 2 +-\n> t/helper/test-ref-store.c | 3 ++-\n> t/t3210-pack-refs.sh | 16 ++++++++++++++++\n> 11 files changed, 69 insertions(+), 20 deletions(-)\n>\n> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> index e011e5fead3..c0f7426e519 100644\n> --- a/Documentation/git-pack-refs.txt\n> +++ b/Documentation/git-pack-refs.txt\n> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n> SYNOPSIS\n> --------\n> [verse]\n> -'git pack-refs' [--all] [--no-prune]\n> +'git pack-refs' [--all] [--no-prune] [--exclude ]\n>\n> DESCRIPTION\n> -----------\n> @@ -60,6 +60,16 @@ interests.\n> The command usually removes loose refs under `$GIT_DIR/refs`\n> hierarchy after packing them. This option tells it not to.\n>\n> +--exclude ::\n> +\n> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n> +patterns. If a ref is already packed, including it with `--exclude` will not\n> +unpack it.\n> +\n> +When used with `--all`, it will use the difference between the set of all refs,\n> +and what is provided to `--exclude`.\n> +\n\nI think this last paragraph could be simplified, though feel free to\ndiscard my suggestion if you think it makes things less clear.\n\n When used with `--all`, pack only loose refs which do not match any of\n the provided `--exclude` patterns.\n\n> int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> {\n> \tunsigned int flags = PACK_REFS_PRUNE;\n> +\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n> +\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n> +\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list_item *item;\n\nSince this list does not appear to be sensitive to its order, have you\nconsidered using the strvec API instead of the string_list one?\n\n> diff --git a/refs.c b/refs.c\n> index d2a98e1c21f..881a0da65cf 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n> }\n>\n> /* backend functions */\n> -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n> +int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n> {\n> -\treturn refs->be->pack_refs(refs, flags);\n> +\treturn refs->be->pack_refs(refs, opts);\n> }\n>\n> int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n> diff --git a/refs.h b/refs.h\n> index 123cfa44244..46020bd335c 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -63,6 +63,11 @@ struct worktree;\n> #define RESOLVE_REF_NO_RECURSE 0x02\n> #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n>\n> +struct pack_refs_opts {\n> +\tunsigned int flags;\n> +\tstruct ref_exclusions *exclusions;\n\nI think this would be OK to include directly in the struct instead of\nvia a pointer, but either is fine.\n\n> @@ -1175,15 +1176,18 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n> */\n> static int should_pack_ref(const char *refname,\n> \t\t\t const struct object_id *oid, unsigned int ref_flags,\n> -\t\t\t unsigned int pack_flags)\n> +\t\t\t struct pack_refs_opts *opts)\n> {\n> \t/* Do not pack per-worktree refs: */\n> \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n> \t REF_WORKTREE_SHARED)\n> \t\treturn 0;\n>\n> +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n> +\t\treturn 0;\n\nLooks good, here is where we throw out refs that we don't want. I wonder\nif ref_excluded() does the right thing with a zero-initialized argument\n(i.e. that it behaves as if nothing matches).\n\nI wonder if it's possible to skip over certain loose references by\navoiding traversal into the sub-directories for simple prefixes. That\nmay be a premature optimization, though, so I don't think you\nnecessarily need to worry about it in this round.\n\n> +test_expect_success 'test excluded refs are not packed' '\n> +\tgit branch dont_pack1 &&\n> +\tgit branch dont_pack2 &&\n> +\tgit branch pack_this &&\n> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n> +\ttest -f .git/refs/heads/dont_pack1 &&\n> +\ttest -f .git/refs/heads/dont_pack2 &&\n> +\t! test -f ./git/refs/heads/pack_this'\n> +\n> +test_expect_success 'test --no-exclude refs clears excluded refs' '\n> +\tgit branch dont_pack3 &&\n> +\tgit branch dont_pack4 &&\n> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n> +\t! test -f .git/refs/heads/dont_pack3 &&\n> +\t! test -f .git/refs/heads/dont_pack4'\n\nTests look good. The trailing quote is a little odd to be placed on the\nlast line of the test instead of off on its own, but I suppose that is\nimitating existing style, which is OK.\n\nThanks,\nTaylor\n"},{"id":"477176","messageId":"7664500D-8C64-46DD-8567-F3AAB532B7EF@gmail.com","threadId":"59694","inReplyTo":"ZF2BmI/SnhfEDB2K@nand.local","subject":"Re: [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-12T12:53:52Z","receivedAt":"2023-05-12T12:54:26Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hey Taylor,\n\nOn 11 May 2023, at 20:00, Taylor Blau wrote:\n\n> On Thu, May 11, 2023 at 06:10:32PM +0000, John Cai via GitGitGadget wrote:\n>> From: John Cai \n>>\n>> At GitLab, we have a system that creates ephemeral internal refs that\n>> don't live long before getting deleted. Having an option to exclude\n>> certain refs from a packed-refs file allows these internal references to\n>> be deleted much more efficiently.\n>>\n>> Add an --exclude option to the pack-refs builtin, and use the ref\n>> exclusions API to exclude certain refs from being packed into the final\n>> packed-refs file\n>>\n>> Signed-off-by: John Cai \n>> ---\n>> Documentation/git-pack-refs.txt | 12 +++++++++++-\n>> builtin/pack-refs.c | 20 ++++++++++++++++----\n>> refs.c | 4 ++--\n>> refs.h | 7 ++++++-\n>> refs/debug.c | 4 ++--\n>> refs/files-backend.c | 16 ++++++++++------\n>> refs/packed-backend.c | 2 +-\n>> refs/refs-internal.h | 3 ++-\n>> revision.h | 2 +-\n>> t/helper/test-ref-store.c | 3 ++-\n>> t/t3210-pack-refs.sh | 16 ++++++++++++++++\n>> 11 files changed, 69 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n>> index e011e5fead3..c0f7426e519 100644\n>> --- a/Documentation/git-pack-refs.txt\n>> +++ b/Documentation/git-pack-refs.txt\n>> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n>> SYNOPSIS\n>> --------\n>> [verse]\n>> -'git pack-refs' [--all] [--no-prune]\n>> +'git pack-refs' [--all] [--no-prune] [--exclude ]\n>>\n>> DESCRIPTION\n>> -----------\n>> @@ -60,6 +60,16 @@ interests.\n>> The command usually removes loose refs under `$GIT_DIR/refs`\n>> hierarchy after packing them. This option tells it not to.\n>>\n>> +--exclude ::\n>> +\n>> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n>> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n>> +patterns. If a ref is already packed, including it with `--exclude` will not\n>> +unpack it.\n>> +\n>> +When used with `--all`, it will use the difference between the set of all refs,\n>> +and what is provided to `--exclude`.\n>> +\n>\n> I think this last paragraph could be simplified, though feel free to\n> discard my suggestion if you think it makes things less clear.\n>\n> When used with `--all`, pack only loose refs which do not match any of\n> the provided `--exclude` patterns.\n\nI like the wording here, thanks\n\n>\n>> int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n>> {\n>> \tunsigned int flags = PACK_REFS_PRUNE;\n>> +\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n>> +\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n>> +\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n>> +\tstruct string_list_item *item;\n>\n> Since this list does not appear to be sensitive to its order, have you\n> considered using the strvec API instead of the string_list one?\n\nThanks for this suggestion--you're right in that the order doesn't matter here.\nThe only thing is, the only option parsing macro I could find that operates on\nstrvec is OPT_PASSTHRU_ARGV. I tried it out, and it seem to work just fine.\n\n>\n>> diff --git a/refs.c b/refs.c\n>> index d2a98e1c21f..881a0da65cf 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n>> }\n>>\n>> /* backend functions */\n>> -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n>> +int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n>> {\n>> -\treturn refs->be->pack_refs(refs, flags);\n>> +\treturn refs->be->pack_refs(refs, opts);\n>> }\n>>\n>> int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n>> diff --git a/refs.h b/refs.h\n>> index 123cfa44244..46020bd335c 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -63,6 +63,11 @@ struct worktree;\n>> #define RESOLVE_REF_NO_RECURSE 0x02\n>> #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n>>\n>> +struct pack_refs_opts {\n>> +\tunsigned int flags;\n>> +\tstruct ref_exclusions *exclusions;\n>\n> I think this would be OK to include directly in the struct instead of\n> via a pointer, but either is fine.\n>\n>> @@ -1175,15 +1176,18 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n>> */\n>> static int should_pack_ref(const char *refname,\n>> \t\t\t const struct object_id *oid, unsigned int ref_flags,\n>> -\t\t\t unsigned int pack_flags)\n>> +\t\t\t struct pack_refs_opts *opts)\n>> {\n>> \t/* Do not pack per-worktree refs: */\n>> \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n>> \t REF_WORKTREE_SHARED)\n>> \t\treturn 0;\n>>\n>> +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n>> +\t\treturn 0;\n>\n> Looks good, here is where we throw out refs that we don't want. I wonder\n> if ref_excluded() does the right thing with a zero-initialized argument\n> (i.e. that it behaves as if nothing matches).\n\nYes, I think we can skip checking if opt->exclusions is not null. Junio had\nfeedback around this as well.\n\n>\n> I wonder if it's possible to skip over certain loose references by\n> avoiding traversal into the sub-directories for simple prefixes. That\n> may be a premature optimization, though, so I don't think you\n> necessarily need to worry about it in this round.\n>\n>> +test_expect_success 'test excluded refs are not packed' '\n>> +\tgit branch dont_pack1 &&\n>> +\tgit branch dont_pack2 &&\n>> +\tgit branch pack_this &&\n>> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n>> +\ttest -f .git/refs/heads/dont_pack1 &&\n>> +\ttest -f .git/refs/heads/dont_pack2 &&\n>> +\t! test -f ./git/refs/heads/pack_this'\n>> +\n>> +test_expect_success 'test --no-exclude refs clears excluded refs' '\n>> +\tgit branch dont_pack3 &&\n>> +\tgit branch dont_pack4 &&\n>> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n>> +\t! test -f .git/refs/heads/dont_pack3 &&\n>> +\t! test -f .git/refs/heads/dont_pack4'\n>\n> Tests look good. The trailing quote is a little odd to be placed on the\n> last line of the test instead of off on its own, but I suppose that is\n> imitating existing style, which is OK.\n\nthanks for the feedback!\nJohn\n>\n> Thanks,\n> Taylor\n"},{"id":"477178","messageId":"80E985AF-40B2-46F4-8873-4FA4A18B881F@gmail.com","threadId":"59694","inReplyTo":"xmqq8rduvdj0.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] pack-refs: teach pack-refs --include option","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-12T14:48:00Z","receivedAt":"2023-05-12T14:48:08Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 11 May 2023, at 16:06, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> @@ -1198,7 +1191,13 @@ static int should_pack_ref(const char *refname,\n>> \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n>> \t\treturn 0;\n>>\n>> -\treturn 1;\n>> +\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n>> +\t\treturn 0;\n>> +\n>> +\tif (opts->visibility && ref_included(opts->visibility, refname))\n>> +\t\treturn 1;\n>> +\n>> +\treturn 0;\n>> }\n>\n> When the user did not say --exclude or --include, can we not have\n> opts->visibility? IOW, can opts->visiblity be NULL?\n>\n> Even if it can be NULL, shouldn't we be defaulting to \"pack\", as we\n> rejected refs to be excluded already?\n>\n>> @@ -33,5 +38,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n>> \tfor_each_string_list_item(item, &option_excluded_refs)\n>> \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n>>\n>> +\tfor_each_string_list_item(item, &option_included_refs)\n>> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n>> +\n>> +\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n>> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n>> +\n>> +\tif (!pack_refs_opts.visibility->included_refs.nr)\n>> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n>\n> Given the above code, I think visibility is always non-NULL, and the\n> inclusion list has at least one element. So the above \"what should\n> be the default?\" question is theoretical. But it would be nicer if\n> we do not check opts->visibility there. By writing\n>\n> \tif (opts->visibility && ref_included(opts->visibility, refname))\n> \t\treturn 1;\n>\n> you are saying \"if visibility is defined and it is included, say\n> YES\", and logically it follows that, if we did not return true from\n> here, we do not know if the end-user supplied inclusion list did not\n> match (i.e. ref_included() said no), or we skipped the check because\n> the end-user did not supply the visibility rule. It is easy to\n> mistake that the next statement after this, i.e. \"return 0;\", is the\n> default action when the end-user did not give any rule.\n>\n> But that is not what is going on. Because visibility is always\n> given,\n>\n> The last part would become\n>\n> \tif (ref_included(opts->visibility, refname))\n> \t\treturn 1;\n> \treturn 0;\n>\n> and the \"return 0\" is no longer confusing. If inclusion check says\n> yes, the result is \"to include\", otherwise the result is \"not to\n> include\". Even shorter, we could say\n>\n> \treturn !ref_excluded(opts->visibility, refname) &&\n> \t\tref_included(opts->visibility, refname) &&\n>\n> which we can trivially read the design decision: exclude list has\n> priority, and include list is consulted only after exclude list does\n> not ban it.\n\nYes, this is the logic. I agree that getting rid of the opts->visibility check\nwould make it more clear.\n\nthanks\nJohn\n\n>\n> Thanks.\n"},{"id":"477181","messageId":"866E98A8-E622-43A3-BCEC-7B6E46D49D14@gmail.com","threadId":"59694","inReplyTo":"xmqqfs82ve34.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] revision: modify ref_exclusions to handle inclusions","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-12T14:56:03Z","receivedAt":"2023-05-12T14:56:33Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 11 May 2023, at 15:54, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> +static int ref_matched(const char *path,\n>> +\t\t const struct string_list *l,\n>> +\t\t const struct string_list *hidden_refs)\n>> {\n>> \tconst char *stripped_path = strip_namespace(path);\n>> \tstruct string_list_item *item;\n>>\n>> -\tfor_each_string_list_item(item, &exclusions->excluded_refs) {\n>> +\tfor_each_string_list_item(item, l) {\n>> \t\tif (!wildmatch(item->string, path, 0))\n>> \t\t\treturn 1;\n>> \t}\n>>\n>> -\tif (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))\n>> +\tif (ref_is_hidden(stripped_path, path, hidden_refs))\n>> \t\treturn 1;\n>>\n>> \treturn 0;\n>> }\n>>\n>> -void init_ref_exclusions(struct ref_exclusions *exclusions)\n>> +int ref_excluded(const struct ref_visibility *visibility, const char *path)\n>> {\n>> -\tstruct ref_exclusions blank = REF_EXCLUSIONS_INIT;\n>> -\tmemcpy(exclusions, &blank, sizeof(*exclusions));\n>> +\treturn ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);\n>> }\n>>\n>> -void clear_ref_exclusions(struct ref_exclusions *exclusions)\n>> +int ref_included(const struct ref_visibility *visibility, const char *path)\n>> {\n>> -\tstring_list_clear(&exclusions->excluded_refs, 0);\n>> -\tstring_list_clear(&exclusions->hidden_refs, 0);\n>> -\texclusions->hidden_refs_configured = 0;\n>> +\treturn ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);\n>> }\n>\n> It is unexpected that we do \"hidden\" inside ref_matched(). I would\n> have expected that no matter what exclusion or inclusion patterns\n> say, hidden things are to be kept hidden. I.e. I expected\n\nOops. Yes this was an oversight.\n\n>\n> - ref_matched(), which takes a path and a list of patterns, checks\n> if the path matches any of the given patterns;\n>\n> - ref_excluded(), whcih takes a path and a visibility, asks\n> ref_matched() if the path matches visibility->excluded_refs and\n> relays its answer to the caller.\n>\n> - ref_included(), which takes a path and a visibility, asks\n> ref_matched() if the path matches visibility->included_refs and\n> relays its answer to the caller.\n>\n> - ref_visibility(), which takes a path and a visibility, goes\n> through the following sequence:\n>\n> - if ref_is_hidden() says that the path is hidden, it is not\n> visible;\n>\n> - if ref_excluded() says the path is excluded, it is not visible;\n>\n> - if ref_included() says the path is included, it is visible;\n>\n> - if none of the above triggers, the fate of the path is\n> determined by some default logic.\n\nThat's a good point. I didn't think about adding a ref_visibility() function\nthat conslidates inclusion and exclusion.\n>\n> or something along that line. That would also allow us to avoid\n> having to call ref_is_hidden() more than once when we need to check\n> both inclusion and exclusion list.\n>\n> But perhaps I am missing some requirements to be taken into\n> account, so let me read on.\n>\n> To be honest, I didn't expect the \"exclusions can be extended\",\n> which I alluded to as a future, possible, follow-on work, to be\n> included as a part of this topic. I appreciate your going an extra\n> mile, but I am not sure if it was a good change in the context of\n> this series. With this patch, it is not trivial to even validate\n> that there is no behaviour change to any users of \"struct\n> ref_exclusions\" when the included_refs list is empty, which is an\n> absolute must to avoid regressions.\n\nOkay, maybe we can include this refactor in a subsequent series then.\n\nthanks\nJohn\n"},{"id":"477200","messageId":"4086BA28-57C5-43FF-BD3D-D4731ACD6E64@gmail.com","threadId":"59694","inReplyTo":"b2f3b98cd2461a25ab708adbcd8a95f5e2b18e5e.1683828635.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/4] pack-refs: teach pack-refs --include option","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-12T19:03:59Z","receivedAt":"2023-05-12T19:04:05Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\nOn 11 May 2023, at 14:10, John Cai via GitGitGadget wrote:\n\n> From: John Cai \n>\n> Allow users to be more selective over which refs to pack by adding an\n> --include option to git-pack-refs.\n>\n> The existing options allow some measure of selectivity. By default\n> git-pack-refs packs all tags. --all can be used to include all refs,\n> and the previous commit added the ability to exclude certain refs with\n> --exclude.\n>\n> While these knobs give the user some selection over which refs to pack,\n> it could be useful to give more control. For instance, a repository may\n> have a set of branches that are rarely updated and would benefit from\n> being packed. --include would allow the user to easily include a set of\n> branches to be packed while leaving everything else unpacked.\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/git-pack-refs.txt | 14 +++++++++++++-\n> builtin/pack-refs.c | 18 ++++++++++++++++--\n> refs/files-backend.c | 15 +++++++--------\n> t/helper/test-ref-store.c | 8 +++++++-\n> t/t3210-pack-refs.sh | 21 +++++++++++++++++++++\n> 5 files changed, 64 insertions(+), 12 deletions(-)\n>\n> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n> index c0f7426e519..85874a5f5dc 100644\n> --- a/Documentation/git-pack-refs.txt\n> +++ b/Documentation/git-pack-refs.txt\n> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n> SYNOPSIS\n> --------\n> [verse]\n> -'git pack-refs' [--all] [--no-prune] [--exclude ]\n> +'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n>\n> DESCRIPTION\n> -----------\n> @@ -60,6 +60,15 @@ interests.\n> The command usually removes loose refs under `$GIT_DIR/refs`\n> hierarchy after packing them. This option tells it not to.\n>\n> +--include ::\n> +\n> +Pack refs based on a `glob(7)` pattern. Repetitions of this option\n> +accumulate inclusion patterns. If a ref is both included in `--include` and\n> +`--exclude`, `--exclude` takes precedence. Using `--include` will preclude all\n> +tags from being included by default. Symbolic refs and broken refs will never\n> +be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n> +and reset the list of patterns.\n> +\n> --exclude ::\n>\n> Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n> @@ -70,6 +79,9 @@ unpack it.\n> When used with `--all`, it will use the difference between the set of all refs,\n> and what is provided to `--exclude`.\n>\n> +When used with `--include`, refs provided to `--include`, minus refs that are\n> +provided to `--exclude` will be packed.\n> +\n>\n> BUGS\n> ----\n> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\n> index 2464575a665..5062206f22e 100644\n> --- a/builtin/pack-refs.c\n> +++ b/builtin/pack-refs.c\n> @@ -5,9 +5,10 @@\n> #include \"refs.h\"\n> #include \"repository.h\"\n> #include \"revision.h\"\n> +#include \"trace.h\"\n>\n> static char const * const pack_refs_usage[] = {\n> -\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n> +\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n> \tNULL\n> };\n>\n> @@ -15,13 +16,17 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> {\n> \tunsigned int flags = PACK_REFS_PRUNE;\n> \tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n> -\tstruct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};\n> +\tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\n> +\t\t\t\t\t\t .flags = flags };\n> \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n> +\tstatic struct string_list option_included_refs = STRING_LIST_INIT_NODUP;\n> \tstruct string_list_item *item;\n>\n> \tstruct option opts[] = {\n> \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n> \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n> +\t\tOPT_STRING_LIST(0, \"include\", &option_included_refs, N_(\"pattern\"),\n> +\t\t\tN_(\"references to include\")),\n> \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n> \t\t\tN_(\"references to exclude\")),\n> \t\tOPT_END(),\n> @@ -33,5 +38,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> \tfor_each_string_list_item(item, &option_excluded_refs)\n> \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n>\n> +\tfor_each_string_list_item(item, &option_included_refs)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n> +\n> +\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n> +\n> +\tif (!pack_refs_opts.visibility->included_refs.nr)\n> +\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n> +\n> \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n> }\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 3ef19199788..c669cf8001a 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1183,13 +1183,6 @@ static int should_pack_ref(const char *refname,\n> \t REF_WORKTREE_SHARED)\n> \t\treturn 0;\n>\n> -\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n> -\t\treturn 0;\n> -\n> -\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n> -\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n> -\t\treturn 0;\n> -\n> \t/* Do not pack symbolic refs: */\n> \tif (ref_flags & REF_ISSYMREF)\n> \t\treturn 0;\n> @@ -1198,7 +1191,13 @@ static int should_pack_ref(const char *refname,\n> \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n> \t\treturn 0;\n>\n> -\treturn 1;\n> +\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n> +\t\treturn 0;\n> +\n> +\tif (opts->visibility && ref_included(opts->visibility, refname))\n> +\t\treturn 1;\n> +\n> +\treturn 0;\n> }\n>\n> static int files_pack_refs(struct ref_store *ref_store,\n> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\n> index de4197708d9..0dec1223362 100644\n> --- a/t/helper/test-ref-store.c\n> +++ b/t/helper/test-ref-store.c\n> @@ -5,6 +5,7 @@\n> #include \"worktree.h\"\n> #include \"object-store.h\"\n> #include \"repository.h\"\n> +#include \"revision.h\"\n>\n> struct flag_definition {\n> \tconst char *name;\n> @@ -116,7 +117,12 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n> static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n> {\n> \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n> -\tstruct pack_refs_opts pack_opts = { .flags = flags };\n> +\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n> +\tstruct pack_refs_opts pack_opts = { .flags = flags,\n> +\t\t\t\t\t .visibility = &visibility };\n> +\n> +\tif (pack_opts.flags & PACK_REFS_ALL)\n> +\t\tadd_ref_inclusion(pack_opts.visibility, \"*\");\n\nI was wondering about this test function. I had to add this in because now we\nare no longer checking the PACK_REFS_ALL flag in refs/files-backend.c and\ninstead relying on the inclusions data structure.\n\nHowever, this does not support --exclude and --include options. With the current\nplumbing code in this file, it doesn't look like it supports options with values\nas it uses a bitmask.\n\nMy question is, do we need to add support for every option we add to\ngit-pack-refs here? Or are the changes in this patch sufficient.\n\nthanks\nJohn\n\n>\n> \treturn refs_pack_refs(refs, &pack_opts);\n> }\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index ddfc1b6e5f1..9ff6326b646 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -124,6 +124,27 @@ test_expect_success 'test --no-exclude refs clears excluded refs' '\n> \t! test -f .git/refs/heads/dont_pack3 &&\n> \t! test -f .git/refs/heads/dont_pack4'\n>\n> +test_expect_success 'test only included refs are packed' '\n> +\tgit branch pack_this1 &&\n> +\tgit branch pack_this2 &&\n> +\tgit tag dont_pack5 &&\n> +\tgit pack-refs --include \"refs/heads/pack_this*\" &&\n> +\ttest -f .git/refs/tags/dont_pack5 &&\n> +\t! test -f ./git/refs/heads/pack_this1 &&\n> +\t! test -f ./git/refs/heads/pack_this2'\n> +\n> +test_expect_success 'test --no-include refs clears included refs' '\n> +\tgit branch pack1 &&\n> +\tgit branch pack2 &&\n> +\tgit pack-refs --include \"refs/heads/pack*\" --no-include &&\n> +\ttest -f .git/refs/heads/pack1 &&\n> +\ttest -f .git/refs/heads/pack2'\n> +\n> +test_expect_success 'test --exclude takes precedence over --include' '\n> +\tgit branch dont_pack5 &&\n> +\tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n> +\ttest -f .git/refs/heads/dont_pack5'\n> +\n> test_expect_success \\\n> \t'see if up-to-date packed refs are preserved' \\\n> \t'git branch q &&\n> -- \n> gitgitgadget\n"},{"id":"477217","messageId":"5D8B0ED5-F122-4E96-9ED4-5D21C0A392A1@gmail.com","threadId":"59694","inReplyTo":"7664500D-8C64-46DD-8567-F3AAB532B7EF@gmail.com","subject":"Re: [PATCH v3 2/4] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-05-12T21:11:34Z","receivedAt":"2023-05-12T21:14:25Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\nOn 12 May 2023, at 8:53, John Cai wrote:\n\n> Hey Taylor,\n>\n> On 11 May 2023, at 20:00, Taylor Blau wrote:\n>\n>> On Thu, May 11, 2023 at 06:10:32PM +0000, John Cai via GitGitGadget wrote:\n>>> From: John Cai \n>>>\n>>> At GitLab, we have a system that creates ephemeral internal refs that\n>>> don't live long before getting deleted. Having an option to exclude\n>>> certain refs from a packed-refs file allows these internal references to\n>>> be deleted much more efficiently.\n>>>\n>>> Add an --exclude option to the pack-refs builtin, and use the ref\n>>> exclusions API to exclude certain refs from being packed into the final\n>>> packed-refs file\n>>>\n>>> Signed-off-by: John Cai \n>>> ---\n>>> Documentation/git-pack-refs.txt | 12 +++++++++++-\n>>> builtin/pack-refs.c | 20 ++++++++++++++++----\n>>> refs.c | 4 ++--\n>>> refs.h | 7 ++++++-\n>>> refs/debug.c | 4 ++--\n>>> refs/files-backend.c | 16 ++++++++++------\n>>> refs/packed-backend.c | 2 +-\n>>> refs/refs-internal.h | 3 ++-\n>>> revision.h | 2 +-\n>>> t/helper/test-ref-store.c | 3 ++-\n>>> t/t3210-pack-refs.sh | 16 ++++++++++++++++\n>>> 11 files changed, 69 insertions(+), 20 deletions(-)\n>>>\n>>> diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\n>>> index e011e5fead3..c0f7426e519 100644\n>>> --- a/Documentation/git-pack-refs.txt\n>>> +++ b/Documentation/git-pack-refs.txt\n>>> @@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n>>> SYNOPSIS\n>>> --------\n>>> [verse]\n>>> -'git pack-refs' [--all] [--no-prune]\n>>> +'git pack-refs' [--all] [--no-prune] [--exclude ]\n>>>\n>>> DESCRIPTION\n>>> -----------\n>>> @@ -60,6 +60,16 @@ interests.\n>>> The command usually removes loose refs under `$GIT_DIR/refs`\n>>> hierarchy after packing them. This option tells it not to.\n>>>\n>>> +--exclude ::\n>>> +\n>>> +Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n>>> +accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n>>> +patterns. If a ref is already packed, including it with `--exclude` will not\n>>> +unpack it.\n>>> +\n>>> +When used with `--all`, it will use the difference between the set of all refs,\n>>> +and what is provided to `--exclude`.\n>>> +\n>>\n>> I think this last paragraph could be simplified, though feel free to\n>> discard my suggestion if you think it makes things less clear.\n>>\n>> When used with `--all`, pack only loose refs which do not match any of\n>> the provided `--exclude` patterns.\n>\n> I like the wording here, thanks\n>\n>>\n>>> int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n>>> {\n>>> \tunsigned int flags = PACK_REFS_PRUNE;\n>>> +\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n>>> +\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n>>> +\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n>>> +\tstruct string_list_item *item;\n>>\n>> Since this list does not appear to be sensitive to its order, have you\n>> considered using the strvec API instead of the string_list one?\n>\n> Thanks for this suggestion--you're right in that the order doesn't matter here.\n> The only thing is, the only option parsing macro I could find that operates on\n> strvec is OPT_PASSTHRU_ARGV. I tried it out, and it seem to work just fine.\n\nActually I had a bug in my test that made it seem like it worked. I realize that\nOPT_PASSTHRU_ARGV passes through the entire option flag eg\n--include=refs/tags/dont_pack* so we would have to strip the flag name. It's not\nas user friendly as OPT_STRING_LIST that is just plug and play.\n\nUnless there is a significant performance improvement in using strvec, I'm\nthinking maybe I'll just stick to string_list.\n\nthanks\nJohn\n\n>\n>>\n>>> diff --git a/refs.c b/refs.c\n>>> index d2a98e1c21f..881a0da65cf 100644\n>>> --- a/refs.c\n>>> +++ b/refs.c\n>>> @@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n>>> }\n>>>\n>>> /* backend functions */\n>>> -int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n>>> +int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n>>> {\n>>> -\treturn refs->be->pack_refs(refs, flags);\n>>> +\treturn refs->be->pack_refs(refs, opts);\n>>> }\n>>>\n>>> int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n>>> diff --git a/refs.h b/refs.h\n>>> index 123cfa44244..46020bd335c 100644\n>>> --- a/refs.h\n>>> +++ b/refs.h\n>>> @@ -63,6 +63,11 @@ struct worktree;\n>>> #define RESOLVE_REF_NO_RECURSE 0x02\n>>> #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n>>>\n>>> +struct pack_refs_opts {\n>>> +\tunsigned int flags;\n>>> +\tstruct ref_exclusions *exclusions;\n>>\n>> I think this would be OK to include directly in the struct instead of\n>> via a pointer, but either is fine.\n>>\n>>> @@ -1175,15 +1176,18 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n>>> */\n>>> static int should_pack_ref(const char *refname,\n>>> \t\t\t const struct object_id *oid, unsigned int ref_flags,\n>>> -\t\t\t unsigned int pack_flags)\n>>> +\t\t\t struct pack_refs_opts *opts)\n>>> {\n>>> \t/* Do not pack per-worktree refs: */\n>>> \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n>>> \t REF_WORKTREE_SHARED)\n>>> \t\treturn 0;\n>>>\n>>> +\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n>>> +\t\treturn 0;\n>>\n>> Looks good, here is where we throw out refs that we don't want. I wonder\n>> if ref_excluded() does the right thing with a zero-initialized argument\n>> (i.e. that it behaves as if nothing matches).\n>\n> Yes, I think we can skip checking if opt->exclusions is not null. Junio had\n> feedback around this as well.\n>\n>>\n>> I wonder if it's possible to skip over certain loose references by\n>> avoiding traversal into the sub-directories for simple prefixes. That\n>> may be a premature optimization, though, so I don't think you\n>> necessarily need to worry about it in this round.\n>>\n>>> +test_expect_success 'test excluded refs are not packed' '\n>>> +\tgit branch dont_pack1 &&\n>>> +\tgit branch dont_pack2 &&\n>>> +\tgit branch pack_this &&\n>>> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n>>> +\ttest -f .git/refs/heads/dont_pack1 &&\n>>> +\ttest -f .git/refs/heads/dont_pack2 &&\n>>> +\t! test -f ./git/refs/heads/pack_this'\n>>> +\n>>> +test_expect_success 'test --no-exclude refs clears excluded refs' '\n>>> +\tgit branch dont_pack3 &&\n>>> +\tgit branch dont_pack4 &&\n>>> +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n>>> +\t! test -f .git/refs/heads/dont_pack3 &&\n>>> +\t! test -f .git/refs/heads/dont_pack4'\n>>\n>> Tests look good. The trailing quote is a little odd to be placed on the\n>> last line of the test instead of off on its own, but I suppose that is\n>> imitating existing style, which is OK.\n>\n> thanks for the feedback!\n> John\n>>\n>> Thanks,\n>> Taylor\n"},{"id":"477222","messageId":"69300845df151f7deaf76f57ef3e796217a0c67b.1683927282.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v4.git.git.1683927282.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] pack-refs: teach --exclude option to exclude refs from being packed","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-12T21:34:41Z","receivedAt":"2023-05-12T21:34:58Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAt GitLab, we have a system that creates ephemeral internal refs that\ndon't live long before getting deleted. Having an option to exclude\ncertain refs from a packed-refs file allows these internal references to\nbe deleted much more efficiently.\n\nAdd an --exclude option to the pack-refs builtin, and use the ref\nexclusions API to exclude certain refs from being packed into the final\npacked-refs file\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 12 +++++++++++-\n builtin/pack-refs.c | 20 ++++++++++++++++----\n refs.c | 4 ++--\n refs.h | 7 ++++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 16 ++++++++++------\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 ++-\n revision.h | 2 +-\n t/helper/test-ref-store.c | 3 ++-\n t/t3210-pack-refs.sh | 16 ++++++++++++++++\n 11 files changed, 69 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 22f00665006..546aa122dff 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune]\n+'git pack-refs' [--all] [--no-prune] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,16 @@ with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--exclude ::\n+\n+Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n+accumulate exclusion patterns. Use `--no-exclude` to clear and reset the list of\n+patterns. If a ref is already packed, including it with `--exclude` will not\n+unpack it.\n+\n+When used with `--all`, pack only loose refs which do not match any of\n+the provided `--exclude` patterns.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 9833815fb30..1d1a64fe386 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -4,22 +4,34 @@\n #include \"parse-options.h\"\n #include \"refs.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n \tNULL\n };\n \n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n+\tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n+\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\n \tstruct option opts[] = {\n-\t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n-\t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n+\t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n+\t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n \t};\n \tgit_config(git_default_config, NULL);\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn refs_pack_refs(get_main_ref_store(the_repository), flags);\n+\n+\tfor_each_string_list_item(item, &option_excluded_refs)\n+\t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n+\n+\treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/refs.c b/refs.c\nindex d2a98e1c21f..881a0da65cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2132,9 +2132,9 @@ void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n }\n \n /* backend functions */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts)\n {\n-\treturn refs->be->pack_refs(refs, flags);\n+\treturn refs->be->pack_refs(refs, opts);\n }\n \n int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\ndiff --git a/refs.h b/refs.h\nindex 123cfa44244..46020bd335c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -63,6 +63,11 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+struct pack_refs_opts {\n+\tunsigned int flags;\n+\tstruct ref_exclusions *exclusions;\n+};\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -405,7 +410,7 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n */\n-int refs_pack_refs(struct ref_store *refs, unsigned int flags);\n+int refs_pack_refs(struct ref_store *refs, struct pack_refs_opts *opts);\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 6f11e6de46c..c0fa707a1da 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -123,10 +123,10 @@ static int debug_initial_transaction_commit(struct ref_store *refs,\n \treturn res;\n }\n \n-static int debug_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *opts)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n-\tint res = drefs->refs->be->pack_refs(drefs->refs, flags);\n+\tint res = drefs->refs->be->pack_refs(drefs->refs, opts);\n \ttrace_printf_key(&trace_refs, \"pack_refs: %d\\n\", res);\n \treturn res;\n }\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bca7b851c5a..5075e6c0d28 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -21,6 +21,7 @@\n #include \"../worktree.h\"\n #include \"../wrapper.h\"\n #include \"../write-or-die.h\"\n+#include \"../revision.h\"\n \n /*\n * This backend uses the following flags in `ref_update::flags` for\n@@ -1175,15 +1176,18 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_\n */\n static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n-\t\t\t unsigned int pack_flags)\n+\t\t\t struct pack_refs_opts *opts)\n {\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n+\tif (ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n-\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n+\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n \t\treturn 0;\n \n \t/* Do not pack symbolic refs: */\n@@ -1197,7 +1201,8 @@ static int should_pack_ref(const char *refname,\n \treturn 1;\n }\n \n-static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+static int files_pack_refs(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n@@ -1222,8 +1227,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * in the packed ref cache. If the reference should be\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n-\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n-\t\t\t\t flags))\n+\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags, opts))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -1237,7 +1241,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\t iter->refname, err.buf);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n-\t\tif ((flags & PACK_REFS_PRUNE)) {\n+\t\tif ((opts->flags & PACK_REFS_PRUNE)) {\n \t\t\tstruct ref_to_prune *n;\n \t\t\tFLEX_ALLOC_STR(n, name, iter->refname);\n \t\t\toidcpy(&n->oid, iter->oid);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 5b412a133be..291e53f5cfe 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1577,7 +1577,7 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n }\n \n static int packed_pack_refs(struct ref_store *ref_store UNUSED,\n-\t\t\t unsigned int flags UNUSED)\n+\t\t\t struct pack_refs_opts *pack_opts UNUSED)\n {\n \t/*\n \t * Packed refs are already packed. It might be that loose refs\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex a85d113123c..f72b7be8941 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -547,7 +547,8 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n-typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n+typedef int pack_refs_fn(struct ref_store *ref_store,\n+\t\t\t struct pack_refs_opts *opts);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\ndiff --git a/revision.h b/revision.h\nindex 31828748dc0..25776af3815 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -87,7 +87,7 @@ struct rev_cmdline_info {\n struct ref_exclusions {\n \t/*\n \t * Excluded refs is a list of wildmatch patterns. If any of the\n-\t * patterns matches, the reference will be excluded.\n+\t * patterns match, the reference will be excluded.\n \t */\n \tstruct string_list excluded_refs;\n \ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 6d8f844e9c7..de4197708d9 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -116,8 +116,9 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n+\tstruct pack_refs_opts pack_opts = { .flags = flags };\n \n-\treturn refs_pack_refs(refs, flags);\n+\treturn refs_pack_refs(refs, &pack_opts);\n }\n \n static int cmd_create_symref(struct ref_store *refs, const char **argv)\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 07a0ff93def..925b90cd3ba 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -108,6 +108,22 @@ test_expect_success \\\n git branch -d n/o/p &&\n git branch n'\n \n+test_expect_success 'test excluded refs are not packed' '\n+\tgit branch dont_pack1 &&\n+\tgit branch dont_pack2 &&\n+\tgit branch pack_this &&\n+\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n+\ttest -f .git/refs/heads/dont_pack1 &&\n+\ttest -f .git/refs/heads/dont_pack2 &&\n+\t! test -f .git/refs/heads/pack_this'\n+\n+test_expect_success 'test --no-exclude refs clears excluded refs' '\n+\tgit branch dont_pack3 &&\n+\tgit branch dont_pack4 &&\n+\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n+\t! test -f .git/refs/heads/dont_pack3 &&\n+\t! test -f .git/refs/heads/dont_pack4'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n\n"},{"id":"477223","messageId":"4bbe4c05cebf5cb15310807d3998eb93a481470e.1683927282.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v4.git.git.1683927282.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] pack-refs: teach pack-refs --include option","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-12T21:34:42Z","receivedAt":"2023-05-12T21:35:01Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nAllow users to be more selective over which refs to pack by adding an\n--include option to git-pack-refs.\n\nThe existing options allow some measure of selectivity. By default\ngit-pack-refs packs all tags. --all can be used to include all refs,\nand the previous commit added the ability to exclude certain refs with\n--exclude.\n\nWhile these knobs give the user some selection over which refs to pack,\nit could be useful to give more control. For instance, a repository may\nhave a set of branches that are rarely updated and would benefit from\nbeing packed. --include would allow the user to easily include a set of\nbranches to be packed while leaving everything else unpacked.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 14 +++++++++++++-\n builtin/pack-refs.c | 15 +++++++++++++--\n refs.h | 1 +\n refs/files-backend.c | 18 ++++++++++--------\n t/helper/test-ref-store.c | 10 +++++++++-\n t/t3210-pack-refs.sh | 21 +++++++++++++++++++++\n 6 files changed, 67 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 546aa122dff..284956acb3c 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -8,7 +8,7 @@ git-pack-refs - Pack heads and tags for efficient repository access\n SYNOPSIS\n --------\n [verse]\n-'git pack-refs' [--all] [--no-prune] [--exclude ]\n+'git pack-refs' [--all] [--no-prune] [--include ] [--exclude ]\n \n DESCRIPTION\n -----------\n@@ -60,6 +60,15 @@ with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n+--include ::\n+\n+Pack refs based on a `glob(7)` pattern. Repetitions of this option\n+accumulate inclusion patterns. If a ref is both included in `--include` and\n+`--exclude`, `--exclude` takes precedence. Using `--include` will preclude all\n+tags from being included by default. Symbolic refs and broken refs will never\n+be packed. When used with `--all`, it will be a noop. Use `--no-include` to clear\n+and reset the list of patterns.\n+\n --exclude ::\n \n Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n@@ -70,6 +79,9 @@ unpack it.\n When used with `--all`, pack only loose refs which do not match any of\n the provided `--exclude` patterns.\n \n+When used with `--include`, refs provided to `--include`, minus refs that are\n+provided to `--exclude` will be packed.\n+\n \n BUGS\n ----\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex 1d1a64fe386..bcf383cac9d 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -7,7 +7,7 @@\n #include \"revision.h\"\n \n static char const * const pack_refs_usage[] = {\n-\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n+\tN_(\"git pack-refs [--all] [--no-prune] [--include ] [--exclude ]\"),\n \tNULL\n };\n \n@@ -15,13 +15,18 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n \tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n-\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n+\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n+\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n+\t\t\t\t\t\t .includes = &included_refs,\n+\t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n+\t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n+\t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n \t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n@@ -33,5 +38,11 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &option_excluded_refs)\n \t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n \n+\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n+\t\tstring_list_append(pack_refs_opts.includes, \"*\");\n+\n+\tif (!pack_refs_opts.includes->nr)\n+\t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n+\n \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\ndiff --git a/refs.h b/refs.h\nindex 46020bd335c..933fdebe584 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -66,6 +66,7 @@ struct worktree;\n struct pack_refs_opts {\n \tunsigned int flags;\n \tstruct ref_exclusions *exclusions;\n+\tstruct string_list *includes;\n };\n \n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5075e6c0d28..629e95ff857 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1178,18 +1178,13 @@ static int should_pack_ref(const char *refname,\n \t\t\t const struct object_id *oid, unsigned int ref_flags,\n \t\t\t struct pack_refs_opts *opts)\n {\n+\tstruct string_list_item *item;\n+\n \t/* Do not pack per-worktree refs: */\n \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n-\tif (ref_excluded(opts->exclusions, refname))\n-\t\treturn 0;\n-\n-\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n-\tif (!(opts->flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n-\t\treturn 0;\n-\n \t/* Do not pack symbolic refs: */\n \tif (ref_flags & REF_ISSYMREF)\n \t\treturn 0;\n@@ -1198,7 +1193,14 @@ static int should_pack_ref(const char *refname,\n \tif (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))\n \t\treturn 0;\n \n-\treturn 1;\n+\tif (ref_excluded(opts->exclusions, refname))\n+\t\treturn 0;\n+\n+\tfor_each_string_list_item(item, opts->includes)\n+\t\tif (!wildmatch(item->string, refname, 0))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n }\n \n static int files_pack_refs(struct ref_store *ref_store,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex de4197708d9..a6977b5e839 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -5,6 +5,7 @@\n #include \"worktree.h\"\n #include \"object-store.h\"\n #include \"repository.h\"\n+#include \"revision.h\"\n \n struct flag_definition {\n \tconst char *name;\n@@ -116,7 +117,14 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),\n static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n-\tstruct pack_refs_opts pack_opts = { .flags = flags };\n+\tstatic struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;\n+\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n+\tstruct pack_refs_opts pack_opts = { .flags = flags,\n+\t\t\t\t\t .exclusions = &exclusions,\n+\t\t\t\t\t .includes = &included_refs };\n+\n+\tif (pack_opts.flags & PACK_REFS_ALL)\n+\t\tstring_list_append(pack_opts.includes, \"*\");\n \n \treturn refs_pack_refs(refs, &pack_opts);\n }\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 925b90cd3ba..9f399d2f75a 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -124,6 +124,27 @@ test_expect_success 'test --no-exclude refs clears excluded refs' '\n \t! test -f .git/refs/heads/dont_pack3 &&\n \t! test -f .git/refs/heads/dont_pack4'\n \n+test_expect_success 'test only included refs are packed' '\n+\tgit branch pack_this1 &&\n+\tgit branch pack_this2 &&\n+\tgit tag dont_pack5 &&\n+\tgit pack-refs --include \"refs/heads/pack_this*\" &&\n+\ttest -f .git/refs/tags/dont_pack5 &&\n+\t! test -f .git/refs/heads/pack_this1 &&\n+\t! test -f .git/refs/heads/pack_this2'\n+\n+test_expect_success 'test --no-include refs clears included refs' '\n+\tgit branch pack1 &&\n+\tgit branch pack2 &&\n+\tgit pack-refs --include \"refs/heads/pack*\" --no-include &&\n+\ttest -f .git/refs/heads/pack1 &&\n+\ttest -f .git/refs/heads/pack2'\n+\n+test_expect_success 'test --exclude takes precedence over --include' '\n+\tgit branch dont_pack5 &&\n+\tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n+\ttest -f .git/refs/heads/dont_pack5'\n+\n test_expect_success \\\n \t'see if up-to-date packed refs are preserved' \\\n \t'git branch q &&\n-- \ngitgitgadget\n"},{"id":"477225","messageId":"pull.1501.v4.git.git.1683927282.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v3.git.git.1683828635.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] pack-refs: allow users control over which refs to pack","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-12T21:34:39Z","receivedAt":"2023-05-12T21:35:04Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"git-pack-refs does not currently give much control over which refs are\npacked. By default, all tags and already packed refs are packed. --all\nallows the user to pack all refs. Beyond this the user does not have\ncontrol. Introduce a pair of options --include and --exclude that will allow\nusers full control over which refs end up in the packed-refs file.\n\nChanges since v3:\n\n * got rid of extending ref_exclusions\n * use a strvec instead of a string_list to keep track of included refs\n\nChanges since v2:\n\n * repurpose ref_exclusions to be used for ref inclusions\n * fixed test formatting\n * adjusted --include to not include default of all tags\n\nChanges since v1:\n\n * Clarify that --all packs not just branch tips but all refs under refs/ in\n the documentation in patch 1\n * Add --include in patch 3\n\nIt's worth noting that [1] discussed a proposal for a pack refs v2 format\nthat would improve deletion speeds. The ref-table backend would also improve\nperformance of deletions. However, both of those proposals are still being\ndiscussed.\n\n 1. https://lore.kernel.org/git/pull.1408.git.1667846164.gitgitgadget@gmail.com/\n\nJohn Cai (3):\n docs: clarify git-pack-refs --all will pack all refs\n pack-refs: teach --exclude option to exclude refs from being packed\n pack-refs: teach pack-refs --include option\n\n Documentation/git-pack-refs.txt | 29 +++++++++++++++++++++++---\n builtin/pack-refs.c | 31 +++++++++++++++++++++++----\n refs.c | 4 ++--\n refs.h | 8 ++++++-\n refs/debug.c | 4 ++--\n refs/files-backend.c | 26 ++++++++++++++---------\n refs/packed-backend.c | 2 +-\n refs/refs-internal.h | 3 ++-\n revision.h | 2 +-\n t/helper/test-ref-store.c | 11 +++++++++-\n t/t3210-pack-refs.sh | 37 +++++++++++++++++++++++++++++++++\n 11 files changed, 131 insertions(+), 26 deletions(-)\n\n\nbase-commit: 91428f078b8a4fe6948a4c955af1a693841e3985\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1501%2Fjohn-cai%2Fjc%2Fexclude-refs-from-pack-refs-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1501/john-cai/jc/exclude-refs-from-pack-refs-v4\nPull-Request: https://github.com/git/git/pull/1501\n\nRange-diff vs v3:\n\n 1: 0d462010b79 ! 1: 554da1dc705 docs: clarify git-pack-refs --all will pack all refs\n @@ Commit message\n Signed-off-by: John Cai \n \n ## Documentation/git-pack-refs.txt ##\n -@@ Documentation/git-pack-refs.txt: OPTIONS\n - \n - The command by default packs all tags and refs that are already\n +@@ Documentation/git-pack-refs.txt: The command by default packs all tags and refs that are already\n packed, and leaves other refs\n --alone. This is because branches are expected to be actively\n -+alone. This is because branches are expected to be actively\n + alone. This is because branches are expected to be actively\n developed and packing their tips does not help performance.\n -This option causes branch tips to be packed as well. Useful for\n -a repository with many branches of historical interests.\n -+This option causes all refs to be packed as well, with the exception of hidden\n -+and broken refs. Useful for a repository with many branches of historical\n -+interests.\n ++This option causes all refs to be packed as well, with the exception\n ++of hidden refs, broken refs, and symbolic refs. Useful for a repository\n ++with many branches of historical interests.\n \n --no-prune::\n \n 2: 8c5c66a3050 ! 2: 69300845df1 pack-refs: teach --exclude option to exclude refs from being packed\n @@ Documentation/git-pack-refs.txt: git-pack-refs - Pack heads and tags for efficie\n \n DESCRIPTION\n -----------\n -@@ Documentation/git-pack-refs.txt: interests.\n +@@ Documentation/git-pack-refs.txt: with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n @@ Documentation/git-pack-refs.txt: interests.\n +patterns. If a ref is already packed, including it with `--exclude` will not\n +unpack it.\n +\n -+When used with `--all`, it will use the difference between the set of all refs,\n -+and what is provided to `--exclude`.\n ++When used with `--all`, pack only loose refs which do not match any of\n ++the provided `--exclude` patterns.\n +\n \n BUGS\n @@ refs/files-backend.c: static void prune_refs(struct files_ref_store *refs, struc\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n -+\tif (opts->exclusions && ref_excluded(opts->exclusions, refname))\n ++\tif (ref_excluded(opts->exclusions, refname))\n +\t\treturn 0;\n +\n \t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n @@ t/t3210-pack-refs.sh: test_expect_success \\\n +\tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n +\ttest -f .git/refs/heads/dont_pack1 &&\n +\ttest -f .git/refs/heads/dont_pack2 &&\n -+\t! test -f ./git/refs/heads/pack_this'\n ++\t! test -f .git/refs/heads/pack_this'\n +\n +test_expect_success 'test --no-exclude refs clears excluded refs' '\n +\tgit branch dont_pack3 &&\n 3: 0a0693ad612 < -: ----------- revision: modify ref_exclusions to handle inclusions\n 4: b2f3b98cd24 ! 3: 4bbe4c05ceb pack-refs: teach pack-refs --include option\n @@ Documentation/git-pack-refs.txt: git-pack-refs - Pack heads and tags for efficie\n \n DESCRIPTION\n -----------\n -@@ Documentation/git-pack-refs.txt: interests.\n +@@ Documentation/git-pack-refs.txt: with many branches of historical interests.\n The command usually removes loose refs under `$GIT_DIR/refs`\n hierarchy after packing them. This option tells it not to.\n \n @@ Documentation/git-pack-refs.txt: interests.\n \n Do not pack refs matching the given `glob(7)` pattern. Repetitions of this option\n @@ Documentation/git-pack-refs.txt: unpack it.\n - When used with `--all`, it will use the difference between the set of all refs,\n - and what is provided to `--exclude`.\n + When used with `--all`, pack only loose refs which do not match any of\n + the provided `--exclude` patterns.\n \n +When used with `--include`, refs provided to `--include`, minus refs that are\n +provided to `--exclude` will be packed.\n @@ Documentation/git-pack-refs.txt: unpack it.\n \n ## builtin/pack-refs.c ##\n @@\n - #include \"refs.h\"\n - #include \"repository.h\"\n #include \"revision.h\"\n -+#include \"trace.h\"\n \n static char const * const pack_refs_usage[] = {\n -\tN_(\"git pack-refs [--all] [--no-prune] [--exclude ]\"),\n @@ builtin/pack-refs.c\n @@ builtin/pack-refs.c: int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n - \tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n --\tstruct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};\n -+\tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\n + \tstatic struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;\n +-\tstruct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};\n ++\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n ++\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n ++\t\t\t\t\t\t .includes = &included_refs,\n +\t\t\t\t\t\t .flags = flags };\n \tstatic struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;\n -+\tstatic struct string_list option_included_refs = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *item;\n \n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &pack_refs_opts.flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &pack_refs_opts.flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n -+\t\tOPT_STRING_LIST(0, \"include\", &option_included_refs, N_(\"pattern\"),\n ++\t\tOPT_STRING_LIST(0, \"include\", pack_refs_opts.includes, N_(\"pattern\"),\n +\t\t\tN_(\"references to include\")),\n \t\tOPT_STRING_LIST(0, \"exclude\", &option_excluded_refs, N_(\"pattern\"),\n \t\t\tN_(\"references to exclude\")),\n \t\tOPT_END(),\n @@ builtin/pack-refs.c: int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &option_excluded_refs)\n - \t\tadd_ref_exclusion(pack_refs_opts.visibility, item->string);\n + \t\tadd_ref_exclusion(pack_refs_opts.exclusions, item->string);\n \n -+\tfor_each_string_list_item(item, &option_included_refs)\n -+\t\tadd_ref_inclusion(pack_refs_opts.visibility, item->string);\n -+\n +\tif (pack_refs_opts.flags & PACK_REFS_ALL)\n -+\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n ++\t\tstring_list_append(pack_refs_opts.includes, \"*\");\n +\n -+\tif (!pack_refs_opts.visibility->included_refs.nr)\n -+\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"refs/tags/*\");\n ++\tif (!pack_refs_opts.includes->nr)\n ++\t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n +\n \treturn refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);\n }\n \n + ## refs.h ##\n +@@ refs.h: struct worktree;\n + struct pack_refs_opts {\n + \tunsigned int flags;\n + \tstruct ref_exclusions *exclusions;\n ++\tstruct string_list *includes;\n + };\n + \n + const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n +\n ## refs/files-backend.c ##\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n + \t\t\t const struct object_id *oid, unsigned int ref_flags,\n + \t\t\t struct pack_refs_opts *opts)\n + {\n ++\tstruct string_list_item *item;\n ++\n + \t/* Do not pack per-worktree refs: */\n + \tif (parse_worktree_ref(refname, NULL, NULL, NULL) !=\n \t REF_WORKTREE_SHARED)\n \t\treturn 0;\n \n --\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n +-\tif (ref_excluded(opts->exclusions, refname))\n -\t\treturn 0;\n -\n -\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n @@ refs/files-backend.c: static int should_pack_ref(const char *refname,\n \t\treturn 0;\n \n -\treturn 1;\n -+\tif (opts->visibility && ref_excluded(opts->visibility, refname))\n ++\tif (ref_excluded(opts->exclusions, refname))\n +\t\treturn 0;\n +\n -+\tif (opts->visibility && ref_included(opts->visibility, refname))\n -+\t\treturn 1;\n ++\tfor_each_string_list_item(item, opts->includes)\n ++\t\tif (!wildmatch(item->string, refname, 0))\n ++\t\t\treturn 1;\n +\n +\treturn 0;\n }\n @@ t/helper/test-ref-store.c: static struct flag_definition pack_flags[] = { FLAG_D\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n -\tstruct pack_refs_opts pack_opts = { .flags = flags };\n -+\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n ++\tstatic struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;\n ++\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n +\tstruct pack_refs_opts pack_opts = { .flags = flags,\n -+\t\t\t\t\t .visibility = &visibility };\n ++\t\t\t\t\t .exclusions = &exclusions,\n ++\t\t\t\t\t .includes = &included_refs };\n +\n +\tif (pack_opts.flags & PACK_REFS_ALL)\n -+\t\tadd_ref_inclusion(pack_opts.visibility, \"*\");\n ++\t\tstring_list_append(pack_opts.includes, \"*\");\n \n \treturn refs_pack_refs(refs, &pack_opts);\n }\n @@ t/t3210-pack-refs.sh: test_expect_success 'test --no-exclude refs clears exclude\n +\tgit tag dont_pack5 &&\n +\tgit pack-refs --include \"refs/heads/pack_this*\" &&\n +\ttest -f .git/refs/tags/dont_pack5 &&\n -+\t! test -f ./git/refs/heads/pack_this1 &&\n -+\t! test -f ./git/refs/heads/pack_this2'\n ++\t! test -f .git/refs/heads/pack_this1 &&\n ++\t! test -f .git/refs/heads/pack_this2'\n +\n +test_expect_success 'test --no-include refs clears included refs' '\n +\tgit branch pack1 &&\n\n-- \ngitgitgadget\n"},{"id":"477226","messageId":"554da1dc705f437c4822b9bc5c8b604abf1e6144.1683927282.git.gitgitgadget@gmail.com","threadId":"59694","inReplyTo":"pull.1501.v4.git.git.1683927282.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] docs: clarify git-pack-refs --all will pack all refs","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-12T21:34:40Z","receivedAt":"2023-05-12T21:35:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n--all packs not just branch tips but anything under refs/ with the\nexception of hidden refs and broken refs. Clarify this in the\ndocumentation.\n\nSigned-off-by: John Cai \n---\n Documentation/git-pack-refs.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt\nindex 154081f2de2..22f00665006 100644\n--- a/Documentation/git-pack-refs.txt\n+++ b/Documentation/git-pack-refs.txt\n@@ -51,8 +51,9 @@ The command by default packs all tags and refs that are already\n packed, and leaves other refs\n alone. This is because branches are expected to be actively\n developed and packing their tips does not help performance.\n-This option causes branch tips to be packed as well. Useful for\n-a repository with many branches of historical interests.\n+This option causes all refs to be packed as well, with the exception\n+of hidden refs, broken refs, and symbolic refs. Useful for a repository\n+with many branches of historical interests.\n \n --no-prune::\n \n-- \ngitgitgadget\n\n"}]}