{"thread":{"id":"54954","subject":"[PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`","startedAt":"2021-01-07T13:52:53Z","lastAt":"2021-01-12T19:20:49Z","messageCount":41,"participants":["Patrick Steinhardt","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"413691","messageId":"e627e729e5cd14c94dcf819f4f87b1412b9a9e9b.1610027375.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610027375.git.ps@pks.im","subject":"[PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-07T13:51:39Z","receivedAt":"2021-01-07T13:52:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The handling of ref updates is completely handled by `s_update_ref()`,\nwhich will manage the complete lifecycle of the reference transaction.\nThis is fine right now given that git-fetch(1) does not support atomic\nfetches, so each reference gets its own transaction. It is quite\ninflexible though, as `s_update_ref()` only knows about a single\nreference update at a time, so it doesn't allow us to alter the\nstrategy.\n\nThis commit prepares `s_update_ref()` and its only caller\n`update_local_ref()` to allow passing an external transaction. If none\nis given, then the existing behaviour is triggered which creates a new\ntransaction and directly commits it. Otherwise, if the caller provides a\ntransaction, then we only queue the update but don't commit it. This\noptionally allows the caller to manage when a transaction will be\ncommitted.\n\nGiven that `update_local_ref()` is always called with a `NULL`\ntransaction for now, no change in behaviour is expected from this\nchange.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 48 +++++++++++++++++++++++++++++++-----------------\n 1 file changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ecf8537605..020a977bc7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -583,13 +583,14 @@ static struct ref *get_ref_map(struct remote *remote,\n \n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n+\t\t\tstruct ref_transaction *transaction,\n \t\t\tint check_old)\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n+\tstruct ref_transaction *transaction_to_free = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n-\tint ret, df_conflict = 0;\n+\tint ret, df_conflict = 0, commit = 0;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -597,26 +598,38 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\ttransaction = ref_transaction_begin(&err);\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name,\n+\t/*\n+\t * If no transaction was passed to us, we manage the transaction\n+\t * ourselves. Otherwise, we trust the caller to handle the transaction\n+\t * lifecycle.\n+\t */\n+\tif (!transaction) {\n+\t\ttransaction = transaction_to_free = ref_transaction_begin(&err);\n+\t\tif (!transaction)\n+\t\t\tgoto fail;\n+\t\tcommit = 1;\n+\t}\n+\n+\tif (ref_transaction_update(transaction, ref->name,\n \t\t\t\t &ref->new_oid,\n \t\t\t\t check_old ? &ref->old_oid : NULL,\n \t\t\t\t 0, msg, &err))\n \t\tgoto fail;\n \n-\tret = ref_transaction_commit(transaction, &err);\n-\tif (ret) {\n-\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n-\t\tgoto fail;\n+\tif (commit) {\n+\t\tret = ref_transaction_commit(transaction, &err);\n+\t\tif (ret) {\n+\t\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n+\t\t\tgoto fail;\n+\t\t}\n \t}\n \n-\tref_transaction_free(transaction);\n+\tref_transaction_free(transaction_to_free);\n \tstrbuf_release(&err);\n \tfree(msg);\n \treturn 0;\n fail:\n-\tref_transaction_free(transaction);\n+\tref_transaction_free(transaction_to_free);\n \terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tfree(msg);\n@@ -759,6 +772,7 @@ static void format_display(struct strbuf *display, char code,\n }\n \n static int update_local_ref(struct ref *ref,\n+\t\t\t struct ref_transaction *transaction,\n \t\t\t const char *remote,\n \t\t\t const struct ref *remote_ref,\n \t\t\t struct strbuf *display,\n@@ -799,7 +813,7 @@ static int update_local_ref(struct ref *ref,\n \t starts_with(ref->name, \"refs/tags/\")) {\n \t\tif (force || ref->force) {\n \t\t\tint r;\n-\t\t\tr = s_update_ref(\"updating tag\", ref, 0);\n+\t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n@@ -836,7 +850,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tr = s_update_ref(msg, ref, 0);\n+\t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -858,7 +872,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"fast-forward\", ref, 1);\n+\t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -870,7 +884,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"forced-update\", ref, 1);\n+\t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n \t\t\t remote, pretty_ref, summary_width);\n@@ -1034,8 +1048,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n-\t\t\t\t\t\t summary_width);\n+\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n-- \n2.30.0\n\n"},{"id":"413692","messageId":"cover.1610027375.git.ps@pks.im","threadId":"54954","inReplyTo":null,"subject":"[PATCH 0/2] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-07T13:51:35Z","receivedAt":"2021-01-07T13:52:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is a short patch series to implement support for atomic reference\nupdates for git-fetch(1). It's similar to `git push --atomic`, only that\nit applies to the local side. That is the fetch will either succeed and\nupdate all remote references or it will fail and update none.\n\nPatrick\n\nPatrick Steinhardt (2):\n fetch: allow passing a transaction to `s_update_ref()`\n fetch: implement support for atomic reference updates\n\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 72 ++++++++++++-----\n t/t5510-fetch.sh | 139 ++++++++++++++++++++++++++++++++\n 3 files changed, 197 insertions(+), 18 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"413693","messageId":"4807344e92bedbac37243434850da1f0787ad496.1610027375.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610027375.git.ps@pks.im","subject":"[PATCH 2/2] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-07T13:51:44Z","receivedAt":"2021-01-07T13:53:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When executing a fetch, then git will currently allocate one reference\ntransaction per reference update and directly commit it. This means that\nfetches are non-atomic: even if some of the reference updates fail,\nothers may still succeed and modify local references.\n\nThis is fine in many scenarios, but this strategy has its downsides.\n\n- The view of remote references may be inconsistent and may show a\n bastardized state of the remote repository.\n\n- Batching together updates may improve performance in certain\n scenarios. While the impact probably isn't as pronounced with loose\n references, the upcoming reftable backend may benefit as it needs to\n write less files in case the update is batched.\n\n- The reference-update hook is currently being executed twice per\n updated reference. While this doesn't matter when there is no such\n hook, we have seen severe performance regressions when doing a\n git-fetch(1) with reference-transaction hook when the remote\n repository has hundreds of thousands of references.\n\nSimilar to `git push --atomic`, this commit thus introduces atomic\nfetches. Instead of allocating one reference transaction per updated\nreference, it causes us to only allocate a single transaction and commit\nit as soon as all updates were received. If locking of any reference\nfails, then we abort the complete transaction and don't update any\nreference, which gives us an all-or-nothing fetch.\n\nNote that this may not completely fix the first of above downsides, as\nthe consistent view also depends on the server-side. If the server\ndoesn't have a consistent view of its own references during the\nreference negotiation phase, then the client would get the same\ninconsistent view the server has. This is a separate problem though and,\nif it actually exists, can be fixed at a later point.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 26 +++++-\n t/t5510-fetch.sh | 139 ++++++++++++++++++++++++++++++++\n 3 files changed, 167 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 2bf77b46fd..07783deee3 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -7,6 +7,10 @@\n \texisting contents of `.git/FETCH_HEAD`. Without this\n \toption old data in `.git/FETCH_HEAD` will be overwritten.\n \n+--atomic::\n+\tUse an atomic transaction to update local refs. Either all refs are\n+\tupdated, or on error, no refs are updated.\n+\n --depth=::\n \tLimit fetching to the specified number of commits from the tip of\n \teach remote branch history. If fetching to a 'shallow' repository\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 020a977bc7..5675ae4ec3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -63,6 +63,7 @@ static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n static int fetch_parallel_config = 1;\n+static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -144,6 +145,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"set upstream for git pull/fetch\")),\n \tOPT_BOOL('a', \"append\", &append,\n \t\t N_(\"append to .git/FETCH_HEAD instead of overwriting\")),\n+\tOPT_BOOL(0, \"atomic\", &atomic_fetch,\n+\t\t N_(\"use atomic transaction to update references\")),\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n@@ -926,7 +929,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tFILE *fp;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -955,6 +959,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (atomic_fetch) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tprepare_format_display(ref_map);\n \n \t/*\n@@ -1048,7 +1060,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1074,6 +1086,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc && transaction) {\n+\t\trc = ref_transaction_commit(transaction, &err);\n+\t\tif (rc) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n@@ -1090,6 +1110,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n+\tstrbuf_release(&err);\n+\tref_transaction_free(transaction);\n \tfree(url);\n \tfclose(fp);\n \treturn rc;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2013051a64..57da3ab2b3 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -176,6 +176,145 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success 'fetch --atomic works with a single branch' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch &&\n+\tgit rev-parse atomic-branch >expected &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse origin/atomic-branch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic works with multiple branches' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch-1 &&\n+\tgit branch atomic-branch-2 &&\n+\tgit branch atomic-branch-3 &&\n+\tgit rev-parse refs/heads/atomic-branch-1 refs/heads/atomic-branch-2 refs/heads/atomic-branch-3 >actual &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-branch-1 refs/remotes/origin/atomic-branch-2 refs/remotes/origin/atomic-branch-3 >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic works with mixed branches and tags' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-mixed-branch &&\n+\tgit tag atomic-mixed-tag &&\n+\tgit rev-parse refs/heads/atomic-mixed-branch refs/tags/atomic-mixed-tag >actual &&\n+\n+\tgit -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-mixed-branch refs/tags/atomic-mixed-tag >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic prunes references' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-prune-delete &&\n+\tgit clone . atomic &&\n+\tgit branch --delete atomic-prune-delete &&\n+\tgit branch atomic-prune-create &&\n+\tgit rev-parse refs/heads/atomic-prune-create >actual &&\n+\n+\tgit -C atomic fetch --prune --atomic origin &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-prune-delete &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-prune-create >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-non-ff &&\n+\tgit clone . atomic &&\n+\tgit rev-parse HEAD >actual &&\n+\n+\tgit branch atomic-new-branch &&\n+\tparent_commit=$(git rev-parse atomic-non-ff~) &&\n+\tgit update-ref refs/heads/atomic-non-ff $parent_commit &&\n+\n+\ttest_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic executes a single reference transaction only' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-1 &&\n+\tgit branch atomic-hooks-2 &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\t\tcommitted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\ttest_cmp expected atomic/actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-abort-1 &&\n+\tgit branch atomic-hooks-abort-2 &&\n+\tgit branch atomic-hooks-abort-3 &&\n+\tgit tag atomic-hooks-abort &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\t\taborted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\t\texit 1\n+\tEOF\n+\n+\tgit -C atomic for-each-ref >expected-refs &&\n+\ttest_must_fail git -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic for-each-ref >actual-refs &&\n+\ttest_cmp expected-refs actual-refs\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n-- \n2.30.0\n\n"},{"id":"413768","messageId":"xmqqtursjqva.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"e627e729e5cd14c94dcf819f4f87b1412b9a9e9b.1610027375.git.ps@pks.im","subject":"Re: [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-07T22:59:05Z","receivedAt":"2021-01-07T22:59:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> static int s_update_ref(const char *action,\n> \t\t\tstruct ref *ref,\n> +\t\t\tstruct ref_transaction *transaction,\n> \t\t\tint check_old)\n> {\n> \tchar *msg;\n> \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n> -\tstruct ref_transaction *transaction;\n> +\tstruct ref_transaction *transaction_to_free = NULL;\n> \tstruct strbuf err = STRBUF_INIT;\n> -\tint ret, df_conflict = 0;\n> +\tint ret, df_conflict = 0, commit = 0;\n\n> @@ -597,26 +598,38 @@ static int s_update_ref(const char *action,\n> \t\trla = default_rla.buf;\n> \tmsg = xstrfmt(\"%s: %s\", rla, action);\n> \n> -\ttransaction = ref_transaction_begin(&err);\n> -\tif (!transaction ||\n> -\t ref_transaction_update(transaction, ref->name,\n> +\t/*\n> +\t * If no transaction was passed to us, we manage the transaction\n> +\t * ourselves. Otherwise, we trust the caller to handle the transaction\n> +\t * lifecycle.\n> +\t */\n> +\tif (!transaction) {\n> +\t\ttransaction = transaction_to_free = ref_transaction_begin(&err);\n> +\t\tif (!transaction)\n> +\t\t\tgoto fail;\n> +\t\tcommit = 1;\n> +\t}\n\nThe idea is that we still allow the caller to pass NULL in which\ncase we begin and commit a transaction ourselves, but if the caller\nis to pass an already initialized transaction to us, we obviously\ndo not have to \"begin\" but also we won't commit.\n\nWhich makes sense, but then do we still need the \"commit\" variable\nthat reminds us \"we are responsible for finishing the transaction we\nstarted\"?\n\nIf we rename \"transaction_to_free\" to a name that makes it more\nclear that it is a transaction that we created and that we are\nresponsible for seeing through to its end (after all, \"to-free\"\ncaptures only one half of that, i.e. before returning, we are\nresponsible for calling ref_transation_free() on it), then we do not\nneed such an extra variable that can go out of sync by mistake, no?\nThe block that protects the call to ref_transaction_commit() can\njust check if the transaction_to_free variable (with a better name)\nis non-NULL, instead of looking at the commit variable.\n\nJust my attempt to come up with an alternative name for\ntransaction_to_free:\n\n - \"our_transaction\" because it is ours?\n\n - \"auto_transaction\" because it is automatically started and\n committed without bothering the caller?\n\n"},{"id":"413773","messageId":"xmqqeeiwjn52.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"4807344e92bedbac37243434850da1f0787ad496.1610027375.git.ps@pks.im","subject":"Re: [PATCH 2/2] fetch: implement support for atomic reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-08T00:19:37Z","receivedAt":"2021-01-08T00:20:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Similar to `git push --atomic`, this commit thus introduces atomic\n> fetches. Instead of allocating one reference transaction per updated\n> reference, it causes us to only allocate a single transaction and commit\n> it as soon as all updates were received. If locking of any reference\n> fails, then we abort the complete transaction and don't update any\n> reference, which gives us an all-or-nothing fetch.\n>\n> Note that this may not completely fix the first of above downsides, as\n> the consistent view also depends on the server-side. If the server\n> doesn't have a consistent view of its own references during the\n> reference negotiation phase, then the client would get the same\n> inconsistent view the server has. This is a separate problem though and,\n> if it actually exists, can be fixed at a later point.\n\nWith the help of the previous step, it is trivial to see that the\nsingle transaction around store_updated_refs() would be sufficient\nto achieve the atomicity.\n\nBut what about FETCH_HEAD? Do we refrain from writing and/or appending\nto it when there is any failure?\n\nThanks.\n"},{"id":"413775","messageId":"CAP8UFD1YxHGHLEHd_Bx1awSskjM6fHgM52nPWpTG2UHOmaOT9A@mail.gmail.com","threadId":"54954","inReplyTo":"xmqqtursjqva.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-08T00:45:47Z","receivedAt":"2021-01-08T00:46:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jan 8, 2021 at 12:04 AM Junio C Hamano wrote:\n>\n> Patrick Steinhardt writes:\n>\n> > static int s_update_ref(const char *action,\n> > struct ref *ref,\n> > + struct ref_transaction *transaction,\n> > int check_old)\n> > {\n> > char *msg;\n> > char *rla = getenv(\"GIT_REFLOG_ACTION\");\n> > - struct ref_transaction *transaction;\n> > + struct ref_transaction *transaction_to_free = NULL;\n> > struct strbuf err = STRBUF_INIT;\n> > - int ret, df_conflict = 0;\n> > + int ret, df_conflict = 0, commit = 0;\n>\n> > @@ -597,26 +598,38 @@ static int s_update_ref(const char *action,\n> > rla = default_rla.buf;\n> > msg = xstrfmt(\"%s: %s\", rla, action);\n> >\n> > - transaction = ref_transaction_begin(&err);\n> > - if (!transaction ||\n> > - ref_transaction_update(transaction, ref->name,\n> > + /*\n> > + * If no transaction was passed to us, we manage the transaction\n> > + * ourselves. Otherwise, we trust the caller to handle the transaction\n> > + * lifecycle.\n> > + */\n> > + if (!transaction) {\n> > + transaction = transaction_to_free = ref_transaction_begin(&err);\n> > + if (!transaction)\n> > + goto fail;\n> > + commit = 1;\n> > + }\n>\n> The idea is that we still allow the caller to pass NULL in which\n> case we begin and commit a transaction ourselves, but if the caller\n> is to pass an already initialized transaction to us, we obviously\n> do not have to \"begin\" but also we won't commit.\n>\n> Which makes sense, but then do we still need the \"commit\" variable\n> that reminds us \"we are responsible for finishing the transaction we\n> started\"?\n\nYeah, I think we also don't need the df_conflict variable, and I don't\nlike the duplication of the following clean up code:\n\n ref_transaction_free(transaction_to_free);\n strbuf_release(&err);\n free(msg);\n\nPatrick's patch didn't introduce them, but we might still want to get\nrid of them (see below).\n\n> If we rename \"transaction_to_free\" to a name that makes it more\n> clear that it is a transaction that we created and that we are\n> responsible for seeing through to its end (after all, \"to-free\"\n> captures only one half of that, i.e. before returning, we are\n> responsible for calling ref_transation_free() on it), then we do not\n> need such an extra variable that can go out of sync by mistake, no?\n> The block that protects the call to ref_transaction_commit() can\n> just check if the transaction_to_free variable (with a better name)\n> is non-NULL, instead of looking at the commit variable.\n>\n> Just my attempt to come up with an alternative name for\n> transaction_to_free:\n>\n> - \"our_transaction\" because it is ours?\n>\n> - \"auto_transaction\" because it is automatically started and\n> committed without bothering the caller?\n\n\"our_transaction\" looks fine.\n\nHere is a suggested refactoring patch that could come before Patrick's patch:\n\n-------------------------------------------------------------------------\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ecf8537605..8017fc19da 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -581,6 +581,22 @@ static struct ref *get_ref_map(struct remote *remote,\n#define STORE_REF_ERROR_OTHER 1\n#define STORE_REF_ERROR_DF_CONFLICT 2\n\n+static int do_s_update_ref(struct ref_transaction *transaction,\n+ struct ref *ref,\n+ int check_old,\n+ struct strbuf *err,\n+ char *msg)\n+{\n+ if (!transaction ||\n+ ref_transaction_update(transaction, ref->name,\n+ &ref->new_oid,\n+ check_old ? &ref->old_oid : NULL,\n+ 0, msg, err))\n+ return TRANSACTION_GENERIC_ERROR;\n+\n+ return ref_transaction_commit(transaction, err);\n+}\n+\nstatic int s_update_ref(const char *action,\n struct ref *ref,\n int check_old)\n@@ -589,7 +605,7 @@ static int s_update_ref(const char *action,\n char *rla = getenv(\"GIT_REFLOG_ACTION\");\n struct ref_transaction *transaction;\n struct strbuf err = STRBUF_INIT;\n- int ret, df_conflict = 0;\n+ int ret = 0;\n\n if (dry_run)\n return 0;\n@@ -598,30 +614,19 @@ static int s_update_ref(const char *action,\n msg = xstrfmt(\"%s: %s\", rla, action);\n\n transaction = ref_transaction_begin(&err);\n- if (!transaction ||\n- ref_transaction_update(transaction, ref->name,\n- &ref->new_oid,\n- check_old ? &ref->old_oid : NULL,\n- 0, msg, &err))\n- goto fail;\n\n- ret = ref_transaction_commit(transaction, &err);\n+ ret = do_s_update_ref(transaction, ref, check_old, &err, msg);\n if (ret) {\n- df_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n- goto fail;\n+ error(\"%s\", err.buf);\n+ ret = (ret == TRANSACTION_NAME_CONFLICT)\n+ ? STORE_REF_ERROR_DF_CONFLICT\n+ : STORE_REF_ERROR_OTHER;\n }\n\n ref_transaction_free(transaction);\n strbuf_release(&err);\n free(msg);\n- return 0;\n-fail:\n- ref_transaction_free(transaction);\n- error(\"%s\", err.buf);\n- strbuf_release(&err);\n- free(msg);\n- return df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n- : STORE_REF_ERROR_OTHER;\n+ return ret;\n}\n\nstatic int refcol_width = 10;\n-------------------------------------------------------------------------\n\nAfter the above patch, Patrick's patch would become:\n\n-------------------------------------------------------------------------\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8017fc19da..d58805c03d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -584,6 +584,7 @@ static struct ref *get_ref_map(struct remote *remote,\nstatic int do_s_update_ref(struct ref_transaction *transaction,\n struct ref *ref,\n int check_old,\n+ int commit,\n struct strbuf *err,\n char *msg)\n{\n@@ -594,16 +595,17 @@ static int do_s_update_ref(struct\nref_transaction *transaction,\n 0, msg, err))\n return TRANSACTION_GENERIC_ERROR;\n\n- return ref_transaction_commit(transaction, err);\n+ return (commit) ? ref_transaction_commit(transaction, err) : 0;\n}\n\nstatic int s_update_ref(const char *action,\n+ struct ref_transaction *transaction,\n struct ref *ref,\n int check_old)\n{\n char *msg;\n char *rla = getenv(\"GIT_REFLOG_ACTION\");\n- struct ref_transaction *transaction;\n+ struct ref_transaction *our_transaction = NULL;\n struct strbuf err = STRBUF_INIT;\n int ret = 0;\n\n@@ -613,9 +615,16 @@ static int s_update_ref(const char *action,\n rla = default_rla.buf;\n msg = xstrfmt(\"%s: %s\", rla, action);\n\n- transaction = ref_transaction_begin(&err);\n+ /*\n+ * If no transaction was passed to us, we manage the\n+ * transaction ourselves. Otherwise, we trust the caller to\n+ * handle the transaction lifecycle.\n+ */\n+ if (!transaction)\n+ transaction = our_transaction = ref_transaction_begin(&err);\n\n- ret = do_s_update_ref(transaction, ref, check_old, &err, msg);\n+ ret = do_s_update_ref(transaction, ref, check_old, !!our_transaction,\n+ &err, msg);\n if (ret) {\n error(\"%s\", err.buf);\n ret = (ret == TRANSACTION_NAME_CONFLICT)\n@@ -623,7 +632,7 @@ static int s_update_ref(const char *action,\n : STORE_REF_ERROR_OTHER;\n }\n\n- ref_transaction_free(transaction);\n+ ref_transaction_free(our_transaction);\n strbuf_release(&err);\n free(msg);\n return ret;\n...\n-------------------------------------------------------------------------\n\nYou can find these patches, with Patrick as the author, there:\n\nhttps://gitlab.com/gitlab-org/gitlab-git/-/commits/cc-improve-s-update-ref/\n"},{"id":"413785","messageId":"xmqqsg7bj3qs.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"CAP8UFD1YxHGHLEHd_Bx1awSskjM6fHgM52nPWpTG2UHOmaOT9A@mail.gmail.com","subject":"Re: [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-08T07:18:35Z","receivedAt":"2021-01-08T07:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Yeah, I think we also don't need the df_conflict variable, and I don't\n> like the duplication of the following clean up code:\n>\n> ref_transaction_free(transaction_to_free);\n> strbuf_release(&err);\n> free(msg);\n>\n> Patrick's patch didn't introduce them, but we might still want to get\n> rid of them (see below).\n\n\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ecf8537605..8017fc19da 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -581,6 +581,22 @@ static struct ref *get_ref_map(struct remote *remote,\n> #define STORE_REF_ERROR_OTHER 1\n> #define STORE_REF_ERROR_DF_CONFLICT 2\n>\n> +static int do_s_update_ref(struct ref_transaction *transaction,\n> ...\n> }\n>\n> static int refcol_width = 10;\n> -------------------------------------------------------------------------\n>\n> After the above patch, Patrick's patch would become:\n\nOK, I think the above as a preliminary clean-up would not hurt, but\nthe \"if the caller gave us NULL as the transaction, we are\nresponsible for handling that bogosity\" bit feels a bit too magical\nto my taste. I do understand that your intention is that it\nreleaves the caller ...\n\n> @@ -613,9 +615,16 @@ static int s_update_ref(const char *action,\n> rla = default_rla.buf;\n> msg = xstrfmt(\"%s: %s\", rla, action);\n>\n> - transaction = ref_transaction_begin(&err);\n> + /*\n> + * If no transaction was passed to us, we manage the\n> + * transaction ourselves. Otherwise, we trust the caller to\n> + * handle the transaction lifecycle.\n> + */\n> + if (!transaction)\n> + transaction = our_transaction = ref_transaction_begin(&err);\n\n... here from having to deal with NULL from ref_transaction_begin(),\nbut I somehow do not see it as a code structure with a good taste.\n\nI dunno.\n\nWithout that \"if (!transaction ||\" bit, I think your do_s_update_ref()\nis a good clean-up, though.\n\nThanks.\n\n> - ret = do_s_update_ref(transaction, ref, check_old, &err, msg);\n> + ret = do_s_update_ref(transaction, ref, check_old, !!our_transaction,\n> + &err, msg);\n> if (ret) {\n> error(\"%s\", err.buf);\n> ret = (ret == TRANSACTION_NAME_CONFLICT)\n> @@ -623,7 +632,7 @@ static int s_update_ref(const char *action,\n> : STORE_REF_ERROR_OTHER;\n> }\n>\n> - ref_transaction_free(transaction);\n> + ref_transaction_free(our_transaction);\n> strbuf_release(&err);\n> free(msg);\n> return ret;\n> ...\n> -------------------------------------------------------------------------\n>\n> You can find these patches, with Patrick as the author, there:\n>\n> https://gitlab.com/gitlab-org/gitlab-git/-/commits/cc-improve-s-update-ref/\n"},{"id":"413809","messageId":"d80dbc5a9c9520621651541e418ee5216d164053.1610107599.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610107599.git.ps@pks.im","subject":"[PATCH v2 1/4] fetch: extract writing to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-08T12:11:14Z","receivedAt":"2021-01-08T12:12:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing a fetch with the default `--write-fetch-head` option, we\nwrite all updated references to FETCH_HEAD while the updates are\nperformed. Given that updates are not performed atomically, it means\nthat we we write to FETCH_HEAD even if some or all of the reference\nupdates fail.\n\nGiven that we simply update FETCH_HEAD ad-hoc with each reference, the\nlogic is completely contained in `store_update_refs` and thus quite hard\nto extend. This can already be seen by the way we skip writing to the\nFETCH_HEAD: instead of having a conditional which simply skips writing,\nwe instead open \"/dev/null\" and needlessly write all updates there.\n\nWe are about to extend git-fetch(1) to accept an `--atomic` flag which\nwill make the fetch an all-or-nothing operation with regards to the\nreference updates. This will also require us to make the updates to\nFETCH_HEAD an all-or-nothing operation, but as explained doing so is not\neasy with the current layout. This commit thus refactors the wa we write\nto FETCH_HEAD and pulls out the logic to open, append to, commit and\nclose the file. While this may seem rather over-the top at first,\npulling out this logic will make it a lot easier to update the code in a\nsubsequent commit. It also allows us to easily skip writing completely\nin case `--no-write-fetch-head` was passed.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 80 ++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 62 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ecf8537605..557ec130db 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -897,6 +897,56 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \treturn 0;\n }\n \n+struct fetch_head {\n+\tFILE *fp;\n+};\n+\n+static int open_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tconst char *filename = git_path_fetch_head(the_repository);\n+\n+\tif (!write_fetch_head)\n+\t\treturn 0;\n+\n+\tfetch_head->fp = fopen(filename, \"a\");\n+\tif (!fetch_head->fp)\n+\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\n+\treturn 0;\n+}\n+\n+static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid,\n+\t\t\t const char *merge_status_marker, const char *note,\n+\t\t\t const char *url, size_t url_len)\n+{\n+\tsize_t i;\n+\n+\tif (!write_fetch_head)\n+\t\treturn;\n+\n+\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n+\t\told_oid, merge_status_marker, note);\n+\tfor (i = 0; i < url_len; ++i)\n+\t\tif ('\\n' == url[i])\n+\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\telse\n+\t\t\tfputc(url[i], fetch_head->fp);\n+\tfputc('\\n', fetch_head->fp);\n+}\n+\n+static void commit_fetch_head(struct fetch_head *fetch_head)\n+{\n+\t/* Nothing to commit yet. */\n+}\n+\n+static void close_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tif (!write_fetch_head)\n+\t\treturn;\n+\n+\tfclose(fetch_head->fp);\n+}\n+\n static const char warn_show_forced_updates[] =\n N_(\"Fetch normally indicates which branches had a forced update,\\n\"\n \"but that check has been disabled. To re-enable, use '--show-forced-updates'\\n\"\n@@ -909,22 +959,19 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked, struct ref *ref_map)\n {\n-\tFILE *fp;\n+\tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n-\tconst char *filename = (!write_fetch_head\n-\t\t\t\t? \"/dev/null\"\n-\t\t\t\t: git_path_fetch_head(the_repository));\n \tint want_status;\n \tint summary_width = transport_summary_width(ref_map);\n \n-\tfp = fopen(filename, \"a\");\n-\tif (!fp)\n-\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\trc = open_fetch_head(&fetch_head);\n+\tif (rc)\n+\t\treturn -1;\n \n \tif (raw_url)\n \t\turl = transport_anonymize_url(raw_url);\n@@ -1016,16 +1063,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tmerge_status_marker = \"not-for-merge\";\n \t\t\t\t/* fall-through */\n \t\t\tcase FETCH_HEAD_MERGE:\n-\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n-\t\t\t\t\toid_to_hex(&rm->old_oid),\n-\t\t\t\t\tmerge_status_marker,\n-\t\t\t\t\tnote.buf);\n-\t\t\t\tfor (i = 0; i < url_len; ++i)\n-\t\t\t\t\tif ('\\n' == url[i])\n-\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n-\t\t\t\t\telse\n-\t\t\t\t\t\tfputc(url[i], fp);\n-\t\t\t\tfputc('\\n', fp);\n+\t\t\t\tappend_fetch_head(&fetch_head,\n+\t\t\t\t\t\t oid_to_hex(&rm->old_oid),\n+\t\t\t\t\t\t merge_status_marker,\n+\t\t\t\t\t\t note.buf, url, url_len);\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\t/* do not write anything to FETCH_HEAD */\n@@ -1060,6 +1101,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc)\n+\t\tcommit_fetch_head(&fetch_head);\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n@@ -1077,7 +1121,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n abort:\n \tstrbuf_release(¬e);\n \tfree(url);\n-\tfclose(fp);\n+\tclose_fetch_head(&fetch_head);\n \treturn rc;\n }\n \n-- \n2.30.0\n\n"},{"id":"413810","messageId":"4162d10fcbb5729dffaaec0651345dd9522a8e05.1610107599.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610107599.git.ps@pks.im","subject":"[PATCH v2 3/4] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-08T12:11:23Z","receivedAt":"2021-01-08T12:12:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The handling of ref updates is completely handled by `s_update_ref()`,\nwhich will manage the complete lifecycle of the reference transaction.\nThis is fine right now given that git-fetch(1) does not support atomic\nfetches, so each reference gets its own transaction. It is quite\ninflexible though, as `s_update_ref()` only knows about a single\nreference update at a time, so it doesn't allow us to alter the\nstrategy.\n\nThis commit prepares `s_update_ref()` and its only caller\n`update_local_ref()` to allow passing an external transaction. If none\nis given, then the existing behaviour is triggered which creates a new\ntransaction and directly commits it. Otherwise, if the caller provides a\ntransaction, then we only queue the update but don't commit it. This\noptionally allows the caller to manage when a transaction will be\ncommitted.\n\nGiven that `update_local_ref()` is always called with a `NULL`\ntransaction for now, no change in behaviour is expected from this\nchange.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 43 +++++++++++++++++++++++++++----------------\n 1 file changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5221a9dbed..654e0bb520 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -583,11 +583,12 @@ static struct ref *get_ref_map(struct remote *remote,\n \n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n+\t\t\tstruct ref_transaction *transaction,\n \t\t\tint check_old)\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n+\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -597,10 +598,17 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\t/*\n+\t * If no transaction was passed to us, we manage the transaction\n+\t * ourselves. Otherwise, we trust the caller to handle the transaction\n+\t * lifecycle.\n+\t */\n \tif (!transaction) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\tret = STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n@@ -611,15 +619,17 @@ static int s_update_ref(const char *action,\n \t\tgoto out;\n \t}\n \n-\tret = ref_transaction_commit(transaction, &err);\n-\tif (ret) {\n-\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n-\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\tif (our_transaction) {\n+\t\tret = ref_transaction_commit(our_transaction, &err);\n+\t\tif (ret) {\n+\t\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n+\t\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n out:\n-\tref_transaction_free(transaction);\n+\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n@@ -762,6 +772,7 @@ static void format_display(struct strbuf *display, char code,\n }\n \n static int update_local_ref(struct ref *ref,\n+\t\t\t struct ref_transaction *transaction,\n \t\t\t const char *remote,\n \t\t\t const struct ref *remote_ref,\n \t\t\t struct strbuf *display,\n@@ -802,7 +813,7 @@ static int update_local_ref(struct ref *ref,\n \t starts_with(ref->name, \"refs/tags/\")) {\n \t\tif (force || ref->force) {\n \t\t\tint r;\n-\t\t\tr = s_update_ref(\"updating tag\", ref, 0);\n+\t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n@@ -839,7 +850,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tr = s_update_ref(msg, ref, 0);\n+\t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -861,7 +872,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"fast-forward\", ref, 1);\n+\t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -873,7 +884,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"forced-update\", ref, 1);\n+\t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n \t\t\t remote, pretty_ref, summary_width);\n@@ -1078,8 +1089,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n-\t\t\t\t\t\t summary_width);\n+\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n-- \n2.30.0\n\n"},{"id":"413811","messageId":"718a8bf5d7a0ed92c3004991a42419279ff38253.1610107599.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610107599.git.ps@pks.im","subject":"[PATCH v2 2/4] fetch: refactor `s_update_ref` to use common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-08T12:11:19Z","receivedAt":"2021-01-08T12:12:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The cleanup code in `s_update_ref()` is currently duplicated for both\nsuccesful and erroneous exit paths. This commit refactors the function\nto have a shared exit path for both cases to remove the duplication.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 37 ++++++++++++++++++++-----------------\n 1 file changed, 20 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 557ec130db..5221a9dbed 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -589,7 +589,7 @@ static int s_update_ref(const char *action,\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n-\tint ret, df_conflict = 0;\n+\tint ret;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -598,30 +598,33 @@ static int s_update_ref(const char *action,\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n \ttransaction = ref_transaction_begin(&err);\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name,\n-\t\t\t\t &ref->new_oid,\n-\t\t\t\t check_old ? &ref->old_oid : NULL,\n-\t\t\t\t 0, msg, &err))\n-\t\tgoto fail;\n+\tif (!transaction) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n+\n+\tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n+\t\t\t\t check_old ? &ref->old_oid : NULL,\n+\t\t\t\t 0, msg, &err);\n+\tif (ret) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n \n \tret = ref_transaction_commit(transaction, &err);\n \tif (ret) {\n-\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n-\t\tgoto fail;\n+\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n+\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n \t}\n \n+out:\n \tref_transaction_free(transaction);\n+\tif (ret)\n+\t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tfree(msg);\n-\treturn 0;\n-fail:\n-\tref_transaction_free(transaction);\n-\terror(\"%s\", err.buf);\n-\tstrbuf_release(&err);\n-\tfree(msg);\n-\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n-\t\t\t : STORE_REF_ERROR_OTHER;\n+\treturn ret;\n }\n \n static int refcol_width = 10;\n-- \n2.30.0\n\n"},{"id":"413813","messageId":"53705281b60285837905137f45fc8607012d2f19.1610107599.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610107599.git.ps@pks.im","subject":"[PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-08T12:11:28Z","receivedAt":"2021-01-08T12:12:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When executing a fetch, then git will currently allocate one reference\ntransaction per reference update and directly commit it. This means that\nfetches are non-atomic: even if some of the reference updates fail,\nothers may still succeed and modify local references.\n\nThis is fine in many scenarios, but this strategy has its downsides.\n\n- The view of remote references may be inconsistent and may show a\n bastardized state of the remote repository.\n\n- Batching together updates may improve performance in certain\n scenarios. While the impact probably isn't as pronounced with loose\n references, the upcoming reftable backend may benefit as it needs to\n write less files in case the update is batched.\n\n- The reference-update hook is currently being executed twice per\n updated reference. While this doesn't matter when there is no such\n hook, we have seen severe performance regressions when doing a\n git-fetch(1) with reference-transaction hook when the remote\n repository has hundreds of thousands of references.\n\nSimilar to `git push --atomic`, this commit thus introduces atomic\nfetches. Instead of allocating one reference transaction per updated\nreference, it causes us to only allocate a single transaction and commit\nit as soon as all updates were received. If locking of any reference\nfails, then we abort the complete transaction and don't update any\nreference, which gives us an all-or-nothing fetch.\n\nNote that this may not completely fix the first of above downsides, as\nthe consistent view also depends on the server-side. If the server\ndoesn't have a consistent view of its own references during the\nreference negotiation phase, then the client would get the same\ninconsistent view the server has. This is a separate problem though and,\nif it actually exists, can be fixed at a later point.\n\nThis commit also changes the way we write FETCH_HEAD in case `--atomic`\nis passed. Instead of writing changes as we go, we need to accumulate\nall changes first and only commit them at the end when we know that all\nreference updates succeeded. Ideally, we'd just do so via a temporary\nfile so that we don't need to carry all updates in-memory. This isn't\ntrivially doable though considering the `--append` mode, where we do not\ntruncate the file but simply append to it. And given that we support\nconcurrent processes appending to FETCH_HEAD at the same time without\nany loss of data, seeding the temporary file with current contents of\nFETCH_HEAD initially and then doing a rename wouldn't work either. So\nthis commit implements the simple strategy of buffering all changes and\nappending them to the file on commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 66 ++++++++++---\n t/t5510-fetch.sh | 168 ++++++++++++++++++++++++++++++++\n 3 files changed, 227 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 2bf77b46fd..07783deee3 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -7,6 +7,10 @@\n \texisting contents of `.git/FETCH_HEAD`. Without this\n \toption old data in `.git/FETCH_HEAD` will be overwritten.\n \n+--atomic::\n+\tUse an atomic transaction to update local refs. Either all refs are\n+\tupdated, or on error, no refs are updated.\n+\n --depth=::\n \tLimit fetching to the specified number of commits from the tip of\n \teach remote branch history. If fetching to a 'shallow' repository\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 654e0bb520..e3446fc493 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -63,6 +63,7 @@ static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n static int fetch_parallel_config = 1;\n+static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -144,6 +145,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"set upstream for git pull/fetch\")),\n \tOPT_BOOL('a', \"append\", &append,\n \t\t N_(\"append to .git/FETCH_HEAD instead of overwriting\")),\n+\tOPT_BOOL(0, \"atomic\", &atomic_fetch,\n+\t\t N_(\"use atomic transaction to update references\")),\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n@@ -913,6 +916,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \n struct fetch_head {\n \tFILE *fp;\n+\tstruct strbuf buf;\n };\n \n static int open_fetch_head(struct fetch_head *fetch_head)\n@@ -922,6 +926,7 @@ static int open_fetch_head(struct fetch_head *fetch_head)\n \tif (!write_fetch_head)\n \t\treturn 0;\n \n+\tstrbuf_init(&fetch_head->buf, 0);\n \tfetch_head->fp = fopen(filename, \"a\");\n \tif (!fetch_head->fp)\n \t\treturn error_errno(_(\"cannot open %s\"), filename);\n@@ -938,19 +943,34 @@ static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid\n \tif (!write_fetch_head)\n \t\treturn;\n \n-\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n-\t\told_oid, merge_status_marker, note);\n-\tfor (i = 0; i < url_len; ++i)\n-\t\tif ('\\n' == url[i])\n-\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n-\t\telse\n-\t\t\tfputc(url[i], fetch_head->fp);\n-\tfputc('\\n', fetch_head->fp);\n+\t/*\n+\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n+\t * any of the reference updates fails. We thus have to write all\n+\t * updates to a buffer first and only commit it as soon as all\n+\t * references have been successfully updated.\n+\t */\n+\tif (atomic_fetch) {\n+\t\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n+\t\t\t old_oid, merge_status_marker, note);\n+\t\tstrbuf_add(&fetch_head->buf, url, url_len);\n+\t\tstrbuf_addch(&fetch_head->buf, '\\n');\n+\t} else {\n+\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n+\t\t\told_oid, merge_status_marker, note);\n+\t\tfor (i = 0; i < url_len; ++i)\n+\t\t\tif ('\\n' == url[i])\n+\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\t\telse\n+\t\t\t\tfputc(url[i], fetch_head->fp);\n+\t\tfputc('\\n', fetch_head->fp);\n+\t}\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n {\n-\t/* Nothing to commit yet. */\n+\tif (!write_fetch_head || !atomic_fetch)\n+\t\treturn;\n+\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n }\n \n static void close_fetch_head(struct fetch_head *fetch_head)\n@@ -959,6 +979,7 @@ static void close_fetch_head(struct fetch_head *fetch_head)\n \t\treturn;\n \n \tfclose(fetch_head->fp);\n+\tstrbuf_release(&fetch_head->buf);\n }\n \n static const char warn_show_forced_updates[] =\n@@ -976,7 +997,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1002,6 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (atomic_fetch) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tprepare_format_display(ref_map);\n \n \t/*\n@@ -1089,7 +1119,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1115,6 +1145,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc && transaction) {\n+\t\trc = ref_transaction_commit(transaction, &err);\n+\t\tif (rc) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tif (!rc)\n \t\tcommit_fetch_head(&fetch_head);\n \n@@ -1134,6 +1172,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n+\tstrbuf_release(&err);\n+\tref_transaction_free(transaction);\n \tfree(url);\n \tclose_fetch_head(&fetch_head);\n \treturn rc;\n@@ -1945,6 +1985,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (atomic_fetch)\n+\t\t\tdie(_(\"--atomic can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (stdin_refspecs)\n \t\t\tdie(_(\"--stdin can only be used when fetching \"\n \t\t\t \"from one remote\"));\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2013051a64..109d15be98 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -176,6 +176,174 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success 'fetch --atomic works with a single branch' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch &&\n+\toid=$(git rev-parse atomic-branch) &&\n+\techo \"$oid\" >expected &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse origin/atomic-branch >actual &&\n+\ttest_cmp expected actual &&\n+\ttest $oid = \"$(git -C atomic rev-parse --verify FETCH_HEAD)\"\n+'\n+\n+test_expect_success 'fetch --atomic works with multiple branches' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch-1 &&\n+\tgit branch atomic-branch-2 &&\n+\tgit branch atomic-branch-3 &&\n+\tgit rev-parse refs/heads/atomic-branch-1 refs/heads/atomic-branch-2 refs/heads/atomic-branch-3 >actual &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-branch-1 refs/remotes/origin/atomic-branch-2 refs/remotes/origin/atomic-branch-3 >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic works with mixed branches and tags' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-mixed-branch &&\n+\tgit tag atomic-mixed-tag &&\n+\tgit rev-parse refs/heads/atomic-mixed-branch refs/tags/atomic-mixed-tag >actual &&\n+\n+\tgit -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-mixed-branch refs/tags/atomic-mixed-tag >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic prunes references' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-prune-delete &&\n+\tgit clone . atomic &&\n+\tgit branch --delete atomic-prune-delete &&\n+\tgit branch atomic-prune-create &&\n+\tgit rev-parse refs/heads/atomic-prune-create >actual &&\n+\n+\tgit -C atomic fetch --prune --atomic origin &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-prune-delete &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-prune-create >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-non-ff &&\n+\tgit clone . atomic &&\n+\tgit rev-parse HEAD >actual &&\n+\n+\tgit branch atomic-new-branch &&\n+\tparent_commit=$(git rev-parse atomic-non-ff~) &&\n+\tgit update-ref refs/heads/atomic-non-ff $parent_commit &&\n+\n+\ttest_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic executes a single reference transaction only' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-1 &&\n+\tgit branch atomic-hooks-2 &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\t\tcommitted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\ttest_cmp expected atomic/actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-abort-1 &&\n+\tgit branch atomic-hooks-abort-2 &&\n+\tgit branch atomic-hooks-abort-3 &&\n+\tgit tag atomic-hooks-abort &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\t\taborted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\t\texit 1\n+\tEOF\n+\n+\tgit -C atomic for-each-ref >expected-refs &&\n+\ttest_must_fail git -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic for-each-ref >actual-refs &&\n+\ttest_cmp expected-refs actual-refs &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\toid=$(git rev-parse HEAD) &&\n+\n+\tgit branch atomic-fetch-head-1 &&\n+\tgit -C atomic fetch --atomic origin atomic-fetch-head-1 &&\n+\ttest_line_count = 1 atomic/.git/FETCH_HEAD &&\n+\n+\tgit branch atomic-fetch-head-2 &&\n+\tgit -C atomic fetch --atomic --append origin atomic-fetch-head-2 &&\n+\ttest_line_count = 2 atomic/.git/FETCH_HEAD &&\n+\tcp atomic/.git/FETCH_HEAD expected &&\n+\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\texit 1\n+\tEOF\n+\n+\tgit branch atomic-fetch-head-3 &&\n+\ttest_must_fail git -C atomic fetch --atomic --append origin atomic-fetch-head-3 &&\n+\ttest_cmp expected atomic/.git/FETCH_HEAD\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n-- \n2.30.0\n\n"},{"id":"413812","messageId":"cover.1610107599.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610027375.git.ps@pks.im","subject":"[PATCH v2 0/4] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-08T12:11:10Z","receivedAt":"2021-01-08T12:12:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series to implement support for\natomic reference updates for git-fetch(1). It's similar to `git push\n--atomic`, only that it applies to the local side. That is the fetch\nwill either succeed and update all remote references or it will fail and\nupdate none.\n\nChanges compared to v1:\n\n - In v1, we still wrote to FETCH_HEAD even if the fetch failed. I've\n fixed this now by pulling out logic to write to FETCH_HEAD in 1/4\n so that we can now easily buffer updates and commit them only if\n the reference transaction succeeds. There's some additional tests\n in 4/4 to test it works as expected.\n\n - As suggested by Christian, I've unified the exit path in\n `s_update_ref()` in 2/4.\n\n - I've dropped the `commit` variable and renamed\n `transaction_to_free` to `our_transaction` as suggested by Junio.\n\nPatrick\n\nPatrick Steinhardt (4):\n fetch: extract writing to FETCH_HEAD\n fetch: refactor `s_update_ref` to use common exit path\n fetch: allow passing a transaction to `s_update_ref()`\n fetch: implement support for atomic reference updates\n\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 198 ++++++++++++++++++++++++--------\n t/t5510-fetch.sh | 168 +++++++++++++++++++++++++++\n 3 files changed, 322 insertions(+), 48 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: d80dbc5a9c fetch: extract writing to FETCH_HEAD\n-: ---------- > 2: 718a8bf5d7 fetch: refactor `s_update_ref` to use common exit path\n1: e627e729e5 ! 3: 4162d10fcb fetch: allow passing a transaction to `s_update_ref()`\n @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n -\tstruct ref_transaction *transaction;\n -+\tstruct ref_transaction *transaction_to_free = NULL;\n ++\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n --\tint ret, df_conflict = 0;\n -+\tint ret, df_conflict = 0, commit = 0;\n + \tint ret;\n \n - \tif (dry_run)\n - \t\treturn 0;\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n -\ttransaction = ref_transaction_begin(&err);\n --\tif (!transaction ||\n --\t ref_transaction_update(transaction, ref->name,\n +\t/*\n +\t * If no transaction was passed to us, we manage the transaction\n +\t * ourselves. Otherwise, we trust the caller to handle the transaction\n +\t * lifecycle.\n +\t */\n -+\tif (!transaction) {\n -+\t\ttransaction = transaction_to_free = ref_transaction_begin(&err);\n -+\t\tif (!transaction)\n -+\t\t\tgoto fail;\n -+\t\tcommit = 1;\n -+\t}\n -+\n -+\tif (ref_transaction_update(transaction, ref->name,\n - \t\t\t\t &ref->new_oid,\n - \t\t\t\t check_old ? &ref->old_oid : NULL,\n - \t\t\t\t 0, msg, &err))\n - \t\tgoto fail;\n - \n --\tret = ref_transaction_commit(transaction, &err);\n --\tif (ret) {\n --\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n --\t\tgoto fail;\n -+\tif (commit) {\n -+\t\tret = ref_transaction_commit(transaction, &err);\n -+\t\tif (ret) {\n -+\t\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n -+\t\t\tgoto fail;\n + \tif (!transaction) {\n +-\t\tret = STORE_REF_ERROR_OTHER;\n +-\t\tgoto out;\n ++\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n ++\t\tif (!transaction) {\n ++\t\t\tret = STORE_REF_ERROR_OTHER;\n ++\t\t\tgoto out;\n +\t\t}\n \t}\n \n + \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n +@@ builtin/fetch.c: static int s_update_ref(const char *action,\n + \t\tgoto out;\n + \t}\n + \n +-\tret = ref_transaction_commit(transaction, &err);\n +-\tif (ret) {\n +-\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n +-\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n +-\t\tgoto out;\n ++\tif (our_transaction) {\n ++\t\tret = ref_transaction_commit(our_transaction, &err);\n ++\t\tif (ret) {\n ++\t\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n ++\t\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n ++\t\t\tgoto out;\n ++\t\t}\n + \t}\n + \n + out:\n -\tref_transaction_free(transaction);\n -+\tref_transaction_free(transaction_to_free);\n ++\tref_transaction_free(our_transaction);\n + \tif (ret)\n + \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n - \tfree(msg);\n - \treturn 0;\n - fail:\n --\tref_transaction_free(transaction);\n -+\tref_transaction_free(transaction_to_free);\n - \terror(\"%s\", err.buf);\n - \tstrbuf_release(&err);\n - \tfree(msg);\n @@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,\n }\n \n2: 4807344e92 ! 4: 53705281b6 fetch: implement support for atomic reference updates\n @@ Commit message\n inconsistent view the server has. This is a separate problem though and,\n if it actually exists, can be fixed at a later point.\n \n + This commit also changes the way we write FETCH_HEAD in case `--atomic`\n + is passed. Instead of writing changes as we go, we need to accumulate\n + all changes first and only commit them at the end when we know that all\n + reference updates succeeded. Ideally, we'd just do so via a temporary\n + file so that we don't need to carry all updates in-memory. This isn't\n + trivially doable though considering the `--append` mode, where we do not\n + truncate the file but simply append to it. And given that we support\n + concurrent processes appending to FETCH_HEAD at the same time without\n + any loss of data, seeding the temporary file with current contents of\n + FETCH_HEAD initially and then doing a rename wouldn't work either. So\n + this commit implements the simple strategy of buffering all changes and\n + appending them to the file on commit.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## Documentation/fetch-options.txt ##\n @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n -@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n +@@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid)\n + \n + struct fetch_head {\n \tFILE *fp;\n ++\tstruct strbuf buf;\n + };\n + \n + static int open_fetch_head(struct fetch_head *fetch_head)\n +@@ builtin/fetch.c: static int open_fetch_head(struct fetch_head *fetch_head)\n + \tif (!write_fetch_head)\n + \t\treturn 0;\n + \n ++\tstrbuf_init(&fetch_head->buf, 0);\n + \tfetch_head->fp = fopen(filename, \"a\");\n + \tif (!fetch_head->fp)\n + \t\treturn error_errno(_(\"cannot open %s\"), filename);\n +@@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid\n + \tif (!write_fetch_head)\n + \t\treturn;\n + \n +-\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n +-\t\told_oid, merge_status_marker, note);\n +-\tfor (i = 0; i < url_len; ++i)\n +-\t\tif ('\\n' == url[i])\n +-\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n +-\t\telse\n +-\t\t\tfputc(url[i], fetch_head->fp);\n +-\tfputc('\\n', fetch_head->fp);\n ++\t/*\n ++\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n ++\t * any of the reference updates fails. We thus have to write all\n ++\t * updates to a buffer first and only commit it as soon as all\n ++\t * references have been successfully updated.\n ++\t */\n ++\tif (atomic_fetch) {\n ++\t\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n ++\t\t\t old_oid, merge_status_marker, note);\n ++\t\tstrbuf_add(&fetch_head->buf, url, url_len);\n ++\t\tstrbuf_addch(&fetch_head->buf, '\\n');\n ++\t} else {\n ++\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n ++\t\t\told_oid, merge_status_marker, note);\n ++\t\tfor (i = 0; i < url_len; ++i)\n ++\t\t\tif ('\\n' == url[i])\n ++\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n ++\t\t\telse\n ++\t\t\t\tfputc(url[i], fetch_head->fp);\n ++\t\tfputc('\\n', fetch_head->fp);\n ++\t}\n + }\n + \n + static void commit_fetch_head(struct fetch_head *fetch_head)\n + {\n +-\t/* Nothing to commit yet. */\n ++\tif (!write_fetch_head || !atomic_fetch)\n ++\t\treturn;\n ++\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n + }\n + \n + static void close_fetch_head(struct fetch_head *fetch_head)\n +@@ builtin/fetch.c: static void close_fetch_head(struct fetch_head *fetch_head)\n + \t\treturn;\n + \n + \tfclose(fetch_head->fp);\n ++\tstrbuf_release(&fetch_head->buf);\n + }\n + \n + static const char warn_show_forced_updates[] =\n +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n + \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n -\tstruct strbuf note = STRBUF_INIT;\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n +\t\t}\n +\t}\n +\n - \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n - \t\terror(_(\"some local refs could not be updated; try running\\n\"\n - \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n + \tif (!rc)\n + \t\tcommit_fetch_head(&fetch_head);\n + \n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n +\tstrbuf_release(&err);\n +\tref_transaction_free(transaction);\n \tfree(url);\n - \tfclose(fp);\n + \tclose_fetch_head(&fetch_head);\n \treturn rc;\n +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n + \t\t\tdie(_(\"--filter can only be used with the remote \"\n + \t\t\t \"configured in extensions.partialclone\"));\n + \n ++\t\tif (atomic_fetch)\n ++\t\t\tdie(_(\"--atomic can only be used when fetching \"\n ++\t\t\t \"from one remote\"));\n ++\n + \t\tif (stdin_refspecs)\n + \t\t\tdie(_(\"--stdin can only be used when fetching \"\n + \t\t\t \"from one remote\"));\n \n ## t/t5510-fetch.sh ##\n @@ t/t5510-fetch.sh: test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n @@ t/t5510-fetch.sh: test_expect_success 'fetch --prune --tags with refspec prunes\n +\tcd \"$D\" &&\n +\tgit clone . atomic &&\n +\tgit branch atomic-branch &&\n -+\tgit rev-parse atomic-branch >expected &&\n ++\toid=$(git rev-parse atomic-branch) &&\n ++\techo \"$oid\" >expected &&\n +\n +\tgit -C atomic fetch --atomic origin &&\n +\tgit -C atomic rev-parse origin/atomic-branch >actual &&\n -+\ttest_cmp expected actual\n ++\ttest_cmp expected actual &&\n ++\ttest $oid = \"$(git -C atomic rev-parse --verify FETCH_HEAD)\"\n +'\n +\n +test_expect_success 'fetch --atomic works with multiple branches' '\n @@ t/t5510-fetch.sh: test_expect_success 'fetch --prune --tags with refspec prunes\n +\ttest_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* &&\n +\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch &&\n +\tgit -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected &&\n -+\ttest_cmp expected actual\n ++\ttest_cmp expected actual &&\n ++\ttest_must_be_empty atomic/.git/FETCH_HEAD\n +'\n +\n +test_expect_success 'fetch --atomic executes a single reference transaction only' '\n @@ t/t5510-fetch.sh: test_expect_success 'fetch --prune --tags with refspec prunes\n +\tgit -C atomic for-each-ref >expected-refs &&\n +\ttest_must_fail git -C atomic fetch --tags --atomic origin &&\n +\tgit -C atomic for-each-ref >actual-refs &&\n -+\ttest_cmp expected-refs actual-refs\n ++\ttest_cmp expected-refs actual-refs &&\n ++\ttest_must_be_empty atomic/.git/FETCH_HEAD\n ++'\n ++\n ++test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n ++\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n ++\n ++\tcd \"$D\" &&\n ++\tgit clone . atomic &&\n ++\toid=$(git rev-parse HEAD) &&\n ++\n ++\tgit branch atomic-fetch-head-1 &&\n ++\tgit -C atomic fetch --atomic origin atomic-fetch-head-1 &&\n ++\ttest_line_count = 1 atomic/.git/FETCH_HEAD &&\n ++\n ++\tgit branch atomic-fetch-head-2 &&\n ++\tgit -C atomic fetch --atomic --append origin atomic-fetch-head-2 &&\n ++\ttest_line_count = 2 atomic/.git/FETCH_HEAD &&\n ++\tcp atomic/.git/FETCH_HEAD expected &&\n ++\n ++\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n ++\t\texit 1\n ++\tEOF\n ++\n ++\tgit branch atomic-fetch-head-3 &&\n ++\ttest_must_fail git -C atomic fetch --atomic --append origin atomic-fetch-head-3 &&\n ++\ttest_cmp expected atomic/.git/FETCH_HEAD\n +'\n +\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n-- \n2.30.0\n\n"},{"id":"413895","messageId":"xmqqmtxjgfq6.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"d80dbc5a9c9520621651541e418ee5216d164053.1610107599.git.ps@pks.im","subject":"Re: [PATCH v2 1/4] fetch: extract writing to FETCH_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-08T23:40:17Z","receivedAt":"2021-01-08T23:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +static int open_fetch_head(struct fetch_head *fetch_head)\n> +{\n> +\tconst char *filename = git_path_fetch_head(the_repository);\n> +\n> +\tif (!write_fetch_head)\n> +\t\treturn 0;\n> +\n> +\tfetch_head->fp = fopen(filename, \"a\");\n> +\tif (!fetch_head->fp)\n> +\t\treturn error_errno(_(\"cannot open %s\"), filename);\n> +\n> +\treturn 0;\n> +}\n\nSo the difference from the original, which used to have a writable\nfilehandle to /dev/null in the dry-run mode, is that fetch_head->fp\nis left as-is (not even NULLed out).\n\n> +static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid,\n\nIt is clear from the type these days but variable names like\n\"old_oid\" hint the readers that they are not a hexadecimal object\nname string but either an array of uchar[40] or a struct object_id;\nperhaps \"old_oid_hex\" would be less misleading.\n\nIf the caller does have struct object_id, then it would be even\nbetter to take it as-is as a parameter and use oid_to_hex_r() on it in\nthis function when it is given to fprintf(). [Nit #1]\n\n> +\t\t\t const char *merge_status_marker, const char *note,\n> +\t\t\t const char *url, size_t url_len)\n> +{\n> +\tsize_t i;\n> +\n> +\tif (!write_fetch_head)\n> +\t\treturn;\n\nPresumably, this check is what makes sure that fetch_head->fp that\nis left uninitialized will never gets used.\n\n> +\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> +\t\told_oid, merge_status_marker, note);\n> +\tfor (i = 0; i < url_len; ++i)\n> +\t\tif ('\\n' == url[i])\n> +\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> +\t\telse\n> +\t\t\tfputc(url[i], fetch_head->fp);\n> +\tfputc('\\n', fetch_head->fp);\n> +}\n\nOK. This is the \"case FETCH_HEAD_NOT_FOR_MERGE\" and \"case\nFETCH_HEAD_MERGE\" parts in the original.\n\nAs an abstraction, it may be better to make the caller pass a\nboolean \"is this for merge?\" and keep the knowledge of what exact\nstring is used for merge_status_marker to this function, instead of\nletting the caller passing it as a parameter in the string form.\nAfter all, we never allow anything other than an empty string or a\nfixed \"not-for-merge\" string in that place in the file format.\n[Nit #2]\n\n> +static void commit_fetch_head(struct fetch_head *fetch_head)\n> +{\n> +\t/* Nothing to commit yet. */\n> +}\n> +\n> +static void close_fetch_head(struct fetch_head *fetch_head)\n> +{\n> +\tif (!write_fetch_head)\n> +\t\treturn;\n\nSo is this check a protection against uninitialized fetch_head->fp.\nBoth changes make sense.\n\n> +\tfclose(fetch_head->fp);\n> +}\n\n> @@ -909,22 +959,19 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n> static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t int connectivity_checked, struct ref *ref_map)\n> {\n> -\tFILE *fp;\n> +\tstruct fetch_head fetch_head;\n\nAnd that is why this variable is left uninitialised on stack and it\nis OK. An alternative design would be to initialize fetch_head.fp\nto NULL, and return early with \"if (!fetch_head->fp)\" in the two\nfunctions that take fetch_head struct. That way, we have less\nreliance on the global variable write_fetch_head.\n\n> \tstruct commit *commit;\n> \tint url_len, i, rc = 0;\n> \tstruct strbuf note = STRBUF_INIT;\n> \tconst char *what, *kind;\n> \tstruct ref *rm;\n> \tchar *url;\n> -\tconst char *filename = (!write_fetch_head\n> -\t\t\t\t? \"/dev/null\"\n> -\t\t\t\t: git_path_fetch_head(the_repository));\n> \tint want_status;\n> \tint summary_width = transport_summary_width(ref_map);\n> \n> -\tfp = fopen(filename, \"a\");\n> -\tif (!fp)\n> -\t\treturn error_errno(_(\"cannot open %s\"), filename);\n> +\trc = open_fetch_head(&fetch_head);\n> +\tif (rc)\n> +\t\treturn -1;\n\nOK, we've already said \"cannot open\" in the open_fetch_head()\nfunction, so we just return an error silently.\n\n> @@ -1016,16 +1063,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\tmerge_status_marker = \"not-for-merge\";\n> \t\t\t\t/* fall-through */\n> \t\t\tcase FETCH_HEAD_MERGE:\n> -\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n> -\t\t\t\t\toid_to_hex(&rm->old_oid),\n> -\t\t\t\t\tmerge_status_marker,\n> -\t\t\t\t\tnote.buf);\n> -\t\t\t\tfor (i = 0; i < url_len; ++i)\n> -\t\t\t\t\tif ('\\n' == url[i])\n> -\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n> -\t\t\t\t\telse\n> -\t\t\t\t\t\tfputc(url[i], fp);\n> -\t\t\t\tfputc('\\n', fp);\n> +\t\t\t\tappend_fetch_head(&fetch_head,\n> +\t\t\t\t\t\t oid_to_hex(&rm->old_oid),\n> +\t\t\t\t\t\t merge_status_marker,\n> +\t\t\t\t\t\t note.buf, url, url_len);\n\nHere, we can lose merge_status_marker variable from this caller, and\nthen this caller becomes:\n\n\t\tswitch (rm->fetch_head_status) {\n\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n\t\tcase FETCH_HEAD_MERGE:\n\t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\n\t\t\t\trm->fetch_head_status == FETCH_HEAD_MERGE,\n note.buf, url, url_len);\n\nNote that I am passing \"is this a ref to be merged?\" boolean to keep\nthe exact string of \"not-for-merge\" in the callee.\n\n> @@ -1060,6 +1101,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t}\n> \t}\n> \n> +\tif (!rc)\n> +\t\tcommit_fetch_head(&fetch_head);\n> +\n> \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n> \t\terror(_(\"some local refs could not be updated; try running\\n\"\n> \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n> @@ -1077,7 +1121,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> abort:\n> \tstrbuf_release(¬e);\n> \tfree(url);\n> -\tfclose(fp);\n> +\tclose_fetch_head(&fetch_head);\n> \treturn rc;\n> }\n\nOther than the above two nits, this step looks good to me.\n\nThanks.\n"},{"id":"413896","messageId":"xmqqim87gf9z.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"718a8bf5d7a0ed92c3004991a42419279ff38253.1610107599.git.ps@pks.im","subject":"Re: [PATCH v2 2/4] fetch: refactor `s_update_ref` to use common exit path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-08T23:50:00Z","receivedAt":"2021-01-08T23:51:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -598,30 +598,33 @@ static int s_update_ref(const char *action,\n> \tmsg = xstrfmt(\"%s: %s\", rla, action);\n> \n> \ttransaction = ref_transaction_begin(&err);\n> -\tif (!transaction ||\n> -\t ref_transaction_update(transaction, ref->name,\n> -\t\t\t\t &ref->new_oid,\n> -\t\t\t\t check_old ? &ref->old_oid : NULL,\n> -\t\t\t\t 0, msg, &err))\n> -\t\tgoto fail;\n> +\tif (!transaction) {\n> +\t\tret = STORE_REF_ERROR_OTHER;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n> +\t\t\t\t check_old ? &ref->old_oid : NULL,\n> +\t\t\t\t 0, msg, &err);\n> +\tif (ret) {\n> +\t\tret = STORE_REF_ERROR_OTHER;\n> +\t\tgoto out;\n> +\t}\n\nThe above certainly got cleaner thanks to Christian's suggestion,\nbut I wonder why\n\n\ttransaction = ref_transaction_begin(&err);\n\tif (!transaction ||\n\t ref_transaction_update(transaction, ref->name,\n\t\t\t\t &ref->new_oid,\n\t\t\t\t check_old ? &ref->old_oid : NULL,\n\t\t\t\t 0, msg, &err)) {\n\t\tret = STORE_REF_ERROR_OTHER;\n\t\tgoto out;\n\t}\n\nshouldn't be sufficient.\n\n> \tret = ref_transaction_commit(transaction, &err);\n> \tif (ret) {\n> -\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n> -\t\tgoto fail;\n> +\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n> +\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n> +\t\tgoto out;\n> \t}\n> \n> +out:\n> \tref_transaction_free(transaction);\n\nIt is a bit funny to see a goto that jumps to the label without\nhaving anything else in between, but we know we will be adding more\ncode just before the \"out:\" label, so it is a good preliminary\npreparation.\n\nI think a variant that is much easier to follow would be to write\nlike this instead:\n\n\tswitch (ref_transaction_commit(transaction, &err)) {\n case 0: /* happy */\n\t\tbreak;\n\tcase TRANSACTION_NAME_CONFLICT:\n\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n\t\tgoto out;\n\tdefault:\n\t\tret = STORE_REF_ERROR_OTHER;\n\t\tgoto out;\n\t}\n\n> +\tif (ret)\n> +\t\terror(\"%s\", err.buf);\n\nOK.\n\n> \tstrbuf_release(&err);\n> \tfree(msg);\n> -\treturn 0;\n> -fail:\n> -\tref_transaction_free(transaction);\n> -\terror(\"%s\", err.buf);\n> -\tstrbuf_release(&err);\n> -\tfree(msg);\n> -\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n> -\t\t\t : STORE_REF_ERROR_OTHER;\n> +\treturn ret;\n> }\n> \n> static int refcol_width = 10;\n\nTHanks.\n"},{"id":"413897","messageId":"xmqqczyfgf3p.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"4162d10fcbb5729dffaaec0651345dd9522a8e05.1610107599.git.ps@pks.im","subject":"Re: [PATCH v2 3/4] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-08T23:53:46Z","receivedAt":"2021-01-08T23:54:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -597,10 +598,17 @@ static int s_update_ref(const char *action,\n> \t\trla = default_rla.buf;\n> \tmsg = xstrfmt(\"%s: %s\", rla, action);\n> \n> -\ttransaction = ref_transaction_begin(&err);\n> +\t/*\n> +\t * If no transaction was passed to us, we manage the transaction\n> +\t * ourselves. Otherwise, we trust the caller to handle the transaction\n> +\t * lifecycle.\n> +\t */\n> \tif (!transaction) {\n> -\t\tret = STORE_REF_ERROR_OTHER;\n> -\t\tgoto out;\n> +\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n> +\t\tif (!transaction) {\n> +\t\t\tret = STORE_REF_ERROR_OTHER;\n> +\t\t\tgoto out;\n> +\t\t}\n> \t}\n\nOK, this answers the question I posed in the review of the previous\nstep. We need to separate out \"if (!transaction || ...)\" into two\nanyway with this step, so it is easier to see what changed in this\nstep if we separated in the previous preparatory clean-up step.\n\n> @@ -611,15 +619,17 @@ static int s_update_ref(const char *action,\n> \t\tgoto out;\n> \t}\n> \n> -\tret = ref_transaction_commit(transaction, &err);\n> -\tif (ret) {\n> -\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n> -\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n> -\t\tgoto out;\n> +\tif (our_transaction) {\n> +\t\tret = ref_transaction_commit(our_transaction, &err);\n> +\t\tif (ret) {\n> +\t\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n> +\t\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n> +\t\t\tgoto out;\n> +\t\t}\n\nThe switch statement suggested earlier would shine when the\nconstants involved have such long names.\n\n> \t}\n> \n> out:\n> -\tref_transaction_free(transaction);\n> +\tref_transaction_free(our_transaction);\n> \tif (ret)\n> \t\terror(\"%s\", err.buf);\n> \tstrbuf_release(&err);\n\nMakes sense. Thanks.\n"},{"id":"413898","messageId":"xmqq7dongeji.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"53705281b60285837905137f45fc8607012d2f19.1610107599.git.ps@pks.im","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-09T00:05:53Z","receivedAt":"2021-01-09T00:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +\t/*\n> +\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n> +\t * any of the reference updates fails. We thus have to write all\n> +\t * updates to a buffer first and only commit it as soon as all\n> +\t * references have been successfully updated.\n> +\t */\n> +\tif (atomic_fetch) {\n> +\t\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n> +\t\t\t old_oid, merge_status_marker, note);\n> +\t\tstrbuf_add(&fetch_head->buf, url, url_len);\n> +\t\tstrbuf_addch(&fetch_head->buf, '\\n');\n> +\t} else {\n> +\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> +\t\t\told_oid, merge_status_marker, note);\n> +\t\tfor (i = 0; i < url_len; ++i)\n> +\t\t\tif ('\\n' == url[i])\n> +\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> +\t\t\telse\n> +\t\t\t\tfputc(url[i], fetch_head->fp);\n> +\t\tfputc('\\n', fetch_head->fp);\n> +\t}\n\nI do not want to see it done like this; formating what ought to come\nout identical with two separate mechanisms will lead to bugs under\nthe road.\n\nAlso what is the deal about \"\\n\" vs \"\\\\n\"? Do we already have\nbehaviour differences between two codepaths from the get-go?\n\nIt would be much more preferrable to see this series go like so:\n\n [1/4] create append_fetch_head() that writes out to\n fetch_head->fp\n\n [1.5/4] convert append_fetch_head() to ALWAYS accumulate in\n fetch_head->buf, and ALWAYS flushes what is accumulated\n at the end.\n\nAfter these two patches are applied, there shouldn't be any\nbehaviour change or change in the format in generated FETCH_HEAD.\n\n [2/4] and [3/4] stay the same\n\n [4/4] this step does not touch append_fetch_head() at all. It\n just changes the way how fetch_head->buf is flushed at the end\n (namely, under atomic mode and after seeing any failure, the\n accumulated output gets discarded without being written).\n\nI also thought briefly about an alternative approach to rewind and\ntruncate the output to its original length upon seeing a failure\nunder the atomic mode, but rejected it because the code gets hairy.\nI think \"accumulate until we know we want to actually write them out\"\nis a good approach to this issue.\n\nThanks.\n"},{"id":"414025","messageId":"X/wnxSn4KjQF0/T+@ncase","threadId":"54954","inReplyTo":"xmqqmtxjgfq6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/4] fetch: extract writing to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T10:26:13Z","receivedAt":"2021-01-11T10:27:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 08, 2021 at 03:40:17PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > +static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid,\n> \n> It is clear from the type these days but variable names like\n> \"old_oid\" hint the readers that they are not a hexadecimal object\n> name string but either an array of uchar[40] or a struct object_id;\n> perhaps \"old_oid_hex\" would be less misleading.\n> \n> If the caller does have struct object_id, then it would be even\n> better to take it as-is as a parameter and use oid_to_hex_r() on it in\n> this function when it is given to fprintf(). [Nit #1]\n\nAgreed.\n\n[snip]\n> > +\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> > +\t\told_oid, merge_status_marker, note);\n> > +\tfor (i = 0; i < url_len; ++i)\n> > +\t\tif ('\\n' == url[i])\n> > +\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> > +\t\telse\n> > +\t\t\tfputc(url[i], fetch_head->fp);\n> > +\tfputc('\\n', fetch_head->fp);\n> > +}\n> \n> OK. This is the \"case FETCH_HEAD_NOT_FOR_MERGE\" and \"case\n> FETCH_HEAD_MERGE\" parts in the original.\n> \n> As an abstraction, it may be better to make the caller pass a\n> boolean \"is this for merge?\" and keep the knowledge of what exact\n> string is used for merge_status_marker to this function, instead of\n> letting the caller passing it as a parameter in the string form.\n> After all, we never allow anything other than an empty string or a\n> fixed \"not-for-merge\" string in that place in the file format.\n> [Nit #2]\n\nI think it's even nicer to just pass in `rm->fetch_head_status`\ndirectly, which allows us to move below switch into `append_fetch_head`.\n\n[snip]\n> > +\tfclose(fetch_head->fp);\n> > +}\n> \n> > @@ -909,22 +959,19 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n> > static int store_updated_refs(const char *raw_url, const char *remote_name,\n> > \t\t\t int connectivity_checked, struct ref *ref_map)\n> > {\n> > -\tFILE *fp;\n> > +\tstruct fetch_head fetch_head;\n> \n> And that is why this variable is left uninitialised on stack and it\n> is OK. An alternative design would be to initialize fetch_head.fp\n> to NULL, and return early with \"if (!fetch_head->fp)\" in the two\n> functions that take fetch_head struct. That way, we have less\n> reliance on the global variable write_fetch_head.\n\nI like your proposal more. I wasn't quite happy with leaving `fp`\nuninitialized, and using it as a sentinel for whether to write something\nor not feels natural to me.\n\n[snip]\n> > @@ -1016,16 +1063,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> > \t\t\t\tmerge_status_marker = \"not-for-merge\";\n> > \t\t\t\t/* fall-through */\n> > \t\t\tcase FETCH_HEAD_MERGE:\n> > -\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n> > -\t\t\t\t\toid_to_hex(&rm->old_oid),\n> > -\t\t\t\t\tmerge_status_marker,\n> > -\t\t\t\t\tnote.buf);\n> > -\t\t\t\tfor (i = 0; i < url_len; ++i)\n> > -\t\t\t\t\tif ('\\n' == url[i])\n> > -\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n> > -\t\t\t\t\telse\n> > -\t\t\t\t\t\tfputc(url[i], fp);\n> > -\t\t\t\tfputc('\\n', fp);\n> > +\t\t\t\tappend_fetch_head(&fetch_head,\n> > +\t\t\t\t\t\t oid_to_hex(&rm->old_oid),\n> > +\t\t\t\t\t\t merge_status_marker,\n> > +\t\t\t\t\t\t note.buf, url, url_len);\n> \n> Here, we can lose merge_status_marker variable from this caller, and\n> then this caller becomes:\n> \n> \t\tswitch (rm->fetch_head_status) {\n> \t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n> \t\tcase FETCH_HEAD_MERGE:\n> \t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\n> \t\t\t\trm->fetch_head_status == FETCH_HEAD_MERGE,\n> note.buf, url, url_len);\n> \n> Note that I am passing \"is this a ref to be merged?\" boolean to keep\n> the exact string of \"not-for-merge\" in the callee.\n\nAs said above, I'm just moving this complete switch into\n`append_fetch_head` and pass `rm->fetch_head_status`.\n\nPatrick\n"},{"id":"414026","messageId":"X/woXrxFqctcX2NB@ncase","threadId":"54954","inReplyTo":"xmqqim87gf9z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/4] fetch: refactor `s_update_ref` to use common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T10:28:46Z","receivedAt":"2021-01-11T10:29:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 08, 2021 at 03:50:00PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n[snip]\n> > \tret = ref_transaction_commit(transaction, &err);\n> > \tif (ret) {\n> > -\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n> > -\t\tgoto fail;\n> > +\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n> > +\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n> > +\t\tgoto out;\n> > \t}\n> > \n> > +out:\n> > \tref_transaction_free(transaction);\n> \n> It is a bit funny to see a goto that jumps to the label without\n> having anything else in between, but we know we will be adding more\n> code just before the \"out:\" label, so it is a good preliminary\n> preparation.\n> \n> I think a variant that is much easier to follow would be to write\n> like this instead:\n> \n> \tswitch (ref_transaction_commit(transaction, &err)) {\n> case 0: /* happy */\n> \t\tbreak;\n> \tcase TRANSACTION_NAME_CONFLICT:\n> \t\tret = STORE_REF_ERROR_DF_CONFLICT;\n> \t\tgoto out;\n> \tdefault:\n> \t\tret = STORE_REF_ERROR_OTHER;\n> \t\tgoto out;\n> \t}\n\nAgreed, that is easier to read. Thanks!\n\nPatrick\n"},{"id":"414027","messageId":"X/wrgYYcZfYZj+4/@ncase","threadId":"54954","inReplyTo":"xmqq7dongeji.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T10:42:09Z","receivedAt":"2021-01-11T10:43:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 08, 2021 at 04:05:53PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > +\t/*\n> > +\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n> > +\t * any of the reference updates fails. We thus have to write all\n> > +\t * updates to a buffer first and only commit it as soon as all\n> > +\t * references have been successfully updated.\n> > +\t */\n> > +\tif (atomic_fetch) {\n> > +\t\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n> > +\t\t\t old_oid, merge_status_marker, note);\n> > +\t\tstrbuf_add(&fetch_head->buf, url, url_len);\n> > +\t\tstrbuf_addch(&fetch_head->buf, '\\n');\n> > +\t} else {\n> > +\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> > +\t\t\told_oid, merge_status_marker, note);\n> > +\t\tfor (i = 0; i < url_len; ++i)\n> > +\t\t\tif ('\\n' == url[i])\n> > +\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> > +\t\t\telse\n> > +\t\t\t\tfputc(url[i], fetch_head->fp);\n> > +\t\tfputc('\\n', fetch_head->fp);\n> > +\t}\n> \n> I do not want to see it done like this; formating what ought to come\n> out identical with two separate mechanisms will lead to bugs under\n> the road.\n> \n> Also what is the deal about \"\\n\" vs \"\\\\n\"? Do we already have\n> behaviour differences between two codepaths from the get-go?\n\nGood point. I'll unify those code paths.\n\n> It would be much more preferrable to see this series go like so:\n> \n> [1/4] create append_fetch_head() that writes out to\n> fetch_head->fp\n> \n> [1.5/4] convert append_fetch_head() to ALWAYS accumulate in\n> fetch_head->buf, and ALWAYS flushes what is accumulated\n> at the end.\n\nThis is a change I'm hesitant to make. The thing is that FETCH_HEAD is\nquite weird as the current design allows different `git fetch --append`\nprocesses to write to FETCH_HEAD at the same time. If we change to\nalways accumulate first and flush once we're done, then we essentially\nhave a change in behaviour as FETCH_HEADs aren't written in an\ninterleaving fashion anymore, but only at the end. Also, if there is any\nunexpected error in between updating references which causes us to die,\nthen we wouldn't have written the already updated references to\nFETCH_HEAD now.\n\nSo I'm all in when it comes to merging formatting directives, even more\nso as I have missed the \"\\\\n\" weirdness. But for now, I'll keep the\ncurrent flushing semantics in the non-atomic case. Please let me know if\nyou're not convinced and I'll have another look for v4.\n\n> After these two patches are applied, there shouldn't be any\n> behaviour change or change in the format in generated FETCH_HEAD.\n> \n> [2/4] and [3/4] stay the same\n> \n> [4/4] this step does not touch append_fetch_head() at all. It\n> just changes the way how fetch_head->buf is flushed at the end\n> (namely, under atomic mode and after seeing any failure, the\n> accumulated output gets discarded without being written).\n> \n> I also thought briefly about an alternative approach to rewind and\n> truncate the output to its original length upon seeing a failure\n> under the atomic mode, but rejected it because the code gets hairy.\n> I think \"accumulate until we know we want to actually write them out\"\n> is a good approach to this issue.\n> \n> Thanks.\n\nPatrick\n"},{"id":"414028","messageId":"61dc19a1cab5b03d07c6635496761108d089eb23.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"[PATCH v3 1/5] fetch: extract writing to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:16Z","receivedAt":"2021-01-11T11:06:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing a fetch with the default `--write-fetch-head` option, we\nwrite all updated references to FETCH_HEAD while the updates are\nperformed. Given that updates are not performed atomically, it means\nthat we we write to FETCH_HEAD even if some or all of the reference\nupdates fail.\n\nGiven that we simply update FETCH_HEAD ad-hoc with each reference, the\nlogic is completely contained in `store_update_refs` and thus quite hard\nto extend. This can already be seen by the way we skip writing to the\nFETCH_HEAD: instead of having a conditional which simply skips writing,\nwe instead open \"/dev/null\" and needlessly write all updates there.\n\nWe are about to extend git-fetch(1) to accept an `--atomic` flag which\nwill make the fetch an all-or-nothing operation with regards to the\nreference updates. This will also require us to make the updates to\nFETCH_HEAD an all-or-nothing operation, but as explained doing so is not\neasy with the current layout. This commit thus refactors the wa we write\nto FETCH_HEAD and pulls out the logic to open, append to, commit and\nclose the file. While this may seem rather over-the top at first,\npulling out this logic will make it a lot easier to update the code in a\nsubsequent commit. It also allows us to easily skip writing completely\nin case `--no-write-fetch-head` was passed.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 108 +++++++++++++++++++++++++++++++++++-------------\n remote.h | 2 +-\n 2 files changed, 80 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ecf8537605..50f0306a92 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -897,6 +897,73 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \treturn 0;\n }\n \n+struct fetch_head {\n+\tFILE *fp;\n+};\n+\n+static int open_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tconst char *filename = git_path_fetch_head(the_repository);\n+\n+\tif (write_fetch_head) {\n+\t\tfetch_head->fp = fopen(filename, \"a\");\n+\t\tif (!fetch_head->fp)\n+\t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\t} else {\n+\t\tfetch_head->fp = NULL;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void append_fetch_head(struct fetch_head *fetch_head,\n+\t\t\t const struct object_id *old_oid,\n+\t\t\t enum fetch_head_status fetch_head_status,\n+\t\t\t const char *note,\n+\t\t\t const char *url, size_t url_len)\n+{\n+\tchar old_oid_hex[GIT_MAX_HEXSZ + 1];\n+\tconst char *merge_status_marker;\n+\tsize_t i;\n+\n+\tif (!fetch_head->fp)\n+\t\treturn;\n+\n+\tswitch (fetch_head_status) {\n+\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n+\t\t\tmerge_status_marker = \"not-for-merge\";\n+\t\t\tbreak;\n+\t\tcase FETCH_HEAD_MERGE:\n+\t\t\tmerge_status_marker = \"\";\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\t/* do not write anything to FETCH_HEAD */\n+\t\t\treturn;\n+\t}\n+\n+\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n+\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n+\tfor (i = 0; i < url_len; ++i)\n+\t\tif ('\\n' == url[i])\n+\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\telse\n+\t\t\tfputc(url[i], fetch_head->fp);\n+\tfputc('\\n', fetch_head->fp);\n+}\n+\n+static void commit_fetch_head(struct fetch_head *fetch_head)\n+{\n+\t/* Nothing to commit yet. */\n+}\n+\n+static void close_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tif (!fetch_head->fp)\n+\t\treturn;\n+\n+\tfclose(fetch_head->fp);\n+}\n+\n static const char warn_show_forced_updates[] =\n N_(\"Fetch normally indicates which branches had a forced update,\\n\"\n \"but that check has been disabled. To re-enable, use '--show-forced-updates'\\n\"\n@@ -909,22 +976,19 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked, struct ref *ref_map)\n {\n-\tFILE *fp;\n+\tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n-\tconst char *filename = (!write_fetch_head\n-\t\t\t\t? \"/dev/null\"\n-\t\t\t\t: git_path_fetch_head(the_repository));\n \tint want_status;\n \tint summary_width = transport_summary_width(ref_map);\n \n-\tfp = fopen(filename, \"a\");\n-\tif (!fp)\n-\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\trc = open_fetch_head(&fetch_head);\n+\tif (rc)\n+\t\treturn -1;\n \n \tif (raw_url)\n \t\turl = transport_anonymize_url(raw_url);\n@@ -953,7 +1017,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t want_status++) {\n \t\tfor (rm = ref_map; rm; rm = rm->next) {\n \t\t\tstruct ref *ref = NULL;\n-\t\t\tconst char *merge_status_marker = \"\";\n \n \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n \t\t\t\tif (want_status == FETCH_HEAD_MERGE)\n@@ -1011,26 +1074,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t\tstrbuf_addf(¬e, \"%s \", kind);\n \t\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n \t\t\t}\n-\t\t\tswitch (rm->fetch_head_status) {\n-\t\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n-\t\t\t\tmerge_status_marker = \"not-for-merge\";\n-\t\t\t\t/* fall-through */\n-\t\t\tcase FETCH_HEAD_MERGE:\n-\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n-\t\t\t\t\toid_to_hex(&rm->old_oid),\n-\t\t\t\t\tmerge_status_marker,\n-\t\t\t\t\tnote.buf);\n-\t\t\t\tfor (i = 0; i < url_len; ++i)\n-\t\t\t\t\tif ('\\n' == url[i])\n-\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n-\t\t\t\t\telse\n-\t\t\t\t\t\tfputc(url[i], fp);\n-\t\t\t\tfputc('\\n', fp);\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\t/* do not write anything to FETCH_HEAD */\n-\t\t\t\tbreak;\n-\t\t\t}\n+\n+\t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\n+\t\t\t\t\t rm->fetch_head_status,\n+\t\t\t\t\t note.buf, url, url_len);\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n@@ -1060,6 +1107,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc)\n+\t\tcommit_fetch_head(&fetch_head);\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n@@ -1077,7 +1127,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n abort:\n \tstrbuf_release(¬e);\n \tfree(url);\n-\tfclose(fp);\n+\tclose_fetch_head(&fetch_head);\n \treturn rc;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 3211abdf05..aad1a0f080 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -134,7 +134,7 @@ struct ref {\n \t * should be 0, so that xcalloc'd structures get it\n \t * by default.\n \t */\n-\tenum {\n+\tenum fetch_head_status {\n \t\tFETCH_HEAD_MERGE = -1,\n \t\tFETCH_HEAD_NOT_FOR_MERGE = 0,\n \t\tFETCH_HEAD_IGNORE = 1\n-- \n2.30.0\n\n"},{"id":"414029","messageId":"a19762690eb7f9957ac31d73e110f0103aeb2307.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"[PATCH v3 2/5] fetch: use strbuf to format FETCH_HEAD updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:20Z","receivedAt":"2021-01-11T11:06:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"This commit refactors `append_fetch_head()` to use a `struct strbuf` for\nformatting the update which we're about to append to the FETCH_HEAD\nfile. While the refactoring doesn't have much of a benefit right now, it\nservers as a preparatory step to implement atomic fetches where we need\nto buffer all updates to FETCH_HEAD and only flush them out if all\nreference updates succeeded.\n\nNo change in behaviour is expected from this commit.\n---\n builtin/fetch.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 50f0306a92..1252f37493 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -899,6 +899,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \n struct fetch_head {\n \tFILE *fp;\n+\tstruct strbuf buf;\n };\n \n static int open_fetch_head(struct fetch_head *fetch_head)\n@@ -909,6 +910,7 @@ static int open_fetch_head(struct fetch_head *fetch_head)\n \t\tfetch_head->fp = fopen(filename, \"a\");\n \t\tif (!fetch_head->fp)\n \t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\t\tstrbuf_init(&fetch_head->buf, 0);\n \t} else {\n \t\tfetch_head->fp = NULL;\n \t}\n@@ -941,14 +943,17 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n \t\t\treturn;\n \t}\n \n-\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n-\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n+\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n+\t\t oid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n \tfor (i = 0; i < url_len; ++i)\n \t\tif ('\\n' == url[i])\n-\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\t\tstrbuf_addstr(&fetch_head->buf, \"\\\\n\");\n \t\telse\n-\t\t\tfputc(url[i], fetch_head->fp);\n-\tfputc('\\n', fetch_head->fp);\n+\t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n+\tstrbuf_addch(&fetch_head->buf, '\\n');\n+\n+\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n+\tstrbuf_reset(&fetch_head->buf);\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n@@ -962,6 +967,7 @@ static void close_fetch_head(struct fetch_head *fetch_head)\n \t\treturn;\n \n \tfclose(fetch_head->fp);\n+\tstrbuf_release(&fetch_head->buf);\n }\n \n static const char warn_show_forced_updates[] =\n-- \n2.30.0\n\n"},{"id":"414030","messageId":"865d357ba7436ccd3de3573dc45e2647b09478e0.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"[PATCH v3 4/5] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:29Z","receivedAt":"2021-01-11T11:06:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The handling of ref updates is completely handled by `s_update_ref()`,\nwhich will manage the complete lifecycle of the reference transaction.\nThis is fine right now given that git-fetch(1) does not support atomic\nfetches, so each reference gets its own transaction. It is quite\ninflexible though, as `s_update_ref()` only knows about a single\nreference update at a time, so it doesn't allow us to alter the\nstrategy.\n\nThis commit prepares `s_update_ref()` and its only caller\n`update_local_ref()` to allow passing an external transaction. If none\nis given, then the existing behaviour is triggered which creates a new\ntransaction and directly commits it. Otherwise, if the caller provides a\ntransaction, then we only queue the update but don't commit it. This\noptionally allows the caller to manage when a transaction will be\ncommitted.\n\nGiven that `update_local_ref()` is always called with a `NULL`\ntransaction for now, no change in behaviour is expected from this\nchange.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 51 ++++++++++++++++++++++++++++++-------------------\n 1 file changed, 31 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 991771f8eb..654c7a7eed 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -583,11 +583,12 @@ static struct ref *get_ref_map(struct remote *remote,\n \n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n+\t\t\tstruct ref_transaction *transaction,\n \t\t\tint check_old)\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n+\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -597,10 +598,17 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\t/*\n+\t * If no transaction was passed to us, we manage the transaction\n+\t * ourselves. Otherwise, we trust the caller to handle the transaction\n+\t * lifecycle.\n+\t */\n \tif (!transaction) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\tret = STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n@@ -611,19 +619,21 @@ static int s_update_ref(const char *action,\n \t\tgoto out;\n \t}\n \n-\tswitch (ref_transaction_commit(transaction, &err)) {\n-\tcase 0:\n-\t\tbreak;\n-\tcase TRANSACTION_NAME_CONFLICT:\n-\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n-\t\tgoto out;\n-\tdefault:\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\tif (our_transaction) {\n+\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n+\t\tcase 0:\n+\t\t\tbreak;\n+\t\tcase TRANSACTION_NAME_CONFLICT:\n+\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n+\t\t\tgoto out;\n+\t\tdefault:\n+\t\t\tret = STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n out:\n-\tref_transaction_free(transaction);\n+\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n@@ -766,6 +776,7 @@ static void format_display(struct strbuf *display, char code,\n }\n \n static int update_local_ref(struct ref *ref,\n+\t\t\t struct ref_transaction *transaction,\n \t\t\t const char *remote,\n \t\t\t const struct ref *remote_ref,\n \t\t\t struct strbuf *display,\n@@ -806,7 +817,7 @@ static int update_local_ref(struct ref *ref,\n \t starts_with(ref->name, \"refs/tags/\")) {\n \t\tif (force || ref->force) {\n \t\t\tint r;\n-\t\t\tr = s_update_ref(\"updating tag\", ref, 0);\n+\t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n@@ -843,7 +854,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tr = s_update_ref(msg, ref, 0);\n+\t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -865,7 +876,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"fast-forward\", ref, 1);\n+\t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -877,7 +888,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"forced-update\", ref, 1);\n+\t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n \t\t\t remote, pretty_ref, summary_width);\n@@ -1094,8 +1105,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n-\t\t\t\t\t\t summary_width);\n+\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n-- \n2.30.0\n\n"},{"id":"414031","messageId":"cover.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610027375.git.ps@pks.im","subject":"[PATCH v3 0/5] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:11Z","receivedAt":"2021-01-11T11:06:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series to implement support for\natomic reference updates for git-fetch(1). It's similar to `git push\n--atomic`, only that it applies to the local side. That is the fetch\nwill either succeed and update all remote references or it will fail and\nupdate none.\n\nChanges compared to v2:\n\n - `append_fetch_head` now takes a `struct object_id *old_oid`\n instead of a `char *old_oid`.\n\n - Handling of the merge status marker was moved into\n `append_fetch_head`.\n\n - I've unified code paths to format the line we're about to write to\n FETCH_HEAD in atomic and non-atomic cases, which is the new 2/5\n patch.\n\n - We now always initialize `fetch_head->fp` and use it to derive\n whether anything should be written at all instead of using the\n global `write_fetch_head` variable.\n\nI think this should address all of Junio's feedback. Thanks for your\ninput!\n\nPatrick\n\nPatrick Steinhardt (5):\n fetch: extract writing to FETCH_HEAD\n fetch: use strbuf to format FETCH_HEAD updates\n fetch: refactor `s_update_ref` to use common exit path\n fetch: allow passing a transaction to `s_update_ref()`\n fetch: implement support for atomic reference updates\n\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 228 +++++++++++++++++++++++---------\n remote.h | 2 +-\n t/t5510-fetch.sh | 168 +++++++++++++++++++++++\n 4 files changed, 342 insertions(+), 60 deletions(-)\n\nRange-diff against v2:\n1: d80dbc5a9c ! 1: 61dc19a1ca fetch: extract writing to FETCH_HEAD\n @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid\n +{\n +\tconst char *filename = git_path_fetch_head(the_repository);\n +\n -+\tif (!write_fetch_head)\n -+\t\treturn 0;\n -+\n -+\tfetch_head->fp = fopen(filename, \"a\");\n -+\tif (!fetch_head->fp)\n -+\t\treturn error_errno(_(\"cannot open %s\"), filename);\n ++\tif (write_fetch_head) {\n ++\t\tfetch_head->fp = fopen(filename, \"a\");\n ++\t\tif (!fetch_head->fp)\n ++\t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n ++\t} else {\n ++\t\tfetch_head->fp = NULL;\n ++\t}\n +\n +\treturn 0;\n +}\n +\n -+static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid,\n -+\t\t\t const char *merge_status_marker, const char *note,\n ++static void append_fetch_head(struct fetch_head *fetch_head,\n ++\t\t\t const struct object_id *old_oid,\n ++\t\t\t enum fetch_head_status fetch_head_status,\n ++\t\t\t const char *note,\n +\t\t\t const char *url, size_t url_len)\n +{\n ++\tchar old_oid_hex[GIT_MAX_HEXSZ + 1];\n ++\tconst char *merge_status_marker;\n +\tsize_t i;\n +\n -+\tif (!write_fetch_head)\n ++\tif (!fetch_head->fp)\n +\t\treturn;\n +\n ++\tswitch (fetch_head_status) {\n ++\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n ++\t\t\tmerge_status_marker = \"not-for-merge\";\n ++\t\t\tbreak;\n ++\t\tcase FETCH_HEAD_MERGE:\n ++\t\t\tmerge_status_marker = \"\";\n ++\t\t\tbreak;\n ++\t\tdefault:\n ++\t\t\t/* do not write anything to FETCH_HEAD */\n ++\t\t\treturn;\n ++\t}\n ++\n +\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n -+\t\told_oid, merge_status_marker, note);\n ++\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n +\tfor (i = 0; i < url_len; ++i)\n +\t\tif ('\\n' == url[i])\n +\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid\n +\n +static void close_fetch_head(struct fetch_head *fetch_head)\n +{\n -+\tif (!write_fetch_head)\n ++\tif (!fetch_head->fp)\n +\t\treturn;\n +\n +\tfclose(fetch_head->fp);\n @@ builtin/fetch.c: N_(\"It took %.2f seconds to check forced updates. You can use\\n\n \tif (raw_url)\n \t\turl = transport_anonymize_url(raw_url);\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n - \t\t\t\tmerge_status_marker = \"not-for-merge\";\n - \t\t\t\t/* fall-through */\n - \t\t\tcase FETCH_HEAD_MERGE:\n + \t want_status++) {\n + \t\tfor (rm = ref_map; rm; rm = rm->next) {\n + \t\t\tstruct ref *ref = NULL;\n +-\t\t\tconst char *merge_status_marker = \"\";\n + \n + \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n + \t\t\t\tif (want_status == FETCH_HEAD_MERGE)\n +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n + \t\t\t\t\tstrbuf_addf(¬e, \"%s \", kind);\n + \t\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n + \t\t\t}\n +-\t\t\tswitch (rm->fetch_head_status) {\n +-\t\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n +-\t\t\t\tmerge_status_marker = \"not-for-merge\";\n +-\t\t\t\t/* fall-through */\n +-\t\t\tcase FETCH_HEAD_MERGE:\n -\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n -\t\t\t\t\toid_to_hex(&rm->old_oid),\n -\t\t\t\t\tmerge_status_marker,\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n -\t\t\t\t\telse\n -\t\t\t\t\t\tfputc(url[i], fp);\n -\t\t\t\tfputc('\\n', fp);\n -+\t\t\t\tappend_fetch_head(&fetch_head,\n -+\t\t\t\t\t\t oid_to_hex(&rm->old_oid),\n -+\t\t\t\t\t\t merge_status_marker,\n -+\t\t\t\t\t\t note.buf, url, url_len);\n - \t\t\t\tbreak;\n - \t\t\tdefault:\n - \t\t\t\t/* do not write anything to FETCH_HEAD */\n +-\t\t\t\tbreak;\n +-\t\t\tdefault:\n +-\t\t\t\t/* do not write anything to FETCH_HEAD */\n +-\t\t\t\tbreak;\n +-\t\t\t}\n ++\n ++\t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\n ++\t\t\t\t\t rm->fetch_head_status,\n ++\t\t\t\t\t note.buf, url, url_len);\n + \n + \t\t\tstrbuf_reset(¬e);\n + \t\t\tif (ref) {\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n \treturn rc;\n }\n \n +\n + ## remote.h ##\n +@@ remote.h: struct ref {\n + \t * should be 0, so that xcalloc'd structures get it\n + \t * by default.\n + \t */\n +-\tenum {\n ++\tenum fetch_head_status {\n + \t\tFETCH_HEAD_MERGE = -1,\n + \t\tFETCH_HEAD_NOT_FOR_MERGE = 0,\n + \t\tFETCH_HEAD_IGNORE = 1\n-: ---------- > 2: a19762690e fetch: use strbuf to format FETCH_HEAD updates\n2: 718a8bf5d7 ! 3: c411f30e09 fetch: refactor `s_update_ref` to use common exit path\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n -\t\t\t\t check_old ? &ref->old_oid : NULL,\n -\t\t\t\t 0, msg, &err))\n -\t\tgoto fail;\n +-\n +-\tret = ref_transaction_commit(transaction, &err);\n +-\tif (ret) {\n +-\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n +-\t\tgoto fail;\n +\tif (!transaction) {\n +\t\tret = STORE_REF_ERROR_OTHER;\n +\t\tgoto out;\n -+\t}\n -+\n + \t}\n + \n +\tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n +\t\t\t\t check_old ? &ref->old_oid : NULL,\n +\t\t\t\t 0, msg, &err);\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n +\t\tret = STORE_REF_ERROR_OTHER;\n +\t\tgoto out;\n +\t}\n - \n - \tret = ref_transaction_commit(transaction, &err);\n - \tif (ret) {\n --\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n --\t\tgoto fail;\n -+\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n -+\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n ++\n ++\tswitch (ref_transaction_commit(transaction, &err)) {\n ++\tcase 0:\n ++\t\tbreak;\n ++\tcase TRANSACTION_NAME_CONFLICT:\n ++\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n +\t\tgoto out;\n - \t}\n - \n ++\tdefault:\n ++\t\tret = STORE_REF_ERROR_OTHER;\n ++\t\tgoto out;\n ++\t}\n ++\n +out:\n \tref_transaction_free(transaction);\n +\tif (ret)\n3: 4162d10fcb ! 4: 865d357ba7 fetch: allow passing a transaction to `s_update_ref()`\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n \t\tgoto out;\n \t}\n \n --\tret = ref_transaction_commit(transaction, &err);\n --\tif (ret) {\n --\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n --\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n +-\tswitch (ref_transaction_commit(transaction, &err)) {\n +-\tcase 0:\n +-\t\tbreak;\n +-\tcase TRANSACTION_NAME_CONFLICT:\n +-\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n +-\t\tgoto out;\n +-\tdefault:\n +-\t\tret = STORE_REF_ERROR_OTHER;\n -\t\tgoto out;\n +\tif (our_transaction) {\n -+\t\tret = ref_transaction_commit(our_transaction, &err);\n -+\t\tif (ret) {\n -+\t\t\tret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT\n -+\t\t\t\t\t\t\t\t : STORE_REF_ERROR_OTHER;\n ++\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n ++\t\tcase 0:\n ++\t\t\tbreak;\n ++\t\tcase TRANSACTION_NAME_CONFLICT:\n ++\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n ++\t\t\tgoto out;\n ++\t\tdefault:\n ++\t\t\tret = STORE_REF_ERROR_OTHER;\n +\t\t\tgoto out;\n +\t\t}\n \t}\n4: 53705281b6 ! 5: 6a79e7adcc fetch: implement support for atomic reference updates\n @@ builtin/fetch.c: static struct option builtin_fetch_options[] = {\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n -@@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid)\n +@@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head,\n + \t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n + \tstrbuf_addch(&fetch_head->buf, '\\n');\n \n - struct fetch_head {\n - \tFILE *fp;\n -+\tstruct strbuf buf;\n - };\n - \n - static int open_fetch_head(struct fetch_head *fetch_head)\n -@@ builtin/fetch.c: static int open_fetch_head(struct fetch_head *fetch_head)\n - \tif (!write_fetch_head)\n - \t\treturn 0;\n - \n -+\tstrbuf_init(&fetch_head->buf, 0);\n - \tfetch_head->fp = fopen(filename, \"a\");\n - \tif (!fetch_head->fp)\n - \t\treturn error_errno(_(\"cannot open %s\"), filename);\n -@@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid\n - \tif (!write_fetch_head)\n - \t\treturn;\n - \n --\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n --\t\told_oid, merge_status_marker, note);\n --\tfor (i = 0; i < url_len; ++i)\n --\t\tif ('\\n' == url[i])\n --\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n --\t\telse\n --\t\t\tfputc(url[i], fetch_head->fp);\n --\tfputc('\\n', fetch_head->fp);\n +-\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n +-\tstrbuf_reset(&fetch_head->buf);\n +\t/*\n +\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n +\t * any of the reference updates fails. We thus have to write all\n +\t * updates to a buffer first and only commit it as soon as all\n +\t * references have been successfully updated.\n +\t */\n -+\tif (atomic_fetch) {\n -+\t\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n -+\t\t\t old_oid, merge_status_marker, note);\n -+\t\tstrbuf_add(&fetch_head->buf, url, url_len);\n -+\t\tstrbuf_addch(&fetch_head->buf, '\\n');\n -+\t} else {\n -+\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n -+\t\t\told_oid, merge_status_marker, note);\n -+\t\tfor (i = 0; i < url_len; ++i)\n -+\t\t\tif ('\\n' == url[i])\n -+\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n -+\t\t\telse\n -+\t\t\t\tfputc(url[i], fetch_head->fp);\n -+\t\tfputc('\\n', fetch_head->fp);\n ++\tif (!atomic_fetch) {\n ++\t\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n ++\t\tstrbuf_reset(&fetch_head->buf);\n +\t}\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n {\n -\t/* Nothing to commit yet. */\n -+\tif (!write_fetch_head || !atomic_fetch)\n ++\tif (!fetch_head->fp || !atomic_fetch)\n +\t\treturn;\n +\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n }\n \n static void close_fetch_head(struct fetch_head *fetch_head)\n -@@ builtin/fetch.c: static void close_fetch_head(struct fetch_head *fetch_head)\n - \t\treturn;\n - \n - \tfclose(fetch_head->fp);\n -+\tstrbuf_release(&fetch_head->buf);\n - }\n - \n - static const char warn_show_forced_updates[] =\n @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n-- \n2.30.0\n\n"},{"id":"414032","messageId":"c411f30e09ccaf11efce35a8d2a46675e1828849.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"[PATCH v3 3/5] fetch: refactor `s_update_ref` to use common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:25Z","receivedAt":"2021-01-11T11:06:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The cleanup code in `s_update_ref()` is currently duplicated for both\nsuccesful and erroneous exit paths. This commit refactors the function\nto have a shared exit path for both cases to remove the duplication.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 47 +++++++++++++++++++++++++++--------------------\n 1 file changed, 27 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 1252f37493..991771f8eb 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -589,7 +589,7 @@ static int s_update_ref(const char *action,\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n-\tint ret, df_conflict = 0;\n+\tint ret;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -598,30 +598,37 @@ static int s_update_ref(const char *action,\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n \ttransaction = ref_transaction_begin(&err);\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name,\n-\t\t\t\t &ref->new_oid,\n-\t\t\t\t check_old ? &ref->old_oid : NULL,\n-\t\t\t\t 0, msg, &err))\n-\t\tgoto fail;\n-\n-\tret = ref_transaction_commit(transaction, &err);\n-\tif (ret) {\n-\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n-\t\tgoto fail;\n+\tif (!transaction) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n \t}\n \n+\tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n+\t\t\t\t check_old ? &ref->old_oid : NULL,\n+\t\t\t\t 0, msg, &err);\n+\tif (ret) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n+\n+\tswitch (ref_transaction_commit(transaction, &err)) {\n+\tcase 0:\n+\t\tbreak;\n+\tcase TRANSACTION_NAME_CONFLICT:\n+\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n+\t\tgoto out;\n+\tdefault:\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n+\n+out:\n \tref_transaction_free(transaction);\n+\tif (ret)\n+\t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tfree(msg);\n-\treturn 0;\n-fail:\n-\tref_transaction_free(transaction);\n-\terror(\"%s\", err.buf);\n-\tstrbuf_release(&err);\n-\tfree(msg);\n-\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n-\t\t\t : STORE_REF_ERROR_OTHER;\n+\treturn ret;\n }\n \n static int refcol_width = 10;\n-- \n2.30.0\n\n"},{"id":"414033","messageId":"6a79e7adccbe4ee1d20352612a43295b3e7f37a7.1610362744.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"[PATCH v3 5/5] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:05:33Z","receivedAt":"2021-01-11T11:07:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When executing a fetch, then git will currently allocate one reference\ntransaction per reference update and directly commit it. This means that\nfetches are non-atomic: even if some of the reference updates fail,\nothers may still succeed and modify local references.\n\nThis is fine in many scenarios, but this strategy has its downsides.\n\n- The view of remote references may be inconsistent and may show a\n bastardized state of the remote repository.\n\n- Batching together updates may improve performance in certain\n scenarios. While the impact probably isn't as pronounced with loose\n references, the upcoming reftable backend may benefit as it needs to\n write less files in case the update is batched.\n\n- The reference-update hook is currently being executed twice per\n updated reference. While this doesn't matter when there is no such\n hook, we have seen severe performance regressions when doing a\n git-fetch(1) with reference-transaction hook when the remote\n repository has hundreds of thousands of references.\n\nSimilar to `git push --atomic`, this commit thus introduces atomic\nfetches. Instead of allocating one reference transaction per updated\nreference, it causes us to only allocate a single transaction and commit\nit as soon as all updates were received. If locking of any reference\nfails, then we abort the complete transaction and don't update any\nreference, which gives us an all-or-nothing fetch.\n\nNote that this may not completely fix the first of above downsides, as\nthe consistent view also depends on the server-side. If the server\ndoesn't have a consistent view of its own references during the\nreference negotiation phase, then the client would get the same\ninconsistent view the server has. This is a separate problem though and,\nif it actually exists, can be fixed at a later point.\n\nThis commit also changes the way we write FETCH_HEAD in case `--atomic`\nis passed. Instead of writing changes as we go, we need to accumulate\nall changes first and only commit them at the end when we know that all\nreference updates succeeded. Ideally, we'd just do so via a temporary\nfile so that we don't need to carry all updates in-memory. This isn't\ntrivially doable though considering the `--append` mode, where we do not\ntruncate the file but simply append to it. And given that we support\nconcurrent processes appending to FETCH_HEAD at the same time without\nany loss of data, seeding the temporary file with current contents of\nFETCH_HEAD initially and then doing a rename wouldn't work either. So\nthis commit implements the simple strategy of buffering all changes and\nappending them to the file on commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 46 ++++++++-\n t/t5510-fetch.sh | 168 ++++++++++++++++++++++++++++++++\n 3 files changed, 213 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 2bf77b46fd..07783deee3 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -7,6 +7,10 @@\n \texisting contents of `.git/FETCH_HEAD`. Without this\n \toption old data in `.git/FETCH_HEAD` will be overwritten.\n \n+--atomic::\n+\tUse an atomic transaction to update local refs. Either all refs are\n+\tupdated, or on error, no refs are updated.\n+\n --depth=::\n \tLimit fetching to the specified number of commits from the tip of\n \teach remote branch history. If fetching to a 'shallow' repository\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 654c7a7eed..51c9930d06 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -63,6 +63,7 @@ static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n static int fetch_parallel_config = 1;\n+static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -144,6 +145,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"set upstream for git pull/fetch\")),\n \tOPT_BOOL('a', \"append\", &append,\n \t\t N_(\"append to .git/FETCH_HEAD instead of overwriting\")),\n+\tOPT_BOOL(0, \"atomic\", &atomic_fetch,\n+\t\t N_(\"use atomic transaction to update references\")),\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n@@ -970,13 +973,23 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n \t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n \tstrbuf_addch(&fetch_head->buf, '\\n');\n \n-\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n-\tstrbuf_reset(&fetch_head->buf);\n+\t/*\n+\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n+\t * any of the reference updates fails. We thus have to write all\n+\t * updates to a buffer first and only commit it as soon as all\n+\t * references have been successfully updated.\n+\t */\n+\tif (!atomic_fetch) {\n+\t\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n+\t\tstrbuf_reset(&fetch_head->buf);\n+\t}\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n {\n-\t/* Nothing to commit yet. */\n+\tif (!fetch_head->fp || !atomic_fetch)\n+\t\treturn;\n+\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n }\n \n static void close_fetch_head(struct fetch_head *fetch_head)\n@@ -1003,7 +1016,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1029,6 +1043,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (atomic_fetch) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tprepare_format_display(ref_map);\n \n \t/*\n@@ -1105,7 +1127,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1131,6 +1153,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc && transaction) {\n+\t\trc = ref_transaction_commit(transaction, &err);\n+\t\tif (rc) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tif (!rc)\n \t\tcommit_fetch_head(&fetch_head);\n \n@@ -1150,6 +1180,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n+\tstrbuf_release(&err);\n+\tref_transaction_free(transaction);\n \tfree(url);\n \tclose_fetch_head(&fetch_head);\n \treturn rc;\n@@ -1961,6 +1993,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (atomic_fetch)\n+\t\t\tdie(_(\"--atomic can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (stdin_refspecs)\n \t\t\tdie(_(\"--stdin can only be used when fetching \"\n \t\t\t \"from one remote\"));\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2013051a64..109d15be98 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -176,6 +176,174 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success 'fetch --atomic works with a single branch' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch &&\n+\toid=$(git rev-parse atomic-branch) &&\n+\techo \"$oid\" >expected &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse origin/atomic-branch >actual &&\n+\ttest_cmp expected actual &&\n+\ttest $oid = \"$(git -C atomic rev-parse --verify FETCH_HEAD)\"\n+'\n+\n+test_expect_success 'fetch --atomic works with multiple branches' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch-1 &&\n+\tgit branch atomic-branch-2 &&\n+\tgit branch atomic-branch-3 &&\n+\tgit rev-parse refs/heads/atomic-branch-1 refs/heads/atomic-branch-2 refs/heads/atomic-branch-3 >actual &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-branch-1 refs/remotes/origin/atomic-branch-2 refs/remotes/origin/atomic-branch-3 >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic works with mixed branches and tags' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-mixed-branch &&\n+\tgit tag atomic-mixed-tag &&\n+\tgit rev-parse refs/heads/atomic-mixed-branch refs/tags/atomic-mixed-tag >actual &&\n+\n+\tgit -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-mixed-branch refs/tags/atomic-mixed-tag >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic prunes references' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-prune-delete &&\n+\tgit clone . atomic &&\n+\tgit branch --delete atomic-prune-delete &&\n+\tgit branch atomic-prune-create &&\n+\tgit rev-parse refs/heads/atomic-prune-create >actual &&\n+\n+\tgit -C atomic fetch --prune --atomic origin &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-prune-delete &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-prune-create >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-non-ff &&\n+\tgit clone . atomic &&\n+\tgit rev-parse HEAD >actual &&\n+\n+\tgit branch atomic-new-branch &&\n+\tparent_commit=$(git rev-parse atomic-non-ff~) &&\n+\tgit update-ref refs/heads/atomic-non-ff $parent_commit &&\n+\n+\ttest_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic executes a single reference transaction only' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-1 &&\n+\tgit branch atomic-hooks-2 &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\t\tcommitted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\ttest_cmp expected atomic/actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-abort-1 &&\n+\tgit branch atomic-hooks-abort-2 &&\n+\tgit branch atomic-hooks-abort-3 &&\n+\tgit tag atomic-hooks-abort &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\t\taborted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\t\texit 1\n+\tEOF\n+\n+\tgit -C atomic for-each-ref >expected-refs &&\n+\ttest_must_fail git -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic for-each-ref >actual-refs &&\n+\ttest_cmp expected-refs actual-refs &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\toid=$(git rev-parse HEAD) &&\n+\n+\tgit branch atomic-fetch-head-1 &&\n+\tgit -C atomic fetch --atomic origin atomic-fetch-head-1 &&\n+\ttest_line_count = 1 atomic/.git/FETCH_HEAD &&\n+\n+\tgit branch atomic-fetch-head-2 &&\n+\tgit -C atomic fetch --atomic --append origin atomic-fetch-head-2 &&\n+\ttest_line_count = 2 atomic/.git/FETCH_HEAD &&\n+\tcp atomic/.git/FETCH_HEAD expected &&\n+\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\texit 1\n+\tEOF\n+\n+\tgit branch atomic-fetch-head-3 &&\n+\ttest_must_fail git -C atomic fetch --atomic --append origin atomic-fetch-head-3 &&\n+\ttest_cmp expected atomic/.git/FETCH_HEAD\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n-- \n2.30.0\n\n"},{"id":"414034","messageId":"X/wyHxLcYtQb7icq@ncase","threadId":"54954","inReplyTo":"a19762690eb7f9957ac31d73e110f0103aeb2307.1610362744.git.ps@pks.im","subject":"Re: [PATCH v3 2/5] fetch: use strbuf to format FETCH_HEAD updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-11T11:10:23Z","receivedAt":"2021-01-11T11:11:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 11, 2021 at 12:05:20PM +0100, Patrick Steinhardt wrote:\n> This commit refactors `append_fetch_head()` to use a `struct strbuf` for\n> formatting the update which we're about to append to the FETCH_HEAD\n> file. While the refactoring doesn't have much of a benefit right now, it\n> servers as a preparatory step to implement atomic fetches where we need\n> to buffer all updates to FETCH_HEAD and only flush them out if all\n> reference updates succeeded.\n> \n> No change in behaviour is expected from this commit.\n\nForgot to add my\n\nSigned-off-by: Patrick Steinhardt \n\nWill amend in v4.\n\nPatrick\n\n> ---\n> builtin/fetch.c | 16 +++++++++++-----\n> 1 file changed, 11 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 50f0306a92..1252f37493 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -899,6 +899,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n> \n> struct fetch_head {\n> \tFILE *fp;\n> +\tstruct strbuf buf;\n> };\n> \n> static int open_fetch_head(struct fetch_head *fetch_head)\n> @@ -909,6 +910,7 @@ static int open_fetch_head(struct fetch_head *fetch_head)\n> \t\tfetch_head->fp = fopen(filename, \"a\");\n> \t\tif (!fetch_head->fp)\n> \t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n> +\t\tstrbuf_init(&fetch_head->buf, 0);\n> \t} else {\n> \t\tfetch_head->fp = NULL;\n> \t}\n> @@ -941,14 +943,17 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n> \t\t\treturn;\n> \t}\n> \n> -\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> -\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n> +\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n> +\t\t oid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n> \tfor (i = 0; i < url_len; ++i)\n> \t\tif ('\\n' == url[i])\n> -\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> +\t\t\tstrbuf_addstr(&fetch_head->buf, \"\\\\n\");\n> \t\telse\n> -\t\t\tfputc(url[i], fetch_head->fp);\n> -\tfputc('\\n', fetch_head->fp);\n> +\t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n> +\tstrbuf_addch(&fetch_head->buf, '\\n');\n> +\n> +\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n> +\tstrbuf_reset(&fetch_head->buf);\n> }\n> \n> static void commit_fetch_head(struct fetch_head *fetch_head)\n> @@ -962,6 +967,7 @@ static void close_fetch_head(struct fetch_head *fetch_head)\n> \t\treturn;\n> \n> \tfclose(fetch_head->fp);\n> +\tstrbuf_release(&fetch_head->buf);\n> }\n> \n> static const char warn_show_forced_updates[] =\n> -- \n> 2.30.0\n> \n\n\n"},{"id":"414035","messageId":"CAP8UFD1MhbGr7pBbF-TEBpry-0ocUWLgWO2F0GobfeR0b88qdg@mail.gmail.com","threadId":"54954","inReplyTo":"a19762690eb7f9957ac31d73e110f0103aeb2307.1610362744.git.ps@pks.im","subject":"Re: [PATCH v3 2/5] fetch: use strbuf to format FETCH_HEAD updates","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-11T11:17:34Z","receivedAt":"2021-01-11T11:18:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 11, 2021 at 12:05 PM Patrick Steinhardt wrote:\n>\n> This commit refactors `append_fetch_head()` to use a `struct strbuf` for\n> formatting the update which we're about to append to the FETCH_HEAD\n> file. While the refactoring doesn't have much of a benefit right now, it\n> servers as a preparatory step to implement atomic fetches where we need\n\ns/servers/serves/\n\n> to buffer all updates to FETCH_HEAD and only flush them out if all\n> reference updates succeeded.\n"},{"id":"414069","messageId":"xmqqa6tf8eg6.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"X/wnxSn4KjQF0/T+@ncase","subject":"Re: [PATCH v2 1/4] fetch: extract writing to FETCH_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-11T19:24:09Z","receivedAt":"2021-01-11T19:25:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> As an abstraction, it may be better to make the caller pass a\n>> boolean \"is this for merge?\" and keep the knowledge of what exact\n>> string is used for merge_status_marker to this function, instead of\n>> letting the caller passing it as a parameter in the string form.\n>> After all, we never allow anything other than an empty string or a\n>> fixed \"not-for-merge\" string in that place in the file format.\n>> [Nit #2]\n>\n> I think it's even nicer to just pass in `rm->fetch_head_status`\n> directly, which allows us to move below switch into `append_fetch_head`.\n\nOK. That may even be better.\n\nThanks.\n"},{"id":"414071","messageId":"xmqq5z438ddv.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"X/wrgYYcZfYZj+4/@ncase","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-11T19:47:08Z","receivedAt":"2021-01-11T19:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> It would be much more preferrable to see this series go like so:\n>> \n>> [1/4] create append_fetch_head() that writes out to\n>> fetch_head->fp\n>> \n>> [1.5/4] convert append_fetch_head() to ALWAYS accumulate in\n>> fetch_head->buf, and ALWAYS flushes what is accumulated\n>> at the end.\n>\n> This is a change I'm hesitant to make. The thing is that FETCH_HEAD is\n> quite weird as the current design allows different `git fetch --append`\n> processes to write to FETCH_HEAD at the same time.\n\nHmph, do you mean \n\n\tgit fetch --append remote-a & git fetch --append remote-b\n\nor something else [*1*]?\n\nWith the current write-out codepath, there is no guarantee that ...\n\n> > +\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> > +\t\t\told_oid, merge_status_marker, note);\n> > +\t\tfor (i = 0; i < url_len; ++i)\n> > +\t\t\tif ('\\n' == url[i])\n> > +\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> > +\t\t\telse\n> > +\t\t\t\tfputc(url[i], fetch_head->fp);\n> > +\t\tfputc('\\n', fetch_head->fp);\n\n... these stdio operations for a single record would result in a\nsingle atomic write() that would not compete with writes by other\nprocesses. So I wouldn't call \"the current design allows ... at the\nsame time.\" It has been broken for years and nobody complained.\n\n> If we change to\n> always accumulate first and flush once we're done, then we essentially\n> have a change in behaviour as FETCH_HEADs aren't written in an\n> interleaving fashion anymore, but only at the end.\n\nI view it a good thing, actually, for another reason [*2*], but that\nwould take a follow-up fix that is much more involved, so let's not\ngo there (yet). At least buffering a single line's worth of output\nin a buffer and writing it out in a single write() would get us much\ncloser to solving the \"mixed up output\" from multiple processes\nproblem the current code seems to already have.\n\n> Also, if there is any\n> unexpected error in between updating references which causes us to die,\n> then we wouldn't have written the already updated references to\n> FETCH_HEAD now.\n\nThat one may be a valid concern.\n\nThanks.\n\n\n[Footnote]\n\n*1* \"git fetch --all/--multiple\" was strictly serial operation to\n avoid such a mixed-up output problem, but perhaps we weren't\n careful enough when we introduced parallel fetch and broke it?\n\n*2* When fetching from a single remote, the code makes sure that a\n record that is not marked as 'not-for-merge' is listed first by\n reordering the records, but it does not work well when fetching\n from multiple remotes. Queuing all in the buffer before showing\n them would give us an opportunity to sort, but as I said, it\n takes coordination among the processes---instead of each process\n writing to FETCH_HEAD on their own, somebody has to collect the\n records from them, reorder as needed and write them all out.\n\n cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/\n\n \n"},{"id":"414082","messageId":"xmqqo8hv58cb.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"cover.1610362744.git.ps@pks.im","subject":"Re: [PATCH v3 0/5] fetch: implement support for atomic reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T00:04:20Z","receivedAt":"2021-01-12T00:27:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Changes compared to v2:\n>\n> - `append_fetch_head` now takes a `struct object_id *old_oid`\n> instead of a `char *old_oid`.\n>\n> - Handling of the merge status marker was moved into\n> `append_fetch_head`.\n>\n> - I've unified code paths to format the line we're about to write to\n> FETCH_HEAD in atomic and non-atomic cases, which is the new 2/5\n> patch.\n>\n> - We now always initialize `fetch_head->fp` and use it to derive\n> whether anything should be written at all instead of using the\n> global `write_fetch_head` variable.\n>\n> I think this should address all of Junio's feedback. Thanks for your\n> input!\n\nLooking good. I left a few comments on remaining or newly\nintroduced issues, but IIRC all of them were rather minor.\n\nThanks, will replace.\n"},{"id":"414084","messageId":"xmqqsg775abk.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"a19762690eb7f9957ac31d73e110f0103aeb2307.1610362744.git.ps@pks.im","subject":"Re: [PATCH v3 2/5] fetch: use strbuf to format FETCH_HEAD updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-11T23:21:35Z","receivedAt":"2021-01-12T00:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -909,6 +910,7 @@ static int open_fetch_head(struct fetch_head *fetch_head)\n> \t\tfetch_head->fp = fopen(filename, \"a\");\n> \t\tif (!fetch_head->fp)\n> \t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n> +\t\tstrbuf_init(&fetch_head->buf, 0);\n> \t} else {\n> \t\tfetch_head->fp = NULL;\n> \t}\n\nLeaving fetch_head->buf uninitialized is probably OK as the caller\nof open_fetch_head() would (or at least should) immediately barf\nupon seeing an error return?\n\nAh, no. Under dry-run mode, we will return success but leave buf\nuninitializesd. It is safe because we do not use the strbuf when fp\nis NULL. OK.\n\n> @@ -941,14 +943,17 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n> \t\t\treturn;\n> \t}\n> \n> -\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> -\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n> +\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n> +\t\t oid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n> \tfor (i = 0; i < url_len; ++i)\n> \t\tif ('\\n' == url[i])\n> -\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> +\t\t\tstrbuf_addstr(&fetch_head->buf, \"\\\\n\");\n> \t\telse\n> -\t\t\tfputc(url[i], fetch_head->fp);\n> -\tfputc('\\n', fetch_head->fp);\n> +\t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n> +\tstrbuf_addch(&fetch_head->buf, '\\n');\n> +\n> +\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n> +\tstrbuf_reset(&fetch_head->buf);\n\nThis gets us closer to fixing the \"one record can be written out\nwith multiple write(2) calls, allowing parallel fetches to corrupt\nFETCH_HEAD file by intermixed records\" problem (even though this\nchange alone would not solve it---for that we need to do write\nourselves, not letting stdio do the flushing).\n\n> }\n> \n> static void commit_fetch_head(struct fetch_head *fetch_head)\n> @@ -962,6 +967,7 @@ static void close_fetch_head(struct fetch_head *fetch_head)\n> \t\treturn;\n> \n> \tfclose(fetch_head->fp);\n> +\tstrbuf_release(&fetch_head->buf);\n> }\n> \n> static const char warn_show_forced_updates[] =\n"},{"id":"414085","messageId":"xmqqwnwj5aku.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"61dc19a1cab5b03d07c6635496761108d089eb23.1610362744.git.ps@pks.im","subject":"Re: [PATCH v3 1/5] fetch: extract writing to FETCH_HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-11T23:16:01Z","receivedAt":"2021-01-12T00:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +\tswitch (fetch_head_status) {\n> +\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n\nIn our codebase, \"switch\", \"case\" and \"default:\" all begin at the\nsame column (see the original this was taken from).\n\nThanks.\n"},{"id":"414133","messageId":"X/2UkKS0AhT6jLXj@ncase","threadId":"54954","inReplyTo":"xmqq5z438ddv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:22:40Z","receivedAt":"2021-01-12T12:23:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> It would be much more preferrable to see this series go like so:\n> >> \n> >> [1/4] create append_fetch_head() that writes out to\n> >> fetch_head->fp\n> >> \n> >> [1.5/4] convert append_fetch_head() to ALWAYS accumulate in\n> >> fetch_head->buf, and ALWAYS flushes what is accumulated\n> >> at the end.\n> >\n> > This is a change I'm hesitant to make. The thing is that FETCH_HEAD is\n> > quite weird as the current design allows different `git fetch --append`\n> > processes to write to FETCH_HEAD at the same time.\n> \n> Hmph, do you mean \n> \n> \tgit fetch --append remote-a & git fetch --append remote-b\n> \n> or something else [*1*]?\n\nThat's what I mean.\n\n> With the current write-out codepath, there is no guarantee that ...\n> \n> > > +\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n> > > +\t\t\told_oid, merge_status_marker, note);\n> > > +\t\tfor (i = 0; i < url_len; ++i)\n> > > +\t\t\tif ('\\n' == url[i])\n> > > +\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n> > > +\t\t\telse\n> > > +\t\t\t\tfputc(url[i], fetch_head->fp);\n> > > +\t\tfputc('\\n', fetch_head->fp);\n> \n> ... these stdio operations for a single record would result in a\n> single atomic write() that would not compete with writes by other\n> processes. So I wouldn't call \"the current design allows ... at the\n> same time.\" It has been broken for years and nobody complained.\n\nFair enough. I somehow blindly assumed that `git fetch --all`, which\ndoes invoke `git fetch --append`, did perform the fetch in parallel. If\nthat's not the case: all the better.\n\n> > If we change to\n> > always accumulate first and flush once we're done, then we essentially\n> > have a change in behaviour as FETCH_HEADs aren't written in an\n> > interleaving fashion anymore, but only at the end.\n> \n> I view it a good thing, actually, for another reason [*2*], but that\n> would take a follow-up fix that is much more involved, so let's not\n> go there (yet). At least buffering a single line's worth of output\n> in a buffer and writing it out in a single write() would get us much\n> closer to solving the \"mixed up output\" from multiple processes\n> problem the current code seems to already have.\n\nThe buffering of a single line is exactly what we're doing now in the\nnon-atomic case. Previously there had been multiple writes, now we only\ncall `strbuf_write()` once on the buffer for each reference.\n\n> > Also, if there is any\n> > unexpected error in between updating references which causes us to die,\n> > then we wouldn't have written the already updated references to\n> > FETCH_HEAD now.\n> \n> That one may be a valid concern.\n> \n> Thanks.\n\nJust to be explicit: are you fine with changes in this patch as-is? They\ndon't solve concurrent writes to FETCH_HEAD, but they should make it\neasier to solve that if we ever need to.\n\nPatrick\n\n> \n> [Footnote]\n> \n> *1* \"git fetch --all/--multiple\" was strictly serial operation to\n> avoid such a mixed-up output problem, but perhaps we weren't\n> careful enough when we introduced parallel fetch and broke it?\n> \n> *2* When fetching from a single remote, the code makes sure that a\n> record that is not marked as 'not-for-merge' is listed first by\n> reordering the records, but it does not work well when fetching\n> from multiple remotes. Queuing all in the buffer before showing\n> them would give us an opportunity to sort, but as I said, it\n> takes coordination among the processes---instead of each process\n> writing to FETCH_HEAD on their own, somebody has to collect the\n> records from them, reorder as needed and write them all out.\n> \n> cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/\n> \n> \n"},{"id":"414141","messageId":"ba6908aa8c2cae55e589a278f8d6114a16bf3840.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610454262.git.ps@pks.im","subject":"[PATCH v4 3/5] fetch: refactor `s_update_ref` to use common exit path","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:43Z","receivedAt":"2021-01-12T12:28:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The cleanup code in `s_update_ref()` is currently duplicated for both\nsuccesful and erroneous exit paths. This commit refactors the function\nto have a shared exit path for both cases to remove the duplication.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 47 +++++++++++++++++++++++++++--------------------\n 1 file changed, 27 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e317e828cd..b24a9e09a4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -589,7 +589,7 @@ static int s_update_ref(const char *action,\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n-\tint ret, df_conflict = 0;\n+\tint ret;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -598,30 +598,37 @@ static int s_update_ref(const char *action,\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n \ttransaction = ref_transaction_begin(&err);\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name,\n-\t\t\t\t &ref->new_oid,\n-\t\t\t\t check_old ? &ref->old_oid : NULL,\n-\t\t\t\t 0, msg, &err))\n-\t\tgoto fail;\n-\n-\tret = ref_transaction_commit(transaction, &err);\n-\tif (ret) {\n-\t\tdf_conflict = (ret == TRANSACTION_NAME_CONFLICT);\n-\t\tgoto fail;\n+\tif (!transaction) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n \t}\n \n+\tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n+\t\t\t\t check_old ? &ref->old_oid : NULL,\n+\t\t\t\t 0, msg, &err);\n+\tif (ret) {\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n+\n+\tswitch (ref_transaction_commit(transaction, &err)) {\n+\tcase 0:\n+\t\tbreak;\n+\tcase TRANSACTION_NAME_CONFLICT:\n+\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n+\t\tgoto out;\n+\tdefault:\n+\t\tret = STORE_REF_ERROR_OTHER;\n+\t\tgoto out;\n+\t}\n+\n+out:\n \tref_transaction_free(transaction);\n+\tif (ret)\n+\t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \tfree(msg);\n-\treturn 0;\n-fail:\n-\tref_transaction_free(transaction);\n-\terror(\"%s\", err.buf);\n-\tstrbuf_release(&err);\n-\tfree(msg);\n-\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n-\t\t\t : STORE_REF_ERROR_OTHER;\n+\treturn ret;\n }\n \n static int refcol_width = 10;\n-- \n2.30.0\n\n"},{"id":"414142","messageId":"9fcc8b54dec06288581497666eaefac7ff0bb736.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610454262.git.ps@pks.im","subject":"[PATCH v4 1/5] fetch: extract writing to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:35Z","receivedAt":"2021-01-12T12:28:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When performing a fetch with the default `--write-fetch-head` option, we\nwrite all updated references to FETCH_HEAD while the updates are\nperformed. Given that updates are not performed atomically, it means\nthat we we write to FETCH_HEAD even if some or all of the reference\nupdates fail.\n\nGiven that we simply update FETCH_HEAD ad-hoc with each reference, the\nlogic is completely contained in `store_update_refs` and thus quite hard\nto extend. This can already be seen by the way we skip writing to the\nFETCH_HEAD: instead of having a conditional which simply skips writing,\nwe instead open \"/dev/null\" and needlessly write all updates there.\n\nWe are about to extend git-fetch(1) to accept an `--atomic` flag which\nwill make the fetch an all-or-nothing operation with regards to the\nreference updates. This will also require us to make the updates to\nFETCH_HEAD an all-or-nothing operation, but as explained doing so is not\neasy with the current layout. This commit thus refactors the wa we write\nto FETCH_HEAD and pulls out the logic to open, append to, commit and\nclose the file. While this may seem rather over-the top at first,\npulling out this logic will make it a lot easier to update the code in a\nsubsequent commit. It also allows us to easily skip writing completely\nin case `--no-write-fetch-head` was passed.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 108 +++++++++++++++++++++++++++++++++++-------------\n remote.h | 2 +-\n 2 files changed, 80 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ecf8537605..2dd5fcb652 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -897,6 +897,73 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \treturn 0;\n }\n \n+struct fetch_head {\n+\tFILE *fp;\n+};\n+\n+static int open_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tconst char *filename = git_path_fetch_head(the_repository);\n+\n+\tif (write_fetch_head) {\n+\t\tfetch_head->fp = fopen(filename, \"a\");\n+\t\tif (!fetch_head->fp)\n+\t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\t} else {\n+\t\tfetch_head->fp = NULL;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void append_fetch_head(struct fetch_head *fetch_head,\n+\t\t\t const struct object_id *old_oid,\n+\t\t\t enum fetch_head_status fetch_head_status,\n+\t\t\t const char *note,\n+\t\t\t const char *url, size_t url_len)\n+{\n+\tchar old_oid_hex[GIT_MAX_HEXSZ + 1];\n+\tconst char *merge_status_marker;\n+\tsize_t i;\n+\n+\tif (!fetch_head->fp)\n+\t\treturn;\n+\n+\tswitch (fetch_head_status) {\n+\tcase FETCH_HEAD_NOT_FOR_MERGE:\n+\t\tmerge_status_marker = \"not-for-merge\";\n+\t\tbreak;\n+\tcase FETCH_HEAD_MERGE:\n+\t\tmerge_status_marker = \"\";\n+\t\tbreak;\n+\tdefault:\n+\t\t/* do not write anything to FETCH_HEAD */\n+\t\treturn;\n+\t}\n+\n+\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n+\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n+\tfor (i = 0; i < url_len; ++i)\n+\t\tif ('\\n' == url[i])\n+\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\telse\n+\t\t\tfputc(url[i], fetch_head->fp);\n+\tfputc('\\n', fetch_head->fp);\n+}\n+\n+static void commit_fetch_head(struct fetch_head *fetch_head)\n+{\n+\t/* Nothing to commit yet. */\n+}\n+\n+static void close_fetch_head(struct fetch_head *fetch_head)\n+{\n+\tif (!fetch_head->fp)\n+\t\treturn;\n+\n+\tfclose(fetch_head->fp);\n+}\n+\n static const char warn_show_forced_updates[] =\n N_(\"Fetch normally indicates which branches had a forced update,\\n\"\n \"but that check has been disabled. To re-enable, use '--show-forced-updates'\\n\"\n@@ -909,22 +976,19 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t int connectivity_checked, struct ref *ref_map)\n {\n-\tFILE *fp;\n+\tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n-\tconst char *filename = (!write_fetch_head\n-\t\t\t\t? \"/dev/null\"\n-\t\t\t\t: git_path_fetch_head(the_repository));\n \tint want_status;\n \tint summary_width = transport_summary_width(ref_map);\n \n-\tfp = fopen(filename, \"a\");\n-\tif (!fp)\n-\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\trc = open_fetch_head(&fetch_head);\n+\tif (rc)\n+\t\treturn -1;\n \n \tif (raw_url)\n \t\turl = transport_anonymize_url(raw_url);\n@@ -953,7 +1017,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t want_status++) {\n \t\tfor (rm = ref_map; rm; rm = rm->next) {\n \t\t\tstruct ref *ref = NULL;\n-\t\t\tconst char *merge_status_marker = \"\";\n \n \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n \t\t\t\tif (want_status == FETCH_HEAD_MERGE)\n@@ -1011,26 +1074,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t\tstrbuf_addf(¬e, \"%s \", kind);\n \t\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n \t\t\t}\n-\t\t\tswitch (rm->fetch_head_status) {\n-\t\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n-\t\t\t\tmerge_status_marker = \"not-for-merge\";\n-\t\t\t\t/* fall-through */\n-\t\t\tcase FETCH_HEAD_MERGE:\n-\t\t\t\tfprintf(fp, \"%s\\t%s\\t%s\",\n-\t\t\t\t\toid_to_hex(&rm->old_oid),\n-\t\t\t\t\tmerge_status_marker,\n-\t\t\t\t\tnote.buf);\n-\t\t\t\tfor (i = 0; i < url_len; ++i)\n-\t\t\t\t\tif ('\\n' == url[i])\n-\t\t\t\t\t\tfputs(\"\\\\n\", fp);\n-\t\t\t\t\telse\n-\t\t\t\t\t\tfputc(url[i], fp);\n-\t\t\t\tfputc('\\n', fp);\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\t/* do not write anything to FETCH_HEAD */\n-\t\t\t\tbreak;\n-\t\t\t}\n+\n+\t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\n+\t\t\t\t\t rm->fetch_head_status,\n+\t\t\t\t\t note.buf, url, url_len);\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n@@ -1060,6 +1107,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc)\n+\t\tcommit_fetch_head(&fetch_head);\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n@@ -1077,7 +1127,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n abort:\n \tstrbuf_release(¬e);\n \tfree(url);\n-\tfclose(fp);\n+\tclose_fetch_head(&fetch_head);\n \treturn rc;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 3211abdf05..aad1a0f080 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -134,7 +134,7 @@ struct ref {\n \t * should be 0, so that xcalloc'd structures get it\n \t * by default.\n \t */\n-\tenum {\n+\tenum fetch_head_status {\n \t\tFETCH_HEAD_MERGE = -1,\n \t\tFETCH_HEAD_NOT_FOR_MERGE = 0,\n \t\tFETCH_HEAD_IGNORE = 1\n-- \n2.30.0\n\n"},{"id":"414143","messageId":"fb8542270a66873c9738a94ac178a06f1c2121d7.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610454262.git.ps@pks.im","subject":"[PATCH v4 2/5] fetch: use strbuf to format FETCH_HEAD updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:39Z","receivedAt":"2021-01-12T12:28:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"This commit refactors `append_fetch_head()` to use a `struct strbuf` for\nformatting the update which we're about to append to the FETCH_HEAD\nfile. While the refactoring doesn't have much of a benefit right now, it\nserves as a preparatory step to implement atomic fetches where we need\nto buffer all updates to FETCH_HEAD and only flush them out if all\nreference updates succeeded.\n\nNo change in behaviour is expected from this commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 2dd5fcb652..e317e828cd 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -899,6 +899,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \n struct fetch_head {\n \tFILE *fp;\n+\tstruct strbuf buf;\n };\n \n static int open_fetch_head(struct fetch_head *fetch_head)\n@@ -909,6 +910,7 @@ static int open_fetch_head(struct fetch_head *fetch_head)\n \t\tfetch_head->fp = fopen(filename, \"a\");\n \t\tif (!fetch_head->fp)\n \t\t\treturn error_errno(_(\"cannot open %s\"), filename);\n+\t\tstrbuf_init(&fetch_head->buf, 0);\n \t} else {\n \t\tfetch_head->fp = NULL;\n \t}\n@@ -941,14 +943,17 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n \t\treturn;\n \t}\n \n-\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n-\t\toid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n+\tstrbuf_addf(&fetch_head->buf, \"%s\\t%s\\t%s\",\n+\t\t oid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note);\n \tfor (i = 0; i < url_len; ++i)\n \t\tif ('\\n' == url[i])\n-\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n+\t\t\tstrbuf_addstr(&fetch_head->buf, \"\\\\n\");\n \t\telse\n-\t\t\tfputc(url[i], fetch_head->fp);\n-\tfputc('\\n', fetch_head->fp);\n+\t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n+\tstrbuf_addch(&fetch_head->buf, '\\n');\n+\n+\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n+\tstrbuf_reset(&fetch_head->buf);\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n@@ -962,6 +967,7 @@ static void close_fetch_head(struct fetch_head *fetch_head)\n \t\treturn;\n \n \tfclose(fetch_head->fp);\n+\tstrbuf_release(&fetch_head->buf);\n }\n \n static const char warn_show_forced_updates[] =\n-- \n2.30.0\n\n"},{"id":"414145","messageId":"cover.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610027375.git.ps@pks.im","subject":"[PATCH v4 0/5] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:29Z","receivedAt":"2021-01-12T12:29:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of my patch series to implement support for\natomic reference updates for git-fetch(1). It's similar to `git push\n--atomic`, only that it applies to the local side. That is the fetch\nwill either succeed and update all remote references or it will fail and\nupdate none.\n\nChanges compared to v3:\n\n - Fixed indentation of the switch statement in 1/5.\n\n - Added my missing SOB to 2/5 and fixed a typo in the commit\n message.\n\nPlease see the attached range-diff for more details.\n\nPatrick\n\n\nPatrick Steinhardt (5):\n fetch: extract writing to FETCH_HEAD\n fetch: use strbuf to format FETCH_HEAD updates\n fetch: refactor `s_update_ref` to use common exit path\n fetch: allow passing a transaction to `s_update_ref()`\n fetch: implement support for atomic reference updates\n\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 228 +++++++++++++++++++++++---------\n remote.h | 2 +-\n t/t5510-fetch.sh | 168 +++++++++++++++++++++++\n 4 files changed, 342 insertions(+), 60 deletions(-)\n\nRange-diff against v3:\n1: 61dc19a1ca ! 1: 9fcc8b54de fetch: extract writing to FETCH_HEAD\n @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid\n +\t\treturn;\n +\n +\tswitch (fetch_head_status) {\n -+\t\tcase FETCH_HEAD_NOT_FOR_MERGE:\n -+\t\t\tmerge_status_marker = \"not-for-merge\";\n -+\t\t\tbreak;\n -+\t\tcase FETCH_HEAD_MERGE:\n -+\t\t\tmerge_status_marker = \"\";\n -+\t\t\tbreak;\n -+\t\tdefault:\n -+\t\t\t/* do not write anything to FETCH_HEAD */\n -+\t\t\treturn;\n ++\tcase FETCH_HEAD_NOT_FOR_MERGE:\n ++\t\tmerge_status_marker = \"not-for-merge\";\n ++\t\tbreak;\n ++\tcase FETCH_HEAD_MERGE:\n ++\t\tmerge_status_marker = \"\";\n ++\t\tbreak;\n ++\tdefault:\n ++\t\t/* do not write anything to FETCH_HEAD */\n ++\t\treturn;\n +\t}\n +\n +\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n2: a19762690e ! 2: fb8542270a fetch: use strbuf to format FETCH_HEAD updates\n @@ Commit message\n This commit refactors `append_fetch_head()` to use a `struct strbuf` for\n formatting the update which we're about to append to the FETCH_HEAD\n file. While the refactoring doesn't have much of a benefit right now, it\n - servers as a preparatory step to implement atomic fetches where we need\n + serves as a preparatory step to implement atomic fetches where we need\n to buffer all updates to FETCH_HEAD and only flush them out if all\n reference updates succeeded.\n \n No change in behaviour is expected from this commit.\n \n + Signed-off-by: Patrick Steinhardt \n +\n ## builtin/fetch.c ##\n @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \n @@ builtin/fetch.c: static int open_fetch_head(struct fetch_head *fetch_head)\n \t\tfetch_head->fp = NULL;\n \t}\n @@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head,\n - \t\t\treturn;\n + \t\treturn;\n \t}\n \n -\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n3: c411f30e09 = 3: ba6908aa8c fetch: refactor `s_update_ref` to use common exit path\n4: 865d357ba7 = 4: 7f820f6f83 fetch: allow passing a transaction to `s_update_ref()`\n5: 6a79e7adcc = 5: 0b57d7a651 fetch: implement support for atomic reference updates\n-- \n2.30.0\n\n"},{"id":"414147","messageId":"7f820f6f83d67b3f604f234af3ca7e1635bd76b0.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610454262.git.ps@pks.im","subject":"[PATCH v4 4/5] fetch: allow passing a transaction to `s_update_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:48Z","receivedAt":"2021-01-12T12:29:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The handling of ref updates is completely handled by `s_update_ref()`,\nwhich will manage the complete lifecycle of the reference transaction.\nThis is fine right now given that git-fetch(1) does not support atomic\nfetches, so each reference gets its own transaction. It is quite\ninflexible though, as `s_update_ref()` only knows about a single\nreference update at a time, so it doesn't allow us to alter the\nstrategy.\n\nThis commit prepares `s_update_ref()` and its only caller\n`update_local_ref()` to allow passing an external transaction. If none\nis given, then the existing behaviour is triggered which creates a new\ntransaction and directly commits it. Otherwise, if the caller provides a\ntransaction, then we only queue the update but don't commit it. This\noptionally allows the caller to manage when a transaction will be\ncommitted.\n\nGiven that `update_local_ref()` is always called with a `NULL`\ntransaction for now, no change in behaviour is expected from this\nchange.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 51 ++++++++++++++++++++++++++++++-------------------\n 1 file changed, 31 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b24a9e09a4..cada732325 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -583,11 +583,12 @@ static struct ref *get_ref_map(struct remote *remote,\n \n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n+\t\t\tstruct ref_transaction *transaction,\n \t\t\tint check_old)\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n+\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -597,10 +598,17 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\ttransaction = ref_transaction_begin(&err);\n+\t/*\n+\t * If no transaction was passed to us, we manage the transaction\n+\t * ourselves. Otherwise, we trust the caller to handle the transaction\n+\t * lifecycle.\n+\t */\n \tif (!transaction) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\tret = STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n@@ -611,19 +619,21 @@ static int s_update_ref(const char *action,\n \t\tgoto out;\n \t}\n \n-\tswitch (ref_transaction_commit(transaction, &err)) {\n-\tcase 0:\n-\t\tbreak;\n-\tcase TRANSACTION_NAME_CONFLICT:\n-\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n-\t\tgoto out;\n-\tdefault:\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n+\tif (our_transaction) {\n+\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n+\t\tcase 0:\n+\t\t\tbreak;\n+\t\tcase TRANSACTION_NAME_CONFLICT:\n+\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n+\t\t\tgoto out;\n+\t\tdefault:\n+\t\t\tret = STORE_REF_ERROR_OTHER;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n out:\n-\tref_transaction_free(transaction);\n+\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n@@ -766,6 +776,7 @@ static void format_display(struct strbuf *display, char code,\n }\n \n static int update_local_ref(struct ref *ref,\n+\t\t\t struct ref_transaction *transaction,\n \t\t\t const char *remote,\n \t\t\t const struct ref *remote_ref,\n \t\t\t struct strbuf *display,\n@@ -806,7 +817,7 @@ static int update_local_ref(struct ref *ref,\n \t starts_with(ref->name, \"refs/tags/\")) {\n \t\tif (force || ref->force) {\n \t\t\tint r;\n-\t\t\tr = s_update_ref(\"updating tag\", ref, 0);\n+\t\t\tr = s_update_ref(\"updating tag\", ref, transaction, 0);\n \t\t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t\t remote, pretty_ref, summary_width);\n@@ -843,7 +854,7 @@ static int update_local_ref(struct ref *ref,\n \t\t\twhat = _(\"[new ref]\");\n \t\t}\n \n-\t\tr = s_update_ref(msg, ref, 0);\n+\t\tr = s_update_ref(msg, ref, transaction, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -865,7 +876,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"..\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"fast-forward\", ref, 1);\n+\t\tr = s_update_ref(\"fast-forward\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : NULL,\n \t\t\t remote, pretty_ref, summary_width);\n@@ -877,7 +888,7 @@ static int update_local_ref(struct ref *ref,\n \t\tstrbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);\n \t\tstrbuf_addstr(&quickref, \"...\");\n \t\tstrbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);\n-\t\tr = s_update_ref(\"forced-update\", ref, 1);\n+\t\tr = s_update_ref(\"forced-update\", ref, transaction, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t r ? _(\"unable to update local ref\") : _(\"forced update\"),\n \t\t\t remote, pretty_ref, summary_width);\n@@ -1094,8 +1105,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, what, rm, ¬e,\n-\t\t\t\t\t\t summary_width);\n+\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n-- \n2.30.0\n\n"},{"id":"414148","messageId":"0b57d7a6518d398160133ed8bed7286dd59d303f.1610454262.git.ps@pks.im","threadId":"54954","inReplyTo":"cover.1610454262.git.ps@pks.im","subject":"[PATCH v4 5/5] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:27:52Z","receivedAt":"2021-01-12T12:29:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When executing a fetch, then git will currently allocate one reference\ntransaction per reference update and directly commit it. This means that\nfetches are non-atomic: even if some of the reference updates fail,\nothers may still succeed and modify local references.\n\nThis is fine in many scenarios, but this strategy has its downsides.\n\n- The view of remote references may be inconsistent and may show a\n bastardized state of the remote repository.\n\n- Batching together updates may improve performance in certain\n scenarios. While the impact probably isn't as pronounced with loose\n references, the upcoming reftable backend may benefit as it needs to\n write less files in case the update is batched.\n\n- The reference-update hook is currently being executed twice per\n updated reference. While this doesn't matter when there is no such\n hook, we have seen severe performance regressions when doing a\n git-fetch(1) with reference-transaction hook when the remote\n repository has hundreds of thousands of references.\n\nSimilar to `git push --atomic`, this commit thus introduces atomic\nfetches. Instead of allocating one reference transaction per updated\nreference, it causes us to only allocate a single transaction and commit\nit as soon as all updates were received. If locking of any reference\nfails, then we abort the complete transaction and don't update any\nreference, which gives us an all-or-nothing fetch.\n\nNote that this may not completely fix the first of above downsides, as\nthe consistent view also depends on the server-side. If the server\ndoesn't have a consistent view of its own references during the\nreference negotiation phase, then the client would get the same\ninconsistent view the server has. This is a separate problem though and,\nif it actually exists, can be fixed at a later point.\n\nThis commit also changes the way we write FETCH_HEAD in case `--atomic`\nis passed. Instead of writing changes as we go, we need to accumulate\nall changes first and only commit them at the end when we know that all\nreference updates succeeded. Ideally, we'd just do so via a temporary\nfile so that we don't need to carry all updates in-memory. This isn't\ntrivially doable though considering the `--append` mode, where we do not\ntruncate the file but simply append to it. And given that we support\nconcurrent processes appending to FETCH_HEAD at the same time without\nany loss of data, seeding the temporary file with current contents of\nFETCH_HEAD initially and then doing a rename wouldn't work either. So\nthis commit implements the simple strategy of buffering all changes and\nappending them to the file on commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/fetch-options.txt | 4 +\n builtin/fetch.c | 46 ++++++++-\n t/t5510-fetch.sh | 168 ++++++++++++++++++++++++++++++++\n 3 files changed, 213 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 2bf77b46fd..07783deee3 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -7,6 +7,10 @@\n \texisting contents of `.git/FETCH_HEAD`. Without this\n \toption old data in `.git/FETCH_HEAD` will be overwritten.\n \n+--atomic::\n+\tUse an atomic transaction to update local refs. Either all refs are\n+\tupdated, or on error, no refs are updated.\n+\n --depth=::\n \tLimit fetching to the specified number of commits from the tip of\n \teach remote branch history. If fetching to a 'shallow' repository\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex cada732325..91f3d20696 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -63,6 +63,7 @@ static int enable_auto_gc = 1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_jobs = -1, submodule_fetch_jobs_config = -1;\n static int fetch_parallel_config = 1;\n+static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n static const char *deepen_since;\n@@ -144,6 +145,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"set upstream for git pull/fetch\")),\n \tOPT_BOOL('a', \"append\", &append,\n \t\t N_(\"append to .git/FETCH_HEAD instead of overwriting\")),\n+\tOPT_BOOL(0, \"atomic\", &atomic_fetch,\n+\t\t N_(\"use atomic transaction to update references\")),\n \tOPT_STRING(0, \"upload-pack\", &upload_pack, N_(\"path\"),\n \t\t N_(\"path to upload pack on remote end\")),\n \tOPT__FORCE(&force, N_(\"force overwrite of local reference\"), 0),\n@@ -970,13 +973,23 @@ static void append_fetch_head(struct fetch_head *fetch_head,\n \t\t\tstrbuf_addch(&fetch_head->buf, url[i]);\n \tstrbuf_addch(&fetch_head->buf, '\\n');\n \n-\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n-\tstrbuf_reset(&fetch_head->buf);\n+\t/*\n+\t * When using an atomic fetch, we do not want to update FETCH_HEAD if\n+\t * any of the reference updates fails. We thus have to write all\n+\t * updates to a buffer first and only commit it as soon as all\n+\t * references have been successfully updated.\n+\t */\n+\tif (!atomic_fetch) {\n+\t\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n+\t\tstrbuf_reset(&fetch_head->buf);\n+\t}\n }\n \n static void commit_fetch_head(struct fetch_head *fetch_head)\n {\n-\t/* Nothing to commit yet. */\n+\tif (!fetch_head->fp || !atomic_fetch)\n+\t\treturn;\n+\tstrbuf_write(&fetch_head->buf, fetch_head->fp);\n }\n \n static void close_fetch_head(struct fetch_head *fetch_head)\n@@ -1003,7 +1016,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n \tint url_len, i, rc = 0;\n-\tstruct strbuf note = STRBUF_INIT;\n+\tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n@@ -1029,6 +1043,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (atomic_fetch) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tprepare_format_display(ref_map);\n \n \t/*\n@@ -1105,7 +1127,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(¬e);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, NULL, what,\n+\t\t\t\trc |= update_local_ref(ref, transaction, what,\n \t\t\t\t\t\t rm, ¬e, summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n@@ -1131,6 +1153,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n+\tif (!rc && transaction) {\n+\t\trc = ref_transaction_commit(transaction, &err);\n+\t\tif (rc) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto abort;\n+\t\t}\n+\t}\n+\n \tif (!rc)\n \t\tcommit_fetch_head(&fetch_head);\n \n@@ -1150,6 +1180,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n abort:\n \tstrbuf_release(¬e);\n+\tstrbuf_release(&err);\n+\tref_transaction_free(transaction);\n \tfree(url);\n \tclose_fetch_head(&fetch_head);\n \treturn rc;\n@@ -1961,6 +1993,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (atomic_fetch)\n+\t\t\tdie(_(\"--atomic can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (stdin_refspecs)\n \t\t\tdie(_(\"--stdin can only be used when fetching \"\n \t\t\t \"from one remote\"));\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 2013051a64..109d15be98 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -176,6 +176,174 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success 'fetch --atomic works with a single branch' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch &&\n+\toid=$(git rev-parse atomic-branch) &&\n+\techo \"$oid\" >expected &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse origin/atomic-branch >actual &&\n+\ttest_cmp expected actual &&\n+\ttest $oid = \"$(git -C atomic rev-parse --verify FETCH_HEAD)\"\n+'\n+\n+test_expect_success 'fetch --atomic works with multiple branches' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-branch-1 &&\n+\tgit branch atomic-branch-2 &&\n+\tgit branch atomic-branch-3 &&\n+\tgit rev-parse refs/heads/atomic-branch-1 refs/heads/atomic-branch-2 refs/heads/atomic-branch-3 >actual &&\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-branch-1 refs/remotes/origin/atomic-branch-2 refs/remotes/origin/atomic-branch-3 >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic works with mixed branches and tags' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-mixed-branch &&\n+\tgit tag atomic-mixed-tag &&\n+\tgit rev-parse refs/heads/atomic-mixed-branch refs/tags/atomic-mixed-tag >actual &&\n+\n+\tgit -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-mixed-branch refs/tags/atomic-mixed-tag >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic prunes references' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-prune-delete &&\n+\tgit clone . atomic &&\n+\tgit branch --delete atomic-prune-delete &&\n+\tgit branch atomic-prune-create &&\n+\tgit rev-parse refs/heads/atomic-prune-create >actual &&\n+\n+\tgit -C atomic fetch --prune --atomic origin &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-prune-delete &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-prune-create >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts with non-fast-forward update' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch atomic-non-ff &&\n+\tgit clone . atomic &&\n+\tgit rev-parse HEAD >actual &&\n+\n+\tgit branch atomic-new-branch &&\n+\tparent_commit=$(git rev-parse atomic-non-ff~) &&\n+\tgit update-ref refs/heads/atomic-non-ff $parent_commit &&\n+\n+\ttest_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* &&\n+\ttest_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch &&\n+\tgit -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected &&\n+\ttest_cmp expected actual &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic executes a single reference transaction only' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-1 &&\n+\tgit branch atomic-hooks-2 &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\t\tcommitted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C atomic fetch --atomic origin &&\n+\ttest_cmp expected atomic/actual\n+'\n+\n+test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\tgit branch atomic-hooks-abort-1 &&\n+\tgit branch atomic-hooks-abort-2 &&\n+\tgit branch atomic-hooks-abort-3 &&\n+\tgit tag atomic-hooks-abort &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\t\taborted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3\n+\t\t$ZERO_OID $head_oid refs/tags/atomic-hooks-abort\n+\tEOF\n+\n+\trm -f atomic/actual &&\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\t\texit 1\n+\tEOF\n+\n+\tgit -C atomic for-each-ref >expected-refs &&\n+\ttest_must_fail git -C atomic fetch --tags --atomic origin &&\n+\tgit -C atomic for-each-ref >actual-refs &&\n+\ttest_cmp expected-refs actual-refs &&\n+\ttest_must_be_empty atomic/.git/FETCH_HEAD\n+'\n+\n+test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit clone . atomic &&\n+\toid=$(git rev-parse HEAD) &&\n+\n+\tgit branch atomic-fetch-head-1 &&\n+\tgit -C atomic fetch --atomic origin atomic-fetch-head-1 &&\n+\ttest_line_count = 1 atomic/.git/FETCH_HEAD &&\n+\n+\tgit branch atomic-fetch-head-2 &&\n+\tgit -C atomic fetch --atomic --append origin atomic-fetch-head-2 &&\n+\ttest_line_count = 2 atomic/.git/FETCH_HEAD &&\n+\tcp atomic/.git/FETCH_HEAD expected &&\n+\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\texit 1\n+\tEOF\n+\n+\tgit branch atomic-fetch-head-3 &&\n+\ttest_must_fail git -C atomic fetch --atomic --append origin atomic-fetch-head-3 &&\n+\ttest_cmp expected atomic/.git/FETCH_HEAD\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n-- \n2.30.0\n\n"},{"id":"414149","messageId":"X/2WDt5Kb6rZNXW5@ncase","threadId":"54954","inReplyTo":"X/2UkKS0AhT6jLXj@ncase","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-01-12T12:29:02Z","receivedAt":"2021-01-12T12:30:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 12, 2021 at 01:22:40PM +0100, Patrick Steinhardt wrote:\n> On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n[snip]\n> > > If we change to\n> > > always accumulate first and flush once we're done, then we essentially\n> > > have a change in behaviour as FETCH_HEADs aren't written in an\n> > > interleaving fashion anymore, but only at the end.\n> > \n> > I view it a good thing, actually, for another reason [*2*], but that\n> > would take a follow-up fix that is much more involved, so let's not\n> > go there (yet). At least buffering a single line's worth of output\n> > in a buffer and writing it out in a single write() would get us much\n> > closer to solving the \"mixed up output\" from multiple processes\n> > problem the current code seems to already have.\n> \n> The buffering of a single line is exactly what we're doing now in the\n> non-atomic case. Previously there had been multiple writes, now we only\n> call `strbuf_write()` once on the buffer for each reference.\n> \n> > > Also, if there is any\n> > > unexpected error in between updating references which causes us to die,\n> > > then we wouldn't have written the already updated references to\n> > > FETCH_HEAD now.\n> > \n> > That one may be a valid concern.\n> > \n> > Thanks.\n> \n> Just to be explicit: are you fine with changes in this patch as-is? They\n> don't solve concurrent writes to FETCH_HEAD, but they should make it\n> easier to solve that if we ever need to.\n> \n> Patrick\n\nNever mind, I forgot that I'm still replying on v2 of this patch series.\n\nPatrick\n"},{"id":"414193","messageId":"xmqqturm3qui.fsf@gitster.c.googlers.com","threadId":"54954","inReplyTo":"X/2UkKS0AhT6jLXj@ncase","subject":"Re: [PATCH v2 4/4] fetch: implement support for atomic reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T19:19:49Z","receivedAt":"2021-01-12T19:20:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> \n>> > > +\t\tfprintf(fetch_head->fp, \"%s\\t%s\\t%s\",\n>> > > +\t\t\told_oid, merge_status_marker, note);\n>> > > +\t\tfor (i = 0; i < url_len; ++i)\n>> > > +\t\t\tif ('\\n' == url[i])\n>> > > +\t\t\t\tfputs(\"\\\\n\", fetch_head->fp);\n>> > > +\t\t\telse\n>> > > +\t\t\t\tfputc(url[i], fetch_head->fp);\n>> > > +\t\tfputc('\\n', fetch_head->fp);\n>> \n>> ... these stdio operations for a single record would result in a\n>> single atomic write() that would not compete with writes by other\n>> processes. So I wouldn't call \"the current design allows ... at the\n>> same time.\" It has been broken for years and nobody complained.\n>\n> Fair enough. I somehow blindly assumed that `git fetch --all`, which\n> does invoke `git fetch --append`, did perform the fetch in parallel. If\n> that's not the case: all the better.\n\nThe \"--all\" option started as \"one after another, one at a time\",\nbut these days goes thru fetch_multiple() where we started to use\nrun_processes_parallel() API without giving it much thought what it\nwould do to the writing of FETCH_HEAD; since around d54dea77 (fetch:\nlet --jobs= parallelize --multiple, too, 2019-10-05), this\ncodepath wrt FETCH_HEAD is utterly broken, I would have to say.\n\n> The buffering of a single line is exactly what we're doing now in the\n> non-atomic case. Previously there had been multiple writes, now we only\n> call `strbuf_write()` once on the buffer for each reference.\n\nExactly.\n"}]}