From: "Bence Ferdinandy" <bence@ferdinandy•com>
To: "Jeff King" <peff@peff•net>, "Junio C Hamano" <gitster@pobox•com>
Cc: "Taylor Blau" <me@ttaylorr•com>, <git@vger•kernel.org>
Subject: Re: jk/fetch-follow-remote-head-fix, was Re: What's cooking in git.git (Mar 2025, #07; Wed, 26)
Date: Sat, 05 Apr 2025 23:11:26 +0200 [thread overview]
Message-ID: <D8Z0IRH55LVQ.3BORI2G7KD8Z2@ferdinandy.com> (raw)
In-Reply-To: <20250404085812.GA772404@coredump.intra.peff.net>
On Fri Apr 04, 2025 at 10:58, Jeff King <peff@peff•net> 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@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!
>
> 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@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
next prev parent reply other threads:[~2025-04-05 21:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-26 12:46 What's cooking in git.git (Mar 2025, #07; Wed, 26) Junio C Hamano
2025-03-26 17:19 ` Taylor Blau
2025-03-26 18:09 ` Jeff King
2025-03-29 0:34 ` D. Ben Knoble
2025-04-08 22:39 ` D. Ben Knoble
2025-04-08 22:49 ` Junio C Hamano
2025-04-04 8:58 ` jk/fetch-follow-remote-head-fix, was " Jeff King
2025-04-05 21:11 ` Bence Ferdinandy [this message]
2025-04-04 9:02 ` jk/zlib-inflate-fixes, " Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D8Z0IRH55LVQ.3BORI2G7KD8Z2@ferdinandy.com \
--to=bence@ferdinandy$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox