{"thread":{"id":"59891","subject":"[PATCH 0/3] revision: refactor ref_excludes to ref_visibility","startedAt":"2023-06-21T19:35:19Z","lastAt":"2023-06-23T20:58:00Z","messageCount":13,"participants":["John Cai via GitGitGadget","Junio C Hamano","Taylor Blau","John Cai"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"478654","messageId":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","threadId":"59891","inReplyTo":null,"subject":"[PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-21T19:35:09Z","receivedAt":"2023-06-21T19:35:19Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"The ref_excludes API is used to tell which refs should be excluded. However,\nthere are times when we would want to add refs to explicitly include as\nwell. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\ntaught pack-refs how to include certain refs, but did it in a more manual\nway by keeping the ref patterns in a separate string list. Instead, we can\neasily extend the ref_excludes API to include refs as well, since this use\ncase fits into the API nicely.\n\nRefactor the API by renaming it to ref_visibility, and add a ref_visible()\nhelper that takes into account ref inclusion.\n\nJohn Cai (3):\n  revision: rename ref_excludes to ref_visibility\n  revision: add ref_visible() helper\n  pack-refs: use new ref_visible() helper\n\n builtin/pack-refs.c       | 20 ++++----\n builtin/rev-parse.c       | 18 +++----\n refs.h                    |  2 +-\n refs/files-backend.c      | 11 +----\n revision.c                | 98 +++++++++++++++++++++++----------------\n revision.h                | 37 +++++++++++----\n t/helper/test-ref-store.c |  8 ++--\n 7 files changed, 111 insertions(+), 83 deletions(-)\n\n\nbase-commit: 6640c2d06d112675426cf436f0594f0e8c614848\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1515%2Fjohn-cai%2Fjc%2Frefactor-ref-excludes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1515/john-cai/jc/refactor-ref-excludes-v1\nPull-Request: https://github.com/git/git/pull/1515\n-- \ngitgitgadget\n"},{"id":"478655","messageId":"70f4797d9d629ab7088e05050f82f5807d30680c.1687376112.git.gitgitgadget@gmail.com","threadId":"59891","inReplyTo":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","subject":"[PATCH 2/3] revision: add ref_visible() helper","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-21T19:35:11Z","receivedAt":"2023-06-21T19:35:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn addition to helpers that check if a ref was excluded, teach the API a\nref_visible() function that takes into account both included\nrefs, excluded refs, and hidden refs.\n\nSince exclusions take precedence over inclusion, a ref_included()\nfunction is not necessary since a caller would always need to check\nseparately if the ref is excluded, in which case it would be easier to\ncall ref_visible().\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n revision.c | 32 ++++++++++++++++++++++++++------\n revision.h | 21 +++++++++++++++++++--\n 2 files changed, 45 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 13e86a96498..54709e3f6fc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1533,20 +1533,34 @@ static void add_rev_cmdline_list(struct rev_info *revs,\n \t}\n }\n \n-int ref_excluded(const struct ref_visibility *visibility, const char *path)\n+static int ref_matched(struct string_list refs, const char *path)\n {\n-\tconst char *stripped_path = strip_namespace(path);\n \tstruct string_list_item *item;\n \n-\tfor_each_string_list_item(item, &visibility->excluded_refs) {\n+\tfor_each_string_list_item(item, &refs)\n \t\tif (!wildmatch(item->string, path, 0))\n \t\t\treturn 1;\n-\t}\n \n-\tif (ref_is_hidden(stripped_path, path, &visibility->hidden_refs))\n+\treturn 0;\n+}\n+\n+int ref_excluded(const struct ref_visibility *visibility, const char *path)\n+{\n+\tif (ref_is_hidden(strip_namespace(path), path, &visibility->hidden_refs))\n \t\treturn 1;\n \n-\treturn 0;\n+\treturn ref_matched(visibility->excluded_refs, path);\n+}\n+\n+int ref_visible(const struct ref_visibility *visibility, const char *path)\n+{\n+\tif (ref_is_hidden(strip_namespace(path), path, &visibility->hidden_refs))\n+\t\treturn 0;\n+\n+\tif (ref_matched(visibility->excluded_refs, path))\n+\t\treturn 0;\n+\n+\treturn ref_matched(visibility->included_refs, path);\n }\n \n void init_ref_visibility(struct ref_visibility *visibility)\n@@ -1558,6 +1572,7 @@ void init_ref_visibility(struct ref_visibility *visibility)\n void clear_ref_visibility(struct ref_visibility *visibility)\n {\n \tstring_list_clear(&visibility->excluded_refs, 0);\n+\tstring_list_clear(&visibility->included_refs, 0);\n \tstring_list_clear(&visibility->hidden_refs, 0);\n \tvisibility->hidden_refs_configured = 0;\n }\n@@ -1567,6 +1582,11 @@ void add_ref_exclusion(struct ref_visibility *visibility, const char *exclude)\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_visibility *visibility;\n \tconst char *section;\ndiff --git a/revision.h b/revision.h\nindex 8eaca125cd1..fc26ec70b28 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -91,6 +91,12 @@ struct ref_visibility {\n \t */\n \tstruct string_list excluded_refs;\n \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+\n \t/*\n \t * Hidden refs is a list of patterns that is to be hidden via\n \t * `ref_is_hidden()`.\n@@ -110,6 +116,7 @@ struct ref_visibility {\n  */\n #define REF_VISIBILITY_INIT { \\\n \t.excluded_refs = STRING_LIST_INIT_DUP, \\\n+\t.included_refs = STRING_LIST_INIT_DUP, \\\n \t.hidden_refs = STRING_LIST_INIT_DUP, \\\n }\n \n@@ -485,12 +492,22 @@ void show_object_with_name(FILE *, struct object *, const char *);\n \n /**\n  * Helpers to check if a reference should be excluded.\n+ *\n+ * ref_excluded() checks if a ref has been explicitly excluded either\n+ * because it is hidden, or it was added via an add_ref_exclusion() call.\n+ *\n+ * ref_visible() checks if a ref is visible by taking into account whether a ref\n+ * has been included via an add_ref_inclusion() call, but also whether it has\n+ * been excluded via add_ref_exclusion(). Exclusions take precedence. If a ref\n+ * is hidden, it will also be treated as not visible.\n+ *\n  */\n-\n-int ref_excluded(const struct ref_visibility *exclusions, const char *path);\n+int ref_excluded(const struct ref_visibility *visibility, const char *path);\n+int ref_visible(const struct ref_visibility *visibility, 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 *include);\n void exclude_hidden_refs(struct ref_visibility *, const char *section);\n \n /**\n-- \ngitgitgadget\n\n"},{"id":"478656","messageId":"43e88a945226e1e08f2bd1a2bdebebda09cd6ec8.1687376112.git.gitgitgadget@gmail.com","threadId":"59891","inReplyTo":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","subject":"[PATCH 1/3] revision: rename ref_excludes to ref_visibility","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-21T19:35:10Z","receivedAt":"2023-06-21T19:35:23Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\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 [1] considered when teaching git-pack-refs the\nability to specify not only refs to exclude but ones to include.\n\nA subsequent commit will actuall extend the API to add the ability to\nkeep track of ref inclusions. As a preparatory patch, rename\n ref_exclusions to ref_visibility.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\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                | 72 +++++++++++++++++++--------------------\n revision.h                | 18 +++++-----\n t/helper/test-ref-store.c |  4 +--\n 7 files changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex bcf383cac9d..ff07986edaf 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -14,9 +14,9 @@ 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+\tstatic struct ref_visibility visibility = REF_VISIBILITY_INIT;\n \tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n-\tstruct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,\n+\tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\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@@ -36,7 +36,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 \tif (pack_refs_opts.flags & PACK_REFS_ALL)\n \t\tstring_list_append(pack_refs_opts.includes, \"*\");\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 852e49e3403..abbf240f498 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 refs_visible = 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(&refs_visible, 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(&refs_visible);\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(&refs_visible);\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 (refs_visible.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 (refs_visible.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 (refs_visible.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(&refs_visible, 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(&refs_visible, 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 933fdebe584..ac334bf545d 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 \tstruct string_list *includes;\n };\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9a8333c0d07..2e716a3e201 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1194,7 +1194,7 @@ 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 (ref_excluded(opts->exclusions, refname))\n+\tif (ref_excluded(opts->visibility, refname))\n \t\treturn 0;\n \n \tfor_each_string_list_item(item, opts->includes)\ndiff --git a/revision.c b/revision.c\nindex b33cc1d106a..13e86a96498 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1533,54 +1533,54 @@ 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+int ref_excluded(const struct ref_visibility *visibility, const char *path)\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, &visibility->excluded_refs) {\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, &visibility->hidden_refs))\n \t\treturn 1;\n \n \treturn 0;\n }\n \n-void init_ref_exclusions(struct ref_exclusions *exclusions)\n+void init_ref_visibility(struct ref_visibility *visibility)\n {\n-\tstruct ref_exclusions blank = REF_EXCLUSIONS_INIT;\n-\tmemcpy(exclusions, &blank, sizeof(*exclusions));\n+\tstruct ref_visibility blank = REF_VISIBILITY_INIT;\n+\tmemcpy(visibility, &blank, sizeof(*visibility));\n }\n \n-void clear_ref_exclusions(struct ref_exclusions *exclusions)\n+void clear_ref_visibility(struct ref_visibility *visibility)\n {\n-\tstring_list_clear(&exclusions->excluded_refs, 0);\n-\tstring_list_clear(&exclusions->hidden_refs, 0);\n-\texclusions->hidden_refs_configured = 0;\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_exclusions *exclusions, const char *exclude)\n+void add_ref_exclusion(struct ref_visibility *visibility, const char *exclude)\n {\n-\tstring_list_append(&exclusions->excluded_refs, exclude);\n+\tstring_list_append(&visibility->excluded_refs, exclude);\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 +1588,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@@ -1612,7 +1612,7 @@ static int handle_one_ref(const char *path, const struct object_id *oid,\n \tstruct all_refs_cb *cb = cb_data;\n \tstruct object *object;\n \n-\tif (ref_excluded(&cb->all_revs->ref_excludes, path))\n+\tif (ref_excluded(&cb->all_revs->refs_visible, path))\n \t    return 0;\n \n \tobject = get_reference(cb->all_revs, path, oid, cb->all_flags);\n@@ -1935,7 +1935,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->refs_visible);\n }\n \n static void add_pending_commit_list(struct rev_info *revs,\n@@ -2724,12 +2724,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->refs_visible);\n \t} else if (!strcmp(arg, \"--branches\")) {\n-\t\tif (revs->ref_excludes.hidden_refs_configured)\n+\t\tif (revs->refs_visible.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->refs_visible);\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@@ -2737,48 +2737,48 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \t\t\t    for_each_good_bisect_ref);\n \t\trevs->bisect = 1;\n \t} else if (!strcmp(arg, \"--tags\")) {\n-\t\tif (revs->ref_excludes.hidden_refs_configured)\n+\t\tif (revs->refs_visible.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->refs_visible);\n \t} else if (!strcmp(arg, \"--remotes\")) {\n-\t\tif (revs->ref_excludes.hidden_refs_configured)\n+\t\tif (revs->refs_visible.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->refs_visible);\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->refs_visible);\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+\t\tadd_ref_exclusion(&revs->refs_visible, optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"exclude-hidden\", argv, &optarg))) {\n-\t\texclude_hidden_refs(&revs->ref_excludes, optarg);\n+\t\texclude_hidden_refs(&revs->refs_visible, optarg);\n \t\treturn argcount;\n \t} else if (skip_prefix(arg, \"--branches=\", &optarg)) {\n \t\tstruct all_refs_cb cb;\n-\t\tif (revs->ref_excludes.hidden_refs_configured)\n+\t\tif (revs->refs_visible.hidden_refs_configured)\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->refs_visible);\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\tif (revs->refs_visible.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->refs_visible);\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\tif (revs->refs_visible.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->refs_visible);\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..8eaca125cd1 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -84,7 +84,7 @@ struct rev_cmdline_info {\n \t} *rev;\n };\n \n-struct ref_exclusions {\n+struct ref_visibility {\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 +106,9 @@ 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.excluded_refs = STRING_LIST_INIT_DUP, \\\n \t.hidden_refs = STRING_LIST_INIT_DUP, \\\n }\n@@ -135,7 +135,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 refs_visible;\n \n \t/* Basic information */\n \tconst char *prefix;\n@@ -487,11 +487,11 @@ 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+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 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\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex a6977b5e839..504935c1d84 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -117,10 +117,10 @@ 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-\tstatic struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;\n+\tstatic struct ref_visibility visibility = REF_VISIBILITY_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    .visibility = &visibility,\n \t\t\t\t\t    .includes = &included_refs };\n \n \tif (pack_opts.flags & PACK_REFS_ALL)\n-- \ngitgitgadget\n\n"},{"id":"478657","messageId":"144ad0b48a4e8491c4e2609458be653087ced804.1687376112.git.gitgitgadget@gmail.com","threadId":"59891","inReplyTo":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","subject":"[PATCH 3/3] pack-refs: use new ref_visible() helper","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-21T19:35:12Z","receivedAt":"2023-06-21T19:35:25Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nreplace the current logic in pack-refs that takes into account included\nrefs with the new ref_visible() helper.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n builtin/pack-refs.c       | 14 ++++++++------\n refs/files-backend.c      | 11 +----------\n t/helper/test-ref-store.c |  6 ++----\n 3 files changed, 11 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex ff07986edaf..5d6dc363085 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -15,17 +15,16 @@ 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-\tstatic struct string_list included_refs = STRING_LIST_INIT_NODUP;\n \tstruct pack_refs_opts pack_refs_opts = { .visibility = &visibility,\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\", pack_refs_opts.includes, 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@@ -38,11 +37,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\tstring_list_append(pack_refs_opts.includes, \"*\");\n+\t\tadd_ref_inclusion(pack_refs_opts.visibility, \"*\");\n \n-\tif (!pack_refs_opts.includes->nr)\n-\t\tstring_list_append(pack_refs_opts.includes, \"refs/tags/*\");\n+\tif (!option_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 2e716a3e201..2dfe7f7e787 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1179,8 +1179,6 @@ 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@@ -1194,14 +1192,7 @@ 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 (ref_excluded(opts->visibility, 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+\treturn ref_visible(opts->visibility, refname);\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 504935c1d84..81e0b5ea0a0 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -118,13 +118,11 @@ static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\", pack_flags);\n \tstatic struct ref_visibility visibility = REF_VISIBILITY_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    .includes = &included_refs };\n+\t\t\t\t\t    .visibility = &visibility };\n \n \tif (pack_opts.flags & PACK_REFS_ALL)\n-\t\tstring_list_append(pack_opts.includes, \"*\");\n+\t\tadd_ref_inclusion(&visibility, \"*\");\n \n \treturn refs_pack_refs(refs, &pack_opts);\n }\n-- \ngitgitgadget\n"},{"id":"478665","messageId":"xmqqy1kcfsb6.fsf@gitster.g","threadId":"59891","inReplyTo":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-21T20:56:13Z","receivedAt":"2023-06-21T20:57:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> The ref_excludes API is used to tell which refs should be excluded. However,\n> there are times when we would want to add refs to explicitly include as\n> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\n> taught pack-refs how to include certain refs, but did it in a more manual\n> way by keeping the ref patterns in a separate string list. Instead, we can\n> easily extend the ref_excludes API to include refs as well, since this use\n> case fits into the API nicely.\n\nHmph, how would this interact with the other topic in flight that\ntouch the ref exclusion logic tb/refs-exclusion-and-packed-refs?\n\nThanks.\n"},{"id":"478683","messageId":"ZJRBsDq8NI9EInel@nand.local","threadId":"59891","inReplyTo":"pull.1515.git.git.1687376112.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:42:24Z","receivedAt":"2023-06-22T12:42:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:\n> The ref_excludes API is used to tell which refs should be excluded. However,\n> there are times when we would want to add refs to explicitly include as\n> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\n> taught pack-refs how to include certain refs, but did it in a more manual\n> way by keeping the ref patterns in a separate string list. Instead, we can\n> easily extend the ref_excludes API to include refs as well, since this use\n> case fits into the API nicely.\n\nAfter reading this description, I am not sure why you can't \"include\" a\nreference that would otherwise be excluded by passing the rules:\n\n  - refs/heads/exclude/*\n  - !refs/heads/exclude/but/include/me\n\n(where the '!' prefix in the last rule is what brings back the included\nreference).\n\nBut let's read on and see if there is something that I'm missing.\n\n> Refactor the API by renaming it to ref_visibility, and add a ref_visible()\n> helper that takes into account ref inclusion.\n\nHmm. Is this a replacement for ref_is_hidden(), which is already public?\n\nThanks,\nTaylor\n"},{"id":"478684","messageId":"ZJRB5GSsFcdc/m+n@nand.local","threadId":"59891","inReplyTo":"43e88a945226e1e08f2bd1a2bdebebda09cd6ec8.1687376112.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] revision: rename ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:43:16Z","receivedAt":"2023-06-22T12:43:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 21, 2023 at 07:35:10PM +0000, John Cai via GitGitGadget wrote:\n> From: John Cai <johncai86@gmail.com>\n>\n> The ref_exclusions API provides the ability to check if certain refs are\n> to be excluded. We can easily extend this API to check if certain refs\n> are included, which [1] considered when teaching git-pack-refs the\n> ability to specify not only refs to exclude but ones to include.\n>\n> A subsequent commit will actuall extend the API to add the ability to\n> keep track of ref inclusions. As a preparatory patch, rename\n>  ref_exclusions to ref_visibility.\n\nSkimming through this patch, it looks like a straight-forward rename\nthat doesn't change any functionality. I think other readers may benefit\nfrom a note that says something to that effect.\n\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                | 72 +++++++++++++++++++--------------------\n>  revision.h                | 18 +++++-----\n>  t/helper/test-ref-store.c |  4 +--\n>  7 files changed, 61 insertions(+), 61 deletions(-)\n\nObviously all of this looks OK.\n\nThanks,\nTaylor\n"},{"id":"478685","messageId":"ZJRDZ7NhyNpTV8jD@nand.local","threadId":"59891","inReplyTo":"ZJRBsDq8NI9EInel@nand.local","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:49:43Z","receivedAt":"2023-06-22T12:49:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote:\n> On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:\n> > The ref_excludes API is used to tell which refs should be excluded. However,\n> > there are times when we would want to add refs to explicitly include as\n> > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\n> > taught pack-refs how to include certain refs, but did it in a more manual\n> > way by keeping the ref patterns in a separate string list. Instead, we can\n> > easily extend the ref_excludes API to include refs as well, since this use\n> > case fits into the API nicely.\n>\n> After reading this description, I am not sure why you can't \"include\" a\n> reference that would otherwise be excluded by passing the rules:\n>\n>   - refs/heads/exclude/*\n>   - !refs/heads/exclude/but/include/me\n>\n> (where the '!' prefix in the last rule is what brings back the included\n> reference).\n>\n> But let's read on and see if there is something that I'm missing.\n\nHaving read this series in detail, I am puzzled. I don't think that\nthere is any limitation of the existing reference hiding rules that\nwouldn't permit what you're trying to do by adding the list of\nreferences you want to include at the end of the exclude list, so long\nas they are each prefixed with the magic \"!\" sentinel.\n\nI think splitting the list of excluded references into individual\nexcluded and non-excluded references creates some awkwardness. For one:\nexcluded references already can cause us to include a reference, so\nsplitting that behavior across two lists seems difficult to reason\nabout.\n\nFor example, if your excluded list contains:\n\n  - refs/heads/foo\n  - refs/heads/bar\n  - !refs/heads/foo/baz\n\nand your included lists contains:\n\n  - refs/heads/bar/baz/quux\n\nI am left wondering: why doesn't the rule pertaining to\nrefs/heads/foo/baz show up in the included list? Likewise, what happens\nwith refs/heads/bar/baz/quux? It is a child of an excluded rule, so the\nquestion is which list takes priority.\n\nMostly, I am wondering if I am missing something that would explain why\nyou couldn't modify the above example's excluded list to contain\nsomething like \"!refs/heads/bar/baz/quux\", eliminating the need for the\ninclude list entirely.\n\nThanks,\nTaylor\n"},{"id":"478686","messageId":"ZJREB4p4RA0T5cO2@nand.local","threadId":"59891","inReplyTo":"xmqqy1kcfsb6.fsf@gitster.g","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:52:23Z","receivedAt":"2023-06-22T12:52:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 21, 2023 at 01:56:13PM -0700, Junio C Hamano wrote:\n> \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > The ref_excludes API is used to tell which refs should be excluded. However,\n> > there are times when we would want to add refs to explicitly include as\n> > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\n> > taught pack-refs how to include certain refs, but did it in a more manual\n> > way by keeping the ref patterns in a separate string list. Instead, we can\n> > easily extend the ref_excludes API to include refs as well, since this use\n> > case fits into the API nicely.\n>\n> Hmph, how would this interact with the other topic in flight that\n> touch the ref exclusion logic tb/refs-exclusion-and-packed-refs?\n\nGood question. Besides trivial conflicts from John's patches to rename\nthis API, I think the sensible thing to do with my\ntb/refs-exclusion-and-packed-refs topic would be to also refuse to use\nthe jump list if there are any non-trivial exclusion *or* inclusion\nentries.\n\nBut I have some more general concerns about the approach taken by this\ntopic, namely that I do not understand a reference \"foo\" cannot be\nincluded by adding a \"!foo\" entry to the excluded list.\n\nThanks,\nTaylor\n"},{"id":"478687","messageId":"ZJREYU0daKlmfjhr@nand.local","threadId":"59891","inReplyTo":"ZJRDZ7NhyNpTV8jD@nand.local","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:53:53Z","receivedAt":"2023-06-22T12:54:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote:\n> I am left wondering: why doesn't the rule pertaining to\n> refs/heads/foo/baz show up in the included list? Likewise, what happens\n> with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the\n> question is which list takes priority.\n>\n> Mostly, I am wondering if I am missing something that would explain why\n> you couldn't modify the above example's excluded list to contain\n> something like \"!refs/heads/bar/baz/quux\", eliminating the need for the\n> include list entirely.\n\nAnother potential quirk that I just now thought of: what are the rules\nfor what can go in the include list? Fully qualified references only? Or\ncan we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to\nhave the namespace-stripping operator ^, but not !, since negating an\ninclude rule seems to imply that it should be in the exclude list.\n\nThanks,\nTaylor\n"},{"id":"478688","messageId":"ZJRFcMR0/f6Olj+Z@nand.local","threadId":"59891","inReplyTo":"ZJREYU0daKlmfjhr@nand.local","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-22T12:58:24Z","receivedAt":"2023-06-22T12:58:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 22, 2023 at 08:53:53AM -0400, Taylor Blau wrote:\n> On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote:\n> > I am left wondering: why doesn't the rule pertaining to\n> > refs/heads/foo/baz show up in the included list? Likewise, what happens\n> > with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the\n> > question is which list takes priority.\n> >\n> > Mostly, I am wondering if I am missing something that would explain why\n> > you couldn't modify the above example's excluded list to contain\n> > something like \"!refs/heads/bar/baz/quux\", eliminating the need for the\n> > include list entirely.\n>\n> Another potential quirk that I just now thought of: what are the rules\n> for what can go in the include list? Fully qualified references only? Or\n> can we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to\n> have the namespace-stripping operator ^, but not !, since negating an\n> include rule seems to imply that it should be in the exclude list.\n\nSorry for the long chain of self-replies. I think one clarifying point\nthat I am not sure of yet is whether or not the exclude rules you're\ntalking about are interpreted as patterns (as in transfer.hideRefs) or\nwildmatch patterns.\n\nIf they are wildmatch patterns, would it suffice to add the references\nyou *do* want to enumerate to the traversal ahead of time? There is also\nthe hidden_refs member of that struct, so perhaps adding negated entries\nthere would work.\n\nEither way, I think emphasizing the difference between ref exclusions as\nit pertains to traversal, and ref exclusions as it pertains to hiding\nwould greatly help other reviewers of this series.\n\nThanks,\nTaylor\n"},{"id":"478739","messageId":"941CCF5B-1FE6-46BE-9ED7-77C11E943E2E@gmail.com","threadId":"59891","inReplyTo":"ZJRDZ7NhyNpTV8jD@nand.local","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-06-23T19:16:52Z","receivedAt":"2023-06-23T19:17:00Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Taylor,\n\nOn 22 Jun 2023, at 8:49, Taylor Blau wrote:\n\n> On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote:\n>> On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:\n>>> The ref_excludes API is used to tell which refs should be excluded. However,\n>>> there are times when we would want to add refs to explicitly include as\n>>> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)\n>>> taught pack-refs how to include certain refs, but did it in a more manual\n>>> way by keeping the ref patterns in a separate string list. Instead, we can\n>>> easily extend the ref_excludes API to include refs as well, since this use\n>>> case fits into the API nicely.\n>>\n>> After reading this description, I am not sure why you can't \"include\" a\n>> reference that would otherwise be excluded by passing the rules:\n>>\n>>   - refs/heads/exclude/*\n>>   - !refs/heads/exclude/but/include/me\n>>\n>> (where the '!' prefix in the last rule is what brings back the included\n>> reference).\n>>\n>> But let's read on and see if there is something that I'm missing.\n>\n> Having read this series in detail, I am puzzled. I don't think that\n> there is any limitation of the existing reference hiding rules that\n> wouldn't permit what you're trying to do by adding the list of\n> references you want to include at the end of the exclude list, so long\n> as they are each prefixed with the magic \"!\" sentinel.\n\nTo be honest, I had no idea \"!\" would have this effect--so thanks for bringing\nit to my attention.\n>\n> I think splitting the list of excluded references into individual\n> excluded and non-excluded references creates some awkwardness. For one:\n> excluded references already can cause us to include a reference, so\n> splitting that behavior across two lists seems difficult to reason\n> about.\n>\n> For example, if your excluded list contains:\n>\n>   - refs/heads/foo\n>   - refs/heads/bar\n>   - !refs/heads/foo/baz\n>\n> and your included lists contains:\n>\n>   - refs/heads/bar/baz/quux\n>\n> I am left wondering: why doesn't the rule pertaining to\n> refs/heads/foo/baz show up in the included list? Likewise, what happens\n> with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the\n> question is which list takes priority.\n\nNow knowing the effect of \"!\", I understand the concerns about having a\nseparate list for included references. However, I do think it would be odd if\nthere is a ref_included() function to also use ref_excluded() to include a ref.\n\nAlso, even with the current API, I think it can be confusing to reason about\nwhat takes precedence if there is a mix of inclusions and exclusions. For\nexample, if the excluded list contains:\n\n- refs/heads/foo/baz\n- !refs/heads/foo\n\nwould the inclusion take precedence, or the exclusion?\n\n>\n> Mostly, I am wondering if I am missing something that would explain why\n> you couldn't modify the above example's excluded list to contain\n> something like \"!refs/heads/bar/baz/quux\", eliminating the need for the\n> include list entirely.\n\nI think my one reservation however, is with usability of the current API. It's\nnot very intuitive to include references by adding them with\nref_excluded(&exclusions, \"!ref/to/be/included\").\n\nI do think it would be easier to reason about if we kept two separate lists, one\nfor inclusion and one for exclusion. The existence of the \"!\" magic sentinel\ndoes make things much more confusing however. I almost want to remove support\nfor the magic sentinel in favor of keeping two distinct lists that cannot be\nmixed. Wondering your thoughts on that approach?\n\n>\n> Thanks,\n> Taylor\n\nthanks!\nJOhn\n"},{"id":"478746","messageId":"xmqqo7l5aoc1.fsf@gitster.g","threadId":"59891","inReplyTo":"941CCF5B-1FE6-46BE-9ED7-77C11E943E2E@gmail.com","subject":"Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-23T20:57:50Z","receivedAt":"2023-06-23T20:58:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai <johncai86@gmail.com> writes:\n\n>>> After reading this description, I am not sure why you can't \"include\" a\n>>> reference that would otherwise be excluded by passing the rules:\n>>>\n>>>   - refs/heads/exclude/*\n>>>   - !refs/heads/exclude/but/include/me\n>>>\n>>> (where the '!' prefix in the last rule is what brings back the included\n>>> reference).\n>>>\n>>> But let's read on and see if there is something that I'm missing.\n>>\n>> Having read this series in detail, I am puzzled. I don't think that\n>> there is any limitation of the existing reference hiding rules that\n>> wouldn't permit what you're trying to do by adding the list of\n>> references you want to include at the end of the exclude list, so long\n>> as they are each prefixed with the magic \"!\" sentinel.\n>\n> To be honest, I had no idea \"!\" would have this effect--so thanks for bringing\n> it to my attention.\n\nFWIW, \"--exclude=!\" gets zero hits in t/ directory.\n\nref_excluded() merely calls wildmatch() like so:\n\n        int ref_excluded(const struct ref_exclusions *exclusions, const char *path)\n        {\n                const char *stripped_path = strip_namespace(path);\n                struct string_list_item *item;\n\n                for_each_string_list_item(item, &exclusions->excluded_refs) {\n                        if (!wildmatch(item->string, path, 0))\n                                return 1;\n                }\n\n                if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))\n                        return 1;\n\n                return 0;\n        }\n\nso I do not know what to think about it.  This is called from inside\ncallback of things like \"log --exclude=A --exclude=B ... --all\" when\nwe are trying to add all refs in response to \"--all\", and it appears\nto me that the first match would already determine the ref's fate\nwithout even looking at the later patterns (prefixed with bang '!'\nor not).  Taylor, am I looking at a wrong code?\n\nPuzzled...\n\n"}]}