Re: [GSoC][PATCH] refspec: clarify function naming and documentation

[Junio C Hamano <gitster@pobox.com>]

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.