Re: jk/fetch-follow-remote-head-fix, was Re: What's cooking in git.git (Mar 2025, #07; Wed, 26)

From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Apr 5, 2025, 21:11 UTC
Message-ID: <D8Z0IRH55LVQ.3BORI2G7KD8Z2@ferdinandy.com>
In-Reply-To: <20250404085812.GA772404@coredump.intra.peff.net>

On Fri Apr 04, 2025 at 10:58, Jeff King <peff@peff.net> wrote:

Show 18 quoted lines

> On Wed, Mar 26, 2025 at 05:46:00AM -0700, Junio C Hamano wrote:
>
>> * jk/fetch-follow-remote-head-fix (2025-03-18) 3 commits
>>  - fetch: don't ask for remote HEAD if followRemoteHEAD is "never"
>>  - fetch: only respect followRemoteHEAD with configured refspecs
>>  - Merge branch 'jk/fetch-ref-prefix-cleanup' into jk/fetch-follow-remote-head-fix
>>  (this branch uses jk/fetch-ref-prefix-cleanup.)
>> 
>>  "git fetch [<remote>]" with only the configured fetch refspec
>>  should be the only thing to update refs/remotes/<remote>/HEAD,
>>  but the code was overly eager to do so in other cases.
>> 
>>  Will merge to 'next'?
>>  source: <20250318053905.GA2051217@coredump.intra.peff.net>
>
> I think so. The design was based on our discussion, and it seemed to get
> positive comments from you and Taylor. It might be nice to get an ack
> from Bence, since this is his feature I'm modifying.

Thanks, I saw the patches, but got swamped. I think it's perfectly reasonable to only update remote/HEAD when we're getting the entire remote, and not just bits and pieces. For bits and pieces update of remote/HEAD there's still remote set-head -a. I'm not sure if I should formally send an Acked-by on the patch?

And thanks for cleaning up the bugs this feature introduced!

Show 22 quoted lines

>
> Taylor did note one place where the resulting code is a little hard to
> read.  That could be addressed by adding this on top (or it could be
> squashed into patch 1):
>
> -- >8 --
> Subject: [PATCH] fetch: make set_head() call easier to read
>
> We ignore any error returned from set_head(), but 638060dcb9 (fetch
> set_head: refactor to use remote directly, 2025-01-26) left its call in
> a noop "if" conditional as a sort of note-to-self.
>
> When c834d1a7ce (fetch: only respect followRemoteHEAD with configured
> refspecs, 2025-03-18) added a "do_set_head" flag, it was rolled into the
> same conditional, putting set_head() on the right-hand side of a
> short-circuit AND.
>
> That's not wrong, but it really hides the point of the line, which
> is (maybe) calling the function.
>
> Instead, let's have a full if() block for the flag, and then our comment
> (with some rewording) will be sufficient to clarify the error handling.

It also makes it more transparent that the comment belongs to the set_head line, so definitely tidier.

Thanks, Bence

Show 28 quoted lines

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 3658509740..dbf741ef5b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1903,12 +1903,13 @@ static int do_fetch(struct transport *transport,
>  				  "you need to specify exactly one branch with the --set-upstream option"));
>  		}
>  	}
> -	if (do_set_head && set_head(remote_refs, transport->remote))
> -		;
> +	if (do_set_head) {
>  		/*
> -		 * Way too many cases where this can go wrong
> -		 * so let's just fail silently for now.
> +		 * Way too many cases where this can go wrong so let's just
> +		 * ignore errors and fail silently for now.
>  		 */
> +		set_head(remote_refs, transport->remote);
> +	}
>  
>  cleanup:
>  	if (retcode) {

-- 
bence.ferdinandy.com