{"thread":{"id":"57431","subject":"[PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag","startedAt":"2022-02-17T13:04:24Z","lastAt":"2022-03-03T06:51:19Z","messageCount":35,"participants":["Patrick Steinhardt","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"448689","messageId":"cover.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":null,"subject":"[PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:11Z","receivedAt":"2022-02-17T13:04:24Z","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 is the second version of my patch series which fixes these\nproblems. Due to a semantic conflict with\nps/avoid-unnecessary-hook-invocation-with-packed-refs and a textual\nconflict with c9e04d905e (fetch --prune: exit with error if pruning\nfails, 2022-01-31) it now applies on top of the former branch merged\nwith 45fe28c951 (The fourth batch, 2022-02-16).\n\nChanges compared to v1:\n\n - Commit messages have been improved.\n\n - A code comment has been fixed to not talk about the past anymore,\n but instead state what the section of code is supposed to do.\n\n - I have introduced a new patch 5/7 which provides an interface to\n access queued updates in reference transactions without requiring\n access to \"refs/refs-internal.h\".\n\n - Removed unnecessary shebangs in tests when using `write_script`.\n\n - Rebased the series to fix a conflict with c9e04d905e, mentioned\n above.\n\nThanks for all the feedback!\n\nPatrick\n\nPatrick Steinhardt (7):\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 refs: add interface to iterate over queued transactional updates\n fetch: make `--atomic` flag cover backfilling of tags\n fetch: make `--atomic` flag cover pruning of refs\n\n builtin/fetch.c | 192 +++++++++++++++++++++++++++++--------------\n refs.c | 16 ++++\n refs.h | 14 ++++\n t/t5503-tagfollow.sh | 74 +++++++++++++++++\n t/t5510-fetch.sh | 29 +++++++\n 5 files changed, 263 insertions(+), 62 deletions(-)\n\nRange-diff against v1:\n1: e133c14f56 ! 1: b4ca3f1f3b fetch: increase test coverage of fetches\n @@ Metadata\n ## Commit message ##\n fetch: increase test coverage of fetches\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. Furthermore, we don't have tests demonstrating\n - error cases for backfilling tags.\n + When using git-fetch(1) with the `--atomic` flag the expectation is that\n + either all of the references are updated, or alternatively none are in\n + case the fetch fails. While we already have tests for this, we do not\n + have any tests which exercise atomicity either when pruning deleted refs\n + or when backfilling tags. This gap in test coverage hides that we indeed\n + don't handle atomicity correctly for both of these cases.\n \n - Add tests to cover those testing gaps.\n + Add test cases which cover these testing gaps to demonstrate the broken\n + behaviour. Note that tests are not marked as `test_expect_failure`: this\n + is done to explicitly demonstrate the current known-wrong behaviour, and\n + they will be fixed up as soon as we fix the underlying bugs.\n +\n + While at it this commit also adds another test case which demonstrates\n + that backfilling of tags does not return an error code in case the\n + backfill fails. This bug will also be fixed by a subsequent commit.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '\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/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '\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 ++\t# should not exist. The fact that it does demonstrates that there is\n ++\t# a bug in the `--atomic` flag.\n +\ttest $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n +'\n +\n @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '\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\n2: 64c94e7a28 ! 2: b0a067bbc1 fetch: backfill tags before setting upstream\n @@ Commit message\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 - a `skip` label which jumps from (3) to (4) in case we fail to update the\n + both (2) and (4), which is further stressed by the point that we use a\n + `skip` goto label to jump from (3) to (4) in case we fail to update the\n gitconfig as expected.\n \n Reorder the code to first update all local references, and only after we\n @@ builtin/fetch.c: static void backfill_tags(struct transport *transport, struct r\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \t\t}\n + \t\t\tretcode = 1;\n \t}\n \tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n -\t\tfree_refs(ref_map);\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\tgoto cleanup;\n \t}\n \n -+\t/* if neither --no-tags nor --tags was specified, do automated tag\n -+\t * following ... */\n ++\t/*\n ++\t * If neither --no-tags nor --tags was specified, do automated tag\n ++\t * following.\n ++\t */\n +\tif (tags == TAGS_DEFAULT && autotags) {\n +\t\tstruct ref *tags_ref_map = NULL, **tail = &tags_ref_map;\n +\n3: 4059d5034b ! 3: 0b9d04622d fetch: control lifecycle of FETCH_HEAD in a single place\n @@ Commit message\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 - wants to append to it.\n + it once in `do_fetch()`, where we pass the structure down to the code\n + which wants to append to it.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \tif (prune) {\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \t\t\t\t transport->url);\n - \t\t}\n + \t\tif (retcode != 0)\n + \t\t\tretcode = 1;\n \t}\n -\tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n +\n4: 54fdee845b ! 4: bc1e396ae0 fetch: report errors when backfilling tags fails\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\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 * If backfilling of tags fails then we want to tell\n ++\t\t\t * the user so, but we have to continue regardless to\n ++\t\t\t * populate upstream information of the references we\n ++\t\t\t * have already fetched above.\n +\t\t\t */\n +\t\t\tif (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))\n +\t\t\t\tretcode = 1;\n-: ---------- > 5: fac2e3555a refs: add interface to iterate over queued transactional updates\n5: 55dbe19a1a ! 6: 331ee40e57 fetch: make `--atomic` flag cover backfilling of tags\n @@ Commit message\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 + which have already been updated with our local references. But when we\n + use a single transaction for both local references and backfilling then\n + it may happen that we try to queue the same reference update twice to\n + the transaction, which consequently triggers a bug. We thus have to skip\n + over any tags which have already been queued.\n \n Signed-off-by: Patrick Steinhardt \n \n ## builtin/fetch.c ##\n -@@\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 @@ builtin/fetch.c: static void clear_item(struct refname_hash_entry *item)\n + \titem->ignore = 1;\n }\n \n ++\n ++static void add_already_queued_tags(const char *refname,\n ++\t\t\t\t const struct object_id *old_oid,\n ++\t\t\t\t const struct object_id *new_oid,\n ++\t\t\t\t void *cb_data)\n ++{\n ++\tstruct hashmap *queued_tags = cb_data;\n ++\tif (starts_with(refname, \"refs/tags/\") && new_oid)\n ++\t\t(void) refname_hash_add(queued_tags, refname, new_oid);\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 @@ builtin/fetch.c: 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 @@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs,\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 ++\tif (transaction)\n ++\t\tref_transaction_for_each_queued_update(transaction,\n ++\t\t\t\t\t\t add_already_queued_tags,\n ++\t\t\t\t\t\t &existing_refs);\n +\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (!starts_with(ref->name, \"refs/tags/\"))\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \tif (prune) {\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \t\t}\n + \t\t\tretcode = 1;\n \t}\n \n -\tif (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\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 -@@ builtin/fetch.c: 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 * If backfilling of tags fails then we want to tell\n + \t\t\t * the user so, but we have to continue regardless to\n + \t\t\t * populate upstream information of the references we\n +-\t\t\t * have already fetched above.\n ++\t\t\t * have already fetched above. The exception though is\n ++\t\t\t * when `--atomic` is passed: in that case we'll abort\n ++\t\t\t * the transaction 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/t5503-tagfollow.sh: test_expect_success 'atomic fetch with failing backfill' '\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 +-\t# should not exist. The fact that it does demonstrates that there is\n +-\t# a bug 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\n6: 682f16117b ! 7: 2ad16530e5 fetch: make `--atomic` flag cover pruning of refs\n @@ builtin/fetch.c: 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\tretcode = prune_refs(rs, ref_map, transport->url);\n ++\t\t\tretcode = prune_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\tretcode = prune_refs(&transport->remote->fetch,\n +-\t\t\t\t\t ref_map,\n ++\t\t\t\t\t transaction, ref_map,\n + \t\t\t\t\t transport->url);\n \t\t}\n - \t}\n + \t\tif (retcode != 0)\n \n ## t/t5510-fetch.sh ##\n @@ t/t5510-fetch.sh: test_expect_success 'fetch --atomic --prune executes a single reference transact\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 ++\t# single transaction.\n \tcat >expected <<-EOF &&\n \t\tprepared\n \t\t$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion\n-- \n2.35.1\n\n"},{"id":"448690","messageId":"b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:16Z","receivedAt":"2022-02-17T13:04:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When using git-fetch(1) with the `--atomic` flag the expectation is that\neither all of the references are updated, or alternatively none are in\ncase the fetch fails. While we already have tests for this, we do not\nhave any tests which exercise atomicity either when pruning deleted refs\nor when backfilling tags. This gap in test coverage hides that we indeed\ndon't handle atomicity correctly for both of these cases.\n\nAdd test cases which cover these testing gaps to demonstrate the broken\nbehaviour. Note that tests are not marked as `test_expect_failure`: this\nis done to explicitly demonstrate the current known-wrong behaviour, and\nthey will be fixed up as soon as we fix the underlying bugs.\n\nWhile at it this commit also adds another test case which demonstrates\nthat backfilling of tags does not return an error code in case the\nbackfill fails. This bug will also be fixed by a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5510-fetch.sh | 33 ++++++++++++++++++\n 2 files changed, 114 insertions(+)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 195fc64dd4..6ffe2a5719 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -160,4 +160,85 @@ 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\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 demonstrates that there is\n+\t# a bug 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\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 ef0da0a63b..70d51f343b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -343,6 +343,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":"448691","messageId":"b0a067bbc1e7b9d3ad76acd437d8095c5a48c5d7.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 2/7] fetch: backfill tags before setting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:20Z","receivedAt":"2022-02-17T13:04:31Z","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 use a\n`skip` goto label to jump 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 | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6f5e157863..904ca9f1ca 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@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\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/*\n+\t * If neither --no-tags nor --tags was specified, do automated tag\n+\t * following.\n+\t */\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@@ -1644,7 +1657,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@@ -1658,7 +1671,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@@ -1678,21 +1691,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":"448692","messageId":"0b9d04622d095f97246ae2603e2cb5312a68def3.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:24Z","receivedAt":"2022-02-17T13:04:33Z","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 the code\nwhich wants 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 904ca9f1ca..f8adb40b45 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@@ -1619,7 +1618,8 @@ static int do_fetch(struct transport *transport,\n \t\tif (retcode != 0)\n \t\t\tretcode = 1;\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@@ -1633,11 +1633,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@@ -1693,6 +1695,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":"448693","messageId":"bc1e396ae0ad380dffd7962cc223cc63e6facffe.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 4/7] fetch: report errors when backfilling tags fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:28Z","receivedAt":"2022-02-17T13:04:37Z","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 | 26 ++++++++++++++++++--------\n t/t5503-tagfollow.sh | 4 +---\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f8adb40b45..d304314f16 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@@ -1632,8 +1634,16 @@ 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 of tags fails then we want to tell\n+\t\t\t * the user so, but we have to continue regardless to\n+\t\t\t * populate upstream information of the references we\n+\t\t\t * have already fetched above.\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 6ffe2a5719..c057c49e80 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -233,9 +233,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":"448694","messageId":"331ee40e57df1f07fe725dc679dd934c777d4eab.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:36Z","receivedAt":"2022-02-17T13:04:46Z","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 have already been updated with our local references. But when we\nuse a single transaction for both local references and backfilling then\nit may happen that we try to queue the same reference update twice to\nthe transaction, which consequently triggers a bug. We thus have to skip\nover any tags which have already been queued.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 92 +++++++++++++++++++++++++++++++-------------\n t/t5503-tagfollow.sh | 11 ++----\n 2 files changed, 69 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d304314f16..67af842091 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)\n \titem->ignore = 1;\n }\n \n+\n+static void add_already_queued_tags(const char *refname,\n+\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct hashmap *queued_tags = cb_data;\n+\tif (starts_with(refname, \"refs/tags/\") && new_oid)\n+\t\t(void) refname_hash_add(queued_tags, refname, new_oid);\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@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *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+\tif (transaction)\n+\t\tref_transaction_for_each_queued_update(transaction,\n+\t\t\t\t\t\t add_already_queued_tags,\n+\t\t\t\t\t\t &existing_refs);\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 +622,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 +1105,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 +1132,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 +1247,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 +1264,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 +1304,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 +1329,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 +1503,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 +1527,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 +1540,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 +1550,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 +1612,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@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\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@@ -1633,21 +1651,37 @@ 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 of tags fails then we want to tell\n \t\t\t * the user so, but we have to continue regardless to\n \t\t\t * populate upstream information of the references we\n-\t\t\t * have already fetched above.\n+\t\t\t * have already fetched above. The exception though is\n+\t\t\t * when `--atomic` is passed: in that case we'll abort\n+\t\t\t * the transaction 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@@ -1705,7 +1739,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 c057c49e80..e72fdc2534 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -180,11 +180,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 demonstrates that there is\n-\t# a bug 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@@ -197,12 +194,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":"448695","messageId":"fac2e3555a4ce976252697e7898e125b5e521bdf.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 5/7] refs: add interface to iterate over queued transactional updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:32Z","receivedAt":"2022-02-17T13:04:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There is no way for a caller to see whether a reference update has\nalready been queued up for a given reference transaction. There are\nmultiple alternatives to provide this functionality:\n\n - We may add a function that simply tells us whether a specific\n reference has already been queued. If implemented naively then\n this would potentially be quadratic in runtime behaviour if this\n question is asked repeatedly because we have to iterate over all\n references every time. The alternative would be to add a hashmap\n of all queued reference updates to speed up the lookup, but this\n adds overhead to all callers.\n\n - We may add a flag to `ref_transaction_add_update()` that causes it\n to skip duplicates, but this has the same runtime concerns as the\n first alternative.\n\n - We may add an interface which lets callers collect all updates\n which have already been queued such that he can avoid re-adding\n them. This is the most flexible approach and puts the burden on\n the caller, but also allows us to not impact any of the existing\n callsites which don't need this information.\n\nThis commit implements the last approach: it allows us to compute the\nmap of already-queued updates once up front such that we can then skip\nall subsequent references which are already part of this map.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 16 ++++++++++++++++\n refs.h | 14 ++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex d680de3bc0..35d4e69687 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \treturn refs->be->initial_transaction_commit(refs, transaction, err);\n }\n \n+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tcb(update->refname,\n+\t\t (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n+\t\t (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n+\t\t cb_data);\n+\t}\n+}\n+\n int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n \t\t struct string_list *refnames, unsigned int flags)\n {\ndiff --git a/refs.h b/refs.h\nindex ff859d5951..1ae12c410a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n+/*\n+ * Execute the given callback function for each of the reference updates which\n+ * have been queued in the given transaction. `old_oid` and `new_oid` may be\n+ * `NULL` pointers depending on whether the update has these object IDs set or\n+ * not.\n+ */\n+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,\n+\t\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t\t void *cb_data);\n+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n+\t\t\t\t\t void *cb_data);\n+\n /*\n * Free `*transaction` and all associated data.\n */\n-- \n2.35.1\n\n"},{"id":"448696","messageId":"2ad16530e5df1119a7c17d3a382b420187c93c63.1645102965.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v2 7/7] fetch: make `--atomic` flag cover pruning of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-17T13:04:41Z","receivedAt":"2022-02-17T13:04:59Z","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 | 8 ++------\n 2 files changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 67af842091..9a2b5c03a4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1338,11 +1338,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@@ -1363,13 +1366,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@@ -1388,6 +1400,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@@ -1629,10 +1643,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\tretcode = prune_refs(rs, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(rs, transaction, ref_map, transport->url);\n \t\t} else {\n \t\t\tretcode = prune_refs(&transport->remote->fetch,\n-\t\t\t\t\t ref_map,\n+\t\t\t\t\t transaction, ref_map,\n \t\t\t\t\t transport->url);\n \t\t}\n \t\tif (retcode != 0)\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 70d51f343b..48e14e2dab 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\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+\t# single transaction.\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":"448707","messageId":"CAP8UFD2kr6WJvv7eoDvytx5Q982XBXTpSTUQw5hXmRrDm-UuBw@mail.gmail.com","threadId":"57431","inReplyTo":"b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-17T15:18:07Z","receivedAt":"2022-02-17T15:18:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt wrote:\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> + while read oldrev newrev reference\n> + do\n> + if test \"$reference\" = refs/tags/tag1\n> + then\n> + exit 1\n> + fi\n\nMaybe the following could save a few lines:\n\n test \"$reference\" = refs/tags/tag1 && exit 1\n\nIt would make the code look a bit different than in another hook\nscript written below though, so not a big deal.\n\n> + done\n> + EOF\n\nOverall it looks good, and I like the improved commit message!\n"},{"id":"448709","messageId":"CAP8UFD1xiTaOvjT1UYZCUbhEF_ukgZ1cAZgbYiV+Px74RskS5Q@mail.gmail.com","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-17T15:50:44Z","receivedAt":"2022-02-17T15:51:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt wrote:\n\n> This is the second version of my patch series which fixes these\n> problems.\n\nI took another look and found only a very minor improvement in one of\nthe tests in patch 1/7.\n\nThanks!\n"},{"id":"448732","messageId":"xmqqczjl8buq.fsf@gitster.g","threadId":"57431","inReplyTo":"b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T20:41:33Z","receivedAt":"2022-02-17T20:41:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When using git-fetch(1) with the `--atomic` flag the expectation is that\n> either all of the references are updated, or alternatively none are in\n> case the fetch fails. While we already have tests for this, we do not\n> have any tests which exercise atomicity either when pruning deleted refs\n> or when backfilling tags. This gap in test coverage hides that we indeed\n> don't handle atomicity correctly for both of these cases.\n>\n> Add test cases which cover these testing gaps to demonstrate the broken\n> behaviour. Note that tests are not marked as `test_expect_failure`: this\n> is done to explicitly demonstrate the current known-wrong behaviour, and\n> they will be fixed up as soon as we fix the underlying bugs.\n>\n> While at it this commit also adds another test case which demonstrates\n> that backfilling of tags does not return an error code in case the\n> backfill fails. This bug will also be fixed by a subsequent commit.\n\nNo need to reorganize the series just to fix this, but in general,\nadding new tests that are designed to fail in an early commit and\nthen give fixes in a separate commit is to be avoided for two\nreasons.\n\nOne is obviously that it breaks bisectability. And another is that\nit makes it unclear what the end-user visible problem was when\nreading the actual fix, and makes reviewing the series harder.\n\nIf a test that demonstrates an existing bug comes with the code to\nfix the bug in the same commit, it obviously will not break\nbisectability, and the new test serves also as a documentation of\nwhat the end-user visible symptoms of the bug being fixed are.\n\nSometimes people work around the bisectability issue by starting\nthese tests that demonstrates known-to-be-unfixed-yet bugs with\ntest_expect_failure, but that is not a good solution for the\nreviewability issue, as the step that fixes the bug will show the\ncode fix plus only the first few lines of the test being changed\nto expect success and it is not readily visible what the symptom\nactually were.\n\nA series of patches, each one of which fixes one issue and\ndemonstrates the issue with tests that expects success when the\nissue is resolved, is much more pleasant to read than a series where\nit has tests for multiple issues, followed by a patch or two to\nhandle each issue.\n\nAnd in such a series, it is much easier to see what breaks if the\nfix weren't there. Reviewers can apply one step of the patch and\ntentatively revert everything outside t/ to see how the added test\nbreaks without the fix. It helps those who may want to cherry-pick\na particular fix, together with its associated test, if a series is\ndone that way.\n\nThanks.\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\nWhat does this paragraph want the phrase `backfilling tags` to mean?\nJust from end-user's perspective, there is only one (i.e. if an\nobject that is tagged gets fetched and that tag is not here, fetch\nit too), but at the mechanism level, there are two distinct code\npaths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to\ncall backfill_tags()). Which failure does this talk about, or it\ndoes not matter which code path gave us the tag?\n\n> +\twrite_script clone3/.git/hooks/reference-transaction <<-\\EOF &&\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 demonstrates that there is\n> +\t# a bug 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\nNicely done.\n\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\nIs there a reason why we cannot do <<-\\EOF here to lose the\nbackslashes in the here-text?\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\nWow, nasty ;-)\n\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\n\n"},{"id":"448737","messageId":"xmqqzgmp6tbo.fsf@gitster.g","threadId":"57431","inReplyTo":"b0a067bbc1e7b9d3ad76acd437d8095c5a48c5d7.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 2/7] fetch: backfill tags before setting upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:07:07Z","receivedAt":"2022-02-17T22:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\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 use a\n> `skip` goto label to jump from (3) to (4) in case we fail to update the\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\nOK, as \"setting upsream\" is more or less unrelated to the act of\nactual fetching the refs and objects reachable from them, moving it\noutside the main code that is about fetching does make sense.\n\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\nThis is needed because we will always do free_refs() on the variable\nafter this patch, and one early \"goto cleanup\" happens even before\nwe touch ref_map, when truncate_fetch_head() fails.\n\n> @@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,\n> \t\t\tretcode = 1;\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\nAnd because we always free_refs(ref_map), we can lose a call here.\nOK.\n\n> +\t/*\n> +\t * If neither --no-tags nor --tags was specified, do automated tag\n> +\t * following.\n> +\t */\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\nHere, the new code uses a local and separete tags_ref_map variable\nand free it before we leave, instead of reusing ref_map variable.\n\nOK.\n\n"},{"id":"448738","messageId":"xmqqv8xd6t2z.fsf@gitster.g","threadId":"57431","inReplyTo":"0b9d04622d095f97246ae2603e2cb5312a68def3.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:12:20Z","receivedAt":"2022-02-17T22:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\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 the code\n> which wants to append to it.\n\nOK. Each call to store_updated_refs() used to be responsible for\nopening, appending, committing, and closing FETCH_HEAD. We now make\ndo_fetch() responsible for opening, committing, and closing, and let\nthe direct and indirect callers of fetch_and_consume_refs() only\nworry about appending to it. Makes sense.\n"},{"id":"448739","messageId":"xmqqo8356svz.fsf@gitster.g","threadId":"57431","inReplyTo":"bc1e396ae0ad380dffd7962cc223cc63e6facffe.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 4/7] fetch: report errors when backfilling tags fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:16:32Z","receivedAt":"2022-02-17T22:16:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +static int backfill_tags(struct transport *transport,\n> ...\n> }\n\nThe change to this function is quite straight-forward.\n\n> static int do_fetch(struct transport *transport,\n> @@ -1632,8 +1634,16 @@ 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 of tags fails then we want to tell\n> +\t\t\t * the user so, but we have to continue regardless to\n> +\t\t\t * populate upstream information of the references we\n> +\t\t\t * have already fetched above.\n> +\t\t\t */\n\nOK. Unless atomic, using the available information on successfully\nupdated ones would be fine. Perhaps under --atomic mode, a failure\nto commit the ref transaction will also prevent the upstream\ninformation from getting updated? We'll see soon enough.\n\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}\n> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n> index 6ffe2a5719..c057c49e80 100755\n> --- a/t/t5503-tagfollow.sh\n> +++ b/t/t5503-tagfollow.sh\n> @@ -233,9 +233,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\nOK.\n\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"},{"id":"448740","messageId":"xmqqiltd6seh.fsf@gitster.g","threadId":"57431","inReplyTo":"331ee40e57df1f07fe725dc679dd934c777d4eab.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:27:02Z","receivedAt":"2022-02-17T22:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\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\nOK, having done the FETCH_HEAD thing, the idea is quite similar.\nInstead of letting two invocations to store_updated_refs() to\nindependently open and close separate transactions, we control\neverything centrally in do_fetch().\n\nMakes sense.\n\n> @@ -197,12 +194,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\nOK.\n\nUnlike the \"expect the behaviour at the end of the series from the\nbeginning with known-to-fail tests\" pattern I cautioned against in\nan earlier step, this is a good way to show how the behaviour\nchanges.\n\nThanks.\n"},{"id":"448741","messageId":"xmqqee416s9c.fsf@gitster.g","threadId":"57431","inReplyTo":"CAP8UFD1xiTaOvjT1UYZCUbhEF_ukgZ1cAZgbYiV+Px74RskS5Q@mail.gmail.com","subject":"Re: [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:30:07Z","receivedAt":"2022-02-17T22:30:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt wrote:\n>\n>> This is the second version of my patch series which fixes these\n>> problems.\n>\n> I took another look and found only a very minor improvement in one of\n> the tests in patch 1/7.\n\nYup, the whole series was a pleasant read.\n\nThanks, both. Will queue.\n"},{"id":"448742","messageId":"xmqqa6ep6rn1.fsf@gitster.g","threadId":"57431","inReplyTo":"xmqqczjl8buq.fsf@gitster.g","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-17T22:43:30Z","receivedAt":"2022-02-17T22:43:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Is there a reason why we cannot do <<-\\EOF here to lose the\n> backslashes in the here-text?\n\nYes. We refer to \"$B\".\n"},{"id":"448761","messageId":"Yg9Bkyp1EDvOzzOp@ncase","threadId":"57431","inReplyTo":"xmqqczjl8buq.fsf@gitster.g","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-18T06:49:55Z","receivedAt":"2022-02-18T06:50:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n[snip]\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> \n> What does this paragraph want the phrase `backfilling tags` to mean?\n> Just from end-user's perspective, there is only one (i.e. if an\n> object that is tagged gets fetched and that tag is not here, fetch\n> it too), but at the mechanism level, there are two distinct code\n> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to\n> call backfill_tags()). Which failure does this talk about, or it\n> does not matter which code path gave us the tag?\n\nIt refers to `backfill_tags()`. Should I update this comment to clarify?\n\nPatrick\n"},{"id":"448769","messageId":"xmqqy2282jrq.fsf@gitster.g","threadId":"57431","inReplyTo":"Yg9Bkyp1EDvOzzOp@ncase","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-18T16:59:21Z","receivedAt":"2022-02-18T16:59:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:\n>> Patrick Steinhardt writes:\n> [snip]\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>> \n>> What does this paragraph want the phrase `backfilling tags` to mean?\n>> Just from end-user's perspective, there is only one (i.e. if an\n>> object that is tagged gets fetched and that tag is not here, fetch\n>> it too), but at the mechanism level, there are two distinct code\n>> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to\n>> call backfill_tags()). Which failure does this talk about, or it\n>> does not matter which code path gave us the tag?\n>\n> It refers to `backfill_tags()`. Should I update this comment to clarify?\n\nAfter reading the patch, the issue is only about the case where we\nperform the second fetch and the case where OPT_FOLLOWTAGS does\neverything necessary is not relevant. So hinting that we are\ninterested to see what a failure in the follow-on fetch does to the\natomicity would be a good idea, and mentioning backfill_tags() would\nhave been a good way to do so. Perhaps like \"whether a failure in\nbackfill_tags() correctly aborts...\" or something?\n\nThanks.\n\n\n \n\n"},{"id":"448947","messageId":"YhNF1Xc9GwGNXzhC@ncase","threadId":"57431","inReplyTo":"CAP8UFD2kr6WJvv7eoDvytx5Q982XBXTpSTUQw5hXmRrDm-UuBw@mail.gmail.com","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T07:57:09Z","receivedAt":"2022-02-21T07:57:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 17, 2022 at 04:18:07PM +0100, Christian Couder wrote:\n> On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt wrote:\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> > + while read oldrev newrev reference\n> > + do\n> > + if test \"$reference\" = refs/tags/tag1\n> > + then\n> > + exit 1\n> > + fi\n> \n> Maybe the following could save a few lines:\n> \n> test \"$reference\" = refs/tags/tag1 && exit 1\n> \n> It would make the code look a bit different than in another hook\n> script written below though, so not a big deal.\n\nIf `$reference` does not match the tag we want to continue and\neventually return successfully from this hook. But:\n\n $ while read foo; do test \"$foo\" = bar && echo match; done < <(echo bar)\n match\n $ while read foo; do test \"$foo\" = bar && echo match; done < <(echo foo)\n $ echo $?\n 1\n\nSo with the proposed change we'd now exit the hook with an error code\ninstead of returning successfully.\n\nPatrick\n"},{"id":"448948","messageId":"cover.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645102965.git.ps@pks.im","subject":"[PATCH v3 0/7] fetch: improve atomicity of `--atomic` flag","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:06Z","receivedAt":"2022-02-21T08:02:16Z","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 is the third version of this patch series fixing these issues. The\nonly change compared to v2 is a clarification in a comment to better\npoint out which type of backfilling is being tested.\n\nPatrick\n\nPatrick Steinhardt (7):\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 refs: add interface to iterate over queued transactional updates\n fetch: make `--atomic` flag cover backfilling of tags\n fetch: make `--atomic` flag cover pruning of refs\n\n builtin/fetch.c | 192 +++++++++++++++++++++++++++++--------------\n refs.c | 16 ++++\n refs.h | 14 ++++\n t/t5503-tagfollow.sh | 74 +++++++++++++++++\n t/t5510-fetch.sh | 29 +++++++\n 5 files changed, 263 insertions(+), 62 deletions(-)\n\nRange-diff against v2:\n1: b4ca3f1f3b ! 1: 081174b7a0 fetch: increase test coverage of fetches\n @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '\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# We want to test whether a failure in `backfill_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.\n2: b0a067bbc1 = 2: 9867e76ac7 fetch: backfill tags before setting upstream\n3: 0b9d04622d = 3: 7c36ecbcf4 fetch: control lifecycle of FETCH_HEAD in a single place\n4: bc1e396ae0 = 4: a7e005dd48 fetch: report errors when backfilling tags fails\n5: fac2e3555a = 5: 0316d770e9 refs: add interface to iterate over queued transactional updates\n6: 331ee40e57 = 6: 2f98e34c84 fetch: make `--atomic` flag cover backfilling of tags\n7: 2ad16530e5 = 7: 7292de826b fetch: make `--atomic` flag cover pruning of refs\n-- \n2.35.1\n\n"},{"id":"448949","messageId":"081174b7a00cf094a7dacd8aba89fb137ea40f66.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:10Z","receivedAt":"2022-02-21T08:02:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When using git-fetch(1) with the `--atomic` flag the expectation is that\neither all of the references are updated, or alternatively none are in\ncase the fetch fails. While we already have tests for this, we do not\nhave any tests which exercise atomicity either when pruning deleted refs\nor when backfilling tags. This gap in test coverage hides that we indeed\ndon't handle atomicity correctly for both of these cases.\n\nAdd test cases which cover these testing gaps to demonstrate the broken\nbehaviour. Note that tests are not marked as `test_expect_failure`: this\nis done to explicitly demonstrate the current known-wrong behaviour, and\nthey will be fixed up as soon as we fix the underlying bugs.\n\nWhile at it this commit also adds another test case which demonstrates\nthat backfilling of tags does not return an error code in case the\nbackfill fails. This bug will also be fixed by a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++\n t/t5510-fetch.sh | 33 ++++++++++++++++++\n 2 files changed, 114 insertions(+)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 195fc64dd4..7dadaafd4b 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -160,4 +160,85 @@ 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 in `backfill_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\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 demonstrates that there is\n+\t# a bug 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\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 ef0da0a63b..70d51f343b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -343,6 +343,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":"448950","messageId":"9867e76ac70c51cc6ccaf3ad7ef64250dad24920.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 2/7] fetch: backfill tags before setting upstream","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:14Z","receivedAt":"2022-02-21T08:02:32Z","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 use a\n`skip` goto label to jump 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 | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6f5e157863..904ca9f1ca 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@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\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/*\n+\t * If neither --no-tags nor --tags was specified, do automated tag\n+\t * following.\n+\t */\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@@ -1644,7 +1657,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@@ -1658,7 +1671,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@@ -1678,21 +1691,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":"448951","messageId":"7c36ecbcf4daf1580c7deb9b7536b413dec8618b.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 3/7] fetch: control lifecycle of FETCH_HEAD in a single place","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:18Z","receivedAt":"2022-02-21T08:02:34Z","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 the code\nwhich wants 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 904ca9f1ca..f8adb40b45 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@@ -1619,7 +1618,8 @@ static int do_fetch(struct transport *transport,\n \t\tif (retcode != 0)\n \t\t\tretcode = 1;\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@@ -1633,11 +1633,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@@ -1693,6 +1695,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":"448952","messageId":"a7e005dd482fe620839fe02b74b4bc55ae5b8850.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 4/7] fetch: report errors when backfilling tags fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:22Z","receivedAt":"2022-02-21T08:02:40Z","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 | 26 ++++++++++++++++++--------\n t/t5503-tagfollow.sh | 4 +---\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f8adb40b45..d304314f16 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@@ -1632,8 +1634,16 @@ 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 of tags fails then we want to tell\n+\t\t\t * the user so, but we have to continue regardless to\n+\t\t\t * populate upstream information of the references we\n+\t\t\t * have already fetched above.\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 7dadaafd4b..27a1dfa3c0 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -233,9 +233,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":"448953","messageId":"0316d770e9590641a942370c84e4d8a42df2ffcd.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 5/7] refs: add interface to iterate over queued transactional updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:26Z","receivedAt":"2022-02-21T08:02:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There is no way for a caller to see whether a reference update has\nalready been queued up for a given reference transaction. There are\nmultiple alternatives to provide this functionality:\n\n - We may add a function that simply tells us whether a specific\n reference has already been queued. If implemented naively then\n this would potentially be quadratic in runtime behaviour if this\n question is asked repeatedly because we have to iterate over all\n references every time. The alternative would be to add a hashmap\n of all queued reference updates to speed up the lookup, but this\n adds overhead to all callers.\n\n - We may add a flag to `ref_transaction_add_update()` that causes it\n to skip duplicates, but this has the same runtime concerns as the\n first alternative.\n\n - We may add an interface which lets callers collect all updates\n which have already been queued such that he can avoid re-adding\n them. This is the most flexible approach and puts the burden on\n the caller, but also allows us to not impact any of the existing\n callsites which don't need this information.\n\nThis commit implements the last approach: it allows us to compute the\nmap of already-queued updates once up front such that we can then skip\nall subsequent references which are already part of this map.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 16 ++++++++++++++++\n refs.h | 14 ++++++++++++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex d680de3bc0..35d4e69687 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \treturn refs->be->initial_transaction_commit(refs, transaction, err);\n }\n \n+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tcb(update->refname,\n+\t\t (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n+\t\t (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n+\t\t cb_data);\n+\t}\n+}\n+\n int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n \t\t struct string_list *refnames, unsigned int flags)\n {\ndiff --git a/refs.h b/refs.h\nindex ff859d5951..1ae12c410a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,\n int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err);\n \n+/*\n+ * Execute the given callback function for each of the reference updates which\n+ * have been queued in the given transaction. `old_oid` and `new_oid` may be\n+ * `NULL` pointers depending on whether the update has these object IDs set or\n+ * not.\n+ */\n+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,\n+\t\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t\t void *cb_data);\n+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n+\t\t\t\t\t void *cb_data);\n+\n /*\n * Free `*transaction` and all associated data.\n */\n-- \n2.35.1\n\n"},{"id":"448954","messageId":"2f98e34c8461f09f7c56c5ec106a2c0be3f396ab.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 6/7] fetch: make `--atomic` flag cover backfilling of tags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:30Z","receivedAt":"2022-02-21T08:03:05Z","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 have already been updated with our local references. But when we\nuse a single transaction for both local references and backfilling then\nit may happen that we try to queue the same reference update twice to\nthe transaction, which consequently triggers a bug. We thus have to skip\nover any tags which have already been queued.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 92 +++++++++++++++++++++++++++++++-------------\n t/t5503-tagfollow.sh | 11 ++----\n 2 files changed, 69 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d304314f16..67af842091 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)\n \titem->ignore = 1;\n }\n \n+\n+static void add_already_queued_tags(const char *refname,\n+\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct hashmap *queued_tags = cb_data;\n+\tif (starts_with(refname, \"refs/tags/\") && new_oid)\n+\t\t(void) refname_hash_add(queued_tags, refname, new_oid);\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@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *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+\tif (transaction)\n+\t\tref_transaction_for_each_queued_update(transaction,\n+\t\t\t\t\t\t add_already_queued_tags,\n+\t\t\t\t\t\t &existing_refs);\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 +622,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 +1105,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 +1132,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 +1247,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 +1264,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 +1304,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 +1329,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 +1503,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 +1527,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 +1540,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 +1550,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 +1612,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@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\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@@ -1633,21 +1651,37 @@ 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 of tags fails then we want to tell\n \t\t\t * the user so, but we have to continue regardless to\n \t\t\t * populate upstream information of the references we\n-\t\t\t * have already fetched above.\n+\t\t\t * have already fetched above. The exception though is\n+\t\t\t * when `--atomic` is passed: in that case we'll abort\n+\t\t\t * the transaction 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@@ -1705,7 +1739,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 27a1dfa3c0..fe23cad4ce 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -180,11 +180,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 demonstrates that there is\n-\t# a bug 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@@ -197,12 +194,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":"448955","messageId":"7292de826bd871bc12d7541f56c9a10b782c705d.1645430423.git.ps@pks.im","threadId":"57431","inReplyTo":"cover.1645430423.git.ps@pks.im","subject":"[PATCH v3 7/7] fetch: make `--atomic` flag cover pruning of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-21T08:02:35Z","receivedAt":"2022-02-21T08:03:13Z","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 | 8 ++------\n 2 files changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 67af842091..9a2b5c03a4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1338,11 +1338,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@@ -1363,13 +1366,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@@ -1388,6 +1400,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@@ -1629,10 +1643,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\tretcode = prune_refs(rs, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(rs, transaction, ref_map, transport->url);\n \t\t} else {\n \t\t\tretcode = prune_refs(&transport->remote->fetch,\n-\t\t\t\t\t ref_map,\n+\t\t\t\t\t transaction, ref_map,\n \t\t\t\t\t transport->url);\n \t\t}\n \t\tif (retcode != 0)\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 70d51f343b..48e14e2dab 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact\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+\t# single transaction.\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":"450220","messageId":"xmqqwnhbevbq.fsf@gitster.g","threadId":"57431","inReplyTo":"b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T00:25:13Z","receivedAt":"2022-03-03T00:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\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\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\nWithout the extra indentation level, all your < +\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 demonstrates that there is\n> +\t# a bug in the `--atomic` flag.\n> +\ttest $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n> +'\n\nMakes sense.\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\nI think we see two transactions here, even though the comment says\notherwise. Is this expecting a known breakage?\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\nNice way to make sure what is and is not in each transaction. I\nfeels a bit too rigid (e.g. in the first transaction, there is no\ninherent reason to expect that the update to head/something is\nmentioned before the update to tags/tag2, for example).\n\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\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\n> +\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n\nI have been wondering if we need to do this from the hook? If we\nhave this ref before we start \"fetch\", would it have the same\neffect, or \"fetch\" notices that this interfering ref exists and\nremoves it to make room for storing refs/tags/tag1, making the whole\nthing fail to fail?\n\n> +\t\t\t\texit \\$!\n\nIn any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\nexit with \"$!\", which is ...? The process ID of the most recent\nbackground command? Puzzled.\n\n> +\t\t\tfi\n> +\t\tdone\n> +\tEOF\n"},{"id":"450221","messageId":"xmqqsfrzev2d.fsf@gitster.g","threadId":"57431","inReplyTo":"081174b7a00cf094a7dacd8aba89fb137ea40f66.1645430423.git.ps@pks.im","subject":"Re: [PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T00:30:50Z","receivedAt":"2022-03-03T00:31:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\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\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\nI think I've reviewed the previous round of these patches in\ndetail. I by mistake sent a comment for this step in v2, but I\nthink the same puzzlement exists in this round, too.\n\n"},{"id":"450222","messageId":"xmqqo82neuz3.fsf@gitster.g","threadId":"57431","inReplyTo":"xmqqsfrzev2d.fsf@gitster.g","subject":"Re: [PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T00:32:48Z","receivedAt":"2022-03-03T00:32:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\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\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> I think I've reviewed the previous round of these patches in\n> detail. I by mistake sent a comment for this step in v2, but I\n> think the same puzzlement exists in this round, too.\n\nNamely:\n\nI have been wondering if we need to do this from the hook? If we\nhave this ref before we start \"fetch\", would it have the same\neffect, or \"fetch\" notices that this interfering ref exists and\nremoves it to make room for storing refs/tags/tag1, making the whole\nthing fail to fail?\n\n> +\t\t\t\texit \\$!\n\nIn any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\nexit with \"$!\", which is ...? The process ID of the most recent\nbackground command? Puzzled.\n"},{"id":"450245","messageId":"YiBjjV6Kw2KHsMxq@ncase","threadId":"57431","inReplyTo":"xmqqo82neuz3.fsf@gitster.g","subject":"Re: [PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-03T06:43:25Z","receivedAt":"2022-03-03T06:43:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 02, 2022 at 04:32:48PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Patrick Steinhardt writes:\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\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> > I think I've reviewed the previous round of these patches in\n> > detail. I by mistake sent a comment for this step in v2, but I\n> > think the same puzzlement exists in this round, too.\n> \n> Namely:\n> \n> I have been wondering if we need to do this from the hook? If we\n> have this ref before we start \"fetch\", would it have the same\n> effect, or \"fetch\" notices that this interfering ref exists and\n> removes it to make room for storing refs/tags/tag1, making the whole\n> thing fail to fail?\n> \n> > +\t\t\t\texit \\$!\n> \n> In any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\n> exit with \"$!\", which is ...? The process ID of the most recent\n> background command? Puzzled.\n\nOof, this was supposed to be `exit \\$?`, thanks for catching this. But\nyour above comment is right: we can indeed just create the D/F conflict\noutside of the hook and thus avoid the hook script altogether. Thanks!\n\nPatrick\n"},{"id":"450246","messageId":"YiBkg05+ncWDODWe@ncase","threadId":"57431","inReplyTo":"xmqqwnhbevbq.fsf@gitster.g","subject":"Re: [PATCH v2 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-03T06:47:31Z","receivedAt":"2022-03-03T06:47:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\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\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> Without the extra indentation level, all your < become easier to read. You are consistently doing this in your\n> patches, which it is better than random mixes of indentation style,\n> though.\n\nPersonally I think it's easier to read this way, but grepping through\nthe codebase shows that what you're proposing is used consistently.\nI'll change it.\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 demonstrates that there is\n> > +\t# a bug in the `--atomic` flag.\n> > +\ttest $B = \"$(git -C clone3 rev-parse --verify refs/heads/something)\"\n> > +'\n> \n> Makes sense.\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> I think we see two transactions here, even though the comment says\n> otherwise. Is this expecting a known breakage?\n\nYes, it is. I've added a comment in this patch to document the breakage,\nwhich is then removed when the bug is fixed.\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> Nice way to make sure what is and is not in each transaction. I\n> feels a bit too rigid (e.g. in the first transaction, there is no\n> inherent reason to expect that the update to head/something is\n> mentioned before the update to tags/tag2, for example).\n> \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\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> \n> > +\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n> \n> I have been wondering if we need to do this from the hook? If we\n> have this ref before we start \"fetch\", would it have the same\n> effect, or \"fetch\" notices that this interfering ref exists and\n> removes it to make room for storing refs/tags/tag1, making the whole\n> thing fail to fail?\n\nNo, it indeed is not, thanks!\n\nPatrick\n\n> > +\t\t\t\texit \\$!\n> \n> In any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\n> exit with \"$!\", which is ...? The process ID of the most recent\n> background command? Puzzled.\n> \n> > +\t\t\tfi\n> > +\t\tdone\n> > +\tEOF\n"},{"id":"450247","messageId":"xmqqmti7bkf2.fsf@gitster.g","threadId":"57431","inReplyTo":"YiBjjV6Kw2KHsMxq@ncase","subject":"Re: [PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T06:49:05Z","receivedAt":"2022-03-03T06:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\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>> > I think I've reviewed the previous round of these patches in\n>> > detail. I by mistake sent a comment for this step in v2, but I\n>> > think the same puzzlement exists in this round, too.\n>> \n>> Namely:\n>> \n>> I have been wondering if we need to do this from the hook? If we\n>> have this ref before we start \"fetch\", would it have the same\n>> effect, or \"fetch\" notices that this interfering ref exists and\n>> removes it to make room for storing refs/tags/tag1, making the whole\n>> thing fail to fail?\n>> \n>> > +\t\t\t\texit \\$!\n>> \n>> In any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\n>> exit with \"$!\", which is ...? The process ID of the most recent\n>> background command? Puzzled.\n>\n> Oof, this was supposed to be `exit \\$?`, thanks for catching this. But\n> your above comment is right: we can indeed just create the D/F conflict\n> outside of the hook and thus avoid the hook script altogether. Thanks!\n\nI see.\n\nAs that shell does not send anything to background, at the point of\nthe reference $! would yield an empty string, and \"exit\" is\nequivalent to \"exit $?\", it is doing the right thing, I presume.\n\nThe topic has been in 'next' for a while, so if you are inclined to\nfix it up, please send an incremental patch. If you do \"exit\" it\nwould be a one-liner change, or if you use a different \"cause D/F\nconflict outside the hook\" approach, the change may become a bit\nmore involved.\n\nThanks.\n"},{"id":"450248","messageId":"YiBlXSrSy1iajd9r@ncase","threadId":"57431","inReplyTo":"xmqqmti7bkf2.fsf@gitster.g","subject":"Re: [PATCH v3 1/7] fetch: increase test coverage of fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-03T06:51:09Z","receivedAt":"2022-03-03T06:51:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 02, 2022 at 10:49:05PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \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> >> > I think I've reviewed the previous round of these patches in\n> >> > detail. I by mistake sent a comment for this step in v2, but I\n> >> > think the same puzzlement exists in this round, too.\n> >> \n> >> Namely:\n> >> \n> >> I have been wondering if we need to do this from the hook? If we\n> >> have this ref before we start \"fetch\", would it have the same\n> >> effect, or \"fetch\" notices that this interfering ref exists and\n> >> removes it to make room for storing refs/tags/tag1, making the whole\n> >> thing fail to fail?\n> >> \n> >> > +\t\t\t\texit \\$!\n> >> \n> >> In any case, \"exit 0\" or \"exit \\$?\" would be understandable, but\n> >> exit with \"$!\", which is ...? The process ID of the most recent\n> >> background command? Puzzled.\n> >\n> > Oof, this was supposed to be `exit \\$?`, thanks for catching this. But\n> > your above comment is right: we can indeed just create the D/F conflict\n> > outside of the hook and thus avoid the hook script altogether. Thanks!\n> \n> I see.\n> \n> As that shell does not send anything to background, at the point of\n> the reference $! would yield an empty string, and \"exit\" is\n> equivalent to \"exit $?\", it is doing the right thing, I presume.\n> \n> The topic has been in 'next' for a while, so if you are inclined to\n> fix it up, please send an incremental patch. If you do \"exit\" it\n> would be a one-liner change, or if you use a different \"cause D/F\n> conflict outside the hook\" approach, the change may become a bit\n> more involved.\n> \n> Thanks.\n\nFair enough, thanks for clarifying the steps. Does it make sense to also\nchange indentantion of the heredocs as per your review of v2 or should I\njust keep that as-is now?\n\nPatrick\n"}]}