On Fri Apr 04, 2025 at 10:58, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxxx>
>
> 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!
>
> 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
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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
