Re: [PATCH 2/6] fetch: backfill tags before setting upstream

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 15, 2022, 06:43 UTC
Message-ID: <CAP8UFD3RpeMcfTDV89gRjhaOsjjbPv3x_pFPqF-gO4Kpv51VtQ@mail.gmail.com>
In-Reply-To: <64c94e7a28d83fbaa1b6308b034d07c7be10e767.1644565025.git.ps@pks.im>

On Fri, Feb 11, 2022 at 5:12 PM Patrick Steinhardt <ps@pks.im> wrote:

Show 11 quoted lines

>
> The fetch code flow is a bit hard to understand right now:
>
>     1. We optionally prune all references which have vanished on the
>        remote side.
>     2. We fetch and update all other references locally.
>     3. We update the upstream branch in the gitconfig.
>     4. We backfill tags pointing into the history we have just fetched.
>
> It is quite confusing that we fetch objects and update references in
> both (2) and (4), which is further stressed by the point that we require

s/require/use/

> a `skip` label which jumps from (3) to (4) in case we fail to update the

s/`skip` label/`skip` goto label/ s/which jumps/to jump/

Show 6 quoted lines

> gitconfig as expected.
>
> Reorder the code to first update all local references, and only after we
> have done so update the upstream branch information. This improves the
> code flow and furthermore makes it easier to refactor the way we update
> references together.

Show 20 quoted lines

> @@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
>  static int do_fetch(struct transport *transport,
>                     struct refspec *rs)
>  {
> -       struct ref *ref_map;
> +       struct ref *ref_map = NULL;
>         int autotags = (transport->remote->fetch_tags == 1);
>         int retcode = 0;
>         const struct ref *remote_refs;
> @@ -1618,11 +1618,22 @@ static int do_fetch(struct transport *transport,
>                 }
>         }
>         if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
> -               free_refs(ref_map);
>                 retcode = 1;
>                 goto cleanup;
>         }
>
> +       /* if neither --no-tags nor --tags was specified, do automated tag
> +        * following ... */

Maybe while at it this could be changed to use our usual style for multi-line comments:

       /*
        * If neither --no-tags nor --tags was specified, do automated tag
        * following...
        */

Show 9 quoted lines

> +       if (tags == TAGS_DEFAULT && autotags) {
> +               struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
> +
> +               find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> +               if (tags_ref_map)
> +                       backfill_tags(transport, tags_ref_map, worktrees);
> +
> +               free_refs(tags_ref_map);
> +       }

Show 5 quoted lines

> @@ -1676,21 +1687,9 @@ static int do_fetch(struct transport *transport,
>                                   "you need to specify exactly one branch with the --set-upstream option"));
>                 }
>         }
> -skip:

I like that it's removing one goto label and making the code simpler.