From: Harald Nordgren Date: Fri, 09 Oct 2026 08:09:59 GMT Subject: Re: [PATCH v7 2/4] fetch: infer branches to fetch from a refmap-only remote Message-ID: In-Reply-To: > Perhaps something like this instead: > > ... but nothing to fetch is specified on the command line, > branches from the remote that are used as '@{upstream}' of > our local branches are fetched. Thanks, will update. > The unified diff is a bit messy to compare the before-and-after > behaviour, so let's see what the preimage said first. > > > struct branch *branch = branch_get(NULL); > > - > > - if (transport->remote->fetch.nr) { > > refspec_ref_prefixes(&transport->remote->fetch, > > &transport_ls_refs_options.ref_prefixes); > > - if (follow_remote_head != FOLLOW_REMOTE_NEVER) > > - do_set_head = 1; > > } > > - if (branch && branch_has_merge_config(branch) && > > - !strcmp(branch->remote_name, transport->remote->name)) { > > int i; > > for (i = 0; i < branch->merge_nr; i++) { > > strvec_push(&transport_ls_refs_options.ref_prefixes, > > So, when !rs->nr (i.e., nothing given on the command line to be fetched) > and there is no fetch refspec, we checked the current branch and if > it has merge config to merge from branches at the remote, we > automatically fetched them. This is the world order before this > "infer with @{u} and refmap" work, and we should behave the same way > when remote.*.refmap is not set. > > Let's see what the postimage says. > > > struct branch *branch = branch_get(NULL); > > + int tracks_this_remote = branch && branch_has_merge_config(branch) && > > + !strcmp(branch->remote_name, transport->remote->name); Yes, this can be improved. > This variable tells the code that it must infer branches, but named > as if it were a list of branches that were inferred. "When remote.*.fetch > does not exist and we have the refmap to use for inferring". > > > + if (inferred_branches) { > > + struct string_list tracked = STRING_LIST_INIT_DUP; > > + struct string_list_item *item; > > + > > + branches_tracking_remote(transport->remote, &tracked); > > + for_each_string_list_item(item, &tracked) > > + strvec_push(&transport_ls_refs_options.ref_prefixes, > > + item->string); > > + string_list_clear(&tracked, 0); > > + } else if (transport->remote->fetch.nr) { > > refspec_ref_prefixes(&transport->remote->fetch, > > &transport_ls_refs_options.ref_prefixes); > > } I'll rename it to 'infer_from_refmap'. > It would have been much easier to follow if the existing code came > first to make it clear that the new code is an add-on. After all, > when transport->remote->fetch.nr is true, inferred_branches is never > true. > > > + if ((transport->remote->fetch.nr || inferred_branches) && > > + follow_remote_head != FOLLOW_REMOTE_NEVER) > > + do_set_head = 1; > > + if (tracks_this_remote) { > > int i; > > for (i = 0; i < branch->merge_nr; i++) { > > strvec_push(&transport_ls_refs_options.ref_prefixes, > > How does tracks_this_remote and inferred_branches interact? Doesn't > the old code that grabs necessary remote-tracking branches for the > current branch add the same branch from the remote? Doesn't @{u} > for the current branch added twice on the list of branches to fetch? Good points. > > @@ -2009,6 +2046,7 @@ static int do_fetch(struct transport *transport, > > > > ref_map = get_ref_map(transport->remote, remote_refs, rs, > > tags, &autotags); > > + > > if (!update_head_ok) > > check_not_current_branch(ref_map); > > Useless patch noise. Fixed. > > diff --git a/remote.c b/remote.c > > index 99a086ea5a..c26312beea 100644 > > --- a/remote.c > > +++ b/remote.c > > @@ -1884,6 +1884,35 @@ int branch_merge_matches(struct branch *branch, > > return refname_match(branch->merge[i]->src, refname); > > } > > > > +struct branches_tracking_remote_cb_data { > > + struct remote *remote; > > + struct string_list *tracked; > > +}; > > + > > +static int add_if_tracking_remote(const struct reference *ref, void *cb_data) > > +{ > > + struct branches_tracking_remote_cb_data *data = cb_data; > > + struct branch *branch; > > + > > + branch = branch_get(ref->name); > > I know branch_get() is defined here and allows implicit use of > the_repository, but can't we pass "struct repository *" around in > cb_data so that we can use repo_branch_get() here? > > > + if (!branch_has_merge_config(branch) || > > + strcmp(branch->remote_name, data->remote->name)) > > + return 0; > > + > > + for (int i = 0; i < branch->merge_nr; i++) > > + string_list_insert(data->tracked, branch->merge[i]->src); > > + > > + return 0; > > +} > > This is more or less identical to the "if current branch integrates > with branches from the remote, then fetch them" code we saw earlier > in the builtin/fetch.c:do_fetch() above. I notice that its return > value is meaningless, as it always returns 0. I'll look into unifying. The 0 return value is there so it can be used with 'refs_for_each_branch_ref' without stopping the iteration. > static int add_if_tracking_remote(...) > { > struct branches_tracking_remote_cb_data *data = cb_data; > > collect_upstream_from_remote(data->tracked, data->remote, ref->name); > } > > This will mean we will have a very small preliminary patch to > introduce collect_upstream_from_remote() function in remote.c and > update the "help current branch by fetching what are merged into it" > code in do_fetch() to use it, which will have the above ontlined > if/else if/ cascade except for your new refmap code. On top, this > step will insert a single "else if" block to add your new logic to > do_fetch(). Yes, good idea. > > +void branches_tracking_remote(struct remote *remote, struct string_list *tracked) > > +{ > > + struct branches_tracking_remote_cb_data data = { remote, tracked }; > > + > > + refs_for_each_branch_ref(get_main_ref_store(the_repository), > > + add_if_tracking_remote, &data); > > +} > > This also hardcodes the_repository, but shouldn't this function take > "struct repository *" pointer (and shove it in data structure to > pass it down)? Good point. Harald