diff mbox series

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

Message ID 4059d5034bd9137ffca4929ed5bd8b7ce75ea09c.1644565025.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series fetch: improve atomicity of `--atomic` flag | expand

Commit Message

Patrick Steinhardt Feb. 11, 2022, 7:46 a.m. UTC 
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
wants to append to it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Christian Couder Feb. 15, 2022, 7:32 a.m. UTC | #1 
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;
Patrick Steinhardt Feb. 17, 2022, 11:18 a.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9c7e4f12cd..627847e2f8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1084,9 +1084,8 @@  N_("it took %.2f seconds to check forced updates; you can use\n"
 
 static int store_updated_refs(const char *raw_url, const char *remote_name,
 			      int connectivity_checked, struct ref *ref_map,
-			      struct worktree **worktrees)
+			      struct fetch_head *fetch_head, struct worktree **worktrees)
 {
-	struct fetch_head fetch_head;
 	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
 	struct ref_transaction *transaction = NULL;
@@ -1096,10 +1095,6 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 	int want_status;
 	int summary_width = transport_summary_width(ref_map);
 
-	rc = open_fetch_head(&fetch_head);
-	if (rc)
-		return -1;
-
 	if (raw_url)
 		url = transport_anonymize_url(raw_url);
 	else
@@ -1206,7 +1201,7 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 				strbuf_addf(&note, "'%s' of ", what);
 			}
 
-			append_fetch_head(&fetch_head, &rm->old_oid,
+			append_fetch_head(fetch_head, &rm->old_oid,
 					  rm->fetch_head_status,
 					  note.buf, url, url_len);
 
@@ -1246,9 +1241,6 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (!rc)
-		commit_fetch_head(&fetch_head);
-
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
 		      " 'git remote prune %s' to remove any old, conflicting "
@@ -1268,7 +1260,6 @@  static int store_updated_refs(const char *raw_url, const char *remote_name,
 	strbuf_release(&err);
 	ref_transaction_free(transaction);
 	free(url);
-	close_fetch_head(&fetch_head);
 	return rc;
 }
 
@@ -1309,6 +1300,7 @@  static int check_exist_and_connected(struct ref *ref_map)
 
 static int fetch_and_consume_refs(struct transport *transport,
 				  struct ref *ref_map,
+				  struct fetch_head *fetch_head,
 				  struct worktree **worktrees)
 {
 	int connectivity_checked = 1;
@@ -1331,7 +1323,7 @@  static int fetch_and_consume_refs(struct transport *transport,
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
 	ret = store_updated_refs(transport->url, transport->remote->name,
-				 connectivity_checked, ref_map, worktrees);
+				 connectivity_checked, ref_map, fetch_head, worktrees);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1503,7 +1495,9 @@  static struct transport *prepare_transport(struct remote *remote, int deepen)
 	return transport;
 }
 
-static void backfill_tags(struct transport *transport, struct ref *ref_map,
+static void backfill_tags(struct transport *transport,
+			  struct ref *ref_map,
+			  struct fetch_head *fetch_head,
 			  struct worktree **worktrees)
 {
 	int cannot_reuse;
@@ -1525,7 +1519,7 @@  static void backfill_tags(struct transport *transport, struct ref *ref_map,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	fetch_and_consume_refs(transport, ref_map, worktrees);
+	fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1544,6 +1538,7 @@  static int do_fetch(struct transport *transport,
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int must_list_refs = 1;
 	struct worktree **worktrees = get_worktrees();
+	struct fetch_head fetch_head = { 0 };
 
 	if (tags == TAGS_DEFAULT) {
 		if (transport->remote->fetch_tags == 2)
@@ -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;
 	}
@@ -1629,11 +1629,13 @@  static int do_fetch(struct transport *transport,
 
 		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
 		if (tags_ref_map)
-			backfill_tags(transport, tags_ref_map, worktrees);
+			backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
 
 		free_refs(tags_ref_map);
 	}
 
+	commit_fetch_head(&fetch_head);
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;
@@ -1689,6 +1691,7 @@  static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
+	close_fetch_head(&fetch_head);
 	free_refs(ref_map);
 	free_worktrees(worktrees);
 	return retcode;