| Message ID | 64c94e7a28d83fbaa1b6308b034d07c7be10e767.1644565025.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: improve atomicity of `--atomic` flag | expand |
On Fri, Feb 11, 2022 at 5:12 PM Patrick Steinhardt <ps@pks.im> wrote: > > 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/ > 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. > @@ -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... */ > + 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); > + } > @@ -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.
diff --git a/builtin/fetch.c b/builtin/fetch.c index 5b3b18a72f..9c7e4f12cd 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -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 ... */ + 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); + } + if (set_upstream) { struct branch *branch = branch_get("HEAD"); struct ref *rm; @@ -1642,7 +1653,7 @@ static int do_fetch(struct transport *transport, if (!rm->peer_ref) { if (source_ref) { warning(_("multiple branches detected, incompatible with --set-upstream")); - goto skip; + goto cleanup; } else { source_ref = rm; } @@ -1656,7 +1667,7 @@ static int do_fetch(struct transport *transport, warning(_("could not set upstream of HEAD to '%s' from '%s' when " "it does not point to any branch."), shortname, transport->remote->name); - goto skip; + goto cleanup; } if (!strcmp(source_ref->name, "HEAD") || @@ -1676,21 +1687,9 @@ static int do_fetch(struct transport *transport, "you need to specify exactly one branch with the --set-upstream option")); } } -skip: - free_refs(ref_map); - - /* if neither --no-tags nor --tags was specified, do automated tag - * following ... */ - if (tags == TAGS_DEFAULT && autotags) { - struct ref **tail = &ref_map; - ref_map = NULL; - find_non_local_tags(remote_refs, &ref_map, &tail); - if (ref_map) - backfill_tags(transport, ref_map, worktrees); - free_refs(ref_map); - } cleanup: + free_refs(ref_map); free_worktrees(worktrees); return retcode; }
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 a `skip` label which jumps from (3) to (4) in case we fail to update the 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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)