Re: [PATCH v2 2/3] refspec: relocate query related functions
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 27, 2025, 19:25 UTC
- Message-ID
- <xmqq7c6grrdl.fsf@gitster.g>
- In-Reply-To
- <20250127103644.36627-3-meetsoni3017@gmail.com>
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?
Show 7 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(-)
Show 22 quoted lines
> 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.