| Message ID | 20250201064202.76116-4-meetsoni3017@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refspec: centralize refspec-related logic | expand |
On Sat, Feb 01, 2025 at 12:12:00PM +0530, Meet Soni wrote: > Rename `query_refspecs()` to `find_refspec_match` for clarity, as it > finds a single matching refspec. > > Rename `query_refspecs_multiple()` to `find_all_refspec_matches` to > better reflect that it collects all matching refspecs instead of > returning just the first match. > > Rename `query_matches_negative_refspec()` to > `find_negative_refspec_match` for consistency with the updated naming > convention. Okay. The message might've read a tiny bit easier if it was a bulleted list of renames. E.g.: We're about to move a couple of functions related to handling of refspecs from "remote.c" into "refspec.c". In preparation for this move, rename them to better reflect their intent: - `query_refspecs()` becomes `find_refspec_match()` for clarity, as it finds a single matching refspec. ... I was wondering a bit about why we rename the static functions, as we wouldn't have to expose them in a subsequent step anyway. Other than that I think we should adhere to our coding guidelines with the renamed public functions: The primary data structure that a subsystem 'S' deals with is called `struct S`. Functions that operate on `struct S` are named `S_<verb>()` and should generally receive a pointer to `struct S` as first parameter. E.g. So: - `query_refspecs()` would be renamed to `refspec_find_match()`. - `query_refspecs_multiple()` would be renamed to `refspec_find_all_matches()`. - `find_negative_refspec_match()` would be renamed to `refspec_find_negative_match()`. Patrick
On Mon, 3 Feb 2025 at 12:16, Patrick Steinhardt <ps@pks.im> wrote: > > On Sat, Feb 01, 2025 at 12:12:00PM +0530, Meet Soni wrote: > > Rename `query_refspecs()` to `find_refspec_match` for clarity, as it > > finds a single matching refspec. > > > > Rename `query_refspecs_multiple()` to `find_all_refspec_matches` to > > better reflect that it collects all matching refspecs instead of > > returning just the first match. > > > > Rename `query_matches_negative_refspec()` to > > `find_negative_refspec_match` for consistency with the updated naming > > convention. > > Okay. The message might've read a tiny bit easier if it was a bulleted > list of renames. E.g.: > > We're about to move a couple of functions related to handling of > refspecs from "remote.c" into "refspec.c". In preparation for this > move, rename them to better reflect their intent: > > - `query_refspecs()` becomes `find_refspec_match()` for clarity, > as it finds a single matching refspec. > > ... Makes sense. > > I was wondering a bit about why we rename the static functions, as we > wouldn't have to expose them in a subsequent step anyway. Other than > that I think we should adhere to our coding guidelines with the renamed > public functions: Since we were renaming the query_* functions that are exposed, I updated the static ones as well to maintain naming consistency across related functions. > > The primary data structure that a subsystem 'S' deals with is called > `struct S`. Functions that operate on `struct S` are named > `S_<verb>()` and should generally receive a pointer to `struct S` as > first parameter. E.g. > > So: > > - `query_refspecs()` would be renamed to `refspec_find_match()`. > > - `query_refspecs_multiple()` would be renamed to > `refspec_find_all_matches()`. > > - `find_negative_refspec_match()` would be renamed to > `refspec_find_negative_match()`. > > Patrick Thanks for the review. Meet
Meet Soni <meetsoni3017@gmail.com> writes: >> So: >> >> - `query_refspecs()` would be renamed to `refspec_find_match()`. >> >> - `query_refspecs_multiple()` would be renamed to >> `refspec_find_all_matches()`. >> >> - `find_negative_refspec_match()` would be renamed to >> `refspec_find_negative_match()`. >> >> Patrick > > Thanks for the review. > Meet Yup, using predictable names that follow patterns based on easy-to-follow rules is a very useful tool to help developers. Thanks, both.
diff --git a/builtin/push.c b/builtin/push.c index 90de3746b5..e6527b0b04 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -78,7 +78,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref, .src = matched->name, }; - if (!query_refspecs(&remote->push, &query) && query.dst) { + if (!find_refspec_match(&remote->push, &query) && query.dst) { refspec_appendf(refspec, "%s%s:%s", query.force ? "+" : "", query.src, query.dst); diff --git a/remote.c b/remote.c index 1da8ec7037..4654bce5d4 100644 --- a/remote.c +++ b/remote.c @@ -925,7 +925,7 @@ 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) +static int find_negative_refspec_match(struct refspec *rs, struct refspec_item *query) { int i, matched_negative = 0; int find_src = !query->src; @@ -982,7 +982,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite return matched_negative; } -static void query_refspecs_multiple(struct refspec *rs, +static void find_all_refspec_matches(struct refspec *rs, struct refspec_item *query, struct string_list *results) { @@ -990,9 +990,9 @@ static void query_refspecs_multiple(struct refspec *rs, int find_src = !query->src; if (find_src && !query->dst) - BUG("query_refspecs_multiple: need either src or dst"); + BUG("find_all_refspec_matches: need either src or dst"); - if (query_matches_negative_refspec(rs, query)) + if (find_negative_refspec_match(rs, query)) return; for (i = 0; i < rs->nr; i++) { @@ -1013,7 +1013,7 @@ static void query_refspecs_multiple(struct refspec *rs, } } -int query_refspecs(struct refspec *rs, struct refspec_item *query) +int find_refspec_match(struct refspec *rs, struct refspec_item *query) { int i; int find_src = !query->src; @@ -1021,9 +1021,9 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query) char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) - BUG("query_refspecs: need either src or dst"); + BUG("find_refspec_match: need either src or dst"); - if (query_matches_negative_refspec(rs, query)) + if (find_negative_refspec_match(rs, query)) return -1; for (i = 0; i < rs->nr; i++) { @@ -1054,7 +1054,7 @@ char *apply_refspecs(struct refspec *rs, const char *name) memset(&query, 0, sizeof(struct refspec_item)); query.src = (char *)name; - if (query_refspecs(rs, &query)) + if (find_refspec_match(rs, &query)) return NULL; return query.dst; @@ -1062,7 +1062,7 @@ char *apply_refspecs(struct refspec *rs, const char *name) int remote_find_tracking(struct remote *remote, struct refspec_item *refspec) { - return query_refspecs(&remote->fetch, refspec); + return find_refspec_match(&remote->fetch, refspec); } static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen, @@ -2487,7 +2487,7 @@ static int get_stale_heads_cb(const char *refname, const char *referent UNUSED, memset(&query, 0, sizeof(struct refspec_item)); query.dst = (char *)refname; - query_refspecs_multiple(info->rs, &query, &matches); + find_all_refspec_matches(info->rs, &query, &matches); if (matches.nr == 0) goto clean_exit; /* No matches */ diff --git a/remote.h b/remote.h index 66ee53411d..f109310eda 100644 --- a/remote.h +++ b/remote.h @@ -269,7 +269,7 @@ int refname_matches_negative_refspec_item(const char *refname, struct refspec *r */ struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs); -int query_refspecs(struct refspec *rs, struct refspec_item *query); +int find_refspec_match(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);
Rename `query_refspecs()` to `find_refspec_match` for clarity, as it finds a single matching refspec. Rename `query_refspecs_multiple()` to `find_all_refspec_matches` to better reflect that it collects all matching refspecs instead of returning just the first match. Rename `query_matches_negative_refspec()` to `find_negative_refspec_match` for consistency with the updated naming convention. Signed-off-by: Meet Soni <meetsoni3017@gmail.com> --- builtin/push.c | 2 +- remote.c | 20 ++++++++++---------- remote.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-)