Re: [PATCH v2 2/3] refspec: relocate query related functions

From: Meet Soni <meetsoni3017@gmail.com>
Date: Jan 29, 2025, 06:32 UTC
Message-ID: <CAPhwyn110E39uksCbSNYy3wRrxmG2QuuXEvPRrUT2SSTLxCKcQ@mail.gmail.com>
In-Reply-To: <xmqq7c6grrdl.fsf@gitster.g>

On Tue, 28 Jan 2025 at 00:55, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>
> Meet Soni <meetsoni3017@gmail.com> writes:
>
> > Move the functions `query_refspecs()`, `query_refspecs_multiple()` and
> > `query_matches_negative_refspec()` from `remote.c` to `refspec.c`. These
> > functions focus on querying refspecs, so centralizing them in `refspec.c`
> > improves code organization by keeping refspec-related logic in one place.
>
> I think query_matches_negative_refspec() is appropriate named (not
> that it matters much, as it becomes a mere private helper in the
> file), unlike the ones in the first patch that are suboptimally
> named.  query_refspecs() could probalby lose the plural 's' at the
> end---there is only single refspec, which is a collection of refspec
> items, involved and it makes a single query---but otherwise it also
> has an appropriate name (this matters a bit more, but not that much,
> as it was already public).
>
> query_refspecs_multiple() is not a great name, though.  It does not
> convey what is multiple.  Does it make multiple questions in one go?
> Does it ask a question that can have multiple answers?
>

I agree that the original names are ambiguous. query_refspecs_multiple() is similar to query_refspecs(), but instead of returning the first match, it collects all matching results.

To improve clarity and consistency, I’d like to propose the following
renames:
    *query_refspecs() -> find_refspec_match()
        `find` better describes its purpose than `query` and `match`
        clarifies that it’s looking for a single result.

    *query_refspecs_multiple() -> find_all_refspec_matches()
        Unlike the previous function, this one collects all matching results
        instead of stopping at the first match. The new name highlights that
        it returns multiple matches.
Let me know what you think!

Show 34 quoted lines

> > Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> > ---
> >  refspec.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  refspec.h |  16 +++++++
> >  remote.c  | 122 -----------------------------------------------------
> >  remote.h  |   1 -
> >  4 files changed, 139 insertions(+), 123 deletions(-)
>
> > diff --git a/refspec.h b/refspec.h
> > index 891d50b159..d0788de782 100644
> > --- a/refspec.h
> > +++ b/refspec.h
> > @@ -30,6 +30,8 @@ struct refspec_item {
> >       char *raw;
> >  };
> >
> > +struct string_list;
> > +
> >  #define REFSPEC_FETCH 1
> >  #define REFSPEC_PUSH 0
> >
> > @@ -84,4 +86,18 @@ int omit_name_by_refspec(const char *name, struct refspec *rs);
> >  int match_name_with_pattern(const char *key, const char *name,
> >                                  const char *value, char **result);
> >
> > +/*
> > + * Queries a refspec for a match and updates the query item.
> > + * Returns 0 on success, -1 if no match is found or negative refspec matches.
> > + */
> > +int query_refspecs(struct refspec *rs, struct refspec_item *query);
>
> This one now has an excellent comment.  Great job.
>
> Thanks.