Re: [PATCH v3 3/5] refactor(remote): rename query_refspecs functions

On Mon, 3 Feb 2025 at 12:16, Patrick Steinhardt [off-list ref] wrote:

On Sat, Feb 01, 2025 at 12:12:00PM +0530, Meet Soni wrote:

quoted

Rename `query_refspecs()` to `find_refspec_match` for clarity, as it
finds a single matching refspec.

Rename `query_refspecs_multiple()` to `find_all_refspec_matches` to
better reflect that it collects all matching refspecs instead of
returning just the first match.

Rename `query_matches_negative_refspec()` to
`find_negative_refspec_match` for consistency with the updated naming
convention.

Okay. The message might've read a tiny bit easier if it was a bulleted
list of renames. E.g.:

    We're about to move a couple of functions related to handling of
    refspecs from "remote.c" into "refspec.c". In preparation for this
    move, rename them to better reflect their intent:

      - `query_refspecs()` becomes `find_refspec_match()` for clarity,
        as it finds a single matching refspec.

    ...

Makes sense.

I was wondering a bit about why we rename the static functions, as we
wouldn't have to expose them in a subsequent step anyway. Other than
that I think we should adhere to our coding guidelines with the renamed
public functions:

Since we were renaming the query_* functions that are exposed, I updated
the static ones as well to maintain naming consistency across related functions.

    The primary data structure that a subsystem 'S' deals with is called
    `struct S`. Functions that operate on `struct S` are named
    `S_<verb>()` and should generally receive a pointer to `struct S` as
    first parameter. E.g.

So:

  - `query_refspecs()` would be renamed to `refspec_find_match()`.

  - `query_refspecs_multiple()` would be renamed to
    `refspec_find_all_matches()`.

  - `find_negative_refspec_match()` would be renamed to
    `refspec_find_negative_match()`.

Patrick

Thanks for the review.
Meet