diff mbox series

[v4,05/16] ref-filter.c: parameterize match functions over patterns

Message ID 39e9b0f50d07cc75219325e7c7e72a801ca0cf16.1687270849.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series refs: implement jump lists for packed backend | expand

Commit Message

Taylor Blau June 20, 2023, 2:21 p.m. UTC 
From: Jeff King <peff@peff.net>

`match_pattern()` and `match_name_as_path()` both take a `struct
ref_filter *`, and then store a stack variable `patterns` pointing at
`filter->patterns`.

The subsequent patch will add a new array of patterns to match over (the
excluded patterns, via a new `git for-each-ref --exclude` option),
treating the return value of these functions differently depending on
which patterns are being used to match.

Tweak `match_pattern()` and `match_name_as_path()` to take an array of
patterns to prepare for passing either in.

Once we start passing either in, `match_pattern()` will have little to
do with a particular `struct ref_filter *` instance. To clarify this,
drop it from the argument list, and replace it with the only bit of the
`ref_filter` that we care about (`filter->ignore_case`).

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 ref-filter.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Jeff King July 3, 2023, 5:27 a.m. UTC | #1 
On Tue, Jun 20, 2023 at 10:21:21AM -0400, Taylor Blau wrote:

> Once we start passing either in, `match_pattern()` will have little to
> do with a particular `struct ref_filter *` instance. To clarify this,
> drop it from the argument list, and replace it with the only bit of the
> `ref_filter` that we care about (`filter->ignore_case`).

Makes sense, but...

> @@ -2134,9 +2134,10 @@ static int match_pattern(const struct ref_filter *filter, const char *refname)
>   * matches a pattern "refs/heads/" but not "refs/heads/m") or a
>   * wildcard (e.g. the same ref matches "refs/heads/m*", too).
>   */
> -static int match_name_as_path(const struct ref_filter *filter, const char *refname)
> +static int match_name_as_path(const struct ref_filter *filter,
> +			      const char **pattern,
> +			      const char *refname)

...wouldn't we then want to do the same for match_name_as_path()?

I.e., this:

diff --git a/ref-filter.c b/ref-filter.c
index 6aacb99be7..cf10c753e2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2134,14 +2134,13 @@ static int match_pattern(const char **patterns, const char *refname,
  * matches a pattern "refs/heads/" but not "refs/heads/m") or a
  * wildcard (e.g. the same ref matches "refs/heads/m*", too).
  */
-static int match_name_as_path(const struct ref_filter *filter,
-			      const char **pattern,
-			      const char *refname)
+static int match_name_as_path(const char **pattern, const char *refname,
+			      int ignore_case)
 {
 	int namelen = strlen(refname);
 	unsigned flags = WM_PATHNAME;
 
-	if (filter->ignore_case)
+	if (ignore_case)
 		flags |= WM_CASEFOLD;
 
 	for (; *pattern; pattern++) {
@@ -2166,7 +2165,8 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)
 	if (!*filter->name_patterns)
 		return 1; /* No pattern always matches */
 	if (filter->match_as_path)
-		return match_name_as_path(filter, filter->name_patterns, refname);
+		return match_name_as_path(filter->name_patterns, refname,
+					  filter->ignore_case);
 	return match_pattern(filter->name_patterns, refname,
 			     filter->ignore_case);
 }

Also, I noticed that you declared it as "const int ignore_case" in
match_pattern(). That's not wrong, but we usually do not bother (it is
passed by value, so const-ness is irrelevant to the caller, and the
compiler can see inside the function that the value is not changed and
optimize appropriately).

-Peff
Taylor Blau July 3, 2023, 5:18 p.m. UTC | #2 
On Mon, Jul 03, 2023 at 01:27:24AM -0400, Jeff King wrote:
> On Tue, Jun 20, 2023 at 10:21:21AM -0400, Taylor Blau wrote:
>
> > Once we start passing either in, `match_pattern()` will have little to
> > do with a particular `struct ref_filter *` instance. To clarify this,
> > drop it from the argument list, and replace it with the only bit of the
> > `ref_filter` that we care about (`filter->ignore_case`).
>
> Makes sense, but...
>
> > @@ -2134,9 +2134,10 @@ static int match_pattern(const struct ref_filter *filter, const char *refname)
> >   * matches a pattern "refs/heads/" but not "refs/heads/m") or a
> >   * wildcard (e.g. the same ref matches "refs/heads/m*", too).
> >   */
> > -static int match_name_as_path(const struct ref_filter *filter, const char *refname)
> > +static int match_name_as_path(const struct ref_filter *filter,
> > +			      const char **pattern,
> > +			      const char *refname)
>
> ...wouldn't we then want to do the same for match_name_as_path()?

Yes, definitely :-). I'm not sure how I missed this, since the patch
message even says that `match_name_as_path()` gets the same treatment as
the other function.

But in any case, I obviously agree (and the diff below makes sense to
me). Applied.

> Also, I noticed that you declared it as "const int ignore_case" in
> match_pattern(). That's not wrong, but we usually do not bother (it is
> passed by value, so const-ness is irrelevant to the caller, and the
> compiler can see inside the function that the value is not changed and
> optimize appropriately).

Indeed :-).

Thanks,
Taylor
Taylor Blau July 3, 2023, 5:22 p.m. UTC | #3 
On Mon, Jul 03, 2023 at 01:18:06PM -0400, Taylor Blau wrote:
> On Mon, Jul 03, 2023 at 01:27:24AM -0400, Jeff King wrote:
> > On Tue, Jun 20, 2023 at 10:21:21AM -0400, Taylor Blau wrote:
> >
> > > Once we start passing either in, `match_pattern()` will have little to
> > > do with a particular `struct ref_filter *` instance. To clarify this,
> > > drop it from the argument list, and replace it with the only bit of the
> > > `ref_filter` that we care about (`filter->ignore_case`).
> >
> > Makes sense, but...
> >
> > > @@ -2134,9 +2134,10 @@ static int match_pattern(const struct ref_filter *filter, const char *refname)
> > >   * matches a pattern "refs/heads/" but not "refs/heads/m") or a
> > >   * wildcard (e.g. the same ref matches "refs/heads/m*", too).
> > >   */
> > > -static int match_name_as_path(const struct ref_filter *filter, const char *refname)
> > > +static int match_name_as_path(const struct ref_filter *filter,
> > > +			      const char **pattern,
> > > +			      const char *refname)
> >
> > ...wouldn't we then want to do the same for match_name_as_path()?
>
> Yes, definitely :-). I'm not sure how I missed this, since the patch
> message even says that `match_name_as_path()` gets the same treatment as
> the other function.
>
> But in any case, I obviously agree (and the diff below makes sense to
> me). Applied.

Ah, this is missing one more spot (that we wouldn't complain about
during a non-DEVELOPER build). This needs to go on top, but I otherwise
agree:

--- 8< ---
diff --git a/ref-filter.c b/ref-filter.c
index de85904b8d..845173a904 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2176,7 +2176,8 @@ static int filter_exclude_match(struct ref_filter *filter, const char *refname)
 	if (!filter->exclude.nr)
 		return 0;
 	if (filter->match_as_path)
-		return match_name_as_path(filter, filter->exclude.v, refname);
+		return match_name_as_path(filter->exclude.v, refname,
+					  filter->ignore_case);
 	return match_pattern(filter->exclude.v, refname, filter->ignore_case);
 }
--- >8 ---

Thanks,
Taylor
Jeff King July 3, 2023, 5:33 p.m. UTC | #4 
On Mon, Jul 03, 2023 at 01:22:28PM -0400, Taylor Blau wrote:

> On Mon, Jul 03, 2023 at 01:18:06PM -0400, Taylor Blau wrote:
> > On Mon, Jul 03, 2023 at 01:27:24AM -0400, Jeff King wrote:
> > > On Tue, Jun 20, 2023 at 10:21:21AM -0400, Taylor Blau wrote:
> > >
> > > > Once we start passing either in, `match_pattern()` will have little to
> > > > do with a particular `struct ref_filter *` instance. To clarify this,
> > > > drop it from the argument list, and replace it with the only bit of the
> > > > `ref_filter` that we care about (`filter->ignore_case`).
> > >
> > > Makes sense, but...
> > >
> > > > @@ -2134,9 +2134,10 @@ static int match_pattern(const struct ref_filter *filter, const char *refname)
> > > >   * matches a pattern "refs/heads/" but not "refs/heads/m") or a
> > > >   * wildcard (e.g. the same ref matches "refs/heads/m*", too).
> > > >   */
> > > > -static int match_name_as_path(const struct ref_filter *filter, const char *refname)
> > > > +static int match_name_as_path(const struct ref_filter *filter,
> > > > +			      const char **pattern,
> > > > +			      const char *refname)
> > >
> > > ...wouldn't we then want to do the same for match_name_as_path()?
> >
> > Yes, definitely :-). I'm not sure how I missed this, since the patch
> > message even says that `match_name_as_path()` gets the same treatment as
> > the other function.
> >
> > But in any case, I obviously agree (and the diff below makes sense to
> > me). Applied.
> 
> Ah, this is missing one more spot (that we wouldn't complain about
> during a non-DEVELOPER build). This needs to go on top, but I otherwise
> agree:

Heh, yes. I was applying them in order, and had to make the same fixup
on top of the next patch. I think that was the only other fallout as I
applied the rest, though.

-Peff
diff mbox series

Patch

diff --git a/ref-filter.c b/ref-filter.c
index d32f426898..6d91c7cb0d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2104,12 +2104,12 @@  static int get_ref_atom_value(struct ref_array_item *ref, int atom,
  * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
  * matches "refs/heads/mas*", too).
  */
-static int match_pattern(const struct ref_filter *filter, const char *refname)
+static int match_pattern(const char **patterns, const char *refname,
+			 const int ignore_case)
 {
-	const char **patterns = filter->name_patterns;
 	unsigned flags = 0;
 
-	if (filter->ignore_case)
+	if (ignore_case)
 		flags |= WM_CASEFOLD;
 
 	/*
@@ -2134,9 +2134,10 @@  static int match_pattern(const struct ref_filter *filter, const char *refname)
  * matches a pattern "refs/heads/" but not "refs/heads/m") or a
  * wildcard (e.g. the same ref matches "refs/heads/m*", too).
  */
-static int match_name_as_path(const struct ref_filter *filter, const char *refname)
+static int match_name_as_path(const struct ref_filter *filter,
+			      const char **pattern,
+			      const char *refname)
 {
-	const char **pattern = filter->name_patterns;
 	int namelen = strlen(refname);
 	unsigned flags = WM_PATHNAME;
 
@@ -2165,8 +2166,9 @@  static int filter_pattern_match(struct ref_filter *filter, const char *refname)
 	if (!*filter->name_patterns)
 		return 1; /* No pattern always matches */
 	if (filter->match_as_path)
-		return match_name_as_path(filter, refname);
-	return match_pattern(filter, refname);
+		return match_name_as_path(filter, filter->name_patterns, refname);
+	return match_pattern(filter->name_patterns, refname,
+			     filter->ignore_case);
 }
 
 /*