Re: [PATCH 1/4] refs: add referent parameter to refs_resolve_ref_unsafe

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2024, 18:21 UTC
Message-ID: <xmqq34pqlyou.fsf@gitster.g>
In-Reply-To: <011c10f488610b0a795a843bff66723477783761.1717694801.git.gitgitgadget@gmail.com>

ADMINISTRIVIA. Check the address you place on the CC: line. What we can see for this message at

https://lore.kernel.org/git/011c10f488610b0a795a843bff66723477783761.1717694801.git.gitgitgadget@gmail.com/raw

looks like this.

    Cc: "Phillip Wood [ ]" <phillip.wood123@gmail.com>,
        Kristoffer Haugsbakk <[code@khaugsbakk.name]>,
        "Jeff King [ ]" <peff@peff.net>,
        "Patrick Steinhardt [ ]" <ps@pks.im>,
        "=?UTF-8?Q?Jean-No=C3=ABl?= Avila [ ]" <avila.jn@gmail.com>,
        John Cai <johncai86@gmail.com>,
        John Cai <johncai86@gmail.com>

I fixed them manually, but it wasn't pleasant. I think we saw a similar breakage earlier coming via GGG, but I do not recall the details of how to cause such breakages (iow, what to avoid repeating this).

Anyway.

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  29 files changed, 64 insertions(+), 52 deletions(-)

Wow, the blast radius of this thing is rather big. Among these existing callers of refs_resolve_ref_unsafe(), how many of them will benefit from being able to pass a non NULL parameter at the end of the series, and more importantly, in the future to take advantage of the new feature possibly with a separate series?

I am assuming that this will benefit only a selected few and the callers that would want to take advantage of the new feature will remain low. Have you considered renaming refs_resolve_ref_unsafe() to a new name (say, refs_resolve_ref_unsafe_with_referent()) and implement the new feature (which is only triggered when the new parameter gets a non NULL value), make refs_resolve_ref_unsafe() a very thin wrapper that passes NULL to the new thing?

That way, you do not have to touch those existing callers that will never benefit from the new capability in the future. You won't risk conflicting with in flight topics semantically, either.

Or will they also benefit from the new feature in the future?

Offhand, I do not know how a caller like this ...

Show 13 quoted lines

> diff --git a/add-interactive.c b/add-interactive.c
> index b5d6cd689a1..041d30cf2b3 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -533,7 +533,7 @@ static int get_modified_files(struct repository *r,
>  {
>  	struct object_id head_oid;
>  	int is_initial = !refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> -						  "HEAD", RESOLVE_REF_READING,
> +						  "HEAD", NULL, RESOLVE_REF_READING,
>  						  &head_oid, NULL);
>  	struct collection_status s = { 0 };
>  	int i;

... would be helped.

Thanks.