[v2,3/3] refspec: relocate apply_refspecs and related funtions

Commit Message

Meet Soni Jan. 27, 2025, 10:36 a.m. UTC

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 <[e-mail]>
---
 refspec.c | 32 ++++++++++++++++++++++++++++++++
 refspec.h | 12 ++++++++++++
 remote.c  | 31 -------------------------------
 remote.h  |  8 --------
 4 files changed, 44 insertions(+), 39 deletions(-)

Comments

Junio C Hamano Jan. 27, 2025, 8:14 p.m. UTC | #1

Meet Soni <[e-mail]> writes:

> +/*
> + * 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);

Excellent.  

    ... it is merely moved from the original so it is not entirely
    your achievement, but still, this is good.

> +/*
> + * Applies refspecs to a name and returns the corresponding destination.
> + * Returns the destination string if a match is found, NULL otherwise.
> + */
> +char *apply_refspecs(struct refspec *rs, const char *name);

Explaining a function whose name has "apply" with a comment that
uses "apply" as the verb does not add as much information as a
comment with a bit rephrased explanation.  What does it mean to
"apply refspec to a name" in the context of this function?

Meet Soni Jan. 29, 2025, 7:03 a.m. UTC | #2

On Tue, 28 Jan 2025 at 01:44, Junio C Hamano <[e-mail]> wrote:
>
> Meet Soni <[e-mail]> writes:
>
> > +/*
> > + * Applies refspecs to a name and returns the corresponding destination.
> > + * Returns the destination string if a match is found, NULL otherwise.
> > + */
> > +char *apply_refspecs(struct refspec *rs, const char *name);
>
> Explaining a function whose name has "apply" with a comment that
> uses "apply" as the verb does not add as much information as a
> comment with a bit rephrased explanation.  What does it mean to
> "apply refspec to a name" in the context of this function?

The term "apply" was intended to convey the idea of mapping the
refspec with the given name, but it’s more helpful to describe the
function’s behavior more explicitly.

I’ll update the comment in the next version of this series.
Here’s the revised comment I plan to use:

/*
 * Search for a refspec that matches the given name and return the
 * corresponding destination (dst) if a match is found, NULL otherwise.
 */

Patch

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 d0788de782..231bcfb33e 100644
--- a/refspec.h
+++ b/refspec.h
@@ -100,4 +100,16 @@  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);
+
+/*
+ * Applies refspecs to a name and returns the corresponding destination.
+ * Returns the destination string if a match is found, NULL otherwise.
+ */
+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);