Re: [PATCH v2 1/3] refspec: relocate omit_name_by_refspec and related functions

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

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.