Re: [PATCH 3/6] fetch: control lifecycle of FETCH_HEAD in a single place

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

On Tue, Feb 15, 2022 at 08:32:56AM +0100, Christian Couder wrote:
> On Sat, Feb 12, 2022 at 5:49 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > There are two different locations where we're appending to FETCH_HEAD:
> > first when storing updated references, and second when backfilling tags.
> > Both times we open the file, append to it and then commit it into place,
> > which is essentially duplicate work.
> >
> > Improve the lifecycle of updating FETCH_HEAD by opening and committing
> > it once in `do_fetch()`, where we pass the structure down to code which
> 
> s/down to code/down to the code/
> 
> > wants to append to it.
> 
> > @@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,
> >         if (!update_head_ok)
> >                 check_not_current_branch(ref_map, worktrees);
> >
> > +       retcode = open_fetch_head(&fetch_head);
> > +       if (retcode)
> > +               goto cleanup;
> > +
> >         if (tags == TAGS_DEFAULT && autotags)
> >                 transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
> >         if (prune) {
> > @@ -1617,7 +1616,8 @@ static int do_fetch(struct transport *transport,
> >                                    transport->url);
> >                 }
> >         }
> > -       if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
> > +
> > +       if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
> >                 retcode = 1;
> >                 goto cleanup;
> >         }
> 
> I think the following (if it works) would be more consistent with
> what's done for open_fetch_head() above:
> 
> retcode = fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)
> if (retcode)
>       goto cleanup;

The code here is really quite fragile, and I'm not much of a fan how we
need to retain `retcode` across the calls. But we can't change it like
you propose because the preceding call to `prune_refs()` may have
already set `retcode = 1`, and we must not clobber that value in case
the fetch succeeds. The effect is thus that we have `retcode == 1` if
either `prune_refs()` or `fetch_and_consume_refs()` fails.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]