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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2025, 18:56 UTC
Message-ID: <xmqqldu8l5hq.fsf@gitster.g>
In-Reply-To: <20250214053938.26807-1-meetsoni3017@gmail.com>

Meet Soni <meetsoni3017@gmail.com> writes:

Show 16 quoted lines

> 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.

Show 5 quoted lines

> -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.