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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 3, 2025, 06:46 UTC
Message-ID: <Z6BmKO-034bqOCjS@pks.im>
In-Reply-To: <20250201064202.76116-4-meetsoni3017@gmail.com>

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

Show 10 quoted lines

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

...

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:

    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