Meet Soni <meetsoni3017@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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.
