| Message ID | 20250127103644.36627-2-meetsoni3017@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refspec: centralize refspec-related logic | expand |
Meet Soni <meetsoni3017@gmail.com> writes: > 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> > --- > ... > diff --git a/refspec.h b/refspec.h > index 69d693c87d..891d50b159 100644 > --- a/refspec.h > +++ b/refspec.h > @@ -71,4 +71,17 @@ struct strvec; > void refspec_ref_prefixes(const struct refspec *rs, > struct strvec *ref_prefixes); Back when these functions were mere local helper functions in remote.c, their name being less descriptive of what they do may have been OK (because readers have more context to understand them), but when we make it a part of a public API, we should re-evaluate if their names are good enough. > +/* > + * 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); Imagine you found this description in the header file and are trying to figure out if it helps you writing the feature you are adding to Git. Are the above description and the name of the function useful enough to you? The first question that came to my mind was "what is exactly a 'name'?" In the context of the original, the caller iterates over a list of "struct ref" and feeds the "name" member of the struct, but this caller does not even have to know it is getting a part of "struct ref"; it only cares about its parameter being a character string. In that context, is "name" the best identifer you can give to this parameter? At least calling it "refname" might boost the signal the name gives to the reader a bit better (and it is in line with how refs.h calls these things). Another thing to consider is if the comment describes the purpose of the function well, instead of just rephrasing what its implementation does. What does it mean to return true iff there is even one negative refspec that matches? What is the conceivable use a caller would want to use such a function? As I said, calling it "omit" was probably OK in the context of the original file, but it was already sloppy. This function merely provides one bit of information (i.e. "does it match any nagative refspec---Yes or No?"), and it is up to its caller how to use that piece of information form. One of its callers, apply_negative_refspecs(), happens to use it to filter a list of "struct ref" it received from its caller to drop the refs from the list that match any negative refspec, but the other existing caller does not even filter or omit anything from a collection it has. My personal preference is to do this kind of change in two separate patches: (1) as a preliminary clean-up, we rename functions and their parameters in the original place; if needed, add clarifying comments. (2) move the resulting functions with the comments to their new home. If these two step conversions results in extern int refname_matches_negative_refspec_item (const char *refname, struct refspec *refspec); I suspect that it is clear enough that there is no need for any extra comment to explain what it does. > +/* > + * Checks whether a name matches a pattern and optionally generates a result. > + * Returns 1 if the name matches the pattern, 0 otherwise. > + */ > +int match_name_with_pattern(const char *key, const char *name, > + const char *value, char **result); > + As this is merely moved from an existing header, I am tempted to say I'll leave it as an exercise to the readers to improve this one, as improving it is outside the scope of this work. Some hints for those who want to tackle the clean-up for extra points, perhaps after the dust settles from this series. The "pattern" in the name refers to the src side of a globbing refspec and is passed in the parameter "key", so we are calling the same thing in three different names, which is already triply bad. "optionally generates a result" does not convey any meaning outside the context of the original, as it does not even talk about what computation is creating the result. It does not even say what controls the optionality---without reading the implementation, it is likely your readers would assume passing NULL to result is all it takes to skip that optional feature, but that is not the case. If I understand correctly, here is what this one does. It takes the source side of a globbing refspec item (e.g. "refs/heads/*" in "refs/heads/*:refs/remotes/origin/*"), a refname that might match the glob pattern, the destination side of the refspec item (e.g. "refs/remotes/origin/*" in the same example), and a pointer that points at a variable to receive the result. If the source pattern matches the given refname, apply the source-to-destination mapping rule to compute the resulting destination refname and store it in the result. The destination side is optional; if you do not need to map the refname to another refname, but are merely interested if the refname matches the glob pattern, you can pass NULL and result location is not touched. In either case, returns true iff the source side of the globbing refspec item matches the given refname. So "name" in the function name should probably become a bit narrower, like "refname". Also the names of its parameters need to be better thought out.
On Mon, 27 Jan 2025 at 22:51, Junio C Hamano <gitster@pobox.com> wrote: > > Meet Soni <meetsoni3017@gmail.com> writes: > > > 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> > > --- > > ... > > diff --git a/refspec.h b/refspec.h > > index 69d693c87d..891d50b159 100644 > > --- a/refspec.h > > +++ b/refspec.h > > @@ -71,4 +71,17 @@ struct strvec; > > void refspec_ref_prefixes(const struct refspec *rs, > > struct strvec *ref_prefixes); > > Back when these functions were mere local helper functions in > remote.c, their name being less descriptive of what they do may have > been OK (because readers have more context to understand them), but > when we make it a part of a public API, we should re-evaluate if > their names are good enough. > > > +/* > > + * 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); > > Imagine you found this description in the header file and are trying > to figure out if it helps you writing the feature you are adding to > Git. Are the above description and the name of the function useful > enough to you? > > The first question that came to my mind was "what is exactly a 'name'?" > > In the context of the original, the caller iterates over a list of > "struct ref" and feeds the "name" member of the struct, but this > caller does not even have to know it is getting a part of "struct > ref"; it only cares about its parameter being a character string. > > In that context, is "name" the best identifer you can give to this > parameter? At least calling it "refname" might boost the signal the > name gives to the reader a bit better (and it is in line with how > refs.h calls these things). > > Another thing to consider is if the comment describes the purpose of > the function well, instead of just rephrasing what its > implementation does. What does it mean to return true iff there is > even one negative refspec that matches? What is the conceivable use > a caller would want to use such a function? > > As I said, calling it "omit" was probably OK in the context of the > original file, but it was already sloppy. This function merely > provides one bit of information (i.e. "does it match any nagative > refspec---Yes or No?"), and it is up to its caller how to use that > piece of information form. > > One of its callers, apply_negative_refspecs(), happens to use it to > filter a list of "struct ref" it received from its caller to drop > the refs from the list that match any negative refspec, but the > other existing caller does not even filter or omit anything from a > collection it has. > > My personal preference is to do this kind of change in two separate > patches: > > (1) as a preliminary clean-up, we rename functions and their > parameters in the original place; if needed, add clarifying > comments. > > (2) move the resulting functions with the comments to their new > home. > > If these two step conversions results in > > extern int refname_matches_negative_refspec_item > (const char *refname, struct refspec *refspec); > > I suspect that it is clear enough that there is no need for any > extra comment to explain what it does. > Makes sense. I'll implement this in the upcoming version of this patch. Since I’ve already prepared a patch for moving the function in the current series, I’ll add a commit to handle the renaming and changing comments. > > +/* > > + * Checks whether a name matches a pattern and optionally generates a result. > > + * Returns 1 if the name matches the pattern, 0 otherwise. > > + */ > > +int match_name_with_pattern(const char *key, const char *name, > > + const char *value, char **result); > > + > > As this is merely moved from an existing header, I am tempted to say > I'll leave it as an exercise to the readers to improve this one, as > improving it is outside the scope of this work. > > Some hints for those who want to tackle the clean-up for extra > points, perhaps after the dust settles from this series. > > The "pattern" in the name refers to the src side of a globbing > refspec and is passed in the parameter "key", so we are calling the > same thing in three different names, which is already triply bad. > > "optionally generates a result" does not convey any meaning outside > the context of the original, as it does not even talk about what > computation is creating the result. It does not even say what > controls the optionality---without reading the implementation, it is > likely your readers would assume passing NULL to result is all it > takes to skip that optional feature, but that is not the case. > > If I understand correctly, here is what this one does. > > It takes the source side of a globbing refspec item (e.g. > "refs/heads/*" in "refs/heads/*:refs/remotes/origin/*"), a > refname that might match the glob pattern, the destination side > of the refspec item (e.g. "refs/remotes/origin/*" in the same > example), and a pointer that points at a variable to receive the > result. If the source pattern matches the given refname, apply > the source-to-destination mapping rule to compute the resulting > destination refname and store it in the result. > > The destination side is optional; if you do not need to map the > refname to another refname, but are merely interested if the > refname matches the glob pattern, you can pass NULL and result > location is not touched. > > In either case, returns true iff the source side of the globbing > refspec item matches the given refname. > > So "name" in the function name should probably become a bit > narrower, like "refname". Also the names of its parameters need to > be better thought out. I agree that the function and its parameters could be improved for clarity. Since you mentioned leaving it as an exercise for readers, I’m happy to take it up and write a follow-up patch to address these issues after finishing the current series, if that works.
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..891d50b159 100644 --- a/refspec.h +++ b/refspec.h @@ -71,4 +71,17 @@ 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); + +/* + * Checks whether a name matches a pattern and optionally generates a result. + * Returns 1 if the name matches the pattern, 0 otherwise. + */ +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.
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 | 13 +++++++++++++ remote.c | 48 ------------------------------------------------ remote.h | 6 ------ 4 files changed, 61 insertions(+), 54 deletions(-)