{"thread":{"id":"57403","subject":"[PATCH 0/6] fetch: improve atomicity of `--atomic` flag","startedAt":"2022-02-11T07:46:51Z","lastAt":"2022-02-17T12:47:20Z","messageCount":27,"participants":["Patrick Steinhardt","Christian Couder","Jonathan Tan","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"448207","messageId":"cover.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":null,"subject":"[PATCH 0/6] fetch: improve atomicity of `--atomic` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:46:39Z","receivedAt":"2022-02-11T07:46:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nin c7b190dabd (fetch: implement support for atomic reference updates,\n2021-01-12), I have added a new `--atomic` flag to git-fetch(1) which is\nsupposed to make it an all-or-nothing operation: either all refs are\nupdated, or none is. I have recently discovered though that there were\ntwo oversights: neither pruning of refs via `--prune` nor the tag\nbackfill mechanism are currently covered by this flag, which breaks the\npromise.\n\nThis patch series extends coverage of the `--atomic` flag to fix my\noversights. It applies on top of ps/avoid-unnecessary-hook-invocation-with-packed-refs\nmerged to v2.35.1 due to a semantic conflict in tests: I use the reftx\nhook to verify some things work as expected, and above branch causes us\nto not execute the hook for packed-refs anymore.\n\nIt is structured as follows:\n\n - Patch 1 adds multiple tests which demonstrate issues with\n backfilling and the lacking atomicity.\n\n - Patch 2 and 3 do some preliminary refactorings which set the stage\n for improved atomicity.\n\n - Patch 4 fixes a bug with backfilling tags: we don't return an\n error code to the user if the backfill fails.\n\n - Patch 5 and 6 then finally extend the atomicity guarantees.\n\nI'm not yet all that happy with patch 5: it currently has to reach into\nthe `ref_transaction` struct to be able to filter out any updates which\nhave already been queued such that we don't accidentally queue the same\ntag update twice. I first wanted to get some feedback on this patch\nseries though, and in case it is agreed that it fixes a real issue I may\nintroduce a new non-internal API which allows iterating over queued\nupdates.\n\nPatrick\n\nPatrick Steinhardt (6):\n fetch: increase test coverage of fetches\n fetch: backfill tags before setting upstream\n fetch: control lifecycle of FETCH_HEAD in a single place\n fetch: report errors when backfilling tags fails\n fetch: make `--atomic` flag cover backfilling of tags\n fetch: make `--atomic` flag cover pruning of refs\n\n builtin/fetch.c | 190 +++++++++++++++++++++++++++++--------------\n t/t5503-tagfollow.sh | 78 ++++++++++++++++++\n t/t5510-fetch.sh | 31 +++++++\n 3 files changed, 236 insertions(+), 63 deletions(-)\n\n-- \n2.35.1\n\n"},{"id":"448208","messageId":"e133c14f569116156bbd3e0ca4874e8eb54b76b8.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 1/6] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:46:45Z","receivedAt":"2022-02-11T07:46:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--atomic` flag is missing test coverage for pruning of deleted\nreferences and backfilling of tags, and of course both aren't covered\ncorrectly by this flag. Furthermore, we don't have tests demonstrating\nerror cases for backfilling tags.\n\nAdd tests to cover those testing gaps.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5503-tagfollow.sh | 85 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5510-fetch.sh | 33 +++++++++++++++++\n 2 files changed, 118 insertions(+)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 195fc64dd4..888305ad4d 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -160,4 +160,89 @@ test_expect_success 'new clone fetch main and tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'atomic fetch with failing backfill' '\n+\tgit init clone3 &&\n+\n+\t# We want to test whether a failure when backfilling tags correctly\n+\t# aborts the complete transaction when `--atomic` is passed: we should\n+\t# neither create the branch nor should we create the tag when either\n+\t# one of both fails to update correctly.\n+\t#\n+\t# To trigger failure we simply abort when backfilling a tag.\n+\twrite_script clone3/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t#!/bin/sh\n+\n+\t\twhile read oldrev newrev reference\n+\t\tdo\n+\t\t\tif test \"$reference\" = refs/tags/tag1\n+\t\t\tthen\n+\t\t\t\texit 1\n+\t\t\tfi\n+\t\tdone\n+\tEOF\n+\n+\ttest_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&\n+\n+\t# Creation of the tag has failed, so ideally refs/heads/something\n+\t# should not exist. The fact that it does is demonstrates that there is\n+\t# missing coverage in the `--atomic` flag.\n+\ttest $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n+'\n+\n+test_expect_success 'atomic fetch with backfill should use single transaction' '\n+\tgit init clone4 &&\n+\n+\t# Fetching with the `--atomic` flag should update all references in a\n+\t# single transaction, including backfilled tags. We thus expect to see\n+\t# a single reference transaction for the created branch and tags.\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $B refs/heads/something\n+\t\t$ZERO_OID $S refs/tags/tag2\n+\t\tcommitted\n+\t\t$ZERO_OID $B refs/heads/something\n+\t\t$ZERO_OID $S refs/tags/tag2\n+\t\tprepared\n+\t\t$ZERO_OID $T refs/tags/tag1\n+\t\tcommitted\n+\t\t$ZERO_OID $T refs/tags/tag1\n+\tEOF\n+\n+\twrite_script clone4/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C clone4 fetch --atomic .. $B:refs/heads/something &&\n+\ttest_cmp expected clone4/actual\n+'\n+\n+test_expect_success 'backfill failure causes command to fail' '\n+\tgit init clone5 &&\n+\n+\twrite_script clone5/.git/hooks/reference-transaction <<-EOF &&\n+\t\t#!/bin/sh\n+\n+\t\twhile read oldrev newrev reference\n+\t\tdo\n+\t\t\tif test \"\\$reference\" = refs/tags/tag1\n+\t\t\tthen\n+\t\t\t\t# Create a nested tag below the actual tag we\n+\t\t\t\t# wanted to write, which causes a D/F conflict\n+\t\t\t\t# later when we want to commit refs/tags/tag1.\n+\t\t\t\t# We cannot just `exit 1` here given that this\n+\t\t\t\t# would cause us to die immediately.\n+\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n+\t\t\t\texit \\$!\n+\t\t\tfi\n+\t\tdone\n+\tEOF\n+\n+\t# Even though we fail to create refs/tags/tag1 the below command\n+\t# unexpectedly succeeds.\n+\tgit -C clone5 fetch .. $B:refs/heads/something &&\n+\ttest $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&\n+\ttest $S = $(git -C clone5 rev-parse --verify tag2) &&\n+\ttest_must_fail git -C clone5 rev-parse --verify tag1\n+'\n+\n test_done\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 20f7110ec1..93a0db3c68 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -332,6 +332,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '\n \ttest_cmp expected atomic/.git/FETCH_HEAD\n '\n \n+test_expect_success 'fetch --atomic --prune executes a single reference transaction only' '\n+\ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n+\n+\tcd \"$D\" &&\n+\tgit branch scheduled-for-deletion &&\n+\tgit clone . atomic &&\n+\tgit branch -D scheduled-for-deletion &&\n+\tgit branch new-branch &&\n+\thead_oid=$(git rev-parse HEAD) &&\n+\n+\t# Fetching with the `--atomic` flag should update all references in a\n+\t# single transaction. It is currently missing coverage of pruned\n+\t# references though, and as a result those may be committed to disk\n+\t# even if updating references fails later.\n+\tcat >expected <<-EOF &&\n+\t\tprepared\n+\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n+\t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n+\t\tprepared\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n+\t\tcommitted\n+\t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n+\tEOF\n+\n+\twrite_script atomic/.git/hooks/reference-transaction <<-\\EOF &&\n+\t\t( echo \"$*\" && cat ) >>actual\n+\tEOF\n+\n+\tgit -C atomic fetch --atomic --prune origin &&\n+\ttest_cmp expected atomic/actual\n+'\n+\n test_expect_success '--refmap=\"\" ignores configured refspec' '\n \tcd \"$TRASH_DIRECTORY\" &&\n \tgit clone \"$D\" remote-refs &&\n-- \n2.35.1\n\n"},{"id":"448209","messageId":"64c94e7a28d83fbaa1b6308b034d07c7be10e767.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 2/6] fetch: backfill tags before setting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:46:51Z","receivedAt":"2022-02-11T07:46:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The fetch code flow is a bit hard to understand right now:\n\n 1. We optionally prune all references which have vanished on the\n remote side.\n 2. We fetch and update all other references locally.\n 3. We update the upstream branch in the gitconfig.\n 4. We backfill tags pointing into the history we have just fetched.\n\nIt is quite confusing that we fetch objects and update references in\nboth (2) and (4), which is further stressed by the point that we require\na `skip` label which jumps from (3) to (4) in case we fail to update the\ngitconfig as expected.\n\nReorder the code to first update all local references, and only after we\nhave done so update the upstream branch information. This improves the\ncode flow and furthermore makes it easier to refactor the way we update\nreferences together.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 33 ++++++++++++++++-----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5b3b18a72f..9c7e4f12cd 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs)\n {\n-\tstruct ref *ref_map;\n+\tstruct ref *ref_map = NULL;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n@@ -1618,11 +1618,22 @@ static int do_fetch(struct transport *transport,\n \t\t}\n \t}\n \tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n-\t\tfree_refs(ref_map);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n \n+\t/* if neither --no-tags nor --tags was specified, do automated tag\n+\t * following ... */\n+\tif (tags == TAGS_DEFAULT && autotags) {\n+\t\tstruct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n+\n+\t\tfind_non_local_tags(remote_refs, &tags_ref_map, &tail);\n+\t\tif (tags_ref_map)\n+\t\t\tbackfill_tags(transport, tags_ref_map, worktrees);\n+\n+\t\tfree_refs(tags_ref_map);\n+\t}\n+\n \tif (set_upstream) {\n \t\tstruct branch *branch = branch_get(\"HEAD\");\n \t\tstruct ref *rm;\n@@ -1642,7 +1653,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tif (!rm->peer_ref) {\n \t\t\t\tif (source_ref) {\n \t\t\t\t\twarning(_(\"multiple branches detected, incompatible with --set-upstream\"));\n-\t\t\t\t\tgoto skip;\n+\t\t\t\t\tgoto cleanup;\n \t\t\t\t} else {\n \t\t\t\t\tsource_ref = rm;\n \t\t\t\t}\n@@ -1656,7 +1667,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n \t\t\t\t\t \"it does not point to any branch.\"),\n \t\t\t\t\tshortname, transport->remote->name);\n-\t\t\t\tgoto skip;\n+\t\t\t\tgoto cleanup;\n \t\t\t}\n \n \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n@@ -1676,21 +1687,9 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t \"you need to specify exactly one branch with the --set-upstream option\"));\n \t\t}\n \t}\n-skip:\n-\tfree_refs(ref_map);\n-\n-\t/* if neither --no-tags nor --tags was specified, do automated tag\n-\t * following ... */\n-\tif (tags == TAGS_DEFAULT && autotags) {\n-\t\tstruct ref **tail = &ref_map;\n-\t\tref_map = NULL;\n-\t\tfind_non_local_tags(remote_refs, &ref_map, &tail);\n-\t\tif (ref_map)\n-\t\t\tbackfill_tags(transport, ref_map, worktrees);\n-\t\tfree_refs(ref_map);\n-\t}\n \n cleanup:\n+\tfree_refs(ref_map);\n \tfree_worktrees(worktrees);\n \treturn retcode;\n }\n-- \n2.35.1\n\n"},{"id":"448210","messageId":"4059d5034bd9137ffca4929ed5bd8b7ce75ea09c.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 3/6] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:46:55Z","receivedAt":"2022-02-11T07:47:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two different locations where we're appending to FETCH_HEAD:\nfirst when storing updated references, and second when backfilling tags.\nBoth times we open the file, append to it and then commit it into place,\nwhich is essentially duplicate work.\n\nImprove the lifecycle of updating FETCH_HEAD by opening and committing\nit once in `do_fetch()`, where we pass the structure down to code which\nwants to append to it.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 35 +++++++++++++++++++----------------\n 1 file changed, 19 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9c7e4f12cd..627847e2f8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1084,9 +1084,8 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\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-\t\t\t struct worktree **worktrees)\n+\t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n-\tstruct fetch_head fetch_head;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction = NULL;\n@@ -1096,10 +1095,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tint want_status;\n \tint summary_width = transport_summary_width(ref_map);\n \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 \telse\n@@ -1206,7 +1201,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tstrbuf_addf(¬e, \"'%s' of \", what);\n \t\t\t}\n \n-\t\t\tappend_fetch_head(&fetch_head, &rm->old_oid,\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@@ -1246,9 +1241,6 @@ 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@@ -1268,7 +1260,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tstrbuf_release(&err);\n \tref_transaction_free(transaction);\n \tfree(url);\n-\tclose_fetch_head(&fetch_head);\n \treturn rc;\n }\n \n@@ -1309,6 +1300,7 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_and_consume_refs(struct transport *transport,\n \t\t\t\t struct ref *ref_map,\n+\t\t\t\t struct fetch_head *fetch_head,\n \t\t\t\t struct worktree **worktrees)\n {\n \tint connectivity_checked = 1;\n@@ -1331,7 +1323,7 @@ static int fetch_and_consume_refs(struct transport *transport,\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url, transport->remote->name,\n-\t\t\t\t connectivity_checked, ref_map, worktrees);\n+\t\t\t\t connectivity_checked, ref_map, fetch_head, worktrees);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1503,7 +1495,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \treturn transport;\n }\n \n-static void backfill_tags(struct transport *transport, struct ref *ref_map,\n+static void backfill_tags(struct transport *transport,\n+\t\t\t struct ref *ref_map,\n+\t\t\t struct fetch_head *fetch_head,\n \t\t\t struct worktree **worktrees)\n {\n \tint cannot_reuse;\n@@ -1525,7 +1519,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tfetch_and_consume_refs(transport, ref_map, worktrees);\n+\tfetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1544,6 +1538,7 @@ static int do_fetch(struct transport *transport,\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n \tint must_list_refs = 1;\n \tstruct worktree **worktrees = get_worktrees();\n+\tstruct fetch_head fetch_head = { 0 };\n \n \tif (tags == TAGS_DEFAULT) {\n \t\tif (transport->remote->fetch_tags == 2)\n@@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,\n \tif (!update_head_ok)\n \t\tcheck_not_current_branch(ref_map, worktrees);\n \n+\tretcode = open_fetch_head(&fetch_head);\n+\tif (retcode)\n+\t\tgoto cleanup;\n+\n \tif (tags == TAGS_DEFAULT && autotags)\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \tif (prune) {\n@@ -1617,7 +1616,8 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t transport->url);\n \t\t}\n \t}\n-\tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n+\n+\tif (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1629,11 +1629,13 @@ static int do_fetch(struct transport *transport,\n \n \t\tfind_non_local_tags(remote_refs, &tags_ref_map, &tail);\n \t\tif (tags_ref_map)\n-\t\t\tbackfill_tags(transport, tags_ref_map, worktrees);\n+\t\t\tbackfill_tags(transport, tags_ref_map, &fetch_head, worktrees);\n \n \t\tfree_refs(tags_ref_map);\n \t}\n \n+\tcommit_fetch_head(&fetch_head);\n+\n \tif (set_upstream) {\n \t\tstruct branch *branch = branch_get(\"HEAD\");\n \t\tstruct ref *rm;\n@@ -1689,6 +1691,7 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\tclose_fetch_head(&fetch_head);\n \tfree_refs(ref_map);\n \tfree_worktrees(worktrees);\n \treturn retcode;\n-- \n2.35.1\n\n"},{"id":"448211","messageId":"54fdee845bea7f67f46817417f8e5a504bd39665.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:46:59Z","receivedAt":"2022-02-11T07:47:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the backfilling of tags fails we do not report this error to the\ncaller, but only report it implicitly at a later point when reporting\nupdated references. This leaves callers unable to act upon the\ninformation of whether the backfilling succeeded or not.\n\nRefactor the function to return an error code and pass it up the\ncallstack. This causes us to correctly propagate the error back to the\nuser of git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 29 +++++++++++++++++++++--------\n t/t5503-tagfollow.sh | 4 +---\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 627847e2f8..1eda0b68ff 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \treturn transport;\n }\n \n-static void backfill_tags(struct transport *transport,\n-\t\t\t struct ref *ref_map,\n-\t\t\t struct fetch_head *fetch_head,\n-\t\t\t struct worktree **worktrees)\n+static int backfill_tags(struct transport *transport,\n+\t\t\t struct ref *ref_map,\n+\t\t\t struct fetch_head *fetch_head,\n+\t\t\t struct worktree **worktrees)\n {\n-\tint cannot_reuse;\n+\tint retcode, cannot_reuse;\n \n \t/*\n \t * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it\n@@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tfetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n+\tretcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n \t\tgsecondary = NULL;\n \t}\n+\n+\treturn retcode;\n }\n \n static int do_fetch(struct transport *transport,\n@@ -1628,8 +1630,19 @@ static int do_fetch(struct transport *transport,\n \t\tstruct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n \n \t\tfind_non_local_tags(remote_refs, &tags_ref_map, &tail);\n-\t\tif (tags_ref_map)\n-\t\t\tbackfill_tags(transport, tags_ref_map, &fetch_head, worktrees);\n+\t\tif (tags_ref_map) {\n+\t\t\t/*\n+\t\t\t * If backfilling tags succeeds we used to not return\n+\t\t\t * an error code to the user at all. Instead, we\n+\t\t\t * silently swallowed that error and updated the local\n+\t\t\t * state of the repository. We now notify the user of\n+\t\t\t * any such errors, but we continue to make sure that\n+\t\t\t * FETCH_HEAD and the upstream branch are configured as\n+\t\t\t * expected.\n+\t\t\t */\n+\t\t\tif (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))\n+\t\t\t\tretcode = 1;\n+\t\t}\n \n \t\tfree_refs(tags_ref_map);\n \t}\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 888305ad4d..549f908b90 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -237,9 +237,7 @@ test_expect_success 'backfill failure causes command to fail' '\n \t\tdone\n \tEOF\n \n-\t# Even though we fail to create refs/tags/tag1 the below command\n-\t# unexpectedly succeeds.\n-\tgit -C clone5 fetch .. $B:refs/heads/something &&\n+\ttest_must_fail git -C clone5 fetch .. $B:refs/heads/something &&\n \ttest $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&\n \ttest $S = $(git -C clone5 rev-parse --verify tag2) &&\n \ttest_must_fail git -C clone5 rev-parse --verify tag1\n-- \n2.35.1\n\n"},{"id":"448212","messageId":"55dbe19a1a4d05d84c81356af1a3f04b65f8aa7b.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:47:03Z","receivedAt":"2022-02-11T07:47:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching references from a remote we by default also fetch all tags\nwhich point into the history we have fetched. This is a separate step\nperformed after updating local references because it requires us to walk\nover the history on the client-side to determine whether the remote has\nannounced any tags which point to one of the fetched commits.\n\nThis backfilling of tags isn't covered by the `--atomic` flag: right\nnow, it only applies to the step where we update our local references.\nThis is an oversight at the time the flag was introduced: its purpose is\nto either update all references or none, but right now we happily update\nlocal references even in the case where backfilling failed.\n\nFix this by pulling up creation of the reference transaction such that\nwe can pass the same transaction to both the code which updates local\nreferences and to the code which backfills tags. This allows us to only\ncommit the transaction in case both actions succeed.\n\nNote that we also have to start passing the transaction into\n`find_non_local_tags()`: this function is responsible for finding all\ntags which we need to backfill. Right now, it will happily return tags\nwhich we have already been updated with our local references. But when\nwe use a single transaction for both local references and backfilling\nthen it may happen that we try to queue the same reference update twice\nto the transaction, which consequentially triggers a bug. We thus have\nto skip over any tags which have already been queued. Unfortunately,\nthis requires us to reach into internals of the reference transaction to\naccess queued updates, but there is no non-internal interface right now\nwhich would allow us to access this information.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 89 ++++++++++++++++++++++++++++++--------------\n t/t5503-tagfollow.sh | 11 ++----\n 2 files changed, 65 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 1eda0b68ff..348e64cf2c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -4,7 +4,7 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"repository.h\"\n-#include \"refs.h\"\n+#include \"refs/refs-internal.h\"\n #include \"refspec.h\"\n #include \"object-store.h\"\n #include \"oidset.h\"\n@@ -350,6 +350,7 @@ static void clear_item(struct refname_hash_entry *item)\n }\n \n static void find_non_local_tags(const struct ref *refs,\n+\t\t\t\tstruct ref_transaction *transaction,\n \t\t\t\tstruct ref **head,\n \t\t\t\tstruct ref ***tail)\n {\n@@ -361,12 +362,28 @@ static void find_non_local_tags(const struct ref *refs,\n \tconst struct ref *ref;\n \tstruct refname_hash_entry *item = NULL;\n \tconst int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;\n+\tint i;\n \n \trefname_hash_init(&existing_refs);\n \trefname_hash_init(&remote_refs);\n \tcreate_fetch_oidset(head, &fetch_oids);\n \n \tfor_each_ref(add_one_refname, &existing_refs);\n+\n+\t/*\n+\t * If we already have a transaction, then we need to filter out all\n+\t * tags which have already been queued up.\n+\t */\n+\tfor (i = 0; transaction && i < transaction->nr; i++) {\n+\t\tif (!starts_with(transaction->updates[i]->refname, \"refs/tags/\") ||\n+\t\t !(transaction->updates[i]->flags & REF_HAVE_NEW))\n+\t\t\tcontinue;\n+\t\t(void) refname_hash_add(&existing_refs,\n+\t\t\t\t\ttransaction->updates[i]->refname,\n+\t\t\t\t\t&transaction->updates[i]->new_oid);\n+\t}\n+\n+\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (!starts_with(ref->name, \"refs/tags/\"))\n \t\t\tcontinue;\n@@ -600,7 +617,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \t\t/* also fetch all tags */\n \t\tget_fetch_map(remote_refs, tag_refspec, &tail, 0);\n \telse if (tags == TAGS_DEFAULT && *autotags)\n-\t\tfind_non_local_tags(remote_refs, &ref_map, &tail);\n+\t\tfind_non_local_tags(remote_refs, NULL, &ref_map, &tail);\n \n \t/* Now append any refs to be updated opportunistically: */\n \t*tail = orefs;\n@@ -1083,12 +1100,12 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\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+\t\t\t int connectivity_checked,\n+\t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head, struct worktree **worktrees)\n {\n \tint url_len, i, rc = 0;\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@@ -1110,14 +1127,6 @@ 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@@ -1233,14 +1242,6 @@ 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@@ -1258,7 +1259,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n abort:\n \tstrbuf_release(¬e);\n \tstrbuf_release(&err);\n-\tref_transaction_free(transaction);\n \tfree(url);\n \treturn rc;\n }\n@@ -1299,6 +1299,7 @@ static int check_exist_and_connected(struct ref *ref_map)\n }\n \n static int fetch_and_consume_refs(struct transport *transport,\n+\t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct ref *ref_map,\n \t\t\t\t struct fetch_head *fetch_head,\n \t\t\t\t struct worktree **worktrees)\n@@ -1323,7 +1324,8 @@ static int fetch_and_consume_refs(struct transport *transport,\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url, transport->remote->name,\n-\t\t\t\t connectivity_checked, ref_map, fetch_head, worktrees);\n+\t\t\t\t connectivity_checked, transaction, ref_map,\n+\t\t\t\t fetch_head, worktrees);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1496,6 +1498,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n }\n \n static int backfill_tags(struct transport *transport,\n+\t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head,\n \t\t\t struct worktree **worktrees)\n@@ -1519,7 +1522,7 @@ static int backfill_tags(struct transport *transport,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tretcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n+\tretcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1532,6 +1535,7 @@ static int backfill_tags(struct transport *transport,\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs)\n {\n+\tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n@@ -1541,6 +1545,7 @@ static int do_fetch(struct transport *transport,\n \tint must_list_refs = 1;\n \tstruct worktree **worktrees = get_worktrees();\n \tstruct fetch_head fetch_head = { 0 };\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (tags == TAGS_DEFAULT) {\n \t\tif (transport->remote->fetch_tags == 2)\n@@ -1602,6 +1607,14 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n+\tif (atomic_fetch) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\tretcode = error(\"%s\", err.buf);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n \tif (tags == TAGS_DEFAULT && autotags)\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \tif (prune) {\n@@ -1619,7 +1632,7 @@ static int do_fetch(struct transport *transport,\n \t\t}\n \t}\n \n-\tif (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {\n+\tif (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1629,7 +1642,7 @@ static int do_fetch(struct transport *transport,\n \tif (tags == TAGS_DEFAULT && autotags) {\n \t\tstruct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n \n-\t\tfind_non_local_tags(remote_refs, &tags_ref_map, &tail);\n+\t\tfind_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);\n \t\tif (tags_ref_map) {\n \t\t\t/*\n \t\t\t * If backfilling tags succeeds we used to not return\n@@ -1638,15 +1651,31 @@ static int do_fetch(struct transport *transport,\n \t\t\t * state of the repository. We now notify the user of\n \t\t\t * any such errors, but we continue to make sure that\n \t\t\t * FETCH_HEAD and the upstream branch are configured as\n-\t\t\t * expected.\n+\t\t\t * expected. The exception though is when `--atomic`\n+\t\t\t * is passed: in that case we'll abort the transaction\n+\t\t\t * and don't commit anything.\n \t\t\t */\n-\t\t\tif (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))\n+\t\t\tif (backfill_tags(transport, transaction, tags_ref_map,\n+\t\t\t\t\t &fetch_head, worktrees))\n \t\t\t\tretcode = 1;\n \t\t}\n \n \t\tfree_refs(tags_ref_map);\n \t}\n \n+\tif (transaction) {\n+\t\tif (retcode)\n+\t\t\tgoto cleanup;\n+\n+\t\tretcode = ref_transaction_commit(transaction, &err);\n+\t\tif (retcode) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tref_transaction_free(transaction);\n+\t\t\ttransaction = NULL;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n \tcommit_fetch_head(&fetch_head);\n \n \tif (set_upstream) {\n@@ -1704,7 +1733,13 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\tif (retcode && transaction) {\n+\t\tref_transaction_abort(transaction, &err);\n+\t\terror(\"%s\", err.buf);\n+\t}\n+\n \tclose_fetch_head(&fetch_head);\n+\tstrbuf_release(&err);\n \tfree_refs(ref_map);\n \tfree_worktrees(worktrees);\n \treturn retcode;\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 549f908b90..4a8e63aa16 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -182,11 +182,8 @@ test_expect_success 'atomic fetch with failing backfill' '\n \tEOF\n \n \ttest_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&\n-\n-\t# Creation of the tag has failed, so ideally refs/heads/something\n-\t# should not exist. The fact that it does is demonstrates that there is\n-\t# missing coverage in the `--atomic` flag.\n-\ttest $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n+\ttest_must_fail git -C clone3 rev-parse --verify refs/heads/something &&\n+\ttest_must_fail git -C clone3 rev-parse --verify refs/tags/tag2\n '\n \n test_expect_success 'atomic fetch with backfill should use single transaction' '\n@@ -199,12 +196,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '\n \t\tprepared\n \t\t$ZERO_OID $B refs/heads/something\n \t\t$ZERO_OID $S refs/tags/tag2\n+\t\t$ZERO_OID $T refs/tags/tag1\n \t\tcommitted\n \t\t$ZERO_OID $B refs/heads/something\n \t\t$ZERO_OID $S refs/tags/tag2\n-\t\tprepared\n-\t\t$ZERO_OID $T refs/tags/tag1\n-\t\tcommitted\n \t\t$ZERO_OID $T refs/tags/tag1\n \tEOF\n \n-- \n2.35.1\n\n"},{"id":"448213","messageId":"682f16117b743bec59c533e15ae5a88d39250222.1644565025.git.ps@pks.im","threadId":"57403","inReplyTo":"cover.1644565025.git.ps@pks.im","subject":"[PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-11T07:47:07Z","receivedAt":"2022-02-11T07:47:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching with the `--prune` flag we will delete any local\nreferences matching the fetch refspec which have disappeared on the\nremote. This step is not currently covered by the `--atomic` flag: we\ndelete branches even though updating of local references has failed,\nwhich means that the fetch is not an all-or-nothing operation.\n\nFix this bug by passing in the global transaction into `prune_refs()`:\nif one is given, then we'll only queue up deletions and not commit them\nright away.\n\nThis change also improves performance when pruning many branches in a\nrepository with a big packed-refs file: every references is pruned in\nits own transaction, which means that we potentially have to rewrite\nthe packed-refs files for every single reference we're about to prune.\n\nThe following benchmark demonstrates this: it performs a pruning fetch\nfrom a repository with a single reference into a repository with 100k\nreferences, which causes us to prune all but one reference. This is of\ncourse a very artificial setup, but serves to demonstrate the impact of\nonly having to write the packed-refs file once:\n\n Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]\n Range (min … max): 2.328 s … 2.407 s 10 runs\n\n Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]\n Range (min … max): 1.346 s … 1.400 s 10 runs\n\n Summary\n 'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n 1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 30 ++++++++++++++++++++++--------\n t/t5510-fetch.sh | 4 +---\n 2 files changed, 23 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 348e64cf2c..75e791a4b4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1333,11 +1333,14 @@ static int fetch_and_consume_refs(struct transport *transport,\n \treturn ret;\n }\n \n-static int prune_refs(struct refspec *rs, struct ref *ref_map,\n+static int prune_refs(struct refspec *rs,\n+\t\t struct ref_transaction *transaction,\n+\t\t struct ref *ref_map,\n \t\t const char *raw_url)\n {\n \tint url_len, i, result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);\n+\tstruct strbuf err = STRBUF_INIT;\n \tchar *url;\n \tint summary_width = transport_summary_width(stale_refs);\n \tconst char *dangling_msg = dry_run\n@@ -1358,13 +1361,22 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \t\turl_len = i - 3;\n \n \tif (!dry_run) {\n-\t\tstruct string_list refnames = STRING_LIST_INIT_NODUP;\n+\t\tif (transaction) {\n+\t\t\tfor (ref = stale_refs; ref; ref = ref->next) {\n+\t\t\t\tresult = ref_transaction_delete(transaction, ref->name, NULL, 0,\n+\t\t\t\t\t\t\t\t\"fetch: prune\", &err);\n+\t\t\t\tif (result)\n+\t\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tstruct string_list refnames = STRING_LIST_INIT_NODUP;\n \n-\t\tfor (ref = stale_refs; ref; ref = ref->next)\n-\t\t\tstring_list_append(&refnames, ref->name);\n+\t\t\tfor (ref = stale_refs; ref; ref = ref->next)\n+\t\t\t\tstring_list_append(&refnames, ref->name);\n \n-\t\tresult = delete_refs(\"fetch: prune\", &refnames, 0);\n-\t\tstring_list_clear(&refnames, 0);\n+\t\t\tresult = delete_refs(\"fetch: prune\", &refnames, 0);\n+\t\t\tstring_list_clear(&refnames, 0);\n+\t\t}\n \t}\n \n \tif (verbosity >= 0) {\n@@ -1383,6 +1395,8 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,\n \t\t}\n \t}\n \n+cleanup:\n+\tstrbuf_release(&err);\n \tfree(url);\n \tfree_refs(stale_refs);\n \treturn result;\n@@ -1624,10 +1638,10 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tprune_refs(rs, ref_map, transport->url);\n+\t\t\tprune_refs(rs, transaction, ref_map, transport->url);\n \t\t} else {\n \t\t\tprune_refs(&transport->remote->fetch,\n-\t\t\t\t ref_map,\n+\t\t\t\t transaction, ref_map,\n \t\t\t\t transport->url);\n \t\t}\n \t}\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 93a0db3c68..afa6bf9f7d 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -349,11 +349,9 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\n \tcat >expected <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n-\t\tcommitted\n-\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n-\t\tprepared\n \t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n \t\tcommitted\n+\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n \t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n \tEOF\n \n-- \n2.35.1\n\n"},{"id":"448409","messageId":"CAP8UFD2HswmXhqYTAMxQ0iYFtsErMS=DB18iv52Ujs=cAW9ytw@mail.gmail.com","threadId":"57403","inReplyTo":"e133c14f569116156bbd3e0ca4874e8eb54b76b8.1644565025.git.ps@pks.im","subject":"Re: [PATCH 1/6] fetch: increase test coverage of fetches","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T06:19:19Z","receivedAt":"2022-02-15T06:19:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 11, 2022 at 8:38 PM Patrick Steinhardt wrote:\n>\n> The `--atomic` flag is missing test coverage for pruning of deleted\n> references and backfilling of tags, and of course both aren't covered\n> correctly by this flag.\n\nIt's not clear to me what \"both aren't covered correctly by this flag\"\nactually means here. If it means that pruning of deleted references\nand backfilling of tags don't work correctly when --atomic is used,\nthen it could be stated more clearly. Otherwise this seems to just be\nrepeating the first part of the sentence.\n\n> Furthermore, we don't have tests demonstrating\n> error cases for backfilling tags.\n>\n> Add tests to cover those testing gaps.\n>\n> Signed-off-by: Patrick Steinhardt \n\n> +test_expect_success 'atomic fetch with failing backfill' '\n> + git init clone3 &&\n> +\n> + # We want to test whether a failure when backfilling tags correctly\n> + # aborts the complete transaction when `--atomic` is passed: we should\n> + # neither create the branch nor should we create the tag when either\n> + # one of both fails to update correctly.\n> + #\n> + # To trigger failure we simply abort when backfilling a tag.\n> + write_script clone3/.git/hooks/reference-transaction <<-\\EOF &&\n> + #!/bin/sh\n> +\n> + while read oldrev newrev reference\n> + do\n> + if test \"$reference\" = refs/tags/tag1\n> + then\n> + exit 1\n> + fi\n> + done\n> + EOF\n> +\n> + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&\n> +\n> + # Creation of the tag has failed, so ideally refs/heads/something\n> + # should not exist. The fact that it does is demonstrates that there is\n\ns/The fact that it does is demonstrates/The fact that it does demonstrates/\n\n> + # missing coverage in the `--atomic` flag.\n\nMaybe s/missing coverage/a bug/ would make things clearer.\n\n> + test $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n> +'\n\nAs this patch series is about fixing buggy parts of the behavior with\n--atomic, I think it would make more sense to use test_expect_failure,\ninstead of test_expect_success, in this test, and to check that we\nhave the correct behavior, instead of checking that we have the buggy\nbehavior.\n\nOf course when later in this patch series the buggy behavior is fixed,\nthen test_expect_failure should be replaced with test_expect_success.\n\n> +test_expect_success 'atomic fetch with backfill should use single transaction' '\n> + git init clone4 &&\n> +\n> + # Fetching with the `--atomic` flag should update all references in a\n> + # single transaction, including backfilled tags. We thus expect to see\n> + # a single reference transaction for the created branch and tags.\n> + cat >expected <<-EOF &&\n> + prepared\n> + $ZERO_OID $B refs/heads/something\n> + $ZERO_OID $S refs/tags/tag2\n> + committed\n> + $ZERO_OID $B refs/heads/something\n> + $ZERO_OID $S refs/tags/tag2\n> + prepared\n> + $ZERO_OID $T refs/tags/tag1\n> + committed\n> + $ZERO_OID $T refs/tags/tag1\n> + EOF\n\nThe comment says that we expect to see a single reference transaction,\nbut the expected file we create seems to show 2 transactions. So I\nthink here too, we should use test_expect_failure, instead of\ntest_expect_success, and check that we have the correct behavior\ninstead of a buggy one.\n\n> + write_script clone4/.git/hooks/reference-transaction <<-\\EOF &&\n\nHere there is no #!/bin/sh while other uses of write_script in your\npatch have it. If it's not necessary, it could be removed in the other\nuses.\n\n> + ( echo \"$*\" && cat ) >>actual\n> + EOF\n> +\n> + git -C clone4 fetch --atomic .. $B:refs/heads/something &&\n> + test_cmp expected clone4/actual\n> +'\n\nI took a quick look at the 2 other tests after this one, and I think\ntest_expect_failure should be used there too, instead of\ntest_expect_success.\n"},{"id":"448410","messageId":"CAP8UFD3RpeMcfTDV89gRjhaOsjjbPv3x_pFPqF-gO4Kpv51VtQ@mail.gmail.com","threadId":"57403","inReplyTo":"64c94e7a28d83fbaa1b6308b034d07c7be10e767.1644565025.git.ps@pks.im","subject":"Re: [PATCH 2/6] fetch: backfill tags before setting upstream","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T06:43:14Z","receivedAt":"2022-02-15T06:43:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 11, 2022 at 5:12 PM Patrick Steinhardt wrote:\n>\n> The fetch code flow is a bit hard to understand right now:\n>\n> 1. We optionally prune all references which have vanished on the\n> remote side.\n> 2. We fetch and update all other references locally.\n> 3. We update the upstream branch in the gitconfig.\n> 4. We backfill tags pointing into the history we have just fetched.\n>\n> It is quite confusing that we fetch objects and update references in\n> both (2) and (4), which is further stressed by the point that we require\n\ns/require/use/\n\n> a `skip` label which jumps from (3) to (4) in case we fail to update the\n\ns/`skip` label/`skip` goto label/\ns/which jumps/to jump/\n\n> gitconfig as expected.\n>\n> Reorder the code to first update all local references, and only after we\n> have done so update the upstream branch information. This improves the\n> code flow and furthermore makes it easier to refactor the way we update\n> references together.\n\n> @@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,\n> static int do_fetch(struct transport *transport,\n> struct refspec *rs)\n> {\n> - struct ref *ref_map;\n> + struct ref *ref_map = NULL;\n> int autotags = (transport->remote->fetch_tags == 1);\n> int retcode = 0;\n> const struct ref *remote_refs;\n> @@ -1618,11 +1618,22 @@ static int do_fetch(struct transport *transport,\n> }\n> }\n> if (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n> - free_refs(ref_map);\n> retcode = 1;\n> goto cleanup;\n> }\n>\n> + /* if neither --no-tags nor --tags was specified, do automated tag\n> + * following ... */\n\nMaybe while at it this could be changed to use our usual style for\nmulti-line comments:\n\n /*\n * If neither --no-tags nor --tags was specified, do automated tag\n * following...\n */\n\n> + if (tags == TAGS_DEFAULT && autotags) {\n> + struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n> +\n> + find_non_local_tags(remote_refs, &tags_ref_map, &tail);\n> + if (tags_ref_map)\n> + backfill_tags(transport, tags_ref_map, worktrees);\n> +\n> + free_refs(tags_ref_map);\n> + }\n\n> @@ -1676,21 +1687,9 @@ static int do_fetch(struct transport *transport,\n> \"you need to specify exactly one branch with the --set-upstream option\"));\n> }\n> }\n> -skip:\n\nI like that it's removing one goto label and making the code simpler.\n"},{"id":"448413","messageId":"CAP8UFD32MQSVQ2uUhmO29jFz=LfqvN8U7e-a=sDgQAxUh+Gadw@mail.gmail.com","threadId":"57403","inReplyTo":"4059d5034bd9137ffca4929ed5bd8b7ce75ea09c.1644565025.git.ps@pks.im","subject":"Re: [PATCH 3/6] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T07:32:56Z","receivedAt":"2022-02-15T07:33:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Feb 12, 2022 at 5:49 PM Patrick Steinhardt wrote:\n>\n> There are two different locations where we're appending to FETCH_HEAD:\n> first when storing updated references, and second when backfilling tags.\n> Both times we open the file, append to it and then commit it into place,\n> which is essentially duplicate work.\n>\n> Improve the lifecycle of updating FETCH_HEAD by opening and committing\n> it once in `do_fetch()`, where we pass the structure down to code which\n\ns/down to code/down to the code/\n\n> wants to append to it.\n\n> @@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,\n> if (!update_head_ok)\n> check_not_current_branch(ref_map, worktrees);\n>\n> + retcode = open_fetch_head(&fetch_head);\n> + if (retcode)\n> + goto cleanup;\n> +\n> if (tags == TAGS_DEFAULT && autotags)\n> transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n> if (prune) {\n> @@ -1617,7 +1616,8 @@ static int do_fetch(struct transport *transport,\n> transport->url);\n> }\n> }\n> - if (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n> +\n> + if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {\n> retcode = 1;\n> goto cleanup;\n> }\n\nI think the following (if it works) would be more consistent with\nwhat's done for open_fetch_head() above:\n\nretcode = fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)\nif (retcode)\n goto cleanup;\n"},{"id":"448414","messageId":"CAP8UFD0MQCpFfWg2FP3S1VpDAxCwp3WGucFPn7K9BZ7UnckNLw@mail.gmail.com","threadId":"57403","inReplyTo":"54fdee845bea7f67f46817417f8e5a504bd39665.1644565025.git.ps@pks.im","subject":"Re: [PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T07:52:14Z","receivedAt":"2022-02-15T07:52:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt wrote:\n>\n> When the backfilling of tags fails we do not report this error to the\n> caller, but only report it implicitly at a later point when reporting\n> updated references.\n\nProbably stupid question: are we sure that it's a bug and not a feature?\n\n> This leaves callers unable to act upon the\n> information of whether the backfilling succeeded or not.\n>\n> Refactor the function to return an error code and pass it up the\n> callstack. This causes us to correctly propagate the error back to the\n> user of git-fetch(1).\n\nEven if it would have been the right behavior when backfilling tags\nwas implemented to return an error when backfilling tags fails, I\nthink it's interesting to ask ourselves if this change could be seen\nas a regression by some users.\n"},{"id":"448415","messageId":"CAP8UFD1bdLESqzbZcYKYfib836vrDTfyCmYfT-9B-1ToJB0EWg@mail.gmail.com","threadId":"57403","inReplyTo":"55dbe19a1a4d05d84c81356af1a3f04b65f8aa7b.1644565025.git.ps@pks.im","subject":"Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T08:11:55Z","receivedAt":"2022-02-15T08:12:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 14, 2022 at 10:13 PM Patrick Steinhardt wrote:\n>\n> When fetching references from a remote we by default also fetch all tags\n> which point into the history we have fetched. This is a separate step\n> performed after updating local references because it requires us to walk\n> over the history on the client-side to determine whether the remote has\n> announced any tags which point to one of the fetched commits.\n>\n> This backfilling of tags isn't covered by the `--atomic` flag: right\n> now, it only applies to the step where we update our local references.\n> This is an oversight at the time the flag was introduced: its purpose is\n> to either update all references or none, but right now we happily update\n> local references even in the case where backfilling failed.\n\nAlso it looks like the backfilling of tags itself isn't atomic, right?\nSome tags could be backfilled while others aren't.\n\n> Fix this by pulling up creation of the reference transaction such that\n> we can pass the same transaction to both the code which updates local\n> references and to the code which backfills tags. This allows us to only\n> commit the transaction in case both actions succeed.\n\nMaybe this could be seen as a regression by users who are mostly\ninterested in the local references though.\n\n> Note that we also have to start passing the transaction into\n> `find_non_local_tags()`: this function is responsible for finding all\n> tags which we need to backfill. Right now, it will happily return tags\n> which we have already been updated with our local references. But when\n\ns/we have/have/\n\n> we use a single transaction for both local references and backfilling\n> then it may happen that we try to queue the same reference update twice\n> to the transaction, which consequentially triggers a bug. We thus have\n\ns/consequentially/consequently/\n\n> to skip over any tags which have already been queued. Unfortunately,\n> this requires us to reach into internals of the reference transaction to\n> access queued updates, but there is no non-internal interface right now\n> which would allow us to access this information.\n\nThis makes me wonder if such a non-internal interface should be\nimplemented first. Or if some function to queue a reference update\ncould check if the same reference update has already been queued.\n"},{"id":"448424","messageId":"CAP8UFD3Fc9315jsbTFNzGunLyrm0P=zDLda2F=7O_5+B-ZtBOA@mail.gmail.com","threadId":"57403","inReplyTo":"682f16117b743bec59c533e15ae5a88d39250222.1644565025.git.ps@pks.im","subject":"Re: [PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-15T09:12:23Z","receivedAt":"2022-02-15T09:12:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 11, 2022 at 9:25 AM Patrick Steinhardt wrote:\n>\n> When fetching with the `--prune` flag we will delete any local\n> references matching the fetch refspec which have disappeared on the\n> remote. This step is not currently covered by the `--atomic` flag: we\n> delete branches even though updating of local references has failed,\n> which means that the fetch is not an all-or-nothing operation.\n\nIt could perhaps be seen as a regression by some users though, if\nupdating of local references doesn't work anymore when deleting a\nlocal reference matching the fetch refspec fails.\n\n> Fix this bug by passing in the global transaction into `prune_refs()`:\n> if one is given, then we'll only queue up deletions and not commit them\n> right away.\n>\n> This change also improves performance when pruning many branches in a\n> repository with a big packed-refs file: every references is pruned in\n> its own transaction, which means that we potentially have to rewrite\n> the packed-refs files for every single reference we're about to prune.\n\nYeah, I wonder if there could be a performance improvement in the\nprevious patch too as it looks like tag backfilling wasn't atomic too.\n\n> The following benchmark demonstrates this: it performs a pruning fetch\n> from a repository with a single reference into a repository with 100k\n> references, which causes us to prune all but one reference. This is of\n> course a very artificial setup, but serves to demonstrate the impact of\n> only having to write the packed-refs file once:\n>\n> Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n> Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]\n> Range (min … max): 2.328 s … 2.407 s 10 runs\n>\n> Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n> Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]\n> Range (min … max): 1.346 s … 1.400 s 10 runs\n>\n> Summary\n> 'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n> 1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n\nNice!\n"},{"id":"448646","messageId":"20220216233513.1973985-1-jonathantanmy@google.com","threadId":"57403","inReplyTo":"54fdee845bea7f67f46817417f8e5a504bd39665.1644565025.git.ps@pks.im","subject":"Re: [PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-16T23:35:12Z","receivedAt":"2022-02-16T23:35:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patrick Steinhardt writes:\n> @@ -1628,8 +1630,19 @@ static int do_fetch(struct transport *transport,\n> \t\tstruct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n> \n> \t\tfind_non_local_tags(remote_refs, &tags_ref_map, &tail);\n> -\t\tif (tags_ref_map)\n> -\t\t\tbackfill_tags(transport, tags_ref_map, &fetch_head, worktrees);\n> +\t\tif (tags_ref_map) {\n> +\t\t\t/*\n> +\t\t\t * If backfilling tags succeeds we used to not return\n> +\t\t\t * an error code to the user at all. Instead, we\n> +\t\t\t * silently swallowed that error and updated the local\n> +\t\t\t * state of the repository. We now notify the user of\n> +\t\t\t * any such errors, but we continue to make sure that\n> +\t\t\t * FETCH_HEAD and the upstream branch are configured as\n> +\t\t\t * expected.\n> +\t\t\t */\n> +\t\t\tif (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))\n> +\t\t\t\tretcode = 1;\n\nI think s/succeeds/fails/\n\nHaving said that, I don't think that we need to describe the past here.\nNotifying the user if something has failed is expected and doesn't\nrequire an explanation about how we didn't do that in the past.\n\nEverything up to this patch looks good to me.\n"},{"id":"448647","messageId":"20220216233915.1975217-1-jonathantanmy@google.com","threadId":"57403","inReplyTo":"682f16117b743bec59c533e15ae5a88d39250222.1644565025.git.ps@pks.im","subject":"Re: [PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-16T23:39:15Z","receivedAt":"2022-02-16T23:39:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patrick Steinhardt writes:\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 93a0db3c68..afa6bf9f7d 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -349,11 +349,9 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\n> \tcat >expected <<-EOF &&\n> \t\tprepared\n> \t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> -\t\tcommitted\n> -\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> -\t\tprepared\n> \t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n> \t\tcommitted\n> +\t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> \t\t$ZERO_OID $head_oid refs/remotes/origin/new-branch\n> \tEOF\n\nI think that there is a comment above this change that needs to be\ndeleted too.\n\nOverall, I think that this patch set is a good change, and I have no\nfurther comments. (Regarding patch 5, I can't think of a better way to\nknow what refs are duplicates.)\n"},{"id":"448648","messageId":"20220216234157.1976077-1-jonathantanmy@google.com","threadId":"57403","inReplyTo":"CAP8UFD1bdLESqzbZcYKYfib836vrDTfyCmYfT-9B-1ToJB0EWg@mail.gmail.com","subject":"Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-02-16T23:41:57Z","receivedAt":"2022-02-16T23:42:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Christian Couder writes:\n> > Fix this by pulling up creation of the reference transaction such that\n> > we can pass the same transaction to both the code which updates local\n> > references and to the code which backfills tags. This allows us to only\n> > commit the transaction in case both actions succeed.\n> \n> Maybe this could be seen as a regression by users who are mostly\n> interested in the local references though.\n\nI don't think we need to worry about this - as far as I know,\nbackfilling of tags only happens when the server doesn't support\ninclude-tag, which was introduced in 2009 (and in my experience, they\nall do). And in the rare case that backfilling happens, I think the user\nwould want to have atomicity in the also rare case that the first fetch\nsucceeds and the second fails.\n"},{"id":"448651","messageId":"CABPp-BFC-BavQEreJQ+APhxSNHenij568Mr-0ME+YF+7gzwtBQ@mail.gmail.com","threadId":"57403","inReplyTo":"54fdee845bea7f67f46817417f8e5a504bd39665.1644565025.git.ps@pks.im","subject":"Re: [PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-17T01:31:56Z","receivedAt":"2022-02-17T01:32:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Feb 11, 2022 at 12:03 PM Patrick Steinhardt wrote:\n>\n> When the backfilling of tags fails we do not report this error to the\n> caller, but only report it implicitly at a later point when reporting\n> updated references. This leaves callers unable to act upon the\n> information of whether the backfilling succeeded or not.\n>\n> Refactor the function to return an error code and pass it up the\n> callstack. This causes us to correctly propagate the error back to the\n> user of git-fetch(1).\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/fetch.c | 29 +++++++++++++++++++++--------\n> t/t5503-tagfollow.sh | 4 +---\n> 2 files changed, 22 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 627847e2f8..1eda0b68ff 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n> return transport;\n> }\n>\n> -static void backfill_tags(struct transport *transport,\n> - struct ref *ref_map,\n> - struct fetch_head *fetch_head,\n> - struct worktree **worktrees)\n> +static int backfill_tags(struct transport *transport,\n> + struct ref *ref_map,\n> + struct fetch_head *fetch_head,\n> + struct worktree **worktrees)\n> {\n> - int cannot_reuse;\n> + int retcode, cannot_reuse;\n>\n> /*\n> * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it\n> @@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,\n> transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n> transport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n> transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n> - fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n> + retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);\n>\n> if (gsecondary) {\n> transport_disconnect(gsecondary);\n> gsecondary = NULL;\n> }\n> +\n> + return retcode;\n> }\n>\n> static int do_fetch(struct transport *transport,\n> @@ -1628,8 +1630,19 @@ static int do_fetch(struct transport *transport,\n> struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n>\n> find_non_local_tags(remote_refs, &tags_ref_map, &tail);\n> - if (tags_ref_map)\n> - backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);\n> + if (tags_ref_map) {\n> + /*\n> + * If backfilling tags succeeds we used to not return\n> + * an error code to the user at all. Instead, we\n> + * silently swallowed that error and updated the local\n> + * state of the repository. We now notify the user of\n> + * any such errors, but we continue to make sure that\n> + * FETCH_HEAD and the upstream branch are configured as\n> + * expected.\n> + */\n\nnit: I'd prefer to see code comments explain what we currently do, and\nmove history lessons to the commit message.\n\n\n> + if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))\n> + retcode = 1;\n> + }\n>\n> free_refs(tags_ref_map);\n> }\n> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n> index 888305ad4d..549f908b90 100755\n> --- a/t/t5503-tagfollow.sh\n> +++ b/t/t5503-tagfollow.sh\n> @@ -237,9 +237,7 @@ test_expect_success 'backfill failure causes command to fail' '\n> done\n> EOF\n>\n> - # Even though we fail to create refs/tags/tag1 the below command\n> - # unexpectedly succeeds.\n> - git -C clone5 fetch .. $B:refs/heads/something &&\n> + test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&\n> test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&\n> test $S = $(git -C clone5 rev-parse --verify tag2) &&\n> test_must_fail git -C clone5 rev-parse --verify tag1\n> --\n> 2.35.1\n>\n"},{"id":"448652","messageId":"CABPp-BFM67LzZFL=w-iA7vButaBKokpetDR5dr8TTnbSCmBdeA@mail.gmail.com","threadId":"57403","inReplyTo":"55dbe19a1a4d05d84c81356af1a3f04b65f8aa7b.1644565025.git.ps@pks.im","subject":"Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-17T01:34:23Z","receivedAt":"2022-02-17T01:34:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 14, 2022 at 1:32 AM Patrick Steinhardt wrote:\n>\n> When fetching references from a remote we by default also fetch all tags\n> which point into the history we have fetched. This is a separate step\n> performed after updating local references because it requires us to walk\n> over the history on the client-side to determine whether the remote has\n> announced any tags which point to one of the fetched commits.\n>\n> This backfilling of tags isn't covered by the `--atomic` flag: right\n> now, it only applies to the step where we update our local references.\n> This is an oversight at the time the flag was introduced: its purpose is\n> to either update all references or none, but right now we happily update\n> local references even in the case where backfilling failed.\n>\n> Fix this by pulling up creation of the reference transaction such that\n> we can pass the same transaction to both the code which updates local\n> references and to the code which backfills tags. This allows us to only\n> commit the transaction in case both actions succeed.\n>\n> Note that we also have to start passing the transaction into\n> `find_non_local_tags()`: this function is responsible for finding all\n> tags which we need to backfill. Right now, it will happily return tags\n> which we have already been updated with our local references. But when\n> we use a single transaction for both local references and backfilling\n> then it may happen that we try to queue the same reference update twice\n> to the transaction, which consequentially triggers a bug. We thus have\n> to skip over any tags which have already been queued. Unfortunately,\n> this requires us to reach into internals of the reference transaction to\n> access queued updates, but there is no non-internal interface right now\n> which would allow us to access this information.\n\nI like the changes you are making here in general, but I do agree that\nreaching into refs-internal feels a bit icky. I'm not familiar with\nthe refs API nor the fetching code, so feel free to ignore these\nideas, but I'm just throwing them out there as possibilities to avoid\nreaching into refs-internal:\n\n - you are trying to check for existing transactions to avoid\nduplicates, but those existing transactions came from elsewhere in the\nsame code we control. Could we store a strset or strmap of the items\nbeing updated (in addition to storing them in the transaction), and\nthen use the strset/strmap to filter out which tags we need to\nbackfill? Or would that require plumbing an extra variable through an\nawful lot of callers to get the information into the right places?\n\n - would it make sense to add a flag to the transaction API to allow\nduplicates if both updates update the ref to the same value? (I'm\nguessing you're updating to the same value, right?)\n\n - should we just add something to the refs API along the lines of\n\"transaction_includes_update_for()\" or something like that?\n\n[...]\n> @@ -361,12 +362,28 @@ static void find_non_local_tags(const struct ref *refs,\n> const struct ref *ref;\n> struct refname_hash_entry *item = NULL;\n> const int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;\n> + int i;\n>\n> refname_hash_init(&existing_refs);\n> refname_hash_init(&remote_refs);\n> create_fetch_oidset(head, &fetch_oids);\n>\n> for_each_ref(add_one_refname, &existing_refs);\n> +\n> + /*\n> + * If we already have a transaction, then we need to filter out all\n> + * tags which have already been queued up.\n> + */\n> + for (i = 0; transaction && i < transaction->nr; i++) {\n> + if (!starts_with(transaction->updates[i]->refname, \"refs/tags/\") ||\n> + !(transaction->updates[i]->flags & REF_HAVE_NEW))\n> + continue;\n> + (void) refname_hash_add(&existing_refs,\n> + transaction->updates[i]->refname,\n> + &transaction->updates[i]->new_oid);\n\nWhy the typecast here?\n"},{"id":"448653","messageId":"CABPp-BE9_RVu28C-6QuY2qDYaEExeqCqph0e37DgiFtPZRHY2A@mail.gmail.com","threadId":"57403","inReplyTo":"682f16117b743bec59c533e15ae5a88d39250222.1644565025.git.ps@pks.im","subject":"Re: [PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-17T01:40:19Z","receivedAt":"2022-02-17T01:40:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Feb 11, 2022 at 12:25 AM Patrick Steinhardt wrote:\n>\n> When fetching with the `--prune` flag we will delete any local\n> references matching the fetch refspec which have disappeared on the\n> remote. This step is not currently covered by the `--atomic` flag: we\n> delete branches even though updating of local references has failed,\n> which means that the fetch is not an all-or-nothing operation.\n>\n> Fix this bug by passing in the global transaction into `prune_refs()`:\n> if one is given, then we'll only queue up deletions and not commit them\n> right away.\n>\n> This change also improves performance when pruning many branches in a\n> repository with a big packed-refs file: every references is pruned in\n> its own transaction, which means that we potentially have to rewrite\n> the packed-refs files for every single reference we're about to prune.\n>\n> The following benchmark demonstrates this: it performs a pruning fetch\n> from a repository with a single reference into a repository with 100k\n> references, which causes us to prune all but one reference. This is of\n> course a very artificial setup, but serves to demonstrate the impact of\n> only having to write the packed-refs file once:\n>\n> Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n> Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]\n> Range (min … max): 2.328 s … 2.407 s 10 runs\n>\n> Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n> Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]\n> Range (min … max): 1.346 s … 1.400 s 10 runs\n>\n> Summary\n> 'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n> 1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n\nVery nice!\n\n[...]\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 93a0db3c68..afa6bf9f7d 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -349,11 +349,9 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\n> cat >expected <<-EOF &&\n> prepared\n> $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> - committed\n> - $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> - prepared\n> $ZERO_OID $head_oid refs/remotes/origin/new-branch\n> committed\n\nUp to here this is just what I expected.\n\n> + $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> $ZERO_OID $head_oid refs/remotes/origin/new-branch\n\nBut now scheduled-for-deletion and new-branch are both listed again\neven with your fixes? Is this some peculiarity of the reference\ntransaction hook that it lists the refs again after the\nprepared...committed block? (It may well be; I'm not that familiar\nwith this area of the code.)\n"},{"id":"448681","messageId":"Yg4tyssgW9OAu6K/@ncase","threadId":"57403","inReplyTo":"CAP8UFD2HswmXhqYTAMxQ0iYFtsErMS=DB18iv52Ujs=cAW9ytw@mail.gmail.com","subject":"Re: [PATCH 1/6] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T11:13:14Z","receivedAt":"2022-02-17T11:13:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 15, 2022 at 07:19:19AM +0100, Christian Couder wrote:\n> On Fri, Feb 11, 2022 at 8:38 PM Patrick Steinhardt wrote:\n> >\n> > The `--atomic` flag is missing test coverage for pruning of deleted\n> > references and backfilling of tags, and of course both aren't covered\n> > correctly by this flag.\n> \n> It's not clear to me what \"both aren't covered correctly by this flag\"\n> actually means here. If it means that pruning of deleted references\n> and backfilling of tags don't work correctly when --atomic is used,\n> then it could be stated more clearly. Otherwise this seems to just be\n> repeating the first part of the sentence.\n\nYeah, the commit message was a bit lazy to be honest. I've reworded it\nto hopefully make clearer what one is looking at.\n\n> > Furthermore, we don't have tests demonstrating\n> > error cases for backfilling tags.\n> >\n> > Add tests to cover those testing gaps.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> \n> > +test_expect_success 'atomic fetch with failing backfill' '\n> > + git init clone3 &&\n> > +\n> > + # We want to test whether a failure when backfilling tags correctly\n> > + # aborts the complete transaction when `--atomic` is passed: we should\n> > + # neither create the branch nor should we create the tag when either\n> > + # one of both fails to update correctly.\n> > + #\n> > + # To trigger failure we simply abort when backfilling a tag.\n> > + write_script clone3/.git/hooks/reference-transaction <<-\\EOF &&\n> > + #!/bin/sh\n> > +\n> > + while read oldrev newrev reference\n> > + do\n> > + if test \"$reference\" = refs/tags/tag1\n> > + then\n> > + exit 1\n> > + fi\n> > + done\n> > + EOF\n> > +\n> > + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&\n> > +\n> > + # Creation of the tag has failed, so ideally refs/heads/something\n> > + # should not exist. The fact that it does is demonstrates that there is\n> \n> s/The fact that it does is demonstrates/The fact that it does demonstrates/\n> \n> > + # missing coverage in the `--atomic` flag.\n> \n> Maybe s/missing coverage/a bug/ would make things clearer.\n> \n> > + test $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n> > +'\n> \n> As this patch series is about fixing buggy parts of the behavior with\n> --atomic, I think it would make more sense to use test_expect_failure,\n> instead of test_expect_success, in this test, and to check that we\n> have the correct behavior, instead of checking that we have the buggy\n> behavior.\n> \n> Of course when later in this patch series the buggy behavior is fixed,\n> then test_expect_failure should be replaced with test_expect_success.\n\nThe downside of using `test_expect_failure` is that one cannot easily\nsee what exactly is broken in the testcase. Yes, you can document it,\nbut when using `test_expect_success` the huge advantage is that you can\nsee exactly what behaviour is changing in subsequent commits by just having a look\nat the diff of the test which adapts it from its initially-broken state\nto the newly-fixed behaviour.\n\n> > +test_expect_success 'atomic fetch with backfill should use single transaction' '\n> > + git init clone4 &&\n> > +\n> > + # Fetching with the `--atomic` flag should update all references in a\n> > + # single transaction, including backfilled tags. We thus expect to see\n> > + # a single reference transaction for the created branch and tags.\n> > + cat >expected <<-EOF &&\n> > + prepared\n> > + $ZERO_OID $B refs/heads/something\n> > + $ZERO_OID $S refs/tags/tag2\n> > + committed\n> > + $ZERO_OID $B refs/heads/something\n> > + $ZERO_OID $S refs/tags/tag2\n> > + prepared\n> > + $ZERO_OID $T refs/tags/tag1\n> > + committed\n> > + $ZERO_OID $T refs/tags/tag1\n> > + EOF\n> \n> The comment says that we expect to see a single reference transaction,\n> but the expected file we create seems to show 2 transactions. So I\n> think here too, we should use test_expect_failure, instead of\n> test_expect_success, and check that we have the correct behavior\n> instead of a buggy one.\n\nSame comment as above. I've also amended the commit message to say why\nwe're introducing the tests like this.\n\n> > + write_script clone4/.git/hooks/reference-transaction <<-\\EOF &&\n> \n> Here there is no #!/bin/sh while other uses of write_script in your\n> patch have it. If it's not necessary, it could be removed in the other\n> uses.\n\nGood point, I always forget that the shebang is added automatically by\nthis helper.\n\n> > + ( echo \"$*\" && cat ) >>actual\n> > + EOF\n> > +\n> > + git -C clone4 fetch --atomic .. $B:refs/heads/something &&\n> > + test_cmp expected clone4/actual\n> > +'\n> \n> I took a quick look at the 2 other tests after this one, and I think\n> test_expect_failure should be used there too, instead of\n> test_expect_success.\n\nPatrick\n"},{"id":"448682","messageId":"Yg4vF/ph9yQRTdMM@ncase","threadId":"57403","inReplyTo":"CAP8UFD32MQSVQ2uUhmO29jFz=LfqvN8U7e-a=sDgQAxUh+Gadw@mail.gmail.com","subject":"Re: [PATCH 3/6] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T11:18:47Z","receivedAt":"2022-02-17T11:18:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 15, 2022 at 08:32:56AM +0100, Christian Couder wrote:\n> On Sat, Feb 12, 2022 at 5:49 PM Patrick Steinhardt wrote:\n> >\n> > There are two different locations where we're appending to FETCH_HEAD:\n> > first when storing updated references, and second when backfilling tags.\n> > Both times we open the file, append to it and then commit it into place,\n> > which is essentially duplicate work.\n> >\n> > Improve the lifecycle of updating FETCH_HEAD by opening and committing\n> > it once in `do_fetch()`, where we pass the structure down to code which\n> \n> s/down to code/down to the code/\n> \n> > wants to append to it.\n> \n> > @@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,\n> > if (!update_head_ok)\n> > check_not_current_branch(ref_map, worktrees);\n> >\n> > + retcode = open_fetch_head(&fetch_head);\n> > + if (retcode)\n> > + goto cleanup;\n> > +\n> > if (tags == TAGS_DEFAULT && autotags)\n> > transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n> > if (prune) {\n> > @@ -1617,7 +1616,8 @@ static int do_fetch(struct transport *transport,\n> > transport->url);\n> > }\n> > }\n> > - if (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n> > +\n> > + if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {\n> > retcode = 1;\n> > goto cleanup;\n> > }\n> \n> I think the following (if it works) would be more consistent with\n> what's done for open_fetch_head() above:\n> \n> retcode = fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)\n> if (retcode)\n> goto cleanup;\n\nThe code here is really quite fragile, and I'm not much of a fan how we\nneed to retain `retcode` across the calls. But we can't change it like\nyou propose because the preceding call to `prune_refs()` may have\nalready set `retcode = 1`, and we must not clobber that value in case\nthe fetch succeeds. The effect is thus that we have `retcode == 1` if\neither `prune_refs()` or `fetch_and_consume_refs()` fails.\n\nPatrick\n"},{"id":"448683","messageId":"Yg4xEQmXyzSeYZ1k@ncase","threadId":"57403","inReplyTo":"CAP8UFD0MQCpFfWg2FP3S1VpDAxCwp3WGucFPn7K9BZ7UnckNLw@mail.gmail.com","subject":"Re: [PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T11:27:13Z","receivedAt":"2022-02-17T11:27:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 15, 2022 at 08:52:14AM +0100, Christian Couder wrote:\n> On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt wrote:\n> >\n> > When the backfilling of tags fails we do not report this error to the\n> > caller, but only report it implicitly at a later point when reporting\n> > updated references.\n> \n> Probably stupid question: are we sure that it's a bug and not a feature?\n\nGood question, and I don't have a definitive answer for it. But to me it\nvery much smells like a bug: if I ask for a fetch and the fetch fails to\npopulate some of the data I have asked for, then I want to get a\nnotification on that failure.\n\n> > This leaves callers unable to act upon the\n> > information of whether the backfilling succeeded or not.\n> >\n> > Refactor the function to return an error code and pass it up the\n> > callstack. This causes us to correctly propagate the error back to the\n> > user of git-fetch(1).\n> \n> Even if it would have been the right behavior when backfilling tags\n> was implemented to return an error when backfilling tags fails, I\n> think it's interesting to ask ourselves if this change could be seen\n> as a regression by some users.\n\nYeah, it's not all that clear-cut because auto-following of tags is a\nbit obscure. But our default behaviour is to fetch tags pointing into\nthe history, and if a user didn't want that they should've passed\n`--no-tags` to git-fetch(1). So conversely, we should assume that a user\nis asking for auto-filling of tags if we're not told otherwise, which\nalso means that it is a failure if this fails.\n\nAt least that's my take, but I'm happy to hear arguments against my\nviewpoint.\n\nPatrick\n"},{"id":"448684","messageId":"Yg4ze+TMUWEQmszJ@ncase","threadId":"57403","inReplyTo":"CAP8UFD1bdLESqzbZcYKYfib836vrDTfyCmYfT-9B-1ToJB0EWg@mail.gmail.com","subject":"Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T11:37:31Z","receivedAt":"2022-02-17T11:37:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 15, 2022 at 09:11:55AM +0100, Christian Couder wrote:\n> On Mon, Feb 14, 2022 at 10:13 PM Patrick Steinhardt wrote:\n> >\n> > When fetching references from a remote we by default also fetch all tags\n> > which point into the history we have fetched. This is a separate step\n> > performed after updating local references because it requires us to walk\n> > over the history on the client-side to determine whether the remote has\n> > announced any tags which point to one of the fetched commits.\n> >\n> > This backfilling of tags isn't covered by the `--atomic` flag: right\n> > now, it only applies to the step where we update our local references.\n> > This is an oversight at the time the flag was introduced: its purpose is\n> > to either update all references or none, but right now we happily update\n> > local references even in the case where backfilling failed.\n> \n> Also it looks like the backfilling of tags itself isn't atomic, right?\n> Some tags could be backfilled while others aren't.\n\nRight.\n\n> > Fix this by pulling up creation of the reference transaction such that\n> > we can pass the same transaction to both the code which updates local\n> > references and to the code which backfills tags. This allows us to only\n> > commit the transaction in case both actions succeed.\n> \n> Maybe this could be seen as a regression by users who are mostly\n> interested in the local references though.\n\nEven though the commit message discern \"local references\" and\n\"backfilled tags\", ultimately they're the same. Both are references that\nend up in your local refdb, so from the point of the user there is no\nreal difference here. Documentation of the `--atomic` flag only says\nthat \"either all refs are updared, or on error, no refs are updated\". I\nthink that the current behaviour does not fit the description.\n\n> > Note that we also have to start passing the transaction into\n> > `find_non_local_tags()`: this function is responsible for finding all\n> > tags which we need to backfill. Right now, it will happily return tags\n> > which we have already been updated with our local references. But when\n> \n> s/we have/have/\n> \n> > we use a single transaction for both local references and backfilling\n> > then it may happen that we try to queue the same reference update twice\n> > to the transaction, which consequentially triggers a bug. We thus have\n> \n> s/consequentially/consequently/\n> \n> > to skip over any tags which have already been queued. Unfortunately,\n> > this requires us to reach into internals of the reference transaction to\n> > access queued updates, but there is no non-internal interface right now\n> > which would allow us to access this information.\n> \n> This makes me wonder if such a non-internal interface should be\n> implemented first. Or if some function to queue a reference update\n> could check if the same reference update has already been queued.\n\nYeah. I noted that ommission in the cover letter already, but didn't yet\nwant to fix that before getting some initial feedback. I'll add\nsomething like a `for_each_queued_reference_update()` in v2 of this\nseries though.\n\nPatrick\n"},{"id":"448685","messageId":"Yg44YMVniOT6RVrv@ncase","threadId":"57403","inReplyTo":"CABPp-BFM67LzZFL=w-iA7vButaBKokpetDR5dr8TTnbSCmBdeA@mail.gmail.com","subject":"Re: [PATCH 5/6] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T11:58:24Z","receivedAt":"2022-02-17T11:58:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 16, 2022 at 05:34:23PM -0800, Elijah Newren wrote:\n> On Mon, Feb 14, 2022 at 1:32 AM Patrick Steinhardt wrote:\n> >\n> > When fetching references from a remote we by default also fetch all tags\n> > which point into the history we have fetched. This is a separate step\n> > performed after updating local references because it requires us to walk\n> > over the history on the client-side to determine whether the remote has\n> > announced any tags which point to one of the fetched commits.\n> >\n> > This backfilling of tags isn't covered by the `--atomic` flag: right\n> > now, it only applies to the step where we update our local references.\n> > This is an oversight at the time the flag was introduced: its purpose is\n> > to either update all references or none, but right now we happily update\n> > local references even in the case where backfilling failed.\n> >\n> > Fix this by pulling up creation of the reference transaction such that\n> > we can pass the same transaction to both the code which updates local\n> > references and to the code which backfills tags. This allows us to only\n> > commit the transaction in case both actions succeed.\n> >\n> > Note that we also have to start passing the transaction into\n> > `find_non_local_tags()`: this function is responsible for finding all\n> > tags which we need to backfill. Right now, it will happily return tags\n> > which we have already been updated with our local references. But when\n> > we use a single transaction for both local references and backfilling\n> > then it may happen that we try to queue the same reference update twice\n> > to the transaction, which consequentially triggers a bug. We thus have\n> > to skip over any tags which have already been queued. Unfortunately,\n> > this requires us to reach into internals of the reference transaction to\n> > access queued updates, but there is no non-internal interface right now\n> > which would allow us to access this information.\n> \n> I like the changes you are making here in general, but I do agree that\n> reaching into refs-internal feels a bit icky. I'm not familiar with\n> the refs API nor the fetching code, so feel free to ignore these\n> ideas, but I'm just throwing them out there as possibilities to avoid\n> reaching into refs-internal:\n> \n> - you are trying to check for existing transactions to avoid\n> duplicates, but those existing transactions came from elsewhere in the\n> same code we control. Could we store a strset or strmap of the items\n> being updated (in addition to storing them in the transaction), and\n> then use the strset/strmap to filter out which tags we need to\n> backfill? Or would that require plumbing an extra variable through an\n> awful lot of callers to get the information into the right places?\n\nWe basically would need to plumb through the variable to most callsites\nwhich also get the transaction as input, and that's rather deep into the\ncallstack. The reason I think it's preferable to instead use the\ntransaction is that it holds the definitive state of all updates we have\nalready queued, and thus we cannot accidentally forget to update another\nauxiliary variable.\n\n> - would it make sense to add a flag to the transaction API to allow\n> duplicates if both updates update the ref to the same value? (I'm\n> guessing you're updating to the same value, right?)\n\nIt should be the same value, yes. There is a race though in the context\nof tag backfilling: if the initial fetch pulls in some tags, then it can\nhappen that second fetch used in some cases for the backfilling\nmechanism pulls in the same tag references but with different target\nobjects. It's an unlikely thing to happen, but cannot be ruled out a\n100%. As Jonathan pointed out the backfilling-fetch is only used when\nthe transport does not use \"include-tag\" though.\n\nThe result in that case would be that the transaction aborts because of\nduplicate addition of the same ref with different values. And I'd say\nthat this is correct behaviour in case the user asked for an atomic\nfetch.\n\n> - should we just add something to the refs API along the lines of\n> \"transaction_includes_update_for()\" or something like that?\n\nI think something in the spirit of this last option would be the easiest\nsolution. Using `includes_updates_for()` or the above solution of a flag\nwhich avoids duplicate updates would potentially be quadratic in\nbehaviour though if implemented naively: we need to walk all queued\nupdates for each of the tags we want to queue. That's easy enough to\navoid if we just add a `for_each_queued_reference_update()` and then\ncontinue to do the same thing like we below. It also gives us greater\nflexibility compared to the other alternatives.\n\n> [...]\n> > @@ -361,12 +362,28 @@ static void find_non_local_tags(const struct ref *refs,\n> > const struct ref *ref;\n> > struct refname_hash_entry *item = NULL;\n> > const int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;\n> > + int i;\n> >\n> > refname_hash_init(&existing_refs);\n> > refname_hash_init(&remote_refs);\n> > create_fetch_oidset(head, &fetch_oids);\n> >\n> > for_each_ref(add_one_refname, &existing_refs);\n> > +\n> > + /*\n> > + * If we already have a transaction, then we need to filter out all\n> > + * tags which have already been queued up.\n> > + */\n> > + for (i = 0; transaction && i < transaction->nr; i++) {\n> > + if (!starts_with(transaction->updates[i]->refname, \"refs/tags/\") ||\n> > + !(transaction->updates[i]->flags & REF_HAVE_NEW))\n> > + continue;\n> > + (void) refname_hash_add(&existing_refs,\n> > + transaction->updates[i]->refname,\n> > + &transaction->updates[i]->new_oid);\n> \n> Why the typecast here?\n\n`refname_hash_add()` returns the newly added entry, and we don't care\nabout that. `add_one_refname()` has the same cast, potentially to\ndemonstrate that we don't need the return value? Compilers shouldn't\ncare I think, but on the other hand some static analysis sites like\nCoverity use to complain about such things.\n\nPatrick\n"},{"id":"448686","messageId":"Yg45d4boNwsRhhPr@ncase","threadId":"57403","inReplyTo":"CAP8UFD3Fc9315jsbTFNzGunLyrm0P=zDLda2F=7O_5+B-ZtBOA@mail.gmail.com","subject":"Re: [PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T12:03:03Z","receivedAt":"2022-02-17T12:03:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 15, 2022 at 10:12:23AM +0100, Christian Couder wrote:\n> On Fri, Feb 11, 2022 at 9:25 AM Patrick Steinhardt wrote:\n> >\n> > When fetching with the `--prune` flag we will delete any local\n> > references matching the fetch refspec which have disappeared on the\n> > remote. This step is not currently covered by the `--atomic` flag: we\n> > delete branches even though updating of local references has failed,\n> > which means that the fetch is not an all-or-nothing operation.\n> \n> It could perhaps be seen as a regression by some users though, if\n> updating of local references doesn't work anymore when deleting a\n> local reference matching the fetch refspec fails.\n\nI guess the same comment applies here as for the other patch: the\ndocumentation says that we either update all or no refs, and that's not\nwhat was happening previous to this patch.\n\n> > Fix this bug by passing in the global transaction into `prune_refs()`:\n> > if one is given, then we'll only queue up deletions and not commit them\n> > right away.\n> >\n> > This change also improves performance when pruning many branches in a\n> > repository with a big packed-refs file: every references is pruned in\n> > its own transaction, which means that we potentially have to rewrite\n> > the packed-refs files for every single reference we're about to prune.\n> \n> Yeah, I wonder if there could be a performance improvement in the\n> previous patch too as it looks like tag backfilling wasn't atomic too.\n\nI doubt it would be as measurable as it is here. The reason why we have\nthis speedup is that for every deleted ref, we need to rewrite the\ncomplete contents of the packed-refs file only with that single ref\nremoved from it. So for 10k refs, we essentially write the file 10k\ntimes.\n\nFor the backfilling case that doesn't happen though: we only write the\nnew loose refs, and that's not any faster in case we use a single\ntransaction. Sure, we'll likely be able to shed some of the overhead by\nusing a single transaction only, but it will not be as pronounced as it\nis here.\n\nThis will be different though as soon as the reftable backend lands:\nthere we'd write all new refs in a single slice, and that's definitely\nmore efficient than writing one slice per backfilled ref.\n\nPatrick\n\n> > The following benchmark demonstrates this: it performs a pruning fetch\n> > from a repository with a single reference into a repository with 100k\n> > references, which causes us to prune all but one reference. This is of\n> > course a very artificial setup, but serves to demonstrate the impact of\n> > only having to write the packed-refs file once:\n> >\n> > Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n> > Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]\n> > Range (min … max): 2.328 s … 2.407 s 10 runs\n> >\n> > Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n> > Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]\n> > Range (min … max): 1.346 s … 1.400 s 10 runs\n> >\n> > Summary\n> > 'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n> > 1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n> \n> Nice!\n"},{"id":"448687","messageId":"Yg46QLSB5VFnK/M5@ncase","threadId":"57403","inReplyTo":"CABPp-BE9_RVu28C-6QuY2qDYaEExeqCqph0e37DgiFtPZRHY2A@mail.gmail.com","subject":"Re: [PATCH 6/6] fetch: make `--atomic` flag cover pruning of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T12:06:24Z","receivedAt":"2022-02-17T12:06:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 16, 2022 at 05:40:19PM -0800, Elijah Newren wrote:\n> On Fri, Feb 11, 2022 at 12:25 AM Patrick Steinhardt wrote:\n> >\n> > When fetching with the `--prune` flag we will delete any local\n> > references matching the fetch refspec which have disappeared on the\n> > remote. This step is not currently covered by the `--atomic` flag: we\n> > delete branches even though updating of local references has failed,\n> > which means that the fetch is not an all-or-nothing operation.\n> >\n> > Fix this bug by passing in the global transaction into `prune_refs()`:\n> > if one is given, then we'll only queue up deletions and not commit them\n> > right away.\n> >\n> > This change also improves performance when pruning many branches in a\n> > repository with a big packed-refs file: every references is pruned in\n> > its own transaction, which means that we potentially have to rewrite\n> > the packed-refs files for every single reference we're about to prune.\n> >\n> > The following benchmark demonstrates this: it performs a pruning fetch\n> > from a repository with a single reference into a repository with 100k\n> > references, which causes us to prune all but one reference. This is of\n> > course a very artificial setup, but serves to demonstrate the impact of\n> > only having to write the packed-refs file once:\n> >\n> > Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)\n> > Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]\n> > Range (min … max): 2.328 s … 2.407 s 10 runs\n> >\n> > Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)\n> > Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]\n> > Range (min … max): 1.346 s … 1.400 s 10 runs\n> >\n> > Summary\n> > 'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran\n> > 1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'\n> \n> Very nice!\n> \n> [...]\n> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > index 93a0db3c68..afa6bf9f7d 100755\n> > --- a/t/t5510-fetch.sh\n> > +++ b/t/t5510-fetch.sh\n> > @@ -349,11 +349,9 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\n> > cat >expected <<-EOF &&\n> > prepared\n> > $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> > - committed\n> > - $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> > - prepared\n> > $ZERO_OID $head_oid refs/remotes/origin/new-branch\n> > committed\n> \n> Up to here this is just what I expected.\n> \n> > + $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n> > $ZERO_OID $head_oid refs/remotes/origin/new-branch\n> \n> But now scheduled-for-deletion and new-branch are both listed again\n> even with your fixes? Is this some peculiarity of the reference\n> transaction hook that it lists the refs again after the\n> prepared...committed block? (It may well be; I'm not that familiar\n> with this area of the code.)\n\nYes, the reference-transaction hook is executed for each of the\n\"prepared\", \"committed\" and \"aborted\" phases and gets as input all the\nrefs that have been queued for that phase.\n\nPatrick\n"},{"id":"448688","messageId":"Yg5Dz/VNW+2osN1s@ncase","threadId":"57403","inReplyTo":"Yg4xEQmXyzSeYZ1k@ncase","subject":"Re: [PATCH 4/6] fetch: report errors when backfilling tags fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T12:47:11Z","receivedAt":"2022-02-17T12:47:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 17, 2022 at 12:27:15PM +0100, Patrick Steinhardt wrote:\n> On Tue, Feb 15, 2022 at 08:52:14AM +0100, Christian Couder wrote:\n> > On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt wrote:\n> > >\n> > > When the backfilling of tags fails we do not report this error to the\n> > > caller, but only report it implicitly at a later point when reporting\n> > > updated references.\n> > \n> > Probably stupid question: are we sure that it's a bug and not a feature?\n> \n> Good question, and I don't have a definitive answer for it. But to me it\n> very much smells like a bug: if I ask for a fetch and the fetch fails to\n> populate some of the data I have asked for, then I want to get a\n> notification on that failure.\n> \n> > > This leaves callers unable to act upon the\n> > > information of whether the backfilling succeeded or not.\n> > >\n> > > Refactor the function to return an error code and pass it up the\n> > > callstack. This causes us to correctly propagate the error back to the\n> > > user of git-fetch(1).\n> > \n> > Even if it would have been the right behavior when backfilling tags\n> > was implemented to return an error when backfilling tags fails, I\n> > think it's interesting to ask ourselves if this change could be seen\n> > as a regression by some users.\n> \n> Yeah, it's not all that clear-cut because auto-following of tags is a\n> bit obscure. But our default behaviour is to fetch tags pointing into\n> the history, and if a user didn't want that they should've passed\n> `--no-tags` to git-fetch(1). So conversely, we should assume that a user\n> is asking for auto-filling of tags if we're not told otherwise, which\n> also means that it is a failure if this fails.\n> \n> At least that's my take, but I'm happy to hear arguments against my\n> viewpoint.\n> \n> Patrick\n\nI just noticed that we have in fact landed a change in the exact same\nspirit on `main` via c9e04d905e (fetch --prune: exit with error if\npruning fails, 2022-01-31). So there is precedent that we fix up these\nmissing error codes, and that gives me more confidence that doing the\nsame fixup for the tag-backfill is the correct thing to do.\n\nPatrick\n"}]}