jk/fetch-follow-remote-head-fix, was Re: What's cooking in git.git (Mar 2025, #07; Wed, 26)
- From
Jeff King <peff@peff.net>
- Date
- Apr 4, 2025, 08:58 UTC
- Message-ID
- <20250404085812.GA772404@coredump.intra.peff.net>
- In-Reply-To
- <xmqqiknwhsdz.fsf@gitster.g>
On Wed, Mar 26, 2025 at 05:46:00AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> * 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.
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.
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) {
-- 2.49.0.578.gaa93496c6a