Taylor Blau <me@xxxxxxxxxxxx> writes:
> So I think the right fix would really be something like:
>
> --- 8< ---
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index fe2b26c74a..0e63621e6c 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1770,9 +1770,8 @@ static int do_fetch(struct transport *transport,
>
> if (tags == TAGS_SET || tags == TAGS_DEFAULT) {
> must_list_refs = 1;
> - if (transport_ls_refs_options.ref_prefixes.nr)
> - strvec_push(&transport_ls_refs_options.ref_prefixes,
> - "refs/tags/");
I guess the original "if we are giving prefixes already, then make
sure we learn about tags" is because the semantics of the ref-prefix
is peculiar, in that without _any_, there is no limitation and the
remote end would advertise all its refs, but if there is even one,
the remote end would advertise only the ones that match any prefix?
> + strvec_push(&transport_ls_refs_options.ref_prefixes,
> + "refs/tags/");
So this change would make sure that, even if somebody else already
added a prefix earlier, we would always ask about tags if we come
into this block. It also means when nobody has added a prefix
before we reach here, we still add prefix to ask about tags. The
implication of which may be rather grave. If a later code relied
on us _not_ asking for tags here (because nobody is limiting the
ls-refs request with prefix), after this change, that code will
now see that there is some prefix requested already, and if we
wanted to retain what that code does, we would need to adjust that
code (and then the need for similar modification cascades X-<).
> }
>
> if (uses_remote_tracking(transport, rs)) {
> --- >8 ---
>
> But I'm unfamiliar enough with this area that I'd appreciate comments
> from the authors of these various commits, all of whom have been CC'd.
> Does this seem right to you, or am I totally down the wrong path?
I have no idea without doing a deep dive myself, for which I have
failed to find time for so far.
I have to wonder if the first thing we should do is to update the
logic to decide what ref prefixes to use, so that it does not do
such an incremental strvec_push(). There are multiple questions in
the form "Do we need to know about X?" the code path need to ask
before deciding what prefixes to use, and before Bence's "We need to
know about HEAD, because we want to know about its symref value",
refs/tags hierarchy was one (and only?) thing we needed to ask
about. The bug under discussion smells like the result of how
brittle the code's assumption about what prefixes other parts would
want to ask. This code did not assume there is anybody else, and
that is why it broke when we started asking about HEAD. Now we have
two possible X's in that question (refs/tags and HEAD). We may want
to make sure the code is robust enough when we add the third one.
Thanks.
