[PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[John Cai via GitGitGadget]

The ref_excludes API is used to tell which refs should be excluded. However,
there are times when we would want to add refs to explicitly include as
well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
taught pack-refs how to include certain refs, but did it in a more manual
way by keeping the ref patterns in a separate string list. Instead, we can
easily extend the ref_excludes API to include refs as well, since this use
case fits into the API nicely.

Refactor the API by renaming it to ref_visibility, and add a ref_visible()
helper that takes into account ref inclusion.

John Cai (3):
  revision: rename ref_excludes to ref_visibility
  revision: add ref_visible() helper
  pack-refs: use new ref_visible() helper

 builtin/pack-refs.c       | 20 ++++----
 builtin/rev-parse.c       | 18 +++----
 refs.h                    |  2 +-
 refs/files-backend.c      | 11 +----
 revision.c                | 98 +++++++++++++++++++++++----------------
 revision.h                | 37 +++++++++++----
 t/helper/test-ref-store.c |  8 ++--
 7 files changed, 111 insertions(+), 83 deletions(-)


base-commit: 6640c2d06d112675426cf436f0594f0e8c614848
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1515%2Fjohn-cai%2Fjc%2Frefactor-ref-excludes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1515/john-cai/jc/refactor-ref-excludes-v1
Pull-Request: https://github.com/git/git/pull/1515
-- 
gitgitgadget

[PATCH 1/3] revision: rename ref_excludes to ref_visibility

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

The ref_exclusions API provides the ability to check if certain refs are
to be excluded. We can easily extend this API to check if certain refs
are included, which [1] considered when teaching git-pack-refs the
ability to specify not only refs to exclude but ones to include.

A subsequent commit will actuall extend the API to add the ability to
keep track of ref inclusions. As a preparatory patch, rename
 ref_exclusions to ref_visibility.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/pack-refs.c       |  6 ++--
 builtin/rev-parse.c       | 18 +++++-----
 refs.h                    |  2 +-
 refs/files-backend.c      |  2 +-
 revision.c                | 72 +++++++++++++++++++--------------------
 revision.h                | 18 +++++-----
 t/helper/test-ref-store.c |  4 +--
 7 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index bcf383cac9d..ff07986edaf 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -14,9 +14,9 @@ static char const * const pack_refs_usage[] = {
 int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	unsigned int flags = PACK_REFS_PRUNE;
-	static struct ref_exclusions excludes = REF_EXCLUSIONS_INIT;
+	static struct ref_visibility visibility = REF_VISIBILITY_INIT;
 	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
-	struct pack_refs_opts pack_refs_opts = { .exclusions = &excludes,
+	struct pack_refs_opts pack_refs_opts = { .visibility = &visibility,
 						 .includes = &included_refs,
 						 .flags = flags };
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
@@ -36,7 +36,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 		usage_with_options(pack_refs_usage, opts);
 
 	for_each_string_list_item(item, &option_excluded_refs)
-		add_ref_exclusion(pack_refs_opts.exclusions, item->string);
+		add_ref_exclusion(pack_refs_opts.visibility, item->string);
 
 	if (pack_refs_opts.flags & PACK_REFS_ALL)
 		string_list_append(pack_refs_opts.includes, "*");
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e3403..abbf240f498 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -46,7 +46,7 @@ static int abbrev_ref_strict;
 static int output_sq;
 
 static int stuck_long;
-static struct ref_exclusions ref_excludes = REF_EXCLUSIONS_INIT;
+static struct ref_visibility refs_visible = REF_VISIBILITY_INIT;
 
 /*
  * Some arguments are relevant "revision" arguments,
@@ -208,7 +208,7 @@ static int show_default(void)
 static int show_reference(const char *refname, const struct object_id *oid,
 			  int flag UNUSED, void *cb_data UNUSED)
 {
-	if (ref_excluded(&ref_excludes, refname))
+	if (ref_excluded(&refs_visible, refname))
 		return 0;
 	show_rev(NORMAL, oid, refname);
 	return 0;
@@ -596,7 +596,7 @@ static void handle_ref_opt(const char *pattern, const char *prefix)
 		for_each_glob_ref_in(show_reference, pattern, prefix, NULL);
 	else
 		for_each_ref_in(prefix, show_reference, NULL);
-	clear_ref_exclusions(&ref_excludes);
+	clear_ref_visibility(&refs_visible);
 }
 
 enum format_type {
@@ -874,7 +874,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--all")) {
 				for_each_ref(show_reference, NULL);
-				clear_ref_exclusions(&ref_excludes);
+				clear_ref_visibility(&refs_visible);
 				continue;
 			}
 			if (skip_prefix(arg, "--disambiguate=", &arg)) {
@@ -888,13 +888,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (opt_with_value(arg, "--branches", &arg)) {
-				if (ref_excludes.hidden_refs_configured)
+				if (refs_visible.hidden_refs_configured)
 					return error(_("--exclude-hidden cannot be used together with --branches"));
 				handle_ref_opt(arg, "refs/heads/");
 				continue;
 			}
 			if (opt_with_value(arg, "--tags", &arg)) {
-				if (ref_excludes.hidden_refs_configured)
+				if (refs_visible.hidden_refs_configured)
 					return error(_("--exclude-hidden cannot be used together with --tags"));
 				handle_ref_opt(arg, "refs/tags/");
 				continue;
@@ -904,17 +904,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (opt_with_value(arg, "--remotes", &arg)) {
-				if (ref_excludes.hidden_refs_configured)
+				if (refs_visible.hidden_refs_configured)
 					return error(_("--exclude-hidden cannot be used together with --remotes"));
 				handle_ref_opt(arg, "refs/remotes/");
 				continue;
 			}
 			if (skip_prefix(arg, "--exclude=", &arg)) {
-				add_ref_exclusion(&ref_excludes, arg);
+				add_ref_exclusion(&refs_visible, arg);
 				continue;
 			}
 			if (skip_prefix(arg, "--exclude-hidden=", &arg)) {
-				exclude_hidden_refs(&ref_excludes, arg);
+				exclude_hidden_refs(&refs_visible, arg);
 				continue;
 			}
 			if (!strcmp(arg, "--show-toplevel")) {
diff --git a/refs.h b/refs.h
index 933fdebe584..ac334bf545d 100644
--- a/refs.h
+++ b/refs.h
@@ -65,7 +65,7 @@ struct worktree;
 
 struct pack_refs_opts {
 	unsigned int flags;
-	struct ref_exclusions *exclusions;
+	struct ref_visibility *visibility;
 	struct string_list *includes;
 };
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9a8333c0d07..2e716a3e201 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1194,7 +1194,7 @@ static int should_pack_ref(const char *refname,
 	if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
 		return 0;
 
-	if (ref_excluded(opts->exclusions, refname))
+	if (ref_excluded(opts->visibility, refname))
 		return 0;
 
 	for_each_string_list_item(item, opts->includes)
diff --git a/revision.c b/revision.c
index b33cc1d106a..13e86a96498 100644
--- a/revision.c
+++ b/revision.c
@@ -1533,54 +1533,54 @@ static void add_rev_cmdline_list(struct rev_info *revs,
 	}
 }
 
-int ref_excluded(const struct ref_exclusions *exclusions, const char *path)
+int ref_excluded(const struct ref_visibility *visibility, const char *path)
 {
 	const char *stripped_path = strip_namespace(path);
 	struct string_list_item *item;
 
-	for_each_string_list_item(item, &exclusions->excluded_refs) {
+	for_each_string_list_item(item, &visibility->excluded_refs) {
 		if (!wildmatch(item->string, path, 0))
 			return 1;
 	}
 
-	if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))
+	if (ref_is_hidden(stripped_path, path, &visibility->hidden_refs))
 		return 1;
 
 	return 0;
 }
 
-void init_ref_exclusions(struct ref_exclusions *exclusions)
+void init_ref_visibility(struct ref_visibility *visibility)
 {
-	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
-	memcpy(exclusions, &blank, sizeof(*exclusions));
+	struct ref_visibility blank = REF_VISIBILITY_INIT;
+	memcpy(visibility, &blank, sizeof(*visibility));
 }
 
-void clear_ref_exclusions(struct ref_exclusions *exclusions)
+void clear_ref_visibility(struct ref_visibility *visibility)
 {
-	string_list_clear(&exclusions->excluded_refs, 0);
-	string_list_clear(&exclusions->hidden_refs, 0);
-	exclusions->hidden_refs_configured = 0;
+	string_list_clear(&visibility->excluded_refs, 0);
+	string_list_clear(&visibility->hidden_refs, 0);
+	visibility->hidden_refs_configured = 0;
 }
 
-void add_ref_exclusion(struct ref_exclusions *exclusions, const char *exclude)
+void add_ref_exclusion(struct ref_visibility *visibility, const char *exclude)
 {
-	string_list_append(&exclusions->excluded_refs, exclude);
+	string_list_append(&visibility->excluded_refs, exclude);
 }
 
 struct exclude_hidden_refs_cb {
-	struct ref_exclusions *exclusions;
+	struct ref_visibility *visibility;
 	const char *section;
 };
 
 static int hide_refs_config(const char *var, const char *value, void *cb_data)
 {
 	struct exclude_hidden_refs_cb *cb = cb_data;
-	cb->exclusions->hidden_refs_configured = 1;
+	cb->visibility->hidden_refs_configured = 1;
 	return parse_hide_refs_config(var, value, cb->section,
-				      &cb->exclusions->hidden_refs);
+				      &cb->visibility->hidden_refs);
 }
 
-void exclude_hidden_refs(struct ref_exclusions *exclusions, const char *section)
+void exclude_hidden_refs(struct ref_visibility *visibility, const char *section)
 {
 	struct exclude_hidden_refs_cb cb;
 
@@ -1588,10 +1588,10 @@ void exclude_hidden_refs(struct ref_exclusions *exclusions, const char *section)
 			strcmp(section, "uploadpack"))
 		die(_("unsupported section for hidden refs: %s"), section);
 
-	if (exclusions->hidden_refs_configured)
+	if (visibility->hidden_refs_configured)
 		die(_("--exclude-hidden= passed more than once"));
 
-	cb.exclusions = exclusions;
+	cb.visibility = visibility;
 	cb.section = section;
 
 	git_config(hide_refs_config, &cb);
@@ -1612,7 +1612,7 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
 	struct all_refs_cb *cb = cb_data;
 	struct object *object;
 
-	if (ref_excluded(&cb->all_revs->ref_excludes, path))
+	if (ref_excluded(&cb->all_revs->refs_visible, path))
 	    return 0;
 
 	object = get_reference(cb->all_revs, path, oid, cb->all_flags);
@@ -1935,7 +1935,7 @@ void repo_init_revisions(struct repository *r,
 
 	init_display_notes(&revs->notes_opt);
 	list_objects_filter_init(&revs->filter);
-	init_ref_exclusions(&revs->ref_excludes);
+	init_ref_visibility(&revs->refs_visible);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
@@ -2724,12 +2724,12 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 			init_all_refs_cb(&cb, revs, *flags);
 			other_head_refs(handle_one_ref, &cb);
 		}
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (!strcmp(arg, "--branches")) {
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --branches"));
 		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
 		handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
@@ -2737,48 +2737,48 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 			    for_each_good_bisect_ref);
 		revs->bisect = 1;
 	} else if (!strcmp(arg, "--tags")) {
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --tags"));
 		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (!strcmp(arg, "--remotes")) {
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --remotes"));
 		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref(handle_one_ref, optarg, &cb);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 		return argcount;
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
-		add_ref_exclusion(&revs->ref_excludes, optarg);
+		add_ref_exclusion(&revs->refs_visible, optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("exclude-hidden", argv, &optarg))) {
-		exclude_hidden_refs(&revs->ref_excludes, optarg);
+		exclude_hidden_refs(&revs->refs_visible, optarg);
 		return argcount;
 	} else if (skip_prefix(arg, "--branches=", &optarg)) {
 		struct all_refs_cb cb;
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --branches"));
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --tags"));
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
-		if (revs->ref_excludes.hidden_refs_configured)
+		if (revs->refs_visible.hidden_refs_configured)
 			return error(_("--exclude-hidden cannot be used together with --remotes"));
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
-		clear_ref_exclusions(&revs->ref_excludes);
+		clear_ref_visibility(&revs->refs_visible);
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
diff --git a/revision.h b/revision.h
index 25776af3815..8eaca125cd1 100644
--- a/revision.h
+++ b/revision.h
@@ -84,7 +84,7 @@ struct rev_cmdline_info {
 	} *rev;
 };
 
-struct ref_exclusions {
+struct ref_visibility {
 	/*
 	 * Excluded refs is a list of wildmatch patterns. If any of the
 	 * patterns match, the reference will be excluded.
@@ -106,9 +106,9 @@ struct ref_exclusions {
 };
 
 /**
- * Initialize a `struct ref_exclusions` with a macro.
+ * Initialize a `struct ref_visibility` with a macro.
  */
-#define REF_EXCLUSIONS_INIT { \
+#define REF_VISIBILITY_INIT { \
 	.excluded_refs = STRING_LIST_INIT_DUP, \
 	.hidden_refs = STRING_LIST_INIT_DUP, \
 }
@@ -135,7 +135,7 @@ struct rev_info {
 	struct list_objects_filter_options filter;
 
 	/* excluding from --branches, --refs, etc. expansion */
-	struct ref_exclusions ref_excludes;
+	struct ref_visibility refs_visible;
 
 	/* Basic information */
 	const char *prefix;
@@ -487,11 +487,11 @@ void show_object_with_name(FILE *, struct object *, const char *);
  * Helpers to check if a reference should be excluded.
  */
 
-int ref_excluded(const struct ref_exclusions *exclusions, const char *path);
-void init_ref_exclusions(struct ref_exclusions *);
-void clear_ref_exclusions(struct ref_exclusions *);
-void add_ref_exclusion(struct ref_exclusions *, const char *exclude);
-void exclude_hidden_refs(struct ref_exclusions *, const char *section);
+int ref_excluded(const struct ref_visibility *exclusions, const char *path);
+void init_ref_visibility(struct ref_visibility *);
+void clear_ref_visibility(struct ref_visibility *);
+void add_ref_exclusion(struct ref_visibility *, const char *exclude);
+void exclude_hidden_refs(struct ref_visibility *, const char *section);
 
 /**
  * This function can be used if you want to add commit objects as revision
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index a6977b5e839..504935c1d84 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -117,10 +117,10 @@ static struct flag_definition pack_flags[] = { FLAG_DEF(PACK_REFS_PRUNE),
 static int cmd_pack_refs(struct ref_store *refs, const char **argv)
 {
 	unsigned int flags = arg_flags(*argv++, "flags", pack_flags);
-	static struct ref_exclusions exclusions = REF_EXCLUSIONS_INIT;
+	static struct ref_visibility visibility = REF_VISIBILITY_INIT;
 	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
 	struct pack_refs_opts pack_opts = { .flags = flags,
-					    .exclusions = &exclusions,
+					    .visibility = &visibility,
 					    .includes = &included_refs };
 
 	if (pack_opts.flags & PACK_REFS_ALL)
-- 
gitgitgadget

Re: [PATCH 1/3] revision: rename ref_excludes to ref_visibility

[Taylor Blau]

On Wed, Jun 21, 2023 at 07:35:10PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> The ref_exclusions API provides the ability to check if certain refs are
> to be excluded. We can easily extend this API to check if certain refs
> are included, which [1] considered when teaching git-pack-refs the
> ability to specify not only refs to exclude but ones to include.
>
> A subsequent commit will actuall extend the API to add the ability to
> keep track of ref inclusions. As a preparatory patch, rename
>  ref_exclusions to ref_visibility.

Skimming through this patch, it looks like a straight-forward rename
that doesn't change any functionality. I think other readers may benefit
from a note that says something to that effect.

> ---
>  builtin/pack-refs.c       |  6 ++--
>  builtin/rev-parse.c       | 18 +++++-----
>  refs.h                    |  2 +-
>  refs/files-backend.c      |  2 +-
>  revision.c                | 72 +++++++++++++++++++--------------------
>  revision.h                | 18 +++++-----
>  t/helper/test-ref-store.c |  4 +--
>  7 files changed, 61 insertions(+), 61 deletions(-)

Obviously all of this looks OK.

Thanks,
Taylor

[PATCH 2/3] revision: add ref_visible() helper

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

In addition to helpers that check if a ref was excluded, teach the API a
ref_visible() function that takes into account both included
refs, excluded refs, and hidden refs.

Since exclusions take precedence over inclusion, a ref_included()
function is not necessary since a caller would always need to check
separately if the ref is excluded, in which case it would be easier to
call ref_visible().

Signed-off-by: John Cai <johncai86@gmail.com>
---
 revision.c | 32 ++++++++++++++++++++++++++------
 revision.h | 21 +++++++++++++++++++--
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 13e86a96498..54709e3f6fc 100644
--- a/revision.c
+++ b/revision.c
@@ -1533,20 +1533,34 @@ static void add_rev_cmdline_list(struct rev_info *revs,
 	}
 }
 
-int ref_excluded(const struct ref_visibility *visibility, const char *path)
+static int ref_matched(struct string_list refs, const char *path)
 {
-	const char *stripped_path = strip_namespace(path);
 	struct string_list_item *item;
 
-	for_each_string_list_item(item, &visibility->excluded_refs) {
+	for_each_string_list_item(item, &refs)
 		if (!wildmatch(item->string, path, 0))
 			return 1;
-	}
 
-	if (ref_is_hidden(stripped_path, path, &visibility->hidden_refs))
+	return 0;
+}
+
+int ref_excluded(const struct ref_visibility *visibility, const char *path)
+{
+	if (ref_is_hidden(strip_namespace(path), path, &visibility->hidden_refs))
 		return 1;
 
-	return 0;
+	return ref_matched(visibility->excluded_refs, path);
+}
+
+int ref_visible(const struct ref_visibility *visibility, const char *path)
+{
+	if (ref_is_hidden(strip_namespace(path), path, &visibility->hidden_refs))
+		return 0;
+
+	if (ref_matched(visibility->excluded_refs, path))
+		return 0;
+
+	return ref_matched(visibility->included_refs, path);
 }
 
 void init_ref_visibility(struct ref_visibility *visibility)
@@ -1558,6 +1572,7 @@ void init_ref_visibility(struct ref_visibility *visibility)
 void clear_ref_visibility(struct ref_visibility *visibility)
 {
 	string_list_clear(&visibility->excluded_refs, 0);
+	string_list_clear(&visibility->included_refs, 0);
 	string_list_clear(&visibility->hidden_refs, 0);
 	visibility->hidden_refs_configured = 0;
 }
@@ -1567,6 +1582,11 @@ void add_ref_exclusion(struct ref_visibility *visibility, const char *exclude)
 	string_list_append(&visibility->excluded_refs, exclude);
 }
 
+void add_ref_inclusion(struct ref_visibility *visibility, const char *include)
+{
+	string_list_append(&visibility->included_refs, include);
+}
+
 struct exclude_hidden_refs_cb {
 	struct ref_visibility *visibility;
 	const char *section;
diff --git a/revision.h b/revision.h
index 8eaca125cd1..fc26ec70b28 100644
--- a/revision.h
+++ b/revision.h
@@ -91,6 +91,12 @@ struct ref_visibility {
 	 */
 	struct string_list excluded_refs;
 
+	/*
+	 * Included refs is a list of wildmatch patterns. If any of the
+	 * patterns match, the reference will be included.
+	 */
+	struct string_list included_refs;
+
 	/*
 	 * Hidden refs is a list of patterns that is to be hidden via
 	 * `ref_is_hidden()`.
@@ -110,6 +116,7 @@ struct ref_visibility {
  */
 #define REF_VISIBILITY_INIT { \
 	.excluded_refs = STRING_LIST_INIT_DUP, \
+	.included_refs = STRING_LIST_INIT_DUP, \
 	.hidden_refs = STRING_LIST_INIT_DUP, \
 }
 
@@ -485,12 +492,22 @@ void show_object_with_name(FILE *, struct object *, const char *);
 
 /**
  * Helpers to check if a reference should be excluded.
+ *
+ * ref_excluded() checks if a ref has been explicitly excluded either
+ * because it is hidden, or it was added via an add_ref_exclusion() call.
+ *
+ * ref_visible() checks if a ref is visible by taking into account whether a ref
+ * has been included via an add_ref_inclusion() call, but also whether it has
+ * been excluded via add_ref_exclusion(). Exclusions take precedence. If a ref
+ * is hidden, it will also be treated as not visible.
+ *
  */
-
-int ref_excluded(const struct ref_visibility *exclusions, const char *path);
+int ref_excluded(const struct ref_visibility *visibility, const char *path);
+int ref_visible(const struct ref_visibility *visibility, const char *path);
 void init_ref_visibility(struct ref_visibility *);
 void clear_ref_visibility(struct ref_visibility *);
 void add_ref_exclusion(struct ref_visibility *, const char *exclude);
+void add_ref_inclusion(struct ref_visibility *, const char *include);
 void exclude_hidden_refs(struct ref_visibility *, const char *section);
 
 /**
-- 
gitgitgadget

[PATCH 3/3] pack-refs: use new ref_visible() helper

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

replace the current logic in pack-refs that takes into account included
refs with the new ref_visible() helper.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/pack-refs.c       | 14 ++++++++------
 refs/files-backend.c      | 11 +----------
 t/helper/test-ref-store.c |  6 ++----
 3 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index ff07986edaf..5d6dc363085 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -15,17 +15,16 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	unsigned int flags = PACK_REFS_PRUNE;
 	static struct ref_visibility visibility = REF_VISIBILITY_INIT;
-	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
 	struct pack_refs_opts pack_refs_opts = { .visibility = &visibility,
-						 .includes = &included_refs,
 						 .flags = flags };
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
+	static struct string_list option_included_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
 
 	struct option opts[] = {
 		OPT_BIT(0, "all",   &pack_refs_opts.flags, N_("pack everything"), PACK_REFS_ALL),
 		OPT_BIT(0, "prune", &pack_refs_opts.flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
-		OPT_STRING_LIST(0, "include", pack_refs_opts.includes, N_("pattern"),
+		OPT_STRING_LIST(0, "include", &option_included_refs, N_("pattern"),
 			N_("references to include")),
 		OPT_STRING_LIST(0, "exclude", &option_excluded_refs, N_("pattern"),
 			N_("references to exclude")),
@@ -38,11 +37,14 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	for_each_string_list_item(item, &option_excluded_refs)
 		add_ref_exclusion(pack_refs_opts.visibility, item->string);
 
+	for_each_string_list_item(item, &option_included_refs)
+		add_ref_inclusion(pack_refs_opts.visibility, item->string);
+
 	if (pack_refs_opts.flags & PACK_REFS_ALL)
-		string_list_append(pack_refs_opts.includes, "*");
+		add_ref_inclusion(pack_refs_opts.visibility, "*");
 
-	if (!pack_refs_opts.includes->nr)
-		string_list_append(pack_refs_opts.includes, "refs/tags/*");
+	if (!option_included_refs.nr)
+		add_ref_inclusion(pack_refs_opts.visibility, "refs/tags/*");
 
 	return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2e716a3e201..2dfe7f7e787 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1179,8 +1179,6 @@ static int should_pack_ref(const char *refname,
 			   const struct object_id *oid, unsigned int ref_flags,
 			   struct pack_refs_opts *opts)
 {
-	struct string_list_item *item;
-
 	/* Do not pack per-worktree refs: */
 	if (parse_worktree_ref(refname, NULL, NULL, NULL) !=
 	    REF_WORKTREE_SHARED)
@@ -1194,14 +1192,7 @@ static int should_pack_ref(const char *refname,
 	if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
 		return 0;
 
-	if (ref_excluded(opts->visibility, refname))
-		return 0;
-
-	for_each_string_list_item(item, opts->includes)
-		if (!wildmatch(item->string, refname, 0))
-			return 1;
-
-	return 0;
+	return ref_visible(opts->visibility, refname);
 }
 
 static int files_pack_refs(struct ref_store *ref_store,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 504935c1d84..81e0b5ea0a0 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -118,13 +118,11 @@ static int cmd_pack_refs(struct ref_store *refs, const char **argv)
 {
 	unsigned int flags = arg_flags(*argv++, "flags", pack_flags);
 	static struct ref_visibility visibility = REF_VISIBILITY_INIT;
-	static struct string_list included_refs = STRING_LIST_INIT_NODUP;
 	struct pack_refs_opts pack_opts = { .flags = flags,
-					    .visibility = &visibility,
-					    .includes = &included_refs };
+					    .visibility = &visibility };
 
 	if (pack_opts.flags & PACK_REFS_ALL)
-		string_list_append(pack_opts.includes, "*");
+		add_ref_inclusion(&visibility, "*");
 
 	return refs_pack_refs(refs, &pack_opts);
 }
-- 
gitgitgadget

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The ref_excludes API is used to tell which refs should be excluded. However,
> there are times when we would want to add refs to explicitly include as
> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
> taught pack-refs how to include certain refs, but did it in a more manual
> way by keeping the ref patterns in a separate string list. Instead, we can
> easily extend the ref_excludes API to include refs as well, since this use
> case fits into the API nicely.

Hmph, how would this interact with the other topic in flight that
touch the ref exclusion logic tb/refs-exclusion-and-packed-refs?

Thanks.

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Taylor Blau]

On Wed, Jun 21, 2023 at 01:56:13PM -0700, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > The ref_excludes API is used to tell which refs should be excluded. However,
> > there are times when we would want to add refs to explicitly include as
> > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
> > taught pack-refs how to include certain refs, but did it in a more manual
> > way by keeping the ref patterns in a separate string list. Instead, we can
> > easily extend the ref_excludes API to include refs as well, since this use
> > case fits into the API nicely.
>
> Hmph, how would this interact with the other topic in flight that
> touch the ref exclusion logic tb/refs-exclusion-and-packed-refs?

Good question. Besides trivial conflicts from John's patches to rename
this API, I think the sensible thing to do with my
tb/refs-exclusion-and-packed-refs topic would be to also refuse to use
the jump list if there are any non-trivial exclusion *or* inclusion
entries.

But I have some more general concerns about the approach taken by this
topic, namely that I do not understand a reference "foo" cannot be
included by adding a "!foo" entry to the excluded list.

Thanks,
Taylor

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Taylor Blau]

On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:
> The ref_excludes API is used to tell which refs should be excluded. However,
> there are times when we would want to add refs to explicitly include as
> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
> taught pack-refs how to include certain refs, but did it in a more manual
> way by keeping the ref patterns in a separate string list. Instead, we can
> easily extend the ref_excludes API to include refs as well, since this use
> case fits into the API nicely.

After reading this description, I am not sure why you can't "include" a
reference that would otherwise be excluded by passing the rules:

  - refs/heads/exclude/*
  - !refs/heads/exclude/but/include/me

(where the '!' prefix in the last rule is what brings back the included
reference).

But let's read on and see if there is something that I'm missing.

> Refactor the API by renaming it to ref_visibility, and add a ref_visible()
> helper that takes into account ref inclusion.

Hmm. Is this a replacement for ref_is_hidden(), which is already public?

Thanks,
Taylor

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Taylor Blau]

On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote:
> On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:
> > The ref_excludes API is used to tell which refs should be excluded. However,
> > there are times when we would want to add refs to explicitly include as
> > well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
> > taught pack-refs how to include certain refs, but did it in a more manual
> > way by keeping the ref patterns in a separate string list. Instead, we can
> > easily extend the ref_excludes API to include refs as well, since this use
> > case fits into the API nicely.
>
> After reading this description, I am not sure why you can't "include" a
> reference that would otherwise be excluded by passing the rules:
>
>   - refs/heads/exclude/*
>   - !refs/heads/exclude/but/include/me
>
> (where the '!' prefix in the last rule is what brings back the included
> reference).
>
> But let's read on and see if there is something that I'm missing.

Having read this series in detail, I am puzzled. I don't think that
there is any limitation of the existing reference hiding rules that
wouldn't permit what you're trying to do by adding the list of
references you want to include at the end of the exclude list, so long
as they are each prefixed with the magic "!" sentinel.

I think splitting the list of excluded references into individual
excluded and non-excluded references creates some awkwardness. For one:
excluded references already can cause us to include a reference, so
splitting that behavior across two lists seems difficult to reason
about.

For example, if your excluded list contains:

  - refs/heads/foo
  - refs/heads/bar
  - !refs/heads/foo/baz

and your included lists contains:

  - refs/heads/bar/baz/quux

I am left wondering: why doesn't the rule pertaining to
refs/heads/foo/baz show up in the included list? Likewise, what happens
with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the
question is which list takes priority.

Mostly, I am wondering if I am missing something that would explain why
you couldn't modify the above example's excluded list to contain
something like "!refs/heads/bar/baz/quux", eliminating the need for the
include list entirely.

Thanks,
Taylor

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Taylor Blau]

On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote:
> I am left wondering: why doesn't the rule pertaining to
> refs/heads/foo/baz show up in the included list? Likewise, what happens
> with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the
> question is which list takes priority.
>
> Mostly, I am wondering if I am missing something that would explain why
> you couldn't modify the above example's excluded list to contain
> something like "!refs/heads/bar/baz/quux", eliminating the need for the
> include list entirely.

Another potential quirk that I just now thought of: what are the rules
for what can go in the include list? Fully qualified references only? Or
can we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to
have the namespace-stripping operator ^, but not !, since negating an
include rule seems to imply that it should be in the exclude list.

Thanks,
Taylor

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Taylor Blau]

On Thu, Jun 22, 2023 at 08:53:53AM -0400, Taylor Blau wrote:
> On Thu, Jun 22, 2023 at 08:49:43AM -0400, Taylor Blau wrote:
> > I am left wondering: why doesn't the rule pertaining to
> > refs/heads/foo/baz show up in the included list? Likewise, what happens
> > with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the
> > question is which list takes priority.
> >
> > Mostly, I am wondering if I am missing something that would explain why
> > you couldn't modify the above example's excluded list to contain
> > something like "!refs/heads/bar/baz/quux", eliminating the need for the
> > include list entirely.
>
> Another potential quirk that I just now thought of: what are the rules
> for what can go in the include list? Fully qualified references only? Or
> can we have patterns (e.g. refs/foo/bar/*). Presumably you'd want to
> have the namespace-stripping operator ^, but not !, since negating an
> include rule seems to imply that it should be in the exclude list.

Sorry for the long chain of self-replies. I think one clarifying point
that I am not sure of yet is whether or not the exclude rules you're
talking about are interpreted as patterns (as in transfer.hideRefs) or
wildmatch patterns.

If they are wildmatch patterns, would it suffice to add the references
you *do* want to enumerate to the traversal ahead of time? There is also
the hidden_refs member of that struct, so perhaps adding negated entries
there would work.

Either way, I think emphasizing the difference between ref exclusions as
it pertains to traversal, and ref exclusions as it pertains to hiding
would greatly help other reviewers of this series.

Thanks,
Taylor

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[John Cai]

Hi Taylor,

On 22 Jun 2023, at 8:49, Taylor Blau wrote:

> On Thu, Jun 22, 2023 at 08:42:24AM -0400, Taylor Blau wrote:
>> On Wed, Jun 21, 2023 at 07:35:09PM +0000, John Cai via GitGitGadget wrote:
>>> The ref_excludes API is used to tell which refs should be excluded. However,
>>> there are times when we would want to add refs to explicitly include as
>>> well. 4fe42f326e (pack-refs: teach pack-refs --include option, 2023-05-12)
>>> taught pack-refs how to include certain refs, but did it in a more manual
>>> way by keeping the ref patterns in a separate string list. Instead, we can
>>> easily extend the ref_excludes API to include refs as well, since this use
>>> case fits into the API nicely.
>>
>> After reading this description, I am not sure why you can't "include" a
>> reference that would otherwise be excluded by passing the rules:
>>
>>   - refs/heads/exclude/*
>>   - !refs/heads/exclude/but/include/me
>>
>> (where the '!' prefix in the last rule is what brings back the included
>> reference).
>>
>> But let's read on and see if there is something that I'm missing.
>
> Having read this series in detail, I am puzzled. I don't think that
> there is any limitation of the existing reference hiding rules that
> wouldn't permit what you're trying to do by adding the list of
> references you want to include at the end of the exclude list, so long
> as they are each prefixed with the magic "!" sentinel.

To be honest, I had no idea "!" would have this effect--so thanks for bringing
it to my attention.
>
> I think splitting the list of excluded references into individual
> excluded and non-excluded references creates some awkwardness. For one:
> excluded references already can cause us to include a reference, so
> splitting that behavior across two lists seems difficult to reason
> about.
>
> For example, if your excluded list contains:
>
>   - refs/heads/foo
>   - refs/heads/bar
>   - !refs/heads/foo/baz
>
> and your included lists contains:
>
>   - refs/heads/bar/baz/quux
>
> I am left wondering: why doesn't the rule pertaining to
> refs/heads/foo/baz show up in the included list? Likewise, what happens
> with refs/heads/bar/baz/quux? It is a child of an excluded rule, so the
> question is which list takes priority.

Now knowing the effect of "!", I understand the concerns about having a
separate list for included references. However, I do think it would be odd if
there is a ref_included() function to also use ref_excluded() to include a ref.

Also, even with the current API, I think it can be confusing to reason about
what takes precedence if there is a mix of inclusions and exclusions. For
example, if the excluded list contains:

- refs/heads/foo/baz
- !refs/heads/foo

would the inclusion take precedence, or the exclusion?

>
> Mostly, I am wondering if I am missing something that would explain why
> you couldn't modify the above example's excluded list to contain
> something like "!refs/heads/bar/baz/quux", eliminating the need for the
> include list entirely.

I think my one reservation however, is with usability of the current API. It's
not very intuitive to include references by adding them with
ref_excluded(&exclusions, "!ref/to/be/included").

I do think it would be easier to reason about if we kept two separate lists, one
for inclusion and one for exclusion. The existence of the "!" magic sentinel
does make things much more confusing however. I almost want to remove support
for the magic sentinel in favor of keeping two distinct lists that cannot be
mixed. Wondering your thoughts on that approach?

>
> Thanks,
> Taylor

thanks!
JOhn

Re: [PATCH 0/3] revision: refactor ref_excludes to ref_visibility

[Junio C Hamano]

John Cai <johncai86@gmail.com> writes:

>>> After reading this description, I am not sure why you can't "include" a
>>> reference that would otherwise be excluded by passing the rules:
>>>
>>>   - refs/heads/exclude/*
>>>   - !refs/heads/exclude/but/include/me
>>>
>>> (where the '!' prefix in the last rule is what brings back the included
>>> reference).
>>>
>>> But let's read on and see if there is something that I'm missing.
>>
>> Having read this series in detail, I am puzzled. I don't think that
>> there is any limitation of the existing reference hiding rules that
>> wouldn't permit what you're trying to do by adding the list of
>> references you want to include at the end of the exclude list, so long
>> as they are each prefixed with the magic "!" sentinel.
>
> To be honest, I had no idea "!" would have this effect--so thanks for bringing
> it to my attention.

FWIW, "--exclude=!" gets zero hits in t/ directory.

ref_excluded() merely calls wildmatch() like so:

        int ref_excluded(const struct ref_exclusions *exclusions, const char *path)
        {
                const char *stripped_path = strip_namespace(path);
                struct string_list_item *item;

                for_each_string_list_item(item, &exclusions->excluded_refs) {
                        if (!wildmatch(item->string, path, 0))
                                return 1;
                }

                if (ref_is_hidden(stripped_path, path, &exclusions->hidden_refs))
                        return 1;

                return 0;
        }

so I do not know what to think about it.  This is called from inside
callback of things like "log --exclude=A --exclude=B ... --all" when
we are trying to add all refs in response to "--all", and it appears
to me that the first match would already determine the ref's fate
without even looking at the later patterns (prefixed with bang '!'
or not).  Taylor, am I looking at a wrong code?

Puzzled...