[GSoC][PATCH] refspec: clarify function naming and documentation

[GSoC][PATCH] refspec: clarify function naming and documentation

[Meet Soni]

Rename `match_name_with_pattern()` to `match_refname_with_pattern()` to
better reflect its purpose and improve documentation comment clarity.
The previous function name and parameter names were inconsistent, making
it harder to understand their roles in refspec matching.

- Rename parameters:
  - `key` -> `src_pattern` (source globbing pattern)
  - `name` -> `refname` (refname to check)
  - `value` -> `dst_pattern` (destination mapping pattern)

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
This change was previously discussed in an earlier patch series [1], where
Junio suggested making this update after the dust settled there.

[1]: https://lore.kernel.org/git/xmqqa5bctbnx.fsf@gitster.g/

 refspec.c | 34 +++++++++++++++++-----------------
 refspec.h |  9 +++++----
 remote.c  |  6 +++---
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/refspec.c b/refspec.c
index 3d6cf4dc92..329de7bb18 100644
--- a/refspec.c
+++ b/refspec.c
@@ -269,28 +269,28 @@ void refspec_ref_prefixes(const struct refspec *rs,
 	}
 }
 
-int match_name_with_pattern(const char *key, const char *name,
-				   const char *value, char **result)
+int match_refname_with_pattern(const char *src_pattern, const char *refname,
+				   const char *dst_pattern, char **result)
 {
-	const char *kstar = strchr(key, '*');
+	const char *kstar = strchr(src_pattern, '*');
 	size_t klen;
 	size_t ksuffixlen;
 	size_t namelen;
 	int ret;
 	if (!kstar)
-		die(_("key '%s' of pattern had no '*'"), key);
-	klen = kstar - key;
+		die(_("source pattern '%s' has no '*'"), src_pattern);
+	klen = kstar - src_pattern;
 	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) {
+	namelen = strlen(refname);
+	ret = !strncmp(refname, src_pattern, klen) && namelen >= klen + ksuffixlen &&
+		!memcmp(refname + namelen - ksuffixlen, kstar + 1, ksuffixlen);
+	if (ret && dst_pattern) {
 		struct strbuf sb = STRBUF_INIT;
-		const char *vstar = strchr(value, '*');
+		const char *vstar = strchr(dst_pattern, '*');
 		if (!vstar)
-			die(_("value '%s' of pattern has no '*'"), value);
-		strbuf_add(&sb, value, vstar - value);
-		strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
+			die(_("destination pattern '%s' has no '*'"), dst_pattern);
+		strbuf_add(&sb, dst_pattern, vstar - dst_pattern);
+		strbuf_add(&sb, refname + klen, namelen - klen - ksuffixlen);
 		strbuf_addstr(&sb, vstar + 1);
 		*result = strbuf_detach(&sb, NULL);
 	}
@@ -301,7 +301,7 @@ 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 match_refname_with_pattern(refspec->src, name, NULL, NULL);
 
 	return !strcmp(refspec->src, name);
 }
@@ -352,7 +352,7 @@ static int refspec_find_negative_match(struct refspec *rs, struct refspec_item *
 			const char *key = refspec->dst ? refspec->dst : refspec->src;
 			const char *value = refspec->src;
 
-			if (match_name_with_pattern(key, needle, value, &expn_name))
+			if (match_refname_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 */
@@ -397,7 +397,7 @@ void refspec_find_all_matches(struct refspec *rs,
 		if (!refspec->dst || refspec->negative)
 			continue;
 		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result))
+			if (match_refname_with_pattern(key, needle, value, result))
 				string_list_append_nodup(results, *result);
 		} else if (!strcmp(needle, key)) {
 			string_list_append(results, value);
@@ -426,7 +426,7 @@ int refspec_find_match(struct refspec *rs, struct refspec_item *query)
 		if (!refspec->dst || refspec->negative)
 			continue;
 		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result)) {
+			if (match_refname_with_pattern(key, needle, value, result)) {
 				query->force = refspec->force;
 				return 0;
 			}
diff --git a/refspec.h b/refspec.h
index f62f83a7ee..6cec985b07 100644
--- a/refspec.h
+++ b/refspec.h
@@ -75,11 +75,12 @@ void refspec_ref_prefixes(const struct refspec *rs,
 int refname_matches_negative_refspec_item(const char *refname, struct refspec *rs);
 
 /*
- * Checks whether a name matches a pattern and optionally generates a result.
- * Returns 1 if the name matches the pattern, 0 otherwise.
+ * Checks if a refname matches a globbing refspec pattern.
+ * If dst_pattern is provided, computes the mapped destination refname.
+ * Returns 1 if refname matches src_pattern, 0 otherwise.
  */
-int match_name_with_pattern(const char *key, const char *name,
-				   const char *value, char **result);
+int match_refname_with_pattern(const char *src_pattern, const char *refname,
+				   const char *dst_pattern, char **result);
 
 /*
  * Queries a refspec for a match and updates the query item.
diff --git a/remote.c b/remote.c
index 5574b6a00f..5d3d1773dc 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,9 +1322,9 @@ static char *get_ref_match(const struct refspec *rs, const struct ref *ref,
 			const char *dst_side = item->dst ? item->dst : item->src;
 			int match;
 			if (direction == FROM_SRC)
-				match = match_name_with_pattern(item->src, ref->name, dst_side, &name);
+				match = match_refname_with_pattern(item->src, ref->name, dst_side, &name);
 			else
-				match = match_name_with_pattern(dst_side, ref->name, item->src, &name);
+				match = match_refname_with_pattern(dst_side, ref->name, item->src, &name);
 			if (match) {
 				matching_refs = i;
 				break;
@@ -1942,7 +1942,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
-		if (match_name_with_pattern(refspec->src, ref->name,
+		if (match_refname_with_pattern(refspec->src, ref->name,
 					    refspec->dst, &expn_name) &&
 		    !ignore_symref_update(expn_name, &scratch)) {
 			struct ref *cpy = copy_ref(ref);

base-commit: e2067b49ecaef9b7f51a17ce251f9207f72ef52d
-- 
2.34.1

Re: [GSoC][PATCH] refspec: clarify function naming and documentation

[Junio C Hamano]

Meet Soni <meetsoni3017@gmail.com> writes:

> Rename `match_name_with_pattern()` to `match_refname_with_pattern()` to
> better reflect its purpose and improve documentation comment clarity.
> The previous function name and parameter names were inconsistent, making
> it harder to understand their roles in refspec matching.
>
> - Rename parameters:
>   - `key` -> `src_pattern` (source globbing pattern)
>   - `name` -> `refname` (refname to check)
>   - `value` -> `dst_pattern` (destination mapping pattern)
>
> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> ---
> This change was previously discussed in an earlier patch series [1], where
> Junio suggested making this update after the dust settled there.
>
> [1]: https://lore.kernel.org/git/xmqqa5bctbnx.fsf@gitster.g/

Yeah, and the dust settled a few days ago when the
ms/refspec-cleanup topic graduated to the 'master' branch.

Thanks for that work.

The tldr is that I like two things in the above rename, and find two
things problematic.  "name->refname" is very good, adding "pattern"
is very good.  using "src" and "dst" is problematic.

One thing to note is that match_refname_with_pattern() can also be
used to reverse map.

A refspec that says "refs/heads/*:refs/remotes/origin/*" can be used
to answer these two questions:

 * I see what they call "refs/heads/master", where should I store it?

 * I have "refs/remotes/origin/main", where did it come from?

The src/dst distinction you updated the parameters to the function
only reflects the first usage, and it is a bit confusing when the
code asks the other question.

    Find the "refname" in A and replace the same glob part in B when
    it finds a match

is what the function does, and we used to call A=key and B=value,
which were not great.  With "pattern" in their names, the new names
"src/dst_pattern" are improvement, but src/dst hints as if they are
directly related to src/dst sides of a refspec, which is the source
of possible confusion when we talk about the "please map from our
remote-tracking branch name to the branch name at the origin" use
case.

So, I very much have problems with the "(*source* globbing pattern)"
you state as the reasoning beind the new name in the proposed log
message and "src/dst" in these names.

What do other people who wrote tools that do something very similar
call these two things?  For example, "sed -e 's/A/B/'" command does
"find A and replace with B".  They call A=RE and B=replacement

Perhaps "key -> pattern" and "value -> replacement" would be a
better pair of names that are easier to understand?  I dunno.

> -int match_name_with_pattern(const char *key, const char *name,
> -				   const char *value, char **result)
> +int match_refname_with_pattern(const char *src_pattern, const char *refname,
> +				   const char *dst_pattern, char **result)
>  {

Thanks.

[GSoC][PATCH v2] refspec: clarify function naming and documentation

[Meet Soni]

Rename `match_name_with_pattern()` to `match_refname_with_pattern()` to
better reflect its purpose and improve documentation comment clarity.
The previous function name and parameter names were inconsistent, making
it harder to understand their roles in refspec matching.

- Rename parameters:
  - `key` -> `pattern` (globbing pattern to match)
  - `name` -> `refname` (refname to check)
  - `value` -> `replacement` (replacement mapping pattern)

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
changes in v2:
    - updated parameter names as per review. Namely:
        - `src_pattern` -> `pattern`
        - `dst_pattern` -> `replacement`
    - updated comment and commit message.

Range-diff against v1:
1:  8bd849b7c2 ! 1:  aa47431720 refspec: clarify function naming and documentation
    @@ Commit message
         it harder to understand their roles in refspec matching.
     
         - Rename parameters:
    -      - `key` -> `src_pattern` (source globbing pattern)
    +      - `key` -> `pattern` (globbing pattern to match)
           - `name` -> `refname` (refname to check)
    -      - `value` -> `dst_pattern` (destination mapping pattern)
    +      - `value` -> `replacement` (replacement mapping pattern)
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     
    @@ refspec.c: void refspec_ref_prefixes(const struct refspec *rs,
      
     -int match_name_with_pattern(const char *key, const char *name,
     -				   const char *value, char **result)
    -+int match_refname_with_pattern(const char *src_pattern, const char *refname,
    -+				   const char *dst_pattern, char **result)
    ++int match_refname_with_pattern(const char *pattern, const char *refname,
    ++				   const char *replacement, char **result)
      {
     -	const char *kstar = strchr(key, '*');
    -+	const char *kstar = strchr(src_pattern, '*');
    ++	const char *kstar = strchr(pattern, '*');
      	size_t klen;
      	size_t ksuffixlen;
      	size_t namelen;
    @@ refspec.c: void refspec_ref_prefixes(const struct refspec *rs,
      	if (!kstar)
     -		die(_("key '%s' of pattern had no '*'"), key);
     -	klen = kstar - key;
    -+		die(_("source pattern '%s' has no '*'"), src_pattern);
    -+	klen = kstar - src_pattern;
    ++		die(_("pattern '%s' has no '*'"), pattern);
    ++	klen = kstar - pattern;
      	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) {
     +	namelen = strlen(refname);
    -+	ret = !strncmp(refname, src_pattern, klen) && namelen >= klen + ksuffixlen &&
    ++	ret = !strncmp(refname, pattern, klen) && namelen >= klen + ksuffixlen &&
     +		!memcmp(refname + namelen - ksuffixlen, kstar + 1, ksuffixlen);
    -+	if (ret && dst_pattern) {
    ++	if (ret && replacement) {
      		struct strbuf sb = STRBUF_INIT;
     -		const char *vstar = strchr(value, '*');
    -+		const char *vstar = strchr(dst_pattern, '*');
    ++		const char *vstar = strchr(replacement, '*');
      		if (!vstar)
     -			die(_("value '%s' of pattern has no '*'"), value);
     -		strbuf_add(&sb, value, vstar - value);
     -		strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
    -+			die(_("destination pattern '%s' has no '*'"), dst_pattern);
    -+		strbuf_add(&sb, dst_pattern, vstar - dst_pattern);
    ++			die(_("replacement '%s' has no '*'"), replacement);
    ++		strbuf_add(&sb, replacement, vstar - replacement);
     +		strbuf_add(&sb, refname + klen, namelen - klen - ksuffixlen);
      		strbuf_addstr(&sb, vstar + 1);
      		*result = strbuf_detach(&sb, NULL);
    @@ refspec.h: void refspec_ref_prefixes(const struct refspec *rs,
     - * Checks whether a name matches a pattern and optionally generates a result.
     - * Returns 1 if the name matches the pattern, 0 otherwise.
     + * Checks if a refname matches a globbing refspec pattern.
    -+ * If dst_pattern is provided, computes the mapped destination refname.
    -+ * Returns 1 if refname matches src_pattern, 0 otherwise.
    ++ * If replacement is provided, computes the corresponding mapped refname.
    ++ * Returns 1 if refname matches pattern, 0 otherwise.
       */
     -int match_name_with_pattern(const char *key, const char *name,
     -				   const char *value, char **result);
    -+int match_refname_with_pattern(const char *src_pattern, const char *refname,
    -+				   const char *dst_pattern, char **result);
    ++int match_refname_with_pattern(const char *pattern, const char *refname,
    ++				   const char *replacement, char **result);
      
      /*
       * Queries a refspec for a match and updates the query item.

 refspec.c | 34 +++++++++++++++++-----------------
 refspec.h |  9 +++++----
 remote.c  |  6 +++---
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/refspec.c b/refspec.c
index 3d6cf4dc92..4cb80b5208 100644
--- a/refspec.c
+++ b/refspec.c
@@ -269,28 +269,28 @@ void refspec_ref_prefixes(const struct refspec *rs,
 	}
 }
 
-int match_name_with_pattern(const char *key, const char *name,
-				   const char *value, char **result)
+int match_refname_with_pattern(const char *pattern, const char *refname,
+				   const char *replacement, char **result)
 {
-	const char *kstar = strchr(key, '*');
+	const char *kstar = strchr(pattern, '*');
 	size_t klen;
 	size_t ksuffixlen;
 	size_t namelen;
 	int ret;
 	if (!kstar)
-		die(_("key '%s' of pattern had no '*'"), key);
-	klen = kstar - key;
+		die(_("pattern '%s' has no '*'"), pattern);
+	klen = kstar - pattern;
 	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) {
+	namelen = strlen(refname);
+	ret = !strncmp(refname, pattern, klen) && namelen >= klen + ksuffixlen &&
+		!memcmp(refname + namelen - ksuffixlen, kstar + 1, ksuffixlen);
+	if (ret && replacement) {
 		struct strbuf sb = STRBUF_INIT;
-		const char *vstar = strchr(value, '*');
+		const char *vstar = strchr(replacement, '*');
 		if (!vstar)
-			die(_("value '%s' of pattern has no '*'"), value);
-		strbuf_add(&sb, value, vstar - value);
-		strbuf_add(&sb, name + klen, namelen - klen - ksuffixlen);
+			die(_("replacement '%s' has no '*'"), replacement);
+		strbuf_add(&sb, replacement, vstar - replacement);
+		strbuf_add(&sb, refname + klen, namelen - klen - ksuffixlen);
 		strbuf_addstr(&sb, vstar + 1);
 		*result = strbuf_detach(&sb, NULL);
 	}
@@ -301,7 +301,7 @@ 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 match_refname_with_pattern(refspec->src, name, NULL, NULL);
 
 	return !strcmp(refspec->src, name);
 }
@@ -352,7 +352,7 @@ static int refspec_find_negative_match(struct refspec *rs, struct refspec_item *
 			const char *key = refspec->dst ? refspec->dst : refspec->src;
 			const char *value = refspec->src;
 
-			if (match_name_with_pattern(key, needle, value, &expn_name))
+			if (match_refname_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 */
@@ -397,7 +397,7 @@ void refspec_find_all_matches(struct refspec *rs,
 		if (!refspec->dst || refspec->negative)
 			continue;
 		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result))
+			if (match_refname_with_pattern(key, needle, value, result))
 				string_list_append_nodup(results, *result);
 		} else if (!strcmp(needle, key)) {
 			string_list_append(results, value);
@@ -426,7 +426,7 @@ int refspec_find_match(struct refspec *rs, struct refspec_item *query)
 		if (!refspec->dst || refspec->negative)
 			continue;
 		if (refspec->pattern) {
-			if (match_name_with_pattern(key, needle, value, result)) {
+			if (match_refname_with_pattern(key, needle, value, result)) {
 				query->force = refspec->force;
 				return 0;
 			}
diff --git a/refspec.h b/refspec.h
index f62f83a7ee..e2b5cc54ef 100644
--- a/refspec.h
+++ b/refspec.h
@@ -75,11 +75,12 @@ void refspec_ref_prefixes(const struct refspec *rs,
 int refname_matches_negative_refspec_item(const char *refname, struct refspec *rs);
 
 /*
- * Checks whether a name matches a pattern and optionally generates a result.
- * Returns 1 if the name matches the pattern, 0 otherwise.
+ * Checks if a refname matches a globbing refspec pattern.
+ * If replacement is provided, computes the corresponding mapped refname.
+ * Returns 1 if refname matches pattern, 0 otherwise.
  */
-int match_name_with_pattern(const char *key, const char *name,
-				   const char *value, char **result);
+int match_refname_with_pattern(const char *pattern, const char *refname,
+				   const char *replacement, char **result);
 
 /*
  * Queries a refspec for a match and updates the query item.
diff --git a/remote.c b/remote.c
index 5574b6a00f..5d3d1773dc 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,9 +1322,9 @@ static char *get_ref_match(const struct refspec *rs, const struct ref *ref,
 			const char *dst_side = item->dst ? item->dst : item->src;
 			int match;
 			if (direction == FROM_SRC)
-				match = match_name_with_pattern(item->src, ref->name, dst_side, &name);
+				match = match_refname_with_pattern(item->src, ref->name, dst_side, &name);
 			else
-				match = match_name_with_pattern(dst_side, ref->name, item->src, &name);
+				match = match_refname_with_pattern(dst_side, ref->name, item->src, &name);
 			if (match) {
 				matching_refs = i;
 				break;
@@ -1942,7 +1942,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
-		if (match_name_with_pattern(refspec->src, ref->name,
+		if (match_refname_with_pattern(refspec->src, ref->name,
 					    refspec->dst, &expn_name) &&
 		    !ignore_symref_update(expn_name, &scratch)) {
 			struct ref *cpy = copy_ref(ref);
-- 
2.34.1