Re: [PATCH v7 2/4] fetch: infer branches to fetch from a refmap-only remote

> 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.

> 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);

> 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);
> >               }

> 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?

> > @@ -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.

> > 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.

>         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().

> > +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)?