From: Harald Nordgren <haraldnordgren@gmail.com>
Date: Sat, 10 Oct 2026 18:36:26 GMT
Subject: Re: [PATCH v8 2/5] fetch: extract collect_upstream_from_remote() helper
Message-ID: <CAHwyqnW2FqKBUK=s9ZL_kwwZDf2WE87qT9YN-syLs2O5FXpMtA@mail.gmail.com>
In-Reply-To: <xmqqik39iipg.fsf@gitster.g>

> IOW I would have expected to see
>
>         } else {
>                 if (transport->remote->fetch.nr) {
>                         struct string_list tracked = STRING_LIST_INIT_DUP;
>                         struct string_list_item *item;
>
>                         refspec_ref_prefixes(&transport->remote->fetch,
>                                              &transport_ls_refs_options.ref_prefixes);
>                         if (follow_remote_head != FOLLOW_REMOTE_NEVER)
>                                 do_set_head = 1;
>                 }
>
>                 /*
>                  * The configured refspec may not cover the current
>                  * branch's upstream (e.g. a narrowed -t refspec), so
>                  * make sure we can still fetch it regardless.
>                  */
>                 collect_upstream_from_remote(the_repository, &tracked,
>                                               transport->remote, NULL);
>                 for_each_string_list_item(item, &tracked)
>                         strvec_push(&transport_ls_refs_options.ref_prefixes,
>                                     item->string);
>                 string_list_clear(&tracked, 0);
>         }
>
> that touches "help the current branch when rs->nr == 0" part and
> nothing else.
>
> Of course, because the next step [3/5] wants to split the above
> else{} block to do different things between the case where
> remote.*.fetch is and is not empty, at that point the extra
> code may be introduced there, ending up with the shape of if/else if
> cascade as we see in your patch.

This is a good point about how commits tell their story, and I'll look into it.

Thanks for the snippet!

> > diff --git a/remote.c b/remote.c
> > index 99a086ea5a..5e980625b8 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1884,6 +1884,21 @@ int branch_merge_matches(struct branch *branch,
> >       return refname_match(branch->merge[i]->src, refname);
> >  }
> >
> > +void collect_upstream_from_remote(struct repository *repo,
> > +                                struct string_list *tracked,
> > +                                struct remote *remote,
> > +                                const char *refname)
> > +{
> > +     struct branch *branch = repo_branch_get(repo, refname);
> > +
> > +     if (!branch_has_merge_config(branch) ||
> > +         strcmp(branch->remote_name, remote->name))
> > +             return;
> > +     for (int i = 0; i < branch->merge_nr; i++)
> > +             string_list_insert(tracked, branch->merge[i]->src);
> > +}
>
> The original tries to avoid branch == NULL causing a segfault, but
> the above does not.  Intended or overlooked?

Overlooked, but I don't think the segfault would actually happen (in a
way that could be reproduced by a test)?


Harald