diff mbox series

[v2,2/3] refspec: relocate query related functions

Message ID 20250127103644.36627-3-meetsoni3017@gmail.com (mailing list archive)
State Superseded
Headers show
Series refspec: centralize refspec-related logic | expand

Commit Message

Meet Soni Jan. 27, 2025, 10:36 a.m. UTC 
Move the functions `query_refspecs()`, `query_refspecs_multiple()` and
`query_matches_negative_refspec()` from `remote.c` to `refspec.c`. These
functions focus on querying refspecs, so centralizing them in `refspec.c`
improves code organization by keeping refspec-related logic in one place.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 refspec.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 refspec.h |  16 +++++++
 remote.c  | 122 -----------------------------------------------------
 remote.h  |   1 -
 4 files changed, 139 insertions(+), 123 deletions(-)

Comments

Junio C Hamano Jan. 27, 2025, 7:25 p.m. UTC | #1 
Meet Soni <meetsoni3017@gmail.com> writes:

> Move the functions `query_refspecs()`, `query_refspecs_multiple()` and
> `query_matches_negative_refspec()` from `remote.c` to `refspec.c`. These
> functions focus on querying refspecs, so centralizing them in `refspec.c`
> improves code organization by keeping refspec-related logic in one place.

I think query_matches_negative_refspec() is appropriate named (not
that it matters much, as it becomes a mere private helper in the
file), unlike the ones in the first patch that are suboptimally
named.  query_refspecs() could probalby lose the plural 's' at the
end---there is only single refspec, which is a collection of refspec
items, involved and it makes a single query---but otherwise it also
has an appropriate name (this matters a bit more, but not that much,
as it was already public).

query_refspecs_multiple() is not a great name, though.  It does not
convey what is multiple.  Does it make multiple questions in one go?
Does it ask a question that can have multiple answers?

> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> ---
>  refspec.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  refspec.h |  16 +++++++
>  remote.c  | 122 -----------------------------------------------------
>  remote.h  |   1 -
>  4 files changed, 139 insertions(+), 123 deletions(-)

> diff --git a/refspec.h b/refspec.h
> index 891d50b159..d0788de782 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -30,6 +30,8 @@ struct refspec_item {
>  	char *raw;
>  };
>  
> +struct string_list;
> +
>  #define REFSPEC_FETCH 1
>  #define REFSPEC_PUSH 0
>  
> @@ -84,4 +86,18 @@ int omit_name_by_refspec(const char *name, struct refspec *rs);
>  int match_name_with_pattern(const char *key, const char *name,
>  				   const char *value, char **result);
>  
> +/*
> + * Queries a refspec for a match and updates the query item.
> + * Returns 0 on success, -1 if no match is found or negative refspec matches.
> + */
> +int query_refspecs(struct refspec *rs, struct refspec_item *query);

This one now has an excellent comment.  Great job.

Thanks.
Meet Soni Jan. 29, 2025, 6:32 a.m. UTC | #2 
On Tue, 28 Jan 2025 at 00:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Meet Soni <meetsoni3017@gmail.com> writes:
>
> > Move the functions `query_refspecs()`, `query_refspecs_multiple()` and
> > `query_matches_negative_refspec()` from `remote.c` to `refspec.c`. These
> > functions focus on querying refspecs, so centralizing them in `refspec.c`
> > improves code organization by keeping refspec-related logic in one place.
>
> I think query_matches_negative_refspec() is appropriate named (not
> that it matters much, as it becomes a mere private helper in the
> file), unlike the ones in the first patch that are suboptimally
> named.  query_refspecs() could probalby lose the plural 's' at the
> end---there is only single refspec, which is a collection of refspec
> items, involved and it makes a single query---but otherwise it also
> has an appropriate name (this matters a bit more, but not that much,
> as it was already public).
>
> query_refspecs_multiple() is not a great name, though.  It does not
> convey what is multiple.  Does it make multiple questions in one go?
> Does it ask a question that can have multiple answers?
>
I agree that the original names are ambiguous. query_refspecs_multiple()
is similar to query_refspecs(), but instead of returning the first match, it
collects all matching results.

To improve clarity and consistency, I’d like to propose the following
renames:
    *query_refspecs() -> find_refspec_match()
        `find` better describes its purpose than `query` and `match`
        clarifies that it’s looking for a single result.

    *query_refspecs_multiple() -> find_all_refspec_matches()
        Unlike the previous function, this one collects all matching results
        instead of stopping at the first match. The new name highlights that
        it returns multiple matches.
Let me know what you think!

> > Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> > ---
> >  refspec.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  refspec.h |  16 +++++++
> >  remote.c  | 122 -----------------------------------------------------
> >  remote.h  |   1 -
> >  4 files changed, 139 insertions(+), 123 deletions(-)
>
> > diff --git a/refspec.h b/refspec.h
> > index 891d50b159..d0788de782 100644
> > --- a/refspec.h
> > +++ b/refspec.h
> > @@ -30,6 +30,8 @@ struct refspec_item {
> >       char *raw;
> >  };
> >
> > +struct string_list;
> > +
> >  #define REFSPEC_FETCH 1
> >  #define REFSPEC_PUSH 0
> >
> > @@ -84,4 +86,18 @@ int omit_name_by_refspec(const char *name, struct refspec *rs);
> >  int match_name_with_pattern(const char *key, const char *name,
> >                                  const char *value, char **result);
> >
> > +/*
> > + * Queries a refspec for a match and updates the query item.
> > + * Returns 0 on success, -1 if no match is found or negative refspec matches.
> > + */
> > +int query_refspecs(struct refspec *rs, struct refspec_item *query);
>
> This one now has an excellent comment.  Great job.
>
> Thanks.
diff mbox series

Patch

diff --git a/refspec.c b/refspec.c
index 66989a1d75..72b3911110 100644
--- a/refspec.c
+++ b/refspec.c
@@ -5,6 +5,7 @@ 
 #include "gettext.h"
 #include "hash.h"
 #include "hex.h"
+#include "string-list.h"
 #include "strvec.h"
 #include "refs.h"
 #include "refspec.h"
@@ -324,3 +325,125 @@  int omit_name_by_refspec(const char *name, struct refspec *rs)
 	}
 	return 0;
 }
+
+static int query_matches_negative_refspec(struct refspec *rs, struct refspec_item *query)
+{
+	int i, matched_negative = 0;
+	int find_src = !query->src;
+	struct string_list reversed = STRING_LIST_INIT_DUP;
+	const char *needle = find_src ? query->dst : query->src;
+
+	/*
+	 * Check whether the queried ref matches any negative refpsec. If so,
+	 * then we should ultimately treat this as not matching the query at
+	 * all.
+	 *
+	 * Note that negative refspecs always match the source, but the query
+	 * item uses the destination. To handle this, we apply pattern
+	 * refspecs in reverse to figure out if the query source matches any
+	 * of the negative refspecs.
+	 *
+	 * The first loop finds and expands all positive refspecs
+	 * matched by the queried ref.
+	 *
+	 * The second loop checks if any of the results of the first loop
+	 * match any negative refspec.
+	 */
+	for (i = 0; i < rs->nr; i++) {
+		struct refspec_item *refspec = &rs->items[i];
+		char *expn_name;
+
+		if (refspec->negative)
+			continue;
+
+		/* Note the reversal of src and dst */
+		if (refspec->pattern) {
+			const char *key = refspec->dst ? refspec->dst : refspec->src;
+			const char *value = refspec->src;
+
+			if (match_name_with_pattern(key, needle, value, &expn_name))
+				string_list_append_nodup(&reversed, expn_name);
+		} else if (refspec->matching) {
+			/* For the special matching refspec, any query should match */
+			string_list_append(&reversed, needle);
+		} else if (!refspec->src) {
+			BUG("refspec->src should not be null here");
+		} else if (!strcmp(needle, refspec->src)) {
+			string_list_append(&reversed, refspec->src);
+		}
+	}
+
+	for (i = 0; !matched_negative && i < reversed.nr; i++) {
+		if (omit_name_by_refspec(reversed.items[i].string, rs))
+			matched_negative = 1;
+	}
+
+	string_list_clear(&reversed, 0);
+
+	return matched_negative;
+}
+
+void query_refspecs_multiple(struct refspec *rs,
+				    struct refspec_item *query,
+				    struct string_list *results)
+{
+	int i;
+	int find_src = !query->src;
+
+	if (find_src && !query->dst)
+		BUG("query_refspecs_multiple: need either src or dst");
+
+	if (query_matches_negative_refspec(rs, query))
+		return;
+
+	for (i = 0; i < rs->nr; i++) {
+		struct refspec_item *refspec = &rs->items[i];
+		const char *key = find_src ? refspec->dst : refspec->src;
+		const char *value = find_src ? refspec->src : refspec->dst;
+		const char *needle = find_src ? query->dst : query->src;
+		char **result = find_src ? &query->src : &query->dst;
+
+		if (!refspec->dst || refspec->negative)
+			continue;
+		if (refspec->pattern) {
+			if (match_name_with_pattern(key, needle, value, result))
+				string_list_append_nodup(results, *result);
+		} else if (!strcmp(needle, key)) {
+			string_list_append(results, value);
+		}
+	}
+}
+
+int query_refspecs(struct refspec *rs, struct refspec_item *query)
+{
+	int i;
+	int find_src = !query->src;
+	const char *needle = find_src ? query->dst : query->src;
+	char **result = find_src ? &query->src : &query->dst;
+
+	if (find_src && !query->dst)
+		BUG("query_refspecs: need either src or dst");
+
+	if (query_matches_negative_refspec(rs, query))
+		return -1;
+
+	for (i = 0; i < rs->nr; i++) {
+		struct refspec_item *refspec = &rs->items[i];
+		const char *key = find_src ? refspec->dst : refspec->src;
+		const char *value = find_src ? refspec->src : refspec->dst;
+
+		if (!refspec->dst || refspec->negative)
+			continue;
+		if (refspec->pattern) {
+			if (match_name_with_pattern(key, needle, value, result)) {
+				query->force = refspec->force;
+				return 0;
+			}
+		} else if (!strcmp(needle, key)) {
+			*result = xstrdup(value);
+			query->force = refspec->force;
+			return 0;
+		}
+	}
+	return -1;
+}
diff --git a/refspec.h b/refspec.h
index 891d50b159..d0788de782 100644
--- a/refspec.h
+++ b/refspec.h
@@ -30,6 +30,8 @@  struct refspec_item {
 	char *raw;
 };
 
+struct string_list;
+
 #define REFSPEC_FETCH 1
 #define REFSPEC_PUSH 0
 
@@ -84,4 +86,18 @@  int omit_name_by_refspec(const char *name, struct refspec *rs);
 int match_name_with_pattern(const char *key, const char *name,
 				   const char *value, char **result);
 
+/*
+ * Queries a refspec for a match and updates the query item.
+ * Returns 0 on success, -1 if no match is found or negative refspec matches.
+ */
+int query_refspecs(struct refspec *rs, struct refspec_item *query);
+
+/*
+ * Queries a refspec for all matches and appends results to the provided string
+ * list.
+ */
+void query_refspecs_multiple(struct refspec *rs,
+				    struct refspec_item *query,
+				    struct string_list *results);
+
 #endif /* REFSPEC_H */
diff --git a/remote.c b/remote.c
index 40c2418065..2c46611821 100644
--- a/remote.c
+++ b/remote.c
@@ -925,128 +925,6 @@  struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)
 	return ref_map;
 }
 
-static int query_matches_negative_refspec(struct refspec *rs, struct refspec_item *query)
-{
-	int i, matched_negative = 0;
-	int find_src = !query->src;
-	struct string_list reversed = STRING_LIST_INIT_DUP;
-	const char *needle = find_src ? query->dst : query->src;
-
-	/*
-	 * Check whether the queried ref matches any negative refpsec. If so,
-	 * then we should ultimately treat this as not matching the query at
-	 * all.
-	 *
-	 * Note that negative refspecs always match the source, but the query
-	 * item uses the destination. To handle this, we apply pattern
-	 * refspecs in reverse to figure out if the query source matches any
-	 * of the negative refspecs.
-	 *
-	 * The first loop finds and expands all positive refspecs
-	 * matched by the queried ref.
-	 *
-	 * The second loop checks if any of the results of the first loop
-	 * match any negative refspec.
-	 */
-	for (i = 0; i < rs->nr; i++) {
-		struct refspec_item *refspec = &rs->items[i];
-		char *expn_name;
-
-		if (refspec->negative)
-			continue;
-
-		/* Note the reversal of src and dst */
-		if (refspec->pattern) {
-			const char *key = refspec->dst ? refspec->dst : refspec->src;
-			const char *value = refspec->src;
-
-			if (match_name_with_pattern(key, needle, value, &expn_name))
-				string_list_append_nodup(&reversed, expn_name);
-		} else if (refspec->matching) {
-			/* For the special matching refspec, any query should match */
-			string_list_append(&reversed, needle);
-		} else if (!refspec->src) {
-			BUG("refspec->src should not be null here");
-		} else if (!strcmp(needle, refspec->src)) {
-			string_list_append(&reversed, refspec->src);
-		}
-	}
-
-	for (i = 0; !matched_negative && i < reversed.nr; i++) {
-		if (omit_name_by_refspec(reversed.items[i].string, rs))
-			matched_negative = 1;
-	}
-
-	string_list_clear(&reversed, 0);
-
-	return matched_negative;
-}
-
-static void query_refspecs_multiple(struct refspec *rs,
-				    struct refspec_item *query,
-				    struct string_list *results)
-{
-	int i;
-	int find_src = !query->src;
-
-	if (find_src && !query->dst)
-		BUG("query_refspecs_multiple: need either src or dst");
-
-	if (query_matches_negative_refspec(rs, query))
-		return;
-
-	for (i = 0; i < rs->nr; i++) {
-		struct refspec_item *refspec = &rs->items[i];
-		const char *key = find_src ? refspec->dst : refspec->src;
-		const char *value = find_src ? refspec->src : refspec->dst;
-		const char *needle = find_src ? query->dst : query->src;
-		char **result = find_src ? &query->src : &query->dst;
-
-		if (!refspec->dst || refspec->negative)
-			continue;
-		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result))
-				string_list_append_nodup(results, *result);
-		} else if (!strcmp(needle, key)) {
-			string_list_append(results, value);
-		}
-	}
-}
-
-int query_refspecs(struct refspec *rs, struct refspec_item *query)
-{
-	int i;
-	int find_src = !query->src;
-	const char *needle = find_src ? query->dst : query->src;
-	char **result = find_src ? &query->src : &query->dst;
-
-	if (find_src && !query->dst)
-		BUG("query_refspecs: need either src or dst");
-
-	if (query_matches_negative_refspec(rs, query))
-		return -1;
-
-	for (i = 0; i < rs->nr; i++) {
-		struct refspec_item *refspec = &rs->items[i];
-		const char *key = find_src ? refspec->dst : refspec->src;
-		const char *value = find_src ? refspec->src : refspec->dst;
-
-		if (!refspec->dst || refspec->negative)
-			continue;
-		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result)) {
-				query->force = refspec->force;
-				return 0;
-			}
-		} else if (!strcmp(needle, key)) {
-			*result = xstrdup(value);
-			query->force = refspec->force;
-			return 0;
-		}
-	}
-	return -1;
-}
-
 char *apply_refspecs(struct refspec *rs, const char *name)
 {
 	struct refspec_item query;
diff --git a/remote.h b/remote.h
index 0d109fa9c9..f3da64dc41 100644
--- a/remote.h
+++ b/remote.h
@@ -267,7 +267,6 @@  struct ref *ref_remove_duplicates(struct ref *ref_map);
  */
 struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);
 
-int query_refspecs(struct refspec *rs, struct refspec_item *query);
 char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, struct refspec *rs);