diff mbox series

[v3,3/4] revision: modify ref_exclusions to handle inclusions

Message ID 0a0693ad612755e675861dfa5a660baf8d325ed0.1683828635.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series pack-refs: allow users control over which refs to pack | expand

Commit Message

John Cai May 11, 2023, 6:10 p.m. UTC 
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 a subsequent commit will make use of.

Rename ref_exclusions to ref_visibility and add a member to keep track
of inclusions. Add add_ref_inclusion(), ref_included() to be used for
refs to explicitly include.

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           | 71 +++++++++++++++++++++++++++-----------------
 revision.h           | 26 ++++++++++------
 6 files changed, 75 insertions(+), 50 deletions(-)

Comments

Junio C Hamano May 11, 2023, 7:54 p.m. UTC | #1 
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static int ref_matched(const char *path,
> +		       const struct string_list *l,
> +		       const struct string_list *hidden_refs)
>  {
>  	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, l) {
>  		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, hidden_refs))
>  		return 1;
>  
>  	return 0;
>  }
>
> -void init_ref_exclusions(struct ref_exclusions *exclusions)
> +int ref_excluded(const struct ref_visibility *visibility, const char *path)
>  {
> -	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
> -	memcpy(exclusions, &blank, sizeof(*exclusions));
> +	return ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);
>  }
>  
> -void clear_ref_exclusions(struct ref_exclusions *exclusions)
> +int ref_included(const struct ref_visibility *visibility, const char *path)
>  {
> -	string_list_clear(&exclusions->excluded_refs, 0);
> -	string_list_clear(&exclusions->hidden_refs, 0);
> -	exclusions->hidden_refs_configured = 0;
> +	return ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);
>  }

It is unexpected that we do "hidden" inside ref_matched().  I would
have expected that no matter what exclusion or inclusion patterns
say, hidden things are to be kept hidden.  I.e.  I expected

 - ref_matched(), which takes a path and a list of patterns, checks
   if the path matches any of the given patterns;

 - ref_excluded(), whcih takes a path and a visibility, asks
   ref_matched() if the path matches visibility->excluded_refs and
   relays its answer to the caller.

 - ref_included(), which takes a path and a visibility, asks
   ref_matched() if the path matches visibility->included_refs and
   relays its answer to the caller.

 - ref_visibility(), which takes a path and a visibility, goes
   through the following sequence:

   - if ref_is_hidden() says that the path is hidden, it is not
     visible;

   - if ref_excluded() says the path is excluded, it is not visible;

   - if ref_included() says the path is included, it is visible;

   - if none of the above triggers, the fate of the path is
     determined by some default logic.

or something along that line.  That would also allow us to avoid
having to call ref_is_hidden() more than once when we need to check
both inclusion and exclusion list.

But perhaps I am missing some requirements to be taken into
account, so let me read on.

To be honest, I didn't expect the "exclusions can be extended",
which I alluded to as a future, possible, follow-on work, to be
included as a part of this topic.  I appreciate your going an extra
mile, but I am not sure if it was a good change in the context of
this series.  With this patch, it is not trivial to even validate
that there is no behaviour change to any users of "struct
ref_exclusions" when the included_refs list is empty, which is an
absolute must to avoid regressions.
John Cai May 12, 2023, 2:56 p.m. UTC | #2 
Hi Junio,

On 11 May 2023, at 15:54, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +static int ref_matched(const char *path,
>> +		       const struct string_list *l,
>> +		       const struct string_list *hidden_refs)
>>  {
>>  	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, l) {
>>  		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, hidden_refs))
>>  		return 1;
>>
>>  	return 0;
>>  }
>>
>> -void init_ref_exclusions(struct ref_exclusions *exclusions)
>> +int ref_excluded(const struct ref_visibility *visibility, const char *path)
>>  {
>> -	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
>> -	memcpy(exclusions, &blank, sizeof(*exclusions));
>> +	return ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);
>>  }
>>
>> -void clear_ref_exclusions(struct ref_exclusions *exclusions)
>> +int ref_included(const struct ref_visibility *visibility, const char *path)
>>  {
>> -	string_list_clear(&exclusions->excluded_refs, 0);
>> -	string_list_clear(&exclusions->hidden_refs, 0);
>> -	exclusions->hidden_refs_configured = 0;
>> +	return ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);
>>  }
>
> It is unexpected that we do "hidden" inside ref_matched().  I would
> have expected that no matter what exclusion or inclusion patterns
> say, hidden things are to be kept hidden.  I.e.  I expected

Oops. Yes this was an oversight.

>
>  - ref_matched(), which takes a path and a list of patterns, checks
>    if the path matches any of the given patterns;
>
>  - ref_excluded(), whcih takes a path and a visibility, asks
>    ref_matched() if the path matches visibility->excluded_refs and
>    relays its answer to the caller.
>
>  - ref_included(), which takes a path and a visibility, asks
>    ref_matched() if the path matches visibility->included_refs and
>    relays its answer to the caller.
>
>  - ref_visibility(), which takes a path and a visibility, goes
>    through the following sequence:
>
>    - if ref_is_hidden() says that the path is hidden, it is not
>      visible;
>
>    - if ref_excluded() says the path is excluded, it is not visible;
>
>    - if ref_included() says the path is included, it is visible;
>
>    - if none of the above triggers, the fate of the path is
>      determined by some default logic.

That's a good point. I didn't think about adding a ref_visibility() function
that conslidates inclusion and exclusion.
>
> or something along that line.  That would also allow us to avoid
> having to call ref_is_hidden() more than once when we need to check
> both inclusion and exclusion list.
>
> But perhaps I am missing some requirements to be taken into
> account, so let me read on.
>
> To be honest, I didn't expect the "exclusions can be extended",
> which I alluded to as a future, possible, follow-on work, to be
> included as a part of this topic.  I appreciate your going an extra
> mile, but I am not sure if it was a good change in the context of
> this series.  With this patch, it is not trivial to even validate
> that there is no behaviour change to any users of "struct
> ref_exclusions" when the included_refs list is empty, which is an
> absolute must to avoid regressions.

Okay, maybe we can include this refactor in a subsequent series then.

thanks
John
diff mbox series

Patch

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 1d1a64fe386..2464575a665 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -14,8 +14,8 @@  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;
-	struct pack_refs_opts pack_refs_opts = {.exclusions = &excludes, .flags = flags};
+	static struct ref_visibility visibility = REF_VISIBILITY_INIT;
+	struct pack_refs_opts pack_refs_opts = {.visibility = &visibility, .flags = flags};
 	static struct string_list option_excluded_refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item;
 
@@ -31,7 +31,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);
 
 	return refs_pack_refs(get_main_ref_store(the_repository), &pack_refs_opts);
 }
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e3403..9036d8876fc 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 ref_visibility = 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(&ref_visibility, 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(&ref_visibility);
 }
 
 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(&ref_visibility);
 				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 (ref_visibility.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 (ref_visibility.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 (ref_visibility.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(&ref_visibility, arg);
 				continue;
 			}
 			if (skip_prefix(arg, "--exclude-hidden=", &arg)) {
-				exclude_hidden_refs(&ref_excludes, arg);
+				exclude_hidden_refs(&ref_visibility, arg);
 				continue;
 			}
 			if (!strcmp(arg, "--show-toplevel")) {
diff --git a/refs.h b/refs.h
index 46020bd335c..57949a1726b 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;
 };
 
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 76aebfde695..3ef19199788 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1183,7 +1183,7 @@  static int should_pack_ref(const char *refname,
 	    REF_WORKTREE_SHARED)
 		return 0;
 
-	if (opts->exclusions && ref_excluded(opts->exclusions, refname))
+	if (opts->visibility && ref_excluded(opts->visibility, refname))
 		return 0;
 
 	/* Do not pack non-tags unless PACK_REFS_ALL is set: */
diff --git a/revision.c b/revision.c
index b33cc1d106a..d59cd728e9e 100644
--- a/revision.c
+++ b/revision.c
@@ -1533,54 +1533,71 @@  static void add_rev_cmdline_list(struct rev_info *revs,
 	}
 }
 
-int ref_excluded(const struct ref_exclusions *exclusions, const char *path)
+static int ref_matched(const char *path,
+		       const struct string_list *l,
+		       const struct string_list *hidden_refs)
 {
 	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, l) {
 		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, hidden_refs))
 		return 1;
 
 	return 0;
 }
 
-void init_ref_exclusions(struct ref_exclusions *exclusions)
+int ref_excluded(const struct ref_visibility *visibility, const char *path)
 {
-	struct ref_exclusions blank = REF_EXCLUSIONS_INIT;
-	memcpy(exclusions, &blank, sizeof(*exclusions));
+	return ref_matched(path, &visibility->excluded_refs, &visibility->hidden_refs);
 }
 
-void clear_ref_exclusions(struct ref_exclusions *exclusions)
+int ref_included(const struct ref_visibility *visibility, const char *path)
 {
-	string_list_clear(&exclusions->excluded_refs, 0);
-	string_list_clear(&exclusions->hidden_refs, 0);
-	exclusions->hidden_refs_configured = 0;
+	return ref_matched(path, &visibility->included_refs, &visibility->hidden_refs);
 }
 
-void add_ref_exclusion(struct ref_exclusions *exclusions, const char *exclude)
+void init_ref_visibility(struct ref_visibility *visibility)
 {
-	string_list_append(&exclusions->excluded_refs, exclude);
+	struct ref_visibility blank = REF_VISIBILITY_INIT;
+	memcpy(visibility, &blank, sizeof(*visibility));
+}
+
+void clear_ref_visibility(struct ref_visibility *visibility)
+{
+	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_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_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 +1605,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);
@@ -1935,7 +1952,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->ref_excludes);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
@@ -2724,12 +2741,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->ref_excludes);
 	} else if (!strcmp(arg, "--branches")) {
 		if (revs->ref_excludes.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->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
 		handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
@@ -2740,17 +2757,17 @@  static int handle_revision_pseudo_opt(struct rev_info *revs,
 		if (revs->ref_excludes.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->ref_excludes);
 	} else if (!strcmp(arg, "--remotes")) {
 		if (revs->ref_excludes.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->ref_excludes);
 	} 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->ref_excludes);
 		return argcount;
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
@@ -2764,21 +2781,21 @@  static int handle_revision_pseudo_opt(struct rev_info *revs,
 			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->ref_excludes);
 	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
 		if (revs->ref_excludes.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->ref_excludes);
 	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
 		if (revs->ref_excludes.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->ref_excludes);
 	} 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..296c520a003 100644
--- a/revision.h
+++ b/revision.h
@@ -84,7 +84,12 @@  struct rev_cmdline_info {
 	} *rev;
 };
 
-struct ref_exclusions {
+struct ref_visibility {
+	/*
+	 * Included refs is a list of wildmatch patterns. If any of the
+	 * patterns match, the reference will be included.
+	 */
+	struct string_list included_refs;
 	/*
 	 * Excluded refs is a list of wildmatch patterns. If any of the
 	 * patterns match, the reference will be excluded.
@@ -106,9 +111,10 @@  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 { \
+	.included_refs = STRING_LIST_INIT_DUP, \
 	.excluded_refs = STRING_LIST_INIT_DUP, \
 	.hidden_refs = STRING_LIST_INIT_DUP, \
 }
@@ -135,7 +141,7 @@  struct rev_info {
 	struct list_objects_filter_options filter;
 
 	/* excluding from --branches, --refs, etc. expansion */
-	struct ref_exclusions ref_excludes;
+	struct ref_visibility ref_excludes;
 
 	/* Basic information */
 	const char *prefix;
@@ -487,11 +493,13 @@  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);
+int ref_included(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 add_ref_inclusion(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