From c834d1a7cef9b29d440af9369d253dab902238cb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 18 Mar 2025 01:40:17 -0400 Subject: fetch: only respect followRemoteHEAD with configured refspecs The new followRemoteHEAD feature is triggered for almost every fetch, causing us to ask the server about the remote "HEAD" and to consider updating our local tracking HEAD symref. This patch limits the feature only to the case when we are fetching a remote using its configured refspecs (typically into its refs/remotes/ hierarchy). There are two reasons for this. One is efficiency. E.g., the fixes in 6c915c3f85 (fetch: do not ask for HEAD unnecessarily, 2024-12-06) and 20010b8c20 (fetch: avoid ls-refs only to ask for HEAD symref update, 2025-03-08) were aimed at reducing the work we do when we would not be able to update HEAD anyway. But they do not quite cover all cases. The remaining one is: git fetch origin refs/heads/foo:refs/remotes/origin/foo which _sometimes_ can update HEAD, but usually not. And that leads us to the second point, which is being simple and explainable. The code for updating the tracking HEAD symref requires both that we learned which ref the remote HEAD points at, and that the server advertised that ref to us. But because the v2 protocol narrows the server's advertisement, the command above would not typically update HEAD at all, unless it happened to point to the "foo" branch. Or even weirder, it probably _would_ update if the server is very old and supports only the v0 protocol, which always gives a full advertisement. This creates confusing behavior for the user: sometimes we may try to update HEAD and sometimes not, depending on vague rules. One option here would be to loosen the update code to accept the remote HEAD even if the server did not advertise that ref. I think that could work, but it may also lead to interesting corner cases (e.g., creating a dangling symref locally, even though the branch is not unborn on the server, if we happen not to have fetched it). So let's instead simplify the rules: we'll only consider updating the tracking HEAD symref when we're doing a full fetch of the remote's configured refs. This is easy to implement; we can just set a flag at the moment we realize we're using the configured refspecs. And we can drop the special case code added by 6c915c3f85 and 20010b8c20, since this covers those cases. The existing tests from those commits still pass. In t5505, an incidental call to "git fetch " updated HEAD, which caused us to adjust the test in 3f763ddf28 (fetch: set remote/HEAD if it does not exist, 2024-11-22). We can now adjust that back to how it was before the feature was added. Even though t5505 is incidentally testing our new desired behavior, we'll add an explicit test in t5510 to make sure it is covered. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) (limited to 'builtin/fetch.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index 02af505469..66f5ae31b6 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1691,21 +1691,6 @@ cleanup: return result; } -static int uses_remote_tracking(struct transport *transport, struct refspec *rs) -{ - if (!remote_is_configured(transport->remote, 0)) - return 0; - - if (!rs->nr) - rs = &transport->remote->fetch; - - for (int i = 0; i < rs->nr; i++) - if (rs->items[i].dst) - return 1; - - return 0; -} - static int do_fetch(struct transport *transport, struct refspec *rs, const struct fetch_config *config) @@ -1720,6 +1705,7 @@ static int do_fetch(struct transport *transport, TRANSPORT_LS_REFS_OPTIONS_INIT; struct fetch_head fetch_head = { 0 }; struct strbuf err = STRBUF_INIT; + int do_set_head = 0; if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2) @@ -1740,9 +1726,11 @@ static int do_fetch(struct transport *transport, } else { struct branch *branch = branch_get(NULL); - if (transport->remote->fetch.nr) + if (transport->remote->fetch.nr) { refspec_ref_prefixes(&transport->remote->fetch, &transport_ls_refs_options.ref_prefixes); + do_set_head = 1; + } if (branch_has_merge_config(branch) && !strcmp(branch->remote_name, transport->remote->name)) { int i; @@ -1765,8 +1753,7 @@ static int do_fetch(struct transport *transport, strvec_push(&transport_ls_refs_options.ref_prefixes, "refs/tags/"); - if (transport_ls_refs_options.ref_prefixes.nr && - uses_remote_tracking(transport, rs)) + if (do_set_head) strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); @@ -1918,7 +1905,7 @@ static int do_fetch(struct transport *transport, "you need to specify exactly one branch with the --set-upstream option")); } } - if (set_head(remote_refs, transport->remote)) + if (do_set_head && set_head(remote_refs, transport->remote)) ; /* * Way too many cases where this can go wrong -- cgit v1.2.3 From aab0f899d9349bed824bf545b7398ab16c27a204 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 18 Mar 2025 01:41:22 -0400 Subject: fetch: don't ask for remote HEAD if followRemoteHEAD is "never" When we are going to consider updating the refs/remotes/*/HEAD symref, we have to ask the remote side where its HEAD points. But if we know that the feature is disabled by config, we don't need to bother! This saves a little bit of work and network communication for the server. And even a little bit of effort on the client, as our local set_head() function did a bit of work matching the remote HEAD before realizing that we're not going to do anything with it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 6 ++---- t/t5510-fetch.sh | 5 ++++- 2 files changed, 6 insertions(+), 5 deletions(-) (limited to 'builtin/fetch.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index 66f5ae31b6..3658509740 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1643,9 +1643,6 @@ static int set_head(const struct ref *remote_refs, struct remote *remote) string_list_append(&heads, strip_refshead(ref->name)); } - if (follow_remote_head == FOLLOW_REMOTE_NEVER) - goto cleanup; - if (!heads.nr) result = 1; else if (heads.nr > 1) @@ -1729,7 +1726,8 @@ static int do_fetch(struct transport *transport, if (transport->remote->fetch.nr) { refspec_ref_prefixes(&transport->remote->fetch, &transport_ls_refs_options.ref_prefixes); - do_set_head = 1; + if (transport->remote->follow_remote_head != FOLLOW_REMOTE_NEVER) + do_set_head = 1; } if (branch_has_merge_config(branch) && !strcmp(branch->remote_name, transport->remote->name)) { diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ad23dd11ef..5f0eb5684e 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -119,7 +119,10 @@ test_expect_success "fetch test followRemoteHEAD never" ' cd two && git update-ref --no-deref -d refs/remotes/origin/HEAD && git config set remote.origin.followRemoteHEAD "never" && - git fetch && + GIT_TRACE_PACKET=$PWD/trace.out git fetch && + # Confirm that we do not even ask for HEAD when we are + # not going to act on it. + test_grep ! "ref-prefix HEAD" trace.out && test_must_fail git rev-parse --verify refs/remotes/origin/HEAD ) ' -- cgit v1.2.3 From f9356f9cb4c2c9c6baab30c1a8579445fddfe502 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 4 Apr 2025 04:58:12 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/fetch.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'builtin/fetch.c') 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) { -- cgit v1.2.3