* [GSoC][PATCH] refspec: clarify function naming and documentation @ 2025-02-14 5:39 Meet Soni 2025-02-14 18:56 ` Junio C Hamano 2025-02-15 8:45 ` [GSoC][PATCH v2] " Meet Soni 0 siblings, 2 replies; 3+ messages in thread From: Meet Soni @ 2025-02-14 5:39 UTC (permalink / raw) To: git; +Cc: gitster, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [GSoC][PATCH] refspec: clarify function naming and documentation 2025-02-14 5:39 [GSoC][PATCH] refspec: clarify function naming and documentation Meet Soni @ 2025-02-14 18:56 ` Junio C Hamano 2025-02-15 8:45 ` [GSoC][PATCH v2] " Meet Soni 1 sibling, 0 replies; 3+ messages in thread From: Junio C Hamano @ 2025-02-14 18:56 UTC (permalink / raw) To: Meet Soni; +Cc: git 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* [GSoC][PATCH v2] refspec: clarify function naming and documentation 2025-02-14 5:39 [GSoC][PATCH] refspec: clarify function naming and documentation Meet Soni 2025-02-14 18:56 ` Junio C Hamano @ 2025-02-15 8:45 ` Meet Soni 1 sibling, 0 replies; 3+ messages in thread From: Meet Soni @ 2025-02-15 8:45 UTC (permalink / raw) To: git; +Cc: gitster, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-15 8:45 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-14 5:39 [GSoC][PATCH] refspec: clarify function naming and documentation Meet Soni 2025-02-14 18:56 ` Junio C Hamano 2025-02-15 8:45 ` [GSoC][PATCH v2] " Meet Soni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox