On Mon, 27 Jan 2025 at 22:51, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Meet Soni <meetsoni3017@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > ... > > 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.
