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@xxxxxx> 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:
signature.asc
Description: PGP signature
