[RFC PATCH 0/3] refspec: centralize refspec-related logic

[RFC PATCH 0/3] refspec: centralize refspec-related logic

[Meet Soni]

As Patrick pointed out in [1], the logic related to refspec is currently
split across multiple headers. This patch series addresses that by
relocating refspec-related logic from remote to refspec for improved
cohesion.

There are a few functions that seem to be on the borderline. They seem
to have refspec-related logic, but also contain some elements related
to remote logic, so I'm unsure whether they should be moved.
Specifically:

    * get_fetch_map()
    * count_refspec_match()
    * check_push_refs()
    * match_push_refs()

[1]: https://lore.kernel.org/git/ZysQvUyxgdRqjvj2@pks.im/

Meet Soni (3):
  refspec: relocate omit_name_by_refspec and related functions
  refspec: relocate query related functions
  refspec: relocate apply_refspecs and related funtions

 refspec.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 refspec.h |  23 +++++++
 remote.c  | 201 -----------------------------------------------------
 remote.h  |  15 ----
 4 files changed, 226 insertions(+), 216 deletions(-)


base-commit: efff4a85a4fce58b2aa850c6fbf4d8828329f51d
-- 
2.34.1

[RFC PATCH 1/3] refspec: relocate omit_name_by_refspec and related functions

[Meet Soni]

Move the functions `omit_name_by_refspec()`, `refspec_match()`, and
`match_name_with_pattern()` from `remote.c` to `refspec.c`. These
functions focus on refspec matching, so placing them in `refspec.c`
aligns with the separation of concerns. Keep refspec-related logic in
`refspec.c` and remote-specific logic in `remote.c` for better code
organization.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 refspec.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 refspec.h |  8 ++++++++
 remote.c  | 48 ------------------------------------------------
 remote.h  |  6 ------
 4 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/refspec.c b/refspec.c
index 6d86e04442..66989a1d75 100644
--- a/refspec.c
+++ b/refspec.c
@@ -276,3 +276,51 @@ void refspec_ref_prefixes(const struct refspec *rs,
 		}
 	}
 }
+
+int match_name_with_pattern(const char *key, const char *name,
+				   const char *value, char **result)
+{
+	const char *kstar = strchr(key, '*');
+	size_t klen;
+	size_t ksuffixlen;
+	size_t namelen;
+	int ret;
+	if (!kstar)
+		die(_("key '%s' of pattern had no '*'"), key);
+	klen = kstar - key;
+	ksuffixlen = strlen(kstar + 1);
+	namelen = strlen(name);
+	ret = !strncmp(name, key, klen) && namelen >= klen + ksuffixlen &&
+		!memcmp(name + namelen - ksuffixlen, kstar + 1, ksuffixlen);
+	if (ret && value) {
+		struct strbuf sb = STRBUF_INIT;
+		const char *vstar = strchr(value, '*');
+		if (!vstar)
+			die(_("value '%s' of pattern has no '*'"), value);
+		strbuf_add(&sb, value, vstar - value);
+		strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
+		strbuf_addstr(&sb, vstar + 1);
+		*result = strbuf_detach(&sb, NULL);
+	}
+	return ret;
+}
+
+static int refspec_match(const struct refspec_item *refspec,
+			 const char *name)
+{
+	if (refspec->pattern)
+		return match_name_with_pattern(refspec->src, name, NULL, NULL);
+
+	return !strcmp(refspec->src, name);
+}
+
+int omit_name_by_refspec(const char *name, struct refspec *rs)
+{
+	int i;
+
+	for (i = 0; i < rs->nr; i++) {
+		if (rs->items[i].negative && refspec_match(&rs->items[i], name))
+			return 1;
+	}
+	return 0;
+}
diff --git a/refspec.h b/refspec.h
index 69d693c87d..9556d08bd5 100644
--- a/refspec.h
+++ b/refspec.h
@@ -71,4 +71,12 @@ struct strvec;
 void refspec_ref_prefixes(const struct refspec *rs,
 			  struct strvec *ref_prefixes);
 
+/*
+ * Check whether a name matches any negative refspec in rs. Returns 1 if the
+ * name matches at least one negative refspec, and 0 otherwise.
+ */
+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);
+
 #endif /* REFSPEC_H */
diff --git a/remote.c b/remote.c
index 0f6fba8562..40c2418065 100644
--- a/remote.c
+++ b/remote.c
@@ -907,54 +907,6 @@ void ref_push_report_free(struct ref_push_report *report)
 	}
 }
 
-static int match_name_with_pattern(const char *key, const char *name,
-				   const char *value, char **result)
-{
-	const char *kstar = strchr(key, '*');
-	size_t klen;
-	size_t ksuffixlen;
-	size_t namelen;
-	int ret;
-	if (!kstar)
-		die(_("key '%s' of pattern had no '*'"), key);
-	klen = kstar - key;
-	ksuffixlen = strlen(kstar + 1);
-	namelen = strlen(name);
-	ret = !strncmp(name, key, klen) && namelen >= klen + ksuffixlen &&
-		!memcmp(name + namelen - ksuffixlen, kstar + 1, ksuffixlen);
-	if (ret && value) {
-		struct strbuf sb = STRBUF_INIT;
-		const char *vstar = strchr(value, '*');
-		if (!vstar)
-			die(_("value '%s' of pattern has no '*'"), value);
-		strbuf_add(&sb, value, vstar - value);
-		strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
-		strbuf_addstr(&sb, vstar + 1);
-		*result = strbuf_detach(&sb, NULL);
-	}
-	return ret;
-}
-
-static int refspec_match(const struct refspec_item *refspec,
-			 const char *name)
-{
-	if (refspec->pattern)
-		return match_name_with_pattern(refspec->src, name, NULL, NULL);
-
-	return !strcmp(refspec->src, name);
-}
-
-int omit_name_by_refspec(const char *name, struct refspec *rs)
-{
-	int i;
-
-	for (i = 0; i < rs->nr; i++) {
-		if (rs->items[i].negative && refspec_match(&rs->items[i], name))
-			return 1;
-	}
-	return 0;
-}
-
 struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)
 {
 	struct ref **tail;
diff --git a/remote.h b/remote.h
index bda10dd5c8..0d109fa9c9 100644
--- a/remote.h
+++ b/remote.h
@@ -261,12 +261,6 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
  */
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
-/*
- * Check whether a name matches any negative refspec in rs. Returns 1 if the
- * name matches at least one negative refspec, and 0 otherwise.
- */
-int omit_name_by_refspec(const char *name, struct refspec *rs);
-
 /*
  * Remove all entries in the input list which match any negative refspec in
  * the refspec list.
-- 
2.34.1

[RFC PATCH 2/3] refspec: relocate query related functions

[Meet Soni]

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 |   7 ++++
 remote.c  | 122 -----------------------------------------------------
 remote.h  |   1 -
 4 files changed, 130 insertions(+), 123 deletions(-)

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 9556d08bd5..d3c97bfdc5 100644
--- a/refspec.h
+++ b/refspec.h
@@ -1,6 +1,8 @@
 #ifndef REFSPEC_H
 #define REFSPEC_H
 
+#include "string-list.h"
+
 #define TAG_REFSPEC "refs/tags/*:refs/tags/*"
 
 /**
@@ -79,4 +81,9 @@ 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);
 
+int query_refspecs(struct refspec *rs, struct refspec_item *query);
+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);
-- 
2.34.1

[RFC PATCH 3/3] refspec: relocate apply_refspecs and related funtions

[Meet Soni]

Move the functions `apply_refspecs()` and `apply_negative_refspecs()`
from `remote.c` to `refspec.c`. These functions focus on applying
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 | 32 ++++++++++++++++++++++++++++++++
 refspec.h |  8 ++++++++
 remote.c  | 31 -------------------------------
 remote.h  |  8 --------
 4 files changed, 40 insertions(+), 39 deletions(-)

diff --git a/refspec.c b/refspec.c
index 72b3911110..d279d6032a 100644
--- a/refspec.c
+++ b/refspec.c
@@ -9,6 +9,7 @@
 #include "strvec.h"
 #include "refs.h"
 #include "refspec.h"
+#include "remote.h"
 #include "strbuf.h"
 
 /*
@@ -447,3 +448,34 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)
 	}
 	return -1;
 }
+
+struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)
+{
+	struct ref **tail;
+
+	for (tail = &ref_map; *tail; ) {
+		struct ref *ref = *tail;
+
+		if (omit_name_by_refspec(ref->name, rs)) {
+			*tail = ref->next;
+			free(ref->peer_ref);
+			free(ref);
+		} else
+			tail = &ref->next;
+	}
+
+	return ref_map;
+}
+
+char *apply_refspecs(struct refspec *rs, const char *name)
+{
+	struct refspec_item query;
+
+	memset(&query, 0, sizeof(struct refspec_item));
+	query.src = (char *)name;
+
+	if (query_refspecs(rs, &query))
+		return NULL;
+
+	return query.dst;
+}
diff --git a/refspec.h b/refspec.h
index d3c97bfdc5..294068d226 100644
--- a/refspec.h
+++ b/refspec.h
@@ -86,4 +86,12 @@ void query_refspecs_multiple(struct refspec *rs,
 				    struct refspec_item *query,
 				    struct string_list *results);
 
+/*
+ * Remove all entries in the input list which match any negative refspec in
+ * the refspec list.
+ */
+struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);
+
+char *apply_refspecs(struct refspec *rs, const char *name);
+
 #endif /* REFSPEC_H */
diff --git a/remote.c b/remote.c
index 2c46611821..641dd1125f 100644
--- a/remote.c
+++ b/remote.c
@@ -907,37 +907,6 @@ void ref_push_report_free(struct ref_push_report *report)
 	}
 }
 
-struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)
-{
-	struct ref **tail;
-
-	for (tail = &ref_map; *tail; ) {
-		struct ref *ref = *tail;
-
-		if (omit_name_by_refspec(ref->name, rs)) {
-			*tail = ref->next;
-			free(ref->peer_ref);
-			free(ref);
-		} else
-			tail = &ref->next;
-	}
-
-	return ref_map;
-}
-
-char *apply_refspecs(struct refspec *rs, const char *name)
-{
-	struct refspec_item query;
-
-	memset(&query, 0, sizeof(struct refspec_item));
-	query.src = (char *)name;
-
-	if (query_refspecs(rs, &query))
-		return NULL;
-
-	return query.dst;
-}
-
 int remote_find_tracking(struct remote *remote, struct refspec_item *refspec)
 {
 	return query_refspecs(&remote->fetch, refspec);
diff --git a/remote.h b/remote.h
index f3da64dc41..b4bb16af0e 100644
--- a/remote.h
+++ b/remote.h
@@ -261,14 +261,6 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
  */
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
-/*
- * Remove all entries in the input list which match any negative refspec in
- * the refspec list.
- */
-struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);
-
-char *apply_refspecs(struct refspec *rs, const char *name);
-
 int check_push_refs(struct ref *src, struct refspec *rs);
 int match_push_refs(struct ref *src, struct ref **dst,
 		    struct refspec *rs, int flags);
-- 
2.34.1

Re: [RFC PATCH 0/3] refspec: centralize refspec-related logic

[Patrick Steinhardt]

On Wed, Jan 22, 2025 at 01:21:51PM +0530, Meet Soni wrote:
> As Patrick pointed out in [1], the logic related to refspec is currently
> split across multiple headers. This patch series addresses that by
> relocating refspec-related logic from remote to refspec for improved
> cohesion.
> 
> There are a few functions that seem to be on the borderline. They seem
> to have refspec-related logic, but also contain some elements related
> to remote logic, so I'm unsure whether they should be moved.
> Specifically:
> 
>     * get_fetch_map()
>     * count_refspec_match()
>     * check_push_refs()
>     * match_push_refs()
> 
> [1]: https://lore.kernel.org/git/ZysQvUyxgdRqjvj2@pks.im/

Thanks for this series! A couple comments from my side, but I think that
the overall goal of this series is sensible (well, I seemingly proposed
it at one point, so no surprise there).

Patrick

Re: [RFC PATCH 1/3] refspec: relocate omit_name_by_refspec and related functions

[Patrick Steinhardt]

On Wed, Jan 22, 2025 at 01:21:52PM +0530, Meet Soni wrote:
> diff --git a/refspec.h b/refspec.h
> index 69d693c87d..9556d08bd5 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -71,4 +71,12 @@ struct strvec;
>  void refspec_ref_prefixes(const struct refspec *rs,
>  			  struct strvec *ref_prefixes);
>  
> +/*
> + * Check whether a name matches any negative refspec in rs. Returns 1 if the
> + * name matches at least one negative refspec, and 0 otherwise.
> + */
> +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);

Reading the signature of `match_name_with_pattern()` I wouldn't have any
clue how to use it. Could we maybe add some documentation to it now that
we expose it?

>  #endif /* REFSPEC_H */
> diff --git a/remote.c b/remote.c
> index 0f6fba8562..40c2418065 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -907,54 +907,6 @@ void ref_push_report_free(struct ref_push_report *report)
>  	}
>  }
>  
> -static int match_name_with_pattern(const char *key, const char *name,
> -				   const char *value, char **result)
> -{

It's a bit unfortunate that we have to convert this static function to a
non-static one, but I guess it makes sense in the bigger picture of what
this series wants to achieve.

Patrick

Re: [RFC PATCH 2/3] refspec: relocate query related functions

[Patrick Steinhardt]

On Wed, Jan 22, 2025 at 01:21:53PM +0530, Meet Soni wrote:
> diff --git a/refspec.h b/refspec.h
> index 9556d08bd5..d3c97bfdc5 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -1,6 +1,8 @@
>  #ifndef REFSPEC_H
>  #define REFSPEC_H
>  
> +#include "string-list.h"
> +

We can avoid including this header by adding a forward-declaration of
`struct string_list` instead.

> @@ -79,4 +81,9 @@ 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);
>  
> +int query_refspecs(struct refspec *rs, struct refspec_item *query);
> +void query_refspecs_multiple(struct refspec *rs,
> +				    struct refspec_item *query,
> +				    struct string_list *results);
> +
>  #endif /* REFSPEC_H */

It would be nice to add some docs here, too.

Patrick

Re: [RFC PATCH 3/3] refspec: relocate apply_refspecs and related funtions

[Patrick Steinhardt]

On Wed, Jan 22, 2025 at 01:21:54PM +0530, Meet Soni wrote:
> diff --git a/refspec.h b/refspec.h
> index d3c97bfdc5..294068d226 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -86,4 +86,12 @@ void query_refspecs_multiple(struct refspec *rs,
>  				    struct refspec_item *query,
>  				    struct string_list *results);
>  
> +/*
> + * Remove all entries in the input list which match any negative refspec in
> + * the refspec list.
> + */
> +struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);
> +
> +char *apply_refspecs(struct refspec *rs, const char *name);
> +
>  #endif /* REFSPEC_H */

Same comment here regarding the documentation of `apply_refspecs()`.

Patrick

Re: [RFC PATCH 0/3] refspec: centralize refspec-related logic

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Jan 22, 2025 at 01:21:51PM +0530, Meet Soni wrote:
>> As Patrick pointed out in [1], the logic related to refspec is currently
>> split across multiple headers. This patch series addresses that by
>> relocating refspec-related logic from remote to refspec for improved
>> cohesion.
>> 
>> There are a few functions that seem to be on the borderline. They seem
>> to have refspec-related logic, but also contain some elements related
>> to remote logic, so I'm unsure whether they should be moved.
>> Specifically:
>> 
>>     * get_fetch_map()
>>     * count_refspec_match()
>>     * check_push_refs()
>>     * match_push_refs()
>> 
>> [1]: https://lore.kernel.org/git/ZysQvUyxgdRqjvj2@pks.im/
>
> Thanks for this series! A couple comments from my side, but I think that
> the overall goal of this series is sensible (well, I seemingly proposed
> it at one point, so no surprise there).

;-).  Nicely done.