[PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag

[PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 16315 bytes --]

Hi,

in c7b190dabd (fetch: implement support for atomic reference updates,
2021-01-12), I have added a new `--atomic` flag to git-fetch(1) which is
supposed to make it an all-or-nothing operation: either all refs are
updated, or none is. I have recently discovered though that there were
two oversights: neither pruning of refs via `--prune` nor the tag
backfill mechanism are currently covered by this flag, which breaks the
promise.

This is the second version of my patch series which fixes these
problems. Due to a semantic conflict with
ps/avoid-unnecessary-hook-invocation-with-packed-refs and a textual
conflict with c9e04d905e (fetch --prune: exit with error if pruning
fails, 2022-01-31) it now applies on top of the former branch merged
with 45fe28c951 (The fourth batch, 2022-02-16).

Changes compared to v1:

    - Commit messages have been improved.

    - A code comment has been fixed to not talk about the past anymore,
      but instead state what the section of code is supposed to do.

    - I have introduced a new patch 5/7 which provides an interface to
      access queued updates in reference transactions without requiring
      access to "refs/refs-internal.h".

    - Removed unnecessary shebangs in tests when using `write_script`.

    - Rebased the series to fix a conflict with c9e04d905e, mentioned
      above.

Thanks for all the feedback!

Patrick

Patrick Steinhardt (7):
  fetch: increase test coverage of fetches
  fetch: backfill tags before setting upstream
  fetch: control lifecycle of FETCH_HEAD in a single place
  fetch: report errors when backfilling tags fails
  refs: add interface to iterate over queued transactional updates
  fetch: make `--atomic` flag cover backfilling of tags
  fetch: make `--atomic` flag cover pruning of refs

 builtin/fetch.c      | 192 +++++++++++++++++++++++++++++--------------
 refs.c               |  16 ++++
 refs.h               |  14 ++++
 t/t5503-tagfollow.sh |  74 +++++++++++++++++
 t/t5510-fetch.sh     |  29 +++++++
 5 files changed, 263 insertions(+), 62 deletions(-)

Range-diff against v1:
1:  e133c14f56 ! 1:  b4ca3f1f3b fetch: increase test coverage of fetches
    @@ Metadata
      ## Commit message ##
         fetch: increase test coverage of fetches
     
    -    The `--atomic` flag is missing test coverage for pruning of deleted
    -    references and backfilling of tags, and of course both aren't covered
    -    correctly by this flag. Furthermore, we don't have tests demonstrating
    -    error cases for backfilling tags.
    +    When using git-fetch(1) with the `--atomic` flag the expectation is that
    +    either all of the references are updated, or alternatively none are in
    +    case the fetch fails. While we already have tests for this, we do not
    +    have any tests which exercise atomicity either when pruning deleted refs
    +    or when backfilling tags. This gap in test coverage hides that we indeed
    +    don't handle atomicity correctly for both of these cases.
     
    -    Add tests to cover those testing gaps.
    +    Add test cases which cover these testing gaps to demonstrate the broken
    +    behaviour. Note that tests are not marked as `test_expect_failure`: this
    +    is done to explicitly demonstrate the current known-wrong behaviour, and
    +    they will be fixed up as soon as we fix the underlying bugs.
    +
    +    While at it this commit also adds another test case which demonstrates
    +    that backfilling of tags does not return an error code in case the
    +    backfill fails. This bug will also be fixed by a subsequent commit.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '
     +	#
     +	# To trigger failure we simply abort when backfilling a tag.
     +	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
    -+		#!/bin/sh
    -+
     +		while read oldrev newrev reference
     +		do
     +			if test "$reference" = refs/tags/tag1
    @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '
     +	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
     +
     +	# Creation of the tag has failed, so ideally refs/heads/something
    -+	# should not exist. The fact that it does is demonstrates that there is
    -+	# missing coverage in the `--atomic` flag.
    ++	# should not exist. The fact that it does demonstrates that there is
    ++	# a bug in the `--atomic` flag.
     +	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
     +'
     +
    @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '
     +	git init clone5 &&
     +
     +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
    -+		#!/bin/sh
    -+
     +		while read oldrev newrev reference
     +		do
     +			if test "\$reference" = refs/tags/tag1
2:  64c94e7a28 ! 2:  b0a067bbc1 fetch: backfill tags before setting upstream
    @@ Commit message
             4. We backfill tags pointing into the history we have just fetched.
     
         It is quite confusing that we fetch objects and update references in
    -    both (2) and (4), which is further stressed by the point that we require
    -    a `skip` label which jumps from (3) to (4) in case we fail to update the
    +    both (2) and (4), which is further stressed by the point that we use a
    +    `skip` goto label to jump from (3) to (4) in case we fail to update the
         gitconfig as expected.
     
         Reorder the code to first update all local references, and only after we
    @@ builtin/fetch.c: static void backfill_tags(struct transport *transport, struct r
      	int retcode = 0;
      	const struct ref *remote_refs;
     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
    - 		}
    + 			retcode = 1;
      	}
      	if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
     -		free_refs(ref_map);
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		goto cleanup;
      	}
      
    -+	/* if neither --no-tags nor --tags was specified, do automated tag
    -+	 * following ... */
    ++	/*
    ++	 * If neither --no-tags nor --tags was specified, do automated tag
    ++	 * following.
    ++	 */
     +	if (tags == TAGS_DEFAULT && autotags) {
     +		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
     +
3:  4059d5034b ! 3:  0b9d04622d fetch: control lifecycle of FETCH_HEAD in a single place
    @@ Commit message
         which is essentially duplicate work.
     
         Improve the lifecycle of updating FETCH_HEAD by opening and committing
    -    it once in `do_fetch()`, where we pass the structure down to code which
    -    wants to append to it.
    +    it once in `do_fetch()`, where we pass the structure down to the code
    +    which wants to append to it.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
      	if (prune) {
     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
    - 				   transport->url);
    - 		}
    + 		if (retcode != 0)
    + 			retcode = 1;
      	}
     -	if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
     +
4:  54fdee845b ! 4:  bc1e396ae0 fetch: report errors when backfilling tags fails
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
     -			backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
     +		if (tags_ref_map) {
     +			/*
    -+			 * If backfilling tags succeeds we used to not return
    -+			 * an error code to the user at all. Instead, we
    -+			 * silently swallowed that error and updated the local
    -+			 * state of the repository. We now notify the user of
    -+			 * any such errors, but we continue to make sure that
    -+			 * FETCH_HEAD and the upstream branch are configured as
    -+			 * expected.
    ++			 * If backfilling of tags fails then we want to tell
    ++			 * the user so, but we have to continue regardless to
    ++			 * populate upstream information of the references we
    ++			 * have already fetched above.
     +			 */
     +			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
     +				retcode = 1;
-:  ---------- > 5:  fac2e3555a refs: add interface to iterate over queued transactional updates
5:  55dbe19a1a ! 6:  331ee40e57 fetch: make `--atomic` flag cover backfilling of tags
    @@ Commit message
         Note that we also have to start passing the transaction into
         `find_non_local_tags()`: this function is responsible for finding all
         tags which we need to backfill. Right now, it will happily return tags
    -    which we have already been updated with our local references. But when
    -    we use a single transaction for both local references and backfilling
    -    then it may happen that we try to queue the same reference update twice
    -    to the transaction, which consequentially triggers a bug. We thus have
    -    to skip over any tags which have already been queued. Unfortunately,
    -    this requires us to reach into internals of the reference transaction to
    -    access queued updates, but there is no non-internal interface right now
    -    which would allow us to access this information.
    +    which have already been updated with our local references. But when we
    +    use a single transaction for both local references and backfilling then
    +    it may happen that we try to queue the same reference update twice to
    +    the transaction, which consequently triggers a bug. We thus have to skip
    +    over any tags which have already been queued.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/fetch.c ##
    -@@
    - #include "cache.h"
    - #include "config.h"
    - #include "repository.h"
    --#include "refs.h"
    -+#include "refs/refs-internal.h"
    - #include "refspec.h"
    - #include "object-store.h"
    - #include "oidset.h"
     @@ builtin/fetch.c: static void clear_item(struct refname_hash_entry *item)
    + 	item->ignore = 1;
      }
      
    ++
    ++static void add_already_queued_tags(const char *refname,
    ++				    const struct object_id *old_oid,
    ++				    const struct object_id *new_oid,
    ++				    void *cb_data)
    ++{
    ++	struct hashmap *queued_tags = cb_data;
    ++	if (starts_with(refname, "refs/tags/") && new_oid)
    ++		(void) refname_hash_add(queued_tags, refname, new_oid);
    ++}
    ++
      static void find_non_local_tags(const struct ref *refs,
     +				struct ref_transaction *transaction,
      				struct ref **head,
      				struct ref ***tail)
      {
     @@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs,
    - 	const struct ref *ref;
    - 	struct refname_hash_entry *item = NULL;
    - 	const int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;
    -+	int i;
    - 
    - 	refname_hash_init(&existing_refs);
    - 	refname_hash_init(&remote_refs);
      	create_fetch_oidset(head, &fetch_oids);
      
      	for_each_ref(add_one_refname, &existing_refs);
    @@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs,
     +	 * If we already have a transaction, then we need to filter out all
     +	 * tags which have already been queued up.
     +	 */
    -+	for (i = 0; transaction && i < transaction->nr; i++) {
    -+		if (!starts_with(transaction->updates[i]->refname, "refs/tags/") ||
    -+		    !(transaction->updates[i]->flags & REF_HAVE_NEW))
    -+			continue;
    -+		(void) refname_hash_add(&existing_refs,
    -+					transaction->updates[i]->refname,
    -+					&transaction->updates[i]->new_oid);
    -+	}
    -+
    ++	if (transaction)
    ++		ref_transaction_for_each_queued_update(transaction,
    ++						       add_already_queued_tags,
    ++						       &existing_refs);
     +
      	for (ref = refs; ref; ref = ref->next) {
      		if (!starts_with(ref->name, "refs/tags/"))
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
      	if (prune) {
     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
    - 		}
    + 			retcode = 1;
      	}
      
     -	if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
     +		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
      		if (tags_ref_map) {
      			/*
    - 			 * If backfilling tags succeeds we used to not return
    -@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
    - 			 * state of the repository. We now notify the user of
    - 			 * any such errors, but we continue to make sure that
    - 			 * FETCH_HEAD and the upstream branch are configured as
    --			 * expected.
    -+			 * expected. The exception though is when `--atomic`
    -+			 * is passed: in that case we'll abort the transaction
    -+			 * and don't commit anything.
    + 			 * If backfilling of tags fails then we want to tell
    + 			 * the user so, but we have to continue regardless to
    + 			 * populate upstream information of the references we
    +-			 * have already fetched above.
    ++			 * have already fetched above. The exception though is
    ++			 * when `--atomic` is passed: in that case we'll abort
    ++			 * the transaction and don't commit anything.
      			 */
     -			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
     +			if (backfill_tags(transport, transaction, tags_ref_map,
    @@ t/t5503-tagfollow.sh: test_expect_success 'atomic fetch with failing backfill' '
      	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
     -
     -	# Creation of the tag has failed, so ideally refs/heads/something
    --	# should not exist. The fact that it does is demonstrates that there is
    --	# missing coverage in the `--atomic` flag.
    +-	# should not exist. The fact that it does demonstrates that there is
    +-	# a bug in the `--atomic` flag.
     -	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
     +	test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
     +	test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
6:  682f16117b ! 7:  2ad16530e5 fetch: make `--atomic` flag cover pruning of refs
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		 * don't care whether --tags was specified.
      		 */
      		if (rs->nr) {
    --			prune_refs(rs, ref_map, transport->url);
    -+			prune_refs(rs, transaction, ref_map, transport->url);
    +-			retcode = prune_refs(rs, ref_map, transport->url);
    ++			retcode = prune_refs(rs, transaction, ref_map, transport->url);
      		} else {
    - 			prune_refs(&transport->remote->fetch,
    --				   ref_map,
    -+				   transaction, ref_map,
    - 				   transport->url);
    + 			retcode = prune_refs(&transport->remote->fetch,
    +-					     ref_map,
    ++					     transaction, ref_map,
    + 					     transport->url);
      		}
    - 	}
    + 		if (retcode != 0)
     
      ## t/t5510-fetch.sh ##
     @@ t/t5510-fetch.sh: test_expect_success 'fetch --atomic --prune executes a single reference transact
    + 	head_oid=$(git rev-parse HEAD) &&
    + 
    + 	# Fetching with the `--atomic` flag should update all references in a
    +-	# single transaction. It is currently missing coverage of pruned
    +-	# references though, and as a result those may be committed to disk
    +-	# even if updating references fails later.
    ++	# single transaction.
      	cat >expected <<-EOF &&
      		prepared
      		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5687 bytes --]

When using git-fetch(1) with the `--atomic` flag the expectation is that
either all of the references are updated, or alternatively none are in
case the fetch fails. While we already have tests for this, we do not
have any tests which exercise atomicity either when pruning deleted refs
or when backfilling tags. This gap in test coverage hides that we indeed
don't handle atomicity correctly for both of these cases.

Add test cases which cover these testing gaps to demonstrate the broken
behaviour. Note that tests are not marked as `test_expect_failure`: this
is done to explicitly demonstrate the current known-wrong behaviour, and
they will be fixed up as soon as we fix the underlying bugs.

While at it this commit also adds another test case which demonstrates
that backfilling of tags does not return an error code in case the
backfill fails. This bug will also be fixed by a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh     | 33 ++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd4..6ffe2a5719 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -160,4 +160,85 @@ test_expect_success 'new clone fetch main and tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'atomic fetch with failing backfill' '
+	git init clone3 &&
+
+	# We want to test whether a failure when backfilling tags correctly
+	# aborts the complete transaction when `--atomic` is passed: we should
+	# neither create the branch nor should we create the tag when either
+	# one of both fails to update correctly.
+	#
+	# To trigger failure we simply abort when backfilling a tag.
+	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
+		while read oldrev newrev reference
+		do
+			if test "$reference" = refs/tags/tag1
+			then
+				exit 1
+			fi
+		done
+	EOF
+
+	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
+
+	# Creation of the tag has failed, so ideally refs/heads/something
+	# should not exist. The fact that it does demonstrates that there is
+	# a bug in the `--atomic` flag.
+	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+'
+
+test_expect_success 'atomic fetch with backfill should use single transaction' '
+	git init clone4 &&
+
+	# Fetching with the `--atomic` flag should update all references in a
+	# single transaction, including backfilled tags. We thus expect to see
+	# a single reference transaction for the created branch and tags.
+	cat >expected <<-EOF &&
+		prepared
+		$ZERO_OID $B refs/heads/something
+		$ZERO_OID $S refs/tags/tag2
+		committed
+		$ZERO_OID $B refs/heads/something
+		$ZERO_OID $S refs/tags/tag2
+		prepared
+		$ZERO_OID $T refs/tags/tag1
+		committed
+		$ZERO_OID $T refs/tags/tag1
+	EOF
+
+	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
+		( echo "$*" && cat ) >>actual
+	EOF
+
+	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
+	test_cmp expected clone4/actual
+'
+
+test_expect_success 'backfill failure causes command to fail' '
+	git init clone5 &&
+
+	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
+		while read oldrev newrev reference
+		do
+			if test "\$reference" = refs/tags/tag1
+			then
+				# Create a nested tag below the actual tag we
+				# wanted to write, which causes a D/F conflict
+				# later when we want to commit refs/tags/tag1.
+				# We cannot just `exit 1` here given that this
+				# would cause us to die immediately.
+				git update-ref refs/tags/tag1/nested $B
+				exit \$!
+			fi
+		done
+	EOF
+
+	# Even though we fail to create refs/tags/tag1 the below command
+	# unexpectedly succeeds.
+	git -C clone5 fetch .. $B:refs/heads/something &&
+	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
+	test $S = $(git -C clone5 rev-parse --verify tag2) &&
+	test_must_fail git -C clone5 rev-parse --verify tag1
+'
+
 test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ef0da0a63b..70d51f343b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -343,6 +343,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
 	test_cmp expected atomic/.git/FETCH_HEAD
 '
 
+test_expect_success 'fetch --atomic --prune executes a single reference transaction only' '
+	test_when_finished "rm -rf \"$D\"/atomic" &&
+
+	cd "$D" &&
+	git branch scheduled-for-deletion &&
+	git clone . atomic &&
+	git branch -D scheduled-for-deletion &&
+	git branch new-branch &&
+	head_oid=$(git rev-parse HEAD) &&
+
+	# Fetching with the `--atomic` flag should update all references in a
+	# single transaction. It is currently missing coverage of pruned
+	# references though, and as a result those may be committed to disk
+	# even if updating references fails later.
+	cat >expected <<-EOF &&
+		prepared
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+		committed
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+		prepared
+		$ZERO_OID $head_oid refs/remotes/origin/new-branch
+		committed
+		$ZERO_OID $head_oid refs/remotes/origin/new-branch
+	EOF
+
+	write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+		( echo "$*" && cat ) >>actual
+	EOF
+
+	git -C atomic fetch --atomic --prune origin &&
+	test_cmp expected atomic/actual
+'
+
 test_expect_success '--refmap="" ignores configured refspec' '
 	cd "$TRASH_DIRECTORY" &&
 	git clone "$D" remote-refs &&
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/7] fetch: backfill tags before setting upstream

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

The fetch code flow is a bit hard to understand right now:

    1. We optionally prune all references which have vanished on the
       remote side.
    2. We fetch and update all other references locally.
    3. We update the upstream branch in the gitconfig.
    4. We backfill tags pointing into the history we have just fetched.

It is quite confusing that we fetch objects and update references in
both (2) and (4), which is further stressed by the point that we use a
`skip` goto label to jump from (3) to (4) in case we fail to update the
gitconfig as expected.

Reorder the code to first update all local references, and only after we
have done so update the upstream branch information. This improves the
code flow and furthermore makes it easier to refactor the way we update
references together.

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6f5e157863..904ca9f1ca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs)
 {
-	struct ref *ref_map;
+	struct ref *ref_map = NULL;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 	if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
-		free_refs(ref_map);
 		retcode = 1;
 		goto cleanup;
 	}
 
+	/*
+	 * If neither --no-tags nor --tags was specified, do automated tag
+	 * following.
+	 */
+	if (tags == TAGS_DEFAULT && autotags) {
+		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
+
+		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+		if (tags_ref_map)
+			backfill_tags(transport, tags_ref_map, worktrees);
+
+		free_refs(tags_ref_map);
+	}
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;
@@ -1644,7 +1657,7 @@ static int do_fetch(struct transport *transport,
 			if (!rm->peer_ref) {
 				if (source_ref) {
 					warning(_("multiple branches detected, incompatible with --set-upstream"));
-					goto skip;
+					goto cleanup;
 				} else {
 					source_ref = rm;
 				}
@@ -1658,7 +1671,7 @@ static int do_fetch(struct transport *transport,
 				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
 					  "it does not point to any branch."),
 					shortname, transport->remote->name);
-				goto skip;
+				goto cleanup;
 			}
 
 			if (!strcmp(source_ref->name, "HEAD") ||
@@ -1678,21 +1691,9 @@ static int do_fetch(struct transport *transport,
 				  "you need to specify exactly one branch with the --set-upstream option"));
 		}
 	}
-skip:
-	free_refs(ref_map);
-
-	/* if neither --no-tags nor --tags was specified, do automated tag
-	 * following ... */
-	if (tags == TAGS_DEFAULT && autotags) {
-		struct ref **tail = &ref_map;
-		ref_map = NULL;
-		find_non_local_tags(remote_refs, &ref_map, &tail);
-		if (ref_map)
-			backfill_tags(transport, ref_map, worktrees);
-		free_refs(ref_map);
-	}
 
 cleanup:
+	free_refs(ref_map);
 	free_worktrees(worktrees);
 	return retcode;
 }
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5634 bytes --]

There are two different locations where we're appending to FETCH_HEAD:
first when storing updated references, and second when backfilling tags.
Both times we open the file, append to it and then commit it into place,
which is essentially duplicate work.

Improve the lifecycle of updating FETCH_HEAD by opening and committing
it once in `do_fetch()`, where we pass the structure down to the code
which wants to append to it.

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/7] fetch: report errors when backfilling tags fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

When the backfilling of tags fails we do not report this error to the
caller, but only report it implicitly at a later point when reporting
updated references. This leaves callers unable to act upon the
information of whether the backfilling succeeded or not.

Refactor the function to return an error code and pass it up the
callstack. This causes us to correctly propagate the error back to the
user of git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c      | 26 ++++++++++++++++++--------
 t/t5503-tagfollow.sh |  4 +---
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f8adb40b45..d304314f16 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	return transport;
 }
 
-static void backfill_tags(struct transport *transport,
-			  struct ref *ref_map,
-			  struct fetch_head *fetch_head,
-			  struct worktree **worktrees)
+static int backfill_tags(struct transport *transport,
+			 struct ref *ref_map,
+			 struct fetch_head *fetch_head,
+			 struct worktree **worktrees)
 {
-	int cannot_reuse;
+	int retcode, cannot_reuse;
 
 	/*
 	 * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it
@@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+	retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
 		gsecondary = NULL;
 	}
+
+	return retcode;
 }
 
 static int do_fetch(struct transport *transport,
@@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
 		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
-		if (tags_ref_map)
-			backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
+		if (tags_ref_map) {
+			/*
+			 * If backfilling of tags fails then we want to tell
+			 * the user so, but we have to continue regardless to
+			 * populate upstream information of the references we
+			 * have already fetched above.
+			 */
+			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+				retcode = 1;
+		}
 
 		free_refs(tags_ref_map);
 	}
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 6ffe2a5719..c057c49e80 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
 		done
 	EOF
 
-	# Even though we fail to create refs/tags/tag1 the below command
-	# unexpectedly succeeds.
-	git -C clone5 fetch .. $B:refs/heads/something &&
+	test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
 	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
 	test $S = $(git -C clone5 rev-parse --verify tag2) &&
 	test_must_fail git -C clone5 rev-parse --verify tag1
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/7] refs: add interface to iterate over queued transactional updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

There is no way for a caller to see whether a reference update has
already been queued up for a given reference transaction. There are
multiple alternatives to provide this functionality:

    - We may add a function that simply tells us whether a specific
      reference has already been queued. If implemented naively then
      this would potentially be quadratic in runtime behaviour if this
      question is asked repeatedly because we have to iterate over all
      references every time. The alternative would be to add a hashmap
      of all queued reference updates to speed up the lookup, but this
      adds overhead to all callers.

    - We may add a flag to `ref_transaction_add_update()` that causes it
      to skip duplicates, but this has the same runtime concerns as the
      first alternative.

    - We may add an interface which lets callers collect all updates
      which have already been queued such that he can avoid re-adding
      them. This is the most flexible approach and puts the burden on
      the caller, but also allows us to not impact any of the existing
      callsites which don't need this information.

This commit implements the last approach: it allows us to compute the
map of already-queued updates once up front such that we can then skip
all subsequent references which are already part of this map.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 16 ++++++++++++++++
 refs.h | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/refs.c b/refs.c
index d680de3bc0..35d4e69687 100644
--- a/refs.c
+++ b/refs.c
@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 	return refs->be->initial_transaction_commit(refs, transaction, err);
 }
 
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+					    ref_transaction_for_each_queued_update_fn cb,
+					    void *cb_data)
+{
+	int i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+
+		cb(update->refname,
+		   (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,
+		   (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,
+		   cb_data);
+	}
+}
+
 int refs_delete_refs(struct ref_store *refs, const char *logmsg,
 		     struct string_list *refnames, unsigned int flags)
 {
diff --git a/refs.h b/refs.h
index ff859d5951..1ae12c410a 100644
--- a/refs.h
+++ b/refs.h
@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err);
 
+/*
+ * Execute the given callback function for each of the reference updates which
+ * have been queued in the given transaction. `old_oid` and `new_oid` may be
+ * `NULL` pointers depending on whether the update has these object IDs set or
+ * not.
+ */
+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,
+						       const struct object_id *old_oid,
+						       const struct object_id *new_oid,
+						       void *cb_data);
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+					    ref_transaction_for_each_queued_update_fn cb,
+					    void *cb_data);
+
 /*
  * Free `*transaction` and all associated data.
  */
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 10780 bytes --]

When fetching references from a remote we by default also fetch all tags
which point into the history we have fetched. This is a separate step
performed after updating local references because it requires us to walk
over the history on the client-side to determine whether the remote has
announced any tags which point to one of the fetched commits.

This backfilling of tags isn't covered by the `--atomic` flag: right
now, it only applies to the step where we update our local references.
This is an oversight at the time the flag was introduced: its purpose is
to either update all references or none, but right now we happily update
local references even in the case where backfilling failed.

Fix this by pulling up creation of the reference transaction such that
we can pass the same transaction to both the code which updates local
references and to the code which backfills tags. This allows us to only
commit the transaction in case both actions succeed.

Note that we also have to start passing the transaction into
`find_non_local_tags()`: this function is responsible for finding all
tags which we need to backfill. Right now, it will happily return tags
which have already been updated with our local references. But when we
use a single transaction for both local references and backfilling then
it may happen that we try to queue the same reference update twice to
the transaction, which consequently triggers a bug. We thus have to skip
over any tags which have already been queued.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c      | 92 +++++++++++++++++++++++++++++++-------------
 t/t5503-tagfollow.sh | 11 ++----
 2 files changed, 69 insertions(+), 34 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d304314f16..67af842091 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)
 	item->ignore = 1;
 }
 
+
+static void add_already_queued_tags(const char *refname,
+				    const struct object_id *old_oid,
+				    const struct object_id *new_oid,
+				    void *cb_data)
+{
+	struct hashmap *queued_tags = cb_data;
+	if (starts_with(refname, "refs/tags/") && new_oid)
+		(void) refname_hash_add(queued_tags, refname, new_oid);
+}
+
 static void find_non_local_tags(const struct ref *refs,
+				struct ref_transaction *transaction,
 				struct ref **head,
 				struct ref ***tail)
 {
@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *refs,
 	create_fetch_oidset(head, &fetch_oids);
 
 	for_each_ref(add_one_refname, &existing_refs);
+
+	/*
+	 * If we already have a transaction, then we need to filter out all
+	 * tags which have already been queued up.
+	 */
+	if (transaction)
+		ref_transaction_for_each_queued_update(transaction,
+						       add_already_queued_tags,
+						       &existing_refs);
+
 	for (ref = refs; ref; ref = ref->next) {
 		if (!starts_with(ref->name, "refs/tags/"))
 			continue;
@@ -600,7 +622,7 @@ static struct ref *get_ref_map(struct remote *remote,
 		/* also fetch all tags */
 		get_fetch_map(remote_refs, tag_refspec, &tail, 0);
 	else if (tags == TAGS_DEFAULT && *autotags)
-		find_non_local_tags(remote_refs, &ref_map, &tail);
+		find_non_local_tags(remote_refs, NULL, &ref_map, &tail);
 
 	/* Now append any refs to be updated opportunistically: */
 	*tail = orefs;
@@ -1083,12 +1105,12 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "to avoid this check\n");
 
 static int store_updated_refs(const char *raw_url, const char *remote_name,
-			      int connectivity_checked, struct ref *ref_map,
+			      int connectivity_checked,
+			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head, struct worktree **worktrees)
 {
 	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
-	struct ref_transaction *transaction = NULL;
 	const char *what, *kind;
 	struct ref *rm;
 	char *url;
@@ -1110,14 +1132,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (atomic_fetch) {
-		transaction = ref_transaction_begin(&err);
-		if (!transaction) {
-			error("%s", err.buf);
-			goto abort;
-		}
-	}
-
 	prepare_format_display(ref_map);
 
 	/*
@@ -1233,14 +1247,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (!rc && transaction) {
-		rc = ref_transaction_commit(transaction, &err);
-		if (rc) {
-			error("%s", err.buf);
-			goto abort;
-		}
-	}
-
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
 		      " 'git remote prune %s' to remove any old, conflicting "
@@ -1258,7 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
  abort:
 	strbuf_release(&note);
 	strbuf_release(&err);
-	ref_transaction_free(transaction);
 	free(url);
 	return rc;
 }
@@ -1299,6 +1304,7 @@ static int check_exist_and_connected(struct ref *ref_map)
 }
 
 static int fetch_and_consume_refs(struct transport *transport,
+				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
 				  struct fetch_head *fetch_head,
 				  struct worktree **worktrees)
@@ -1323,7 +1329,8 @@ static int fetch_and_consume_refs(struct transport *transport,
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
 	ret = store_updated_refs(transport->url, transport->remote->name,
-				 connectivity_checked, ref_map, fetch_head, worktrees);
+				 connectivity_checked, transaction, ref_map,
+				 fetch_head, worktrees);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1496,6 +1503,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 }
 
 static int backfill_tags(struct transport *transport,
+			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
 			 struct fetch_head *fetch_head,
 			 struct worktree **worktrees)
@@ -1519,7 +1527,7 @@ static int backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1532,6 +1540,7 @@ static int backfill_tags(struct transport *transport,
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs)
 {
+	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
@@ -1541,6 +1550,7 @@ static int do_fetch(struct transport *transport,
 	int must_list_refs = 1;
 	struct worktree **worktrees = get_worktrees();
 	struct fetch_head fetch_head = { 0 };
+	struct strbuf err = STRBUF_INIT;
 
 	if (tags == TAGS_DEFAULT) {
 		if (transport->remote->fetch_tags == 2)
@@ -1602,6 +1612,14 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
+	if (atomic_fetch) {
+		transaction = ref_transaction_begin(&err);
+		if (!transaction) {
+			retcode = error("%s", err.buf);
+			goto cleanup;
+		}
+	}
+
 	if (tags == TAGS_DEFAULT && autotags)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 	if (prune) {
@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
+	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1633,21 +1651,37 @@ static int do_fetch(struct transport *transport,
 	if (tags == TAGS_DEFAULT && autotags) {
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
-		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
 			 * the user so, but we have to continue regardless to
 			 * populate upstream information of the references we
-			 * have already fetched above.
+			 * have already fetched above. The exception though is
+			 * when `--atomic` is passed: in that case we'll abort
+			 * the transaction and don't commit anything.
 			 */
-			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+			if (backfill_tags(transport, transaction, tags_ref_map,
+					  &fetch_head, worktrees))
 				retcode = 1;
 		}
 
 		free_refs(tags_ref_map);
 	}
 
+	if (transaction) {
+		if (retcode)
+			goto cleanup;
+
+		retcode = ref_transaction_commit(transaction, &err);
+		if (retcode) {
+			error("%s", err.buf);
+			ref_transaction_free(transaction);
+			transaction = NULL;
+			goto cleanup;
+		}
+	}
+
 	commit_fetch_head(&fetch_head);
 
 	if (set_upstream) {
@@ -1705,7 +1739,13 @@ static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
+	if (retcode && transaction) {
+		ref_transaction_abort(transaction, &err);
+		error("%s", err.buf);
+	}
+
 	close_fetch_head(&fetch_head);
+	strbuf_release(&err);
 	free_refs(ref_map);
 	free_worktrees(worktrees);
 	return retcode;
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index c057c49e80..e72fdc2534 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -180,11 +180,8 @@ test_expect_success 'atomic fetch with failing backfill' '
 	EOF
 
 	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
-
-	# Creation of the tag has failed, so ideally refs/heads/something
-	# should not exist. The fact that it does demonstrates that there is
-	# a bug in the `--atomic` flag.
-	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+	test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
+	test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
 '
 
 test_expect_success 'atomic fetch with backfill should use single transaction' '
@@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
 		prepared
 		$ZERO_OID $B refs/heads/something
 		$ZERO_OID $S refs/tags/tag2
+		$ZERO_OID $T refs/tags/tag1
 		committed
 		$ZERO_OID $B refs/heads/something
 		$ZERO_OID $S refs/tags/tag2
-		prepared
-		$ZERO_OID $T refs/tags/tag1
-		committed
 		$ZERO_OID $T refs/tags/tag1
 	EOF
 
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 7/7] fetch: make `--atomic` flag cover pruning of refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5171 bytes --]

When fetching with the `--prune` flag we will delete any local
references matching the fetch refspec which have disappeared on the
remote. This step is not currently covered by the `--atomic` flag: we
delete branches even though updating of local references has failed,
which means that the fetch is not an all-or-nothing operation.

Fix this bug by passing in the global transaction into `prune_refs()`:
if one is given, then we'll only queue up deletions and not commit them
right away.

This change also improves performance when pruning many branches in a
repository with a big packed-refs file: every references is pruned in
its own transaction, which means that we potentially have to rewrite
the packed-refs files for every single reference we're about to prune.

The following benchmark demonstrates this: it performs a pruning fetch
from a repository with a single reference into a repository with 100k
references, which causes us to prune all but one reference. This is of
course a very artificial setup, but serves to demonstrate the impact of
only having to write the packed-refs file once:

    Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)
      Time (mean ± σ):      2.366 s ±  0.021 s    [User: 0.858 s, System: 1.508 s]
      Range (min … max):    2.328 s …  2.407 s    10 runs

    Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)
      Time (mean ± σ):      1.369 s ±  0.017 s    [User: 0.715 s, System: 0.641 s]
      Range (min … max):    1.346 s …  1.400 s    10 runs

    Summary
      'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran
        1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c  | 30 ++++++++++++++++++++++--------
 t/t5510-fetch.sh |  8 ++------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67af842091..9a2b5c03a4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1338,11 +1338,14 @@ static int fetch_and_consume_refs(struct transport *transport,
 	return ret;
 }
 
-static int prune_refs(struct refspec *rs, struct ref *ref_map,
+static int prune_refs(struct refspec *rs,
+		      struct ref_transaction *transaction,
+		      struct ref *ref_map,
 		      const char *raw_url)
 {
 	int url_len, i, result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
+	struct strbuf err = STRBUF_INIT;
 	char *url;
 	int summary_width = transport_summary_width(stale_refs);
 	const char *dangling_msg = dry_run
@@ -1363,13 +1366,22 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 		url_len = i - 3;
 
 	if (!dry_run) {
-		struct string_list refnames = STRING_LIST_INIT_NODUP;
+		if (transaction) {
+			for (ref = stale_refs; ref; ref = ref->next) {
+				result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+								"fetch: prune", &err);
+				if (result)
+					goto cleanup;
+			}
+		} else {
+			struct string_list refnames = STRING_LIST_INIT_NODUP;
 
-		for (ref = stale_refs; ref; ref = ref->next)
-			string_list_append(&refnames, ref->name);
+			for (ref = stale_refs; ref; ref = ref->next)
+				string_list_append(&refnames, ref->name);
 
-		result = delete_refs("fetch: prune", &refnames, 0);
-		string_list_clear(&refnames, 0);
+			result = delete_refs("fetch: prune", &refnames, 0);
+			string_list_clear(&refnames, 0);
+		}
 	}
 
 	if (verbosity >= 0) {
@@ -1388,6 +1400,8 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 		}
 	}
 
+cleanup:
+	strbuf_release(&err);
 	free(url);
 	free_refs(stale_refs);
 	return result;
@@ -1629,10 +1643,10 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(rs, ref_map, transport->url);
+			retcode = prune_refs(rs, transaction, ref_map, transport->url);
 		} else {
 			retcode = prune_refs(&transport->remote->fetch,
-					     ref_map,
+					     transaction, ref_map,
 					     transport->url);
 		}
 		if (retcode != 0)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 70d51f343b..48e14e2dab 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact
 	head_oid=$(git rev-parse HEAD) &&
 
 	# Fetching with the `--atomic` flag should update all references in a
-	# single transaction. It is currently missing coverage of pruned
-	# references though, and as a result those may be committed to disk
-	# even if updating references fails later.
+	# single transaction.
 	cat >expected <<-EOF &&
 		prepared
 		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
-		committed
-		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
-		prepared
 		$ZERO_OID $head_oid refs/remotes/origin/new-branch
 		committed
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
 		$ZERO_OID $head_oid refs/remotes/origin/new-branch
 	EOF
 
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Christian Couder]

On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:

> +test_expect_success 'atomic fetch with failing backfill' '
> +       git init clone3 &&
> +
> +       # We want to test whether a failure when backfilling tags correctly
> +       # aborts the complete transaction when `--atomic` is passed: we should
> +       # neither create the branch nor should we create the tag when either
> +       # one of both fails to update correctly.
> +       #
> +       # To trigger failure we simply abort when backfilling a tag.
> +       write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> +               while read oldrev newrev reference
> +               do
> +                       if test "$reference" = refs/tags/tag1
> +                       then
> +                               exit 1
> +                       fi

Maybe the following could save a few lines:

                       test "$reference" = refs/tags/tag1 && exit 1

It would make the code look a bit different than in another hook
script written below though, so not a big deal.

> +               done
> +       EOF

Overall it looks good, and I like the improved commit message!

Re: [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag

[Christian Couder]

On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:

> This is the second version of my patch series which fixes these
> problems.

I took another look and found only a very minor improvement in one of
the tests in patch 1/7.

Thanks!

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When using git-fetch(1) with the `--atomic` flag the expectation is that
> either all of the references are updated, or alternatively none are in
> case the fetch fails. While we already have tests for this, we do not
> have any tests which exercise atomicity either when pruning deleted refs
> or when backfilling tags. This gap in test coverage hides that we indeed
> don't handle atomicity correctly for both of these cases.
>
> Add test cases which cover these testing gaps to demonstrate the broken
> behaviour. Note that tests are not marked as `test_expect_failure`: this
> is done to explicitly demonstrate the current known-wrong behaviour, and
> they will be fixed up as soon as we fix the underlying bugs.
>
> While at it this commit also adds another test case which demonstrates
> that backfilling of tags does not return an error code in case the
> backfill fails. This bug will also be fixed by a subsequent commit.

No need to reorganize the series just to fix this, but in general,
adding new tests that are designed to fail in an early commit and
then give fixes in a separate commit is to be avoided for two
reasons.

One is obviously that it breaks bisectability.  And another is that
it makes it unclear what the end-user visible problem was when
reading the actual fix, and makes reviewing the series harder.

If a test that demonstrates an existing bug comes with the code to
fix the bug in the same commit, it obviously will not break
bisectability, and the new test serves also as a documentation of
what the end-user visible symptoms of the bug being fixed are.

Sometimes people work around the bisectability issue by starting
these tests that demonstrates known-to-be-unfixed-yet bugs with
test_expect_failure, but that is not a good solution for the
reviewability issue, as the step that fixes the bug will show the
code fix plus only the first few lines of the test being changed
to expect success and it is not readily visible what the symptom
actually were.

A series of patches, each one of which fixes one issue and
demonstrates the issue with tests that expects success when the
issue is resolved, is much more pleasant to read than a series where
it has tests for multiple issues, followed by a patch or two to
handle each issue.

And in such a series, it is much easier to see what breaks if the
fix weren't there.  Reviewers can apply one step of the patch and
tentatively revert everything outside t/ to see how the added test
breaks without the fix.  It helps those who may want to cherry-pick
a particular fix, together with its associated test, if a series is
done that way.

Thanks.

> +test_expect_success 'atomic fetch with failing backfill' '
> +	git init clone3 &&
> +
> +	# We want to test whether a failure when backfilling tags correctly
> +	# aborts the complete transaction when `--atomic` is passed: we should
> +	# neither create the branch nor should we create the tag when either
> +	# one of both fails to update correctly.
> +	#
> +	# To trigger failure we simply abort when backfilling a tag.

What does this paragraph want the phrase `backfilling tags` to mean?
Just from end-user's perspective, there is only one (i.e. if an
object that is tagged gets fetched and that tag is not here, fetch
it too), but at the mechanism level, there are two distinct code
paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
call backfill_tags()).  Which failure does this talk about, or it
does not matter which code path gave us the tag?

> +	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> +		while read oldrev newrev reference
> +		do
> +			if test "$reference" = refs/tags/tag1
> +			then
> +				exit 1
> +			fi
> +		done
> +	EOF
> +
> +	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> +
> +	# Creation of the tag has failed, so ideally refs/heads/something
> +	# should not exist. The fact that it does demonstrates that there is
> +	# a bug in the `--atomic` flag.
> +	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> +'
> +
> +test_expect_success 'atomic fetch with backfill should use single transaction' '
> +	git init clone4 &&
> +
> +	# Fetching with the `--atomic` flag should update all references in a
> +	# single transaction, including backfilled tags. We thus expect to see
> +	# a single reference transaction for the created branch and tags.
> +	cat >expected <<-EOF &&
> +		prepared
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		committed
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		prepared
> +		$ZERO_OID $T refs/tags/tag1
> +		committed
> +		$ZERO_OID $T refs/tags/tag1
> +	EOF
> +
> +	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> +		( echo "$*" && cat ) >>actual
> +	EOF

Nicely done.

> +
> +	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> +	test_cmp expected clone4/actual
> +'
> +
> +test_expect_success 'backfill failure causes command to fail' '
> +	git init clone5 &&
> +
> +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&

Is there a reason why we cannot do <<-\EOF here to lose the
backslashes in the here-text?

> +		while read oldrev newrev reference
> +		do
> +			if test "\$reference" = refs/tags/tag1
> +			then
> +				# Create a nested tag below the actual tag we
> +				# wanted to write, which causes a D/F conflict
> +				# later when we want to commit refs/tags/tag1.
> +				# We cannot just `exit 1` here given that this
> +				# would cause us to die immediately.
> +				git update-ref refs/tags/tag1/nested $B

Wow, nasty ;-)

> +				exit \$!
> +			fi
> +		done
> +	EOF
> +
> +	# Even though we fail to create refs/tags/tag1 the below command
> +	# unexpectedly succeeds.
> +	git -C clone5 fetch .. $B:refs/heads/something &&
> +	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
> +	test $S = $(git -C clone5 rev-parse --verify tag2) &&
> +	test_must_fail git -C clone5 rev-parse --verify tag1
> +'
> +
>  test_done

Re: [PATCH v2 2/7] fetch: backfill tags before setting upstream

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The fetch code flow is a bit hard to understand right now:
>
>     1. We optionally prune all references which have vanished on the
>        remote side.
>     2. We fetch and update all other references locally.
>     3. We update the upstream branch in the gitconfig.
>     4. We backfill tags pointing into the history we have just fetched.
>
> It is quite confusing that we fetch objects and update references in
> both (2) and (4), which is further stressed by the point that we use a
> `skip` goto label to jump from (3) to (4) in case we fail to update the
> gitconfig as expected.
>
> Reorder the code to first update all local references, and only after we
> have done so update the upstream branch information. This improves the
> code flow and furthermore makes it easier to refactor the way we update
> references together.

OK, as "setting upsream" is more or less unrelated to the act of
actual fetching the refs and objects reachable from them, moving it
outside the main code that is about fetching does make sense.

>  static int do_fetch(struct transport *transport,
>  		    struct refspec *rs)
>  {
> -	struct ref *ref_map;
> +	struct ref *ref_map = NULL;

This is needed because we will always do free_refs() on the variable
after this patch, and one early "goto cleanup" happens even before
we touch ref_map, when truncate_fetch_head() fails.

> @@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
>  			retcode = 1;
>  	}
>  	if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
> -		free_refs(ref_map);
>  		retcode = 1;
>  		goto cleanup;

And because we always free_refs(ref_map), we can lose a call here.
OK.

> +	/*
> +	 * If neither --no-tags nor --tags was specified, do automated tag
> +	 * following.
> +	 */
> +	if (tags == TAGS_DEFAULT && autotags) {
> +		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
> +
> +		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> +		if (tags_ref_map)
> +			backfill_tags(transport, tags_ref_map, worktrees);
> +
> +		free_refs(tags_ref_map);
> +	}

Here, the new code uses a local and separete tags_ref_map variable
and free it before we leave, instead of reusing ref_map variable.

OK.

Re: [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> There are two different locations where we're appending to FETCH_HEAD:
> first when storing updated references, and second when backfilling tags.
> Both times we open the file, append to it and then commit it into place,
> which is essentially duplicate work.
>
> Improve the lifecycle of updating FETCH_HEAD by opening and committing
> it once in `do_fetch()`, where we pass the structure down to the code
> which wants to append to it.

OK.  Each call to store_updated_refs() used to be responsible for
opening, appending, committing, and closing FETCH_HEAD.  We now make
do_fetch() responsible for opening, committing, and closing, and let
the direct and indirect callers of fetch_and_consume_refs() only
worry about appending to it.  Makes sense.

Re: [PATCH v2 4/7] fetch: report errors when backfilling tags fails

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +static int backfill_tags(struct transport *transport,
> ...
>  }

The change to this function is quite straight-forward.

>  static int do_fetch(struct transport *transport,
> @@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> -		if (tags_ref_map)
> -			backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
> +		if (tags_ref_map) {
> +			/*
> +			 * If backfilling of tags fails then we want to tell
> +			 * the user so, but we have to continue regardless to
> +			 * populate upstream information of the references we
> +			 * have already fetched above.
> +			 */

OK.  Unless atomic, using the available information on successfully
updated ones would be fine.  Perhaps under --atomic mode, a failure
to commit the ref transaction will also prevent the upstream
information from getting updated?  We'll see soon enough.

> +			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
> +				retcode = 1;
> +		}

>  		free_refs(tags_ref_map);
>  	}
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 6ffe2a5719..c057c49e80 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
>  		done
>  	EOF
>  
> -	# Even though we fail to create refs/tags/tag1 the below command
> -	# unexpectedly succeeds.
> -	git -C clone5 fetch .. $B:refs/heads/something &&
> +	test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&

OK.

>  	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
>  	test $S = $(git -C clone5 rev-parse --verify tag2) &&
>  	test_must_fail git -C clone5 rev-parse --verify tag1

Re: [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Fix this by pulling up creation of the reference transaction such that
> we can pass the same transaction to both the code which updates local
> references and to the code which backfills tags. This allows us to only
> commit the transaction in case both actions succeed.

OK, having done the FETCH_HEAD thing, the idea is quite similar.
Instead of letting two invocations to store_updated_refs() to
independently open and close separate transactions, we control
everything centrally in do_fetch().

Makes sense.

> @@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
>  		prepared
>  		$ZERO_OID $B refs/heads/something
>  		$ZERO_OID $S refs/tags/tag2
> +		$ZERO_OID $T refs/tags/tag1
>  		committed
>  		$ZERO_OID $B refs/heads/something
>  		$ZERO_OID $S refs/tags/tag2
> -		prepared
> -		$ZERO_OID $T refs/tags/tag1
> -		committed
>  		$ZERO_OID $T refs/tags/tag1
>  	EOF

OK.

Unlike the "expect the behaviour at the end of the series from the
beginning with known-to-fail tests" pattern I cautioned against in
an earlier step, this is a good way to show how the behaviour
changes.

Thanks.

Re: [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:
>
>> This is the second version of my patch series which fixes these
>> problems.
>
> I took another look and found only a very minor improvement in one of
> the tests in patch 1/7.

Yup, the whole series was a pleasant read.

Thanks, both.  Will queue.

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Is there a reason why we cannot do <<-\EOF here to lose the
> backslashes in the here-text?

Yes.  We refer to "$B".

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > +test_expect_success 'atomic fetch with failing backfill' '
> > +	git init clone3 &&
> > +
> > +	# We want to test whether a failure when backfilling tags correctly
> > +	# aborts the complete transaction when `--atomic` is passed: we should
> > +	# neither create the branch nor should we create the tag when either
> > +	# one of both fails to update correctly.
> > +	#
> > +	# To trigger failure we simply abort when backfilling a tag.
> 
> What does this paragraph want the phrase `backfilling tags` to mean?
> Just from end-user's perspective, there is only one (i.e. if an
> object that is tagged gets fetched and that tag is not here, fetch
> it too), but at the mechanism level, there are two distinct code
> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
> call backfill_tags()).  Which failure does this talk about, or it
> does not matter which code path gave us the tag?

It refers to `backfill_tags()`. Should I update this comment to clarify?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
> [snip]
>> > +test_expect_success 'atomic fetch with failing backfill' '
>> > +	git init clone3 &&
>> > +
>> > +	# We want to test whether a failure when backfilling tags correctly
>> > +	# aborts the complete transaction when `--atomic` is passed: we should
>> > +	# neither create the branch nor should we create the tag when either
>> > +	# one of both fails to update correctly.
>> > +	#
>> > +	# To trigger failure we simply abort when backfilling a tag.
>> 
>> What does this paragraph want the phrase `backfilling tags` to mean?
>> Just from end-user's perspective, there is only one (i.e. if an
>> object that is tagged gets fetched and that tag is not here, fetch
>> it too), but at the mechanism level, there are two distinct code
>> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
>> call backfill_tags()).  Which failure does this talk about, or it
>> does not matter which code path gave us the tag?
>
> It refers to `backfill_tags()`. Should I update this comment to clarify?

After reading the patch, the issue is only about the case where we
perform the second fetch and the case where OPT_FOLLOWTAGS does
everything necessary is not relevant.  So hinting that we are
interested to see what a failure in the follow-on fetch does to the
atomicity would be a good idea, and mentioning backfill_tags() would
have been a good way to do so.  Perhaps like "whether a failure in
backfill_tags() correctly aborts..." or something?

Thanks.


    

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On Thu, Feb 17, 2022 at 04:18:07PM +0100, Christian Couder wrote:
> On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > +test_expect_success 'atomic fetch with failing backfill' '
> > +       git init clone3 &&
> > +
> > +       # We want to test whether a failure when backfilling tags correctly
> > +       # aborts the complete transaction when `--atomic` is passed: we should
> > +       # neither create the branch nor should we create the tag when either
> > +       # one of both fails to update correctly.
> > +       #
> > +       # To trigger failure we simply abort when backfilling a tag.
> > +       write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> > +               while read oldrev newrev reference
> > +               do
> > +                       if test "$reference" = refs/tags/tag1
> > +                       then
> > +                               exit 1
> > +                       fi
> 
> Maybe the following could save a few lines:
> 
>                        test "$reference" = refs/tags/tag1 && exit 1
> 
> It would make the code look a bit different than in another hook
> script written below though, so not a big deal.

If `$reference` does not match the tag we want to continue and
eventually return successfully from this hook. But:

    $ while read foo; do test "$foo" = bar && echo match; done < <(echo bar)
    match
    $ while read foo; do test "$foo" = bar && echo match; done < <(echo foo)
    $ echo $?
    1

So with the proposed change we'd now exit the hook with an error code
instead of returning successfully.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 0/7] fetch: improve atomicity of `--atomic` flag

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]

Hi,

in c7b190dabd (fetch: implement support for atomic reference updates,
2021-01-12), I have added a new `--atomic` flag to git-fetch(1) which is
supposed to make it an all-or-nothing operation: either all refs are
updated, or none is. I have recently discovered though that there were
two oversights: neither pruning of refs via `--prune` nor the tag
backfill mechanism are currently covered by this flag, which breaks the
promise.

This is the third version of this patch series fixing these issues. The
only change compared to v2 is a clarification in a comment to better
point out which type of backfilling is being tested.

Patrick

Patrick Steinhardt (7):
  fetch: increase test coverage of fetches
  fetch: backfill tags before setting upstream
  fetch: control lifecycle of FETCH_HEAD in a single place
  fetch: report errors when backfilling tags fails
  refs: add interface to iterate over queued transactional updates
  fetch: make `--atomic` flag cover backfilling of tags
  fetch: make `--atomic` flag cover pruning of refs

 builtin/fetch.c      | 192 +++++++++++++++++++++++++++++--------------
 refs.c               |  16 ++++
 refs.h               |  14 ++++
 t/t5503-tagfollow.sh |  74 +++++++++++++++++
 t/t5510-fetch.sh     |  29 +++++++
 5 files changed, 263 insertions(+), 62 deletions(-)

Range-diff against v2:
1:  b4ca3f1f3b ! 1:  081174b7a0 fetch: increase test coverage of fetches
    @@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '
     +test_expect_success 'atomic fetch with failing backfill' '
     +	git init clone3 &&
     +
    -+	# We want to test whether a failure when backfilling tags correctly
    ++	# We want to test whether a failure in `backfill_tags()` correctly
     +	# aborts the complete transaction when `--atomic` is passed: we should
     +	# neither create the branch nor should we create the tag when either
     +	# one of both fails to update correctly.
2:  b0a067bbc1 = 2:  9867e76ac7 fetch: backfill tags before setting upstream
3:  0b9d04622d = 3:  7c36ecbcf4 fetch: control lifecycle of FETCH_HEAD in a single place
4:  bc1e396ae0 = 4:  a7e005dd48 fetch: report errors when backfilling tags fails
5:  fac2e3555a = 5:  0316d770e9 refs: add interface to iterate over queued transactional updates
6:  331ee40e57 = 6:  2f98e34c84 fetch: make `--atomic` flag cover backfilling of tags
7:  2ad16530e5 = 7:  7292de826b fetch: make `--atomic` flag cover pruning of refs
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5686 bytes --]

When using git-fetch(1) with the `--atomic` flag the expectation is that
either all of the references are updated, or alternatively none are in
case the fetch fails. While we already have tests for this, we do not
have any tests which exercise atomicity either when pruning deleted refs
or when backfilling tags. This gap in test coverage hides that we indeed
don't handle atomicity correctly for both of these cases.

Add test cases which cover these testing gaps to demonstrate the broken
behaviour. Note that tests are not marked as `test_expect_failure`: this
is done to explicitly demonstrate the current known-wrong behaviour, and
they will be fixed up as soon as we fix the underlying bugs.

While at it this commit also adds another test case which demonstrates
that backfilling of tags does not return an error code in case the
backfill fails. This bug will also be fixed by a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh     | 33 ++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd4..7dadaafd4b 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -160,4 +160,85 @@ test_expect_success 'new clone fetch main and tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'atomic fetch with failing backfill' '
+	git init clone3 &&
+
+	# We want to test whether a failure in `backfill_tags()` correctly
+	# aborts the complete transaction when `--atomic` is passed: we should
+	# neither create the branch nor should we create the tag when either
+	# one of both fails to update correctly.
+	#
+	# To trigger failure we simply abort when backfilling a tag.
+	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
+		while read oldrev newrev reference
+		do
+			if test "$reference" = refs/tags/tag1
+			then
+				exit 1
+			fi
+		done
+	EOF
+
+	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
+
+	# Creation of the tag has failed, so ideally refs/heads/something
+	# should not exist. The fact that it does demonstrates that there is
+	# a bug in the `--atomic` flag.
+	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+'
+
+test_expect_success 'atomic fetch with backfill should use single transaction' '
+	git init clone4 &&
+
+	# Fetching with the `--atomic` flag should update all references in a
+	# single transaction, including backfilled tags. We thus expect to see
+	# a single reference transaction for the created branch and tags.
+	cat >expected <<-EOF &&
+		prepared
+		$ZERO_OID $B refs/heads/something
+		$ZERO_OID $S refs/tags/tag2
+		committed
+		$ZERO_OID $B refs/heads/something
+		$ZERO_OID $S refs/tags/tag2
+		prepared
+		$ZERO_OID $T refs/tags/tag1
+		committed
+		$ZERO_OID $T refs/tags/tag1
+	EOF
+
+	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
+		( echo "$*" && cat ) >>actual
+	EOF
+
+	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
+	test_cmp expected clone4/actual
+'
+
+test_expect_success 'backfill failure causes command to fail' '
+	git init clone5 &&
+
+	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
+		while read oldrev newrev reference
+		do
+			if test "\$reference" = refs/tags/tag1
+			then
+				# Create a nested tag below the actual tag we
+				# wanted to write, which causes a D/F conflict
+				# later when we want to commit refs/tags/tag1.
+				# We cannot just `exit 1` here given that this
+				# would cause us to die immediately.
+				git update-ref refs/tags/tag1/nested $B
+				exit \$!
+			fi
+		done
+	EOF
+
+	# Even though we fail to create refs/tags/tag1 the below command
+	# unexpectedly succeeds.
+	git -C clone5 fetch .. $B:refs/heads/something &&
+	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
+	test $S = $(git -C clone5 rev-parse --verify tag2) &&
+	test_must_fail git -C clone5 rev-parse --verify tag1
+'
+
 test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ef0da0a63b..70d51f343b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -343,6 +343,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
 	test_cmp expected atomic/.git/FETCH_HEAD
 '
 
+test_expect_success 'fetch --atomic --prune executes a single reference transaction only' '
+	test_when_finished "rm -rf \"$D\"/atomic" &&
+
+	cd "$D" &&
+	git branch scheduled-for-deletion &&
+	git clone . atomic &&
+	git branch -D scheduled-for-deletion &&
+	git branch new-branch &&
+	head_oid=$(git rev-parse HEAD) &&
+
+	# Fetching with the `--atomic` flag should update all references in a
+	# single transaction. It is currently missing coverage of pruned
+	# references though, and as a result those may be committed to disk
+	# even if updating references fails later.
+	cat >expected <<-EOF &&
+		prepared
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+		committed
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+		prepared
+		$ZERO_OID $head_oid refs/remotes/origin/new-branch
+		committed
+		$ZERO_OID $head_oid refs/remotes/origin/new-branch
+	EOF
+
+	write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+		( echo "$*" && cat ) >>actual
+	EOF
+
+	git -C atomic fetch --atomic --prune origin &&
+	test_cmp expected atomic/actual
+'
+
 test_expect_success '--refmap="" ignores configured refspec' '
 	cd "$TRASH_DIRECTORY" &&
 	git clone "$D" remote-refs &&
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/7] fetch: backfill tags before setting upstream

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

The fetch code flow is a bit hard to understand right now:

    1. We optionally prune all references which have vanished on the
       remote side.
    2. We fetch and update all other references locally.
    3. We update the upstream branch in the gitconfig.
    4. We backfill tags pointing into the history we have just fetched.

It is quite confusing that we fetch objects and update references in
both (2) and (4), which is further stressed by the point that we use a
`skip` goto label to jump from (3) to (4) in case we fail to update the
gitconfig as expected.

Reorder the code to first update all local references, and only after we
have done so update the upstream branch information. This improves the
code flow and furthermore makes it easier to refactor the way we update
references together.

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6f5e157863..904ca9f1ca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs)
 {
-	struct ref *ref_map;
+	struct ref *ref_map = NULL;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
 	const struct ref *remote_refs;
@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 	if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
-		free_refs(ref_map);
 		retcode = 1;
 		goto cleanup;
 	}
 
+	/*
+	 * If neither --no-tags nor --tags was specified, do automated tag
+	 * following.
+	 */
+	if (tags == TAGS_DEFAULT && autotags) {
+		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
+
+		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+		if (tags_ref_map)
+			backfill_tags(transport, tags_ref_map, worktrees);
+
+		free_refs(tags_ref_map);
+	}
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;
@@ -1644,7 +1657,7 @@ static int do_fetch(struct transport *transport,
 			if (!rm->peer_ref) {
 				if (source_ref) {
 					warning(_("multiple branches detected, incompatible with --set-upstream"));
-					goto skip;
+					goto cleanup;
 				} else {
 					source_ref = rm;
 				}
@@ -1658,7 +1671,7 @@ static int do_fetch(struct transport *transport,
 				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
 					  "it does not point to any branch."),
 					shortname, transport->remote->name);
-				goto skip;
+				goto cleanup;
 			}
 
 			if (!strcmp(source_ref->name, "HEAD") ||
@@ -1678,21 +1691,9 @@ static int do_fetch(struct transport *transport,
 				  "you need to specify exactly one branch with the --set-upstream option"));
 		}
 	}
-skip:
-	free_refs(ref_map);
-
-	/* if neither --no-tags nor --tags was specified, do automated tag
-	 * following ... */
-	if (tags == TAGS_DEFAULT && autotags) {
-		struct ref **tail = &ref_map;
-		ref_map = NULL;
-		find_non_local_tags(remote_refs, &ref_map, &tail);
-		if (ref_map)
-			backfill_tags(transport, ref_map, worktrees);
-		free_refs(ref_map);
-	}
 
 cleanup:
+	free_refs(ref_map);
 	free_worktrees(worktrees);
 	return retcode;
 }
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/7] fetch: control lifecycle of FETCH_HEAD in a single place

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5634 bytes --]

There are two different locations where we're appending to FETCH_HEAD:
first when storing updated references, and second when backfilling tags.
Both times we open the file, append to it and then commit it into place,
which is essentially duplicate work.

Improve the lifecycle of updating FETCH_HEAD by opening and committing
it once in `do_fetch()`, where we pass the structure down to the code
which wants to append to it.

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 4/7] fetch: report errors when backfilling tags fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

When the backfilling of tags fails we do not report this error to the
caller, but only report it implicitly at a later point when reporting
updated references. This leaves callers unable to act upon the
information of whether the backfilling succeeded or not.

Refactor the function to return an error code and pass it up the
callstack. This causes us to correctly propagate the error back to the
user of git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c      | 26 ++++++++++++++++++--------
 t/t5503-tagfollow.sh |  4 +---
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f8adb40b45..d304314f16 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 	return transport;
 }
 
-static void backfill_tags(struct transport *transport,
-			  struct ref *ref_map,
-			  struct fetch_head *fetch_head,
-			  struct worktree **worktrees)
+static int backfill_tags(struct transport *transport,
+			 struct ref *ref_map,
+			 struct fetch_head *fetch_head,
+			 struct worktree **worktrees)
 {
-	int cannot_reuse;
+	int retcode, cannot_reuse;
 
 	/*
 	 * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it
@@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+	retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
 		gsecondary = NULL;
 	}
+
+	return retcode;
 }
 
 static int do_fetch(struct transport *transport,
@@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
 		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
-		if (tags_ref_map)
-			backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
+		if (tags_ref_map) {
+			/*
+			 * If backfilling of tags fails then we want to tell
+			 * the user so, but we have to continue regardless to
+			 * populate upstream information of the references we
+			 * have already fetched above.
+			 */
+			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+				retcode = 1;
+		}
 
 		free_refs(tags_ref_map);
 	}
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 7dadaafd4b..27a1dfa3c0 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
 		done
 	EOF
 
-	# Even though we fail to create refs/tags/tag1 the below command
-	# unexpectedly succeeds.
-	git -C clone5 fetch .. $B:refs/heads/something &&
+	test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
 	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
 	test $S = $(git -C clone5 rev-parse --verify tag2) &&
 	test_must_fail git -C clone5 rev-parse --verify tag1
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 5/7] refs: add interface to iterate over queued transactional updates

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

There is no way for a caller to see whether a reference update has
already been queued up for a given reference transaction. There are
multiple alternatives to provide this functionality:

    - We may add a function that simply tells us whether a specific
      reference has already been queued. If implemented naively then
      this would potentially be quadratic in runtime behaviour if this
      question is asked repeatedly because we have to iterate over all
      references every time. The alternative would be to add a hashmap
      of all queued reference updates to speed up the lookup, but this
      adds overhead to all callers.

    - We may add a flag to `ref_transaction_add_update()` that causes it
      to skip duplicates, but this has the same runtime concerns as the
      first alternative.

    - We may add an interface which lets callers collect all updates
      which have already been queued such that he can avoid re-adding
      them. This is the most flexible approach and puts the burden on
      the caller, but also allows us to not impact any of the existing
      callsites which don't need this information.

This commit implements the last approach: it allows us to compute the
map of already-queued updates once up front such that we can then skip
all subsequent references which are already part of this map.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 16 ++++++++++++++++
 refs.h | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/refs.c b/refs.c
index d680de3bc0..35d4e69687 100644
--- a/refs.c
+++ b/refs.c
@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 	return refs->be->initial_transaction_commit(refs, transaction, err);
 }
 
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+					    ref_transaction_for_each_queued_update_fn cb,
+					    void *cb_data)
+{
+	int i;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = transaction->updates[i];
+
+		cb(update->refname,
+		   (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,
+		   (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,
+		   cb_data);
+	}
+}
+
 int refs_delete_refs(struct ref_store *refs, const char *logmsg,
 		     struct string_list *refnames, unsigned int flags)
 {
diff --git a/refs.h b/refs.h
index ff859d5951..1ae12c410a 100644
--- a/refs.h
+++ b/refs.h
@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err);
 
+/*
+ * Execute the given callback function for each of the reference updates which
+ * have been queued in the given transaction. `old_oid` and `new_oid` may be
+ * `NULL` pointers depending on whether the update has these object IDs set or
+ * not.
+ */
+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,
+						       const struct object_id *old_oid,
+						       const struct object_id *new_oid,
+						       void *cb_data);
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+					    ref_transaction_for_each_queued_update_fn cb,
+					    void *cb_data);
+
 /*
  * Free `*transaction` and all associated data.
  */
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 6/7] fetch: make `--atomic` flag cover backfilling of tags

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 10780 bytes --]

When fetching references from a remote we by default also fetch all tags
which point into the history we have fetched. This is a separate step
performed after updating local references because it requires us to walk
over the history on the client-side to determine whether the remote has
announced any tags which point to one of the fetched commits.

This backfilling of tags isn't covered by the `--atomic` flag: right
now, it only applies to the step where we update our local references.
This is an oversight at the time the flag was introduced: its purpose is
to either update all references or none, but right now we happily update
local references even in the case where backfilling failed.

Fix this by pulling up creation of the reference transaction such that
we can pass the same transaction to both the code which updates local
references and to the code which backfills tags. This allows us to only
commit the transaction in case both actions succeed.

Note that we also have to start passing the transaction into
`find_non_local_tags()`: this function is responsible for finding all
tags which we need to backfill. Right now, it will happily return tags
which have already been updated with our local references. But when we
use a single transaction for both local references and backfilling then
it may happen that we try to queue the same reference update twice to
the transaction, which consequently triggers a bug. We thus have to skip
over any tags which have already been queued.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c      | 92 +++++++++++++++++++++++++++++++-------------
 t/t5503-tagfollow.sh | 11 ++----
 2 files changed, 69 insertions(+), 34 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d304314f16..67af842091 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)
 	item->ignore = 1;
 }
 
+
+static void add_already_queued_tags(const char *refname,
+				    const struct object_id *old_oid,
+				    const struct object_id *new_oid,
+				    void *cb_data)
+{
+	struct hashmap *queued_tags = cb_data;
+	if (starts_with(refname, "refs/tags/") && new_oid)
+		(void) refname_hash_add(queued_tags, refname, new_oid);
+}
+
 static void find_non_local_tags(const struct ref *refs,
+				struct ref_transaction *transaction,
 				struct ref **head,
 				struct ref ***tail)
 {
@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *refs,
 	create_fetch_oidset(head, &fetch_oids);
 
 	for_each_ref(add_one_refname, &existing_refs);
+
+	/*
+	 * If we already have a transaction, then we need to filter out all
+	 * tags which have already been queued up.
+	 */
+	if (transaction)
+		ref_transaction_for_each_queued_update(transaction,
+						       add_already_queued_tags,
+						       &existing_refs);
+
 	for (ref = refs; ref; ref = ref->next) {
 		if (!starts_with(ref->name, "refs/tags/"))
 			continue;
@@ -600,7 +622,7 @@ static struct ref *get_ref_map(struct remote *remote,
 		/* also fetch all tags */
 		get_fetch_map(remote_refs, tag_refspec, &tail, 0);
 	else if (tags == TAGS_DEFAULT && *autotags)
-		find_non_local_tags(remote_refs, &ref_map, &tail);
+		find_non_local_tags(remote_refs, NULL, &ref_map, &tail);
 
 	/* Now append any refs to be updated opportunistically: */
 	*tail = orefs;
@@ -1083,12 +1105,12 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
    "to avoid this check\n");
 
 static int store_updated_refs(const char *raw_url, const char *remote_name,
-			      int connectivity_checked, struct ref *ref_map,
+			      int connectivity_checked,
+			      struct ref_transaction *transaction, struct ref *ref_map,
 			      struct fetch_head *fetch_head, struct worktree **worktrees)
 {
 	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
-	struct ref_transaction *transaction = NULL;
 	const char *what, *kind;
 	struct ref *rm;
 	char *url;
@@ -1110,14 +1132,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (atomic_fetch) {
-		transaction = ref_transaction_begin(&err);
-		if (!transaction) {
-			error("%s", err.buf);
-			goto abort;
-		}
-	}
-
 	prepare_format_display(ref_map);
 
 	/*
@@ -1233,14 +1247,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (!rc && transaction) {
-		rc = ref_transaction_commit(transaction, &err);
-		if (rc) {
-			error("%s", err.buf);
-			goto abort;
-		}
-	}
-
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
 		      " 'git remote prune %s' to remove any old, conflicting "
@@ -1258,7 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
  abort:
 	strbuf_release(&note);
 	strbuf_release(&err);
-	ref_transaction_free(transaction);
 	free(url);
 	return rc;
 }
@@ -1299,6 +1304,7 @@ static int check_exist_and_connected(struct ref *ref_map)
 }
 
 static int fetch_and_consume_refs(struct transport *transport,
+				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
 				  struct fetch_head *fetch_head,
 				  struct worktree **worktrees)
@@ -1323,7 +1329,8 @@ static int fetch_and_consume_refs(struct transport *transport,
 
 	trace2_region_enter("fetch", "consume_refs", the_repository);
 	ret = store_updated_refs(transport->url, transport->remote->name,
-				 connectivity_checked, ref_map, fetch_head, worktrees);
+				 connectivity_checked, transaction, ref_map,
+				 fetch_head, worktrees);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1496,6 +1503,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
 }
 
 static int backfill_tags(struct transport *transport,
+			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
 			 struct fetch_head *fetch_head,
 			 struct worktree **worktrees)
@@ -1519,7 +1527,7 @@ static int backfill_tags(struct transport *transport,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+	retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1532,6 +1540,7 @@ static int backfill_tags(struct transport *transport,
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs)
 {
+	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
 	int autotags = (transport->remote->fetch_tags == 1);
 	int retcode = 0;
@@ -1541,6 +1550,7 @@ static int do_fetch(struct transport *transport,
 	int must_list_refs = 1;
 	struct worktree **worktrees = get_worktrees();
 	struct fetch_head fetch_head = { 0 };
+	struct strbuf err = STRBUF_INIT;
 
 	if (tags == TAGS_DEFAULT) {
 		if (transport->remote->fetch_tags == 2)
@@ -1602,6 +1612,14 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
+	if (atomic_fetch) {
+		transaction = ref_transaction_begin(&err);
+		if (!transaction) {
+			retcode = error("%s", err.buf);
+			goto cleanup;
+		}
+	}
+
 	if (tags == TAGS_DEFAULT && autotags)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 	if (prune) {
@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
+	if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1633,21 +1651,37 @@ static int do_fetch(struct transport *transport,
 	if (tags == TAGS_DEFAULT && autotags) {
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
-		find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
 			 * the user so, but we have to continue regardless to
 			 * populate upstream information of the references we
-			 * have already fetched above.
+			 * have already fetched above. The exception though is
+			 * when `--atomic` is passed: in that case we'll abort
+			 * the transaction and don't commit anything.
 			 */
-			if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+			if (backfill_tags(transport, transaction, tags_ref_map,
+					  &fetch_head, worktrees))
 				retcode = 1;
 		}
 
 		free_refs(tags_ref_map);
 	}
 
+	if (transaction) {
+		if (retcode)
+			goto cleanup;
+
+		retcode = ref_transaction_commit(transaction, &err);
+		if (retcode) {
+			error("%s", err.buf);
+			ref_transaction_free(transaction);
+			transaction = NULL;
+			goto cleanup;
+		}
+	}
+
 	commit_fetch_head(&fetch_head);
 
 	if (set_upstream) {
@@ -1705,7 +1739,13 @@ static int do_fetch(struct transport *transport,
 	}
 
 cleanup:
+	if (retcode && transaction) {
+		ref_transaction_abort(transaction, &err);
+		error("%s", err.buf);
+	}
+
 	close_fetch_head(&fetch_head);
+	strbuf_release(&err);
 	free_refs(ref_map);
 	free_worktrees(worktrees);
 	return retcode;
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 27a1dfa3c0..fe23cad4ce 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -180,11 +180,8 @@ test_expect_success 'atomic fetch with failing backfill' '
 	EOF
 
 	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
-
-	# Creation of the tag has failed, so ideally refs/heads/something
-	# should not exist. The fact that it does demonstrates that there is
-	# a bug in the `--atomic` flag.
-	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+	test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
+	test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
 '
 
 test_expect_success 'atomic fetch with backfill should use single transaction' '
@@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
 		prepared
 		$ZERO_OID $B refs/heads/something
 		$ZERO_OID $S refs/tags/tag2
+		$ZERO_OID $T refs/tags/tag1
 		committed
 		$ZERO_OID $B refs/heads/something
 		$ZERO_OID $S refs/tags/tag2
-		prepared
-		$ZERO_OID $T refs/tags/tag1
-		committed
 		$ZERO_OID $T refs/tags/tag1
 	EOF
 
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 7/7] fetch: make `--atomic` flag cover pruning of refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5171 bytes --]

When fetching with the `--prune` flag we will delete any local
references matching the fetch refspec which have disappeared on the
remote. This step is not currently covered by the `--atomic` flag: we
delete branches even though updating of local references has failed,
which means that the fetch is not an all-or-nothing operation.

Fix this bug by passing in the global transaction into `prune_refs()`:
if one is given, then we'll only queue up deletions and not commit them
right away.

This change also improves performance when pruning many branches in a
repository with a big packed-refs file: every references is pruned in
its own transaction, which means that we potentially have to rewrite
the packed-refs files for every single reference we're about to prune.

The following benchmark demonstrates this: it performs a pruning fetch
from a repository with a single reference into a repository with 100k
references, which causes us to prune all but one reference. This is of
course a very artificial setup, but serves to demonstrate the impact of
only having to write the packed-refs file once:

    Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)
      Time (mean ± σ):      2.366 s ±  0.021 s    [User: 0.858 s, System: 1.508 s]
      Range (min … max):    2.328 s …  2.407 s    10 runs

    Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)
      Time (mean ± σ):      1.369 s ±  0.017 s    [User: 0.715 s, System: 0.641 s]
      Range (min … max):    1.346 s …  1.400 s    10 runs

    Summary
      'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran
        1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c  | 30 ++++++++++++++++++++++--------
 t/t5510-fetch.sh |  8 ++------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67af842091..9a2b5c03a4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1338,11 +1338,14 @@ static int fetch_and_consume_refs(struct transport *transport,
 	return ret;
 }
 
-static int prune_refs(struct refspec *rs, struct ref *ref_map,
+static int prune_refs(struct refspec *rs,
+		      struct ref_transaction *transaction,
+		      struct ref *ref_map,
 		      const char *raw_url)
 {
 	int url_len, i, result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
+	struct strbuf err = STRBUF_INIT;
 	char *url;
 	int summary_width = transport_summary_width(stale_refs);
 	const char *dangling_msg = dry_run
@@ -1363,13 +1366,22 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 		url_len = i - 3;
 
 	if (!dry_run) {
-		struct string_list refnames = STRING_LIST_INIT_NODUP;
+		if (transaction) {
+			for (ref = stale_refs; ref; ref = ref->next) {
+				result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+								"fetch: prune", &err);
+				if (result)
+					goto cleanup;
+			}
+		} else {
+			struct string_list refnames = STRING_LIST_INIT_NODUP;
 
-		for (ref = stale_refs; ref; ref = ref->next)
-			string_list_append(&refnames, ref->name);
+			for (ref = stale_refs; ref; ref = ref->next)
+				string_list_append(&refnames, ref->name);
 
-		result = delete_refs("fetch: prune", &refnames, 0);
-		string_list_clear(&refnames, 0);
+			result = delete_refs("fetch: prune", &refnames, 0);
+			string_list_clear(&refnames, 0);
+		}
 	}
 
 	if (verbosity >= 0) {
@@ -1388,6 +1400,8 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 		}
 	}
 
+cleanup:
+	strbuf_release(&err);
 	free(url);
 	free_refs(stale_refs);
 	return result;
@@ -1629,10 +1643,10 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (rs->nr) {
-			retcode = prune_refs(rs, ref_map, transport->url);
+			retcode = prune_refs(rs, transaction, ref_map, transport->url);
 		} else {
 			retcode = prune_refs(&transport->remote->fetch,
-					     ref_map,
+					     transaction, ref_map,
 					     transport->url);
 		}
 		if (retcode != 0)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 70d51f343b..48e14e2dab 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact
 	head_oid=$(git rev-parse HEAD) &&
 
 	# Fetching with the `--atomic` flag should update all references in a
-	# single transaction. It is currently missing coverage of pruned
-	# references though, and as a result those may be committed to disk
-	# even if updating references fails later.
+	# single transaction.
 	cat >expected <<-EOF &&
 		prepared
 		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
-		committed
-		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
-		prepared
 		$ZERO_OID $head_oid refs/remotes/origin/new-branch
 		committed
+		$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
 		$ZERO_OID $head_oid refs/remotes/origin/new-branch
 	EOF
 
-- 
2.35.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +test_expect_success 'atomic fetch with failing backfill' '
> +	git init clone3 &&
> +
> +	# We want to test whether a failure when backfilling tags correctly
> +	# aborts the complete transaction when `--atomic` is passed: we should
> +	# neither create the branch nor should we create the tag when either
> +	# one of both fails to update correctly.
> +	#
> +	# To trigger failure we simply abort when backfilling a tag.
> +	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> +		while read oldrev newrev reference
> +		do
> +			if test "$reference" = refs/tags/tag1
> +			then
> +				exit 1
> +			fi
> +		done
> +	EOF

Without the extra indentation level, all your <<here-doc would
become easier to read.  You are consistently doing this in your
patches, which it is better than random mixes of indentation style,
though.

> +	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> +
> +	# Creation of the tag has failed, so ideally refs/heads/something
> +	# should not exist. The fact that it does demonstrates that there is
> +	# a bug in the `--atomic` flag.
> +	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> +'

Makes sense.

> +test_expect_success 'atomic fetch with backfill should use single transaction' '
> +	git init clone4 &&
> +
> +	# Fetching with the `--atomic` flag should update all references in a
> +	# single transaction, including backfilled tags. We thus expect to see
> +	# a single reference transaction for the created branch and tags.
> +	cat >expected <<-EOF &&
> +		prepared
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		committed
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		prepared
> +		$ZERO_OID $T refs/tags/tag1
> +		committed
> +		$ZERO_OID $T refs/tags/tag1
> +	EOF

I think we see two transactions here, even though the comment says
otherwise.  Is this expecting a known breakage?

> +	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> +		( echo "$*" && cat ) >>actual
> +	EOF
> +
> +	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> +	test_cmp expected clone4/actual

Nice way to make sure what is and is not in each transaction.  I
feels a bit too rigid (e.g. in the first transaction, there is no
inherent reason to expect that the update to head/something is
mentioned before the update to tags/tag2, for example).

> +'
> +
> +test_expect_success 'backfill failure causes command to fail' '
> +	git init clone5 &&
> +
> +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> +		while read oldrev newrev reference
> +		do
> +			if test "\$reference" = refs/tags/tag1
> +			then
> +				# Create a nested tag below the actual tag we
> +				# wanted to write, which causes a D/F conflict
> +				# later when we want to commit refs/tags/tag1.
> +				# We cannot just `exit 1` here given that this
> +				# would cause us to die immediately.

> +				git update-ref refs/tags/tag1/nested $B

I have been wondering if we need to do this from the hook?  If we
have this ref before we start "fetch", would it have the same
effect, or "fetch" notices that this interfering ref exists and
removes it to make room for storing refs/tags/tag1, making the whole
thing fail to fail?

> +				exit \$!

In any case, "exit 0" or "exit \$?" would be understandable, but
exit with "$!", which is ...?  The process ID of the most recent
background command?  Puzzled.

> +			fi
> +		done
> +	EOF

Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +test_expect_success 'backfill failure causes command to fail' '
> +	git init clone5 &&
> +
> +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> +		while read oldrev newrev reference
> +		do
> +			if test "\$reference" = refs/tags/tag1
> +			then
> +				# Create a nested tag below the actual tag we
> +				# wanted to write, which causes a D/F conflict
> +				# later when we want to commit refs/tags/tag1.
> +				# We cannot just `exit 1` here given that this
> +				# would cause us to die immediately.
> +				git update-ref refs/tags/tag1/nested $B
> +				exit \$!
> +			fi
> +		done
> +	EOF

I think I've reviewed the previous round of these patches in
detail.  I by mistake sent a comment for this step in v2, but I
think the same puzzlement exists in this round, too.

Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> +test_expect_success 'backfill failure causes command to fail' '
>> +	git init clone5 &&
>> +
>> +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
>> +		while read oldrev newrev reference
>> +		do
>> +			if test "\$reference" = refs/tags/tag1
>> +			then
>> +				# Create a nested tag below the actual tag we
>> +				# wanted to write, which causes a D/F conflict
>> +				# later when we want to commit refs/tags/tag1.
>> +				# We cannot just `exit 1` here given that this
>> +				# would cause us to die immediately.
>> +				git update-ref refs/tags/tag1/nested $B
>> +				exit \$!
>> +			fi
>> +		done
>> +	EOF
>
> I think I've reviewed the previous round of these patches in
> detail.  I by mistake sent a comment for this step in v2, but I
> think the same puzzlement exists in this round, too.

Namely:

I have been wondering if we need to do this from the hook?  If we
have this ref before we start "fetch", would it have the same
effect, or "fetch" notices that this interfering ref exists and
removes it to make room for storing refs/tags/tag1, making the whole
thing fail to fail?

> +				exit \$!

In any case, "exit 0" or "exit \$?" would be understandable, but
exit with "$!", which is ...?  The process ID of the most recent
background command?  Puzzled.

Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On Wed, Mar 02, 2022 at 04:32:48PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> +test_expect_success 'backfill failure causes command to fail' '
> >> +	git init clone5 &&
> >> +
> >> +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> >> +		while read oldrev newrev reference
> >> +		do
> >> +			if test "\$reference" = refs/tags/tag1
> >> +			then
> >> +				# Create a nested tag below the actual tag we
> >> +				# wanted to write, which causes a D/F conflict
> >> +				# later when we want to commit refs/tags/tag1.
> >> +				# We cannot just `exit 1` here given that this
> >> +				# would cause us to die immediately.
> >> +				git update-ref refs/tags/tag1/nested $B
> >> +				exit \$!
> >> +			fi
> >> +		done
> >> +	EOF
> >
> > I think I've reviewed the previous round of these patches in
> > detail.  I by mistake sent a comment for this step in v2, but I
> > think the same puzzlement exists in this round, too.
> 
> Namely:
> 
> I have been wondering if we need to do this from the hook?  If we
> have this ref before we start "fetch", would it have the same
> effect, or "fetch" notices that this interfering ref exists and
> removes it to make room for storing refs/tags/tag1, making the whole
> thing fail to fail?
> 
> > +				exit \$!
> 
> In any case, "exit 0" or "exit \$?" would be understandable, but
> exit with "$!", which is ...?  The process ID of the most recent
> background command?  Puzzled.

Oof, this was supposed to be `exit \$?`, thanks for catching this. But
your above comment is right: we can indeed just create the D/F conflict
outside of the hook and thus avoid the hook script altogether. Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]

On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +test_expect_success 'atomic fetch with failing backfill' '
> > +	git init clone3 &&
> > +
> > +	# We want to test whether a failure when backfilling tags correctly
> > +	# aborts the complete transaction when `--atomic` is passed: we should
> > +	# neither create the branch nor should we create the tag when either
> > +	# one of both fails to update correctly.
> > +	#
> > +	# To trigger failure we simply abort when backfilling a tag.
> > +	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> > +		while read oldrev newrev reference
> > +		do
> > +			if test "$reference" = refs/tags/tag1
> > +			then
> > +				exit 1
> > +			fi
> > +		done
> > +	EOF
> 
> Without the extra indentation level, all your <<here-doc would
> become easier to read.  You are consistently doing this in your
> patches, which it is better than random mixes of indentation style,
> though.

Personally I think it's easier to read this way, but grepping through
the codebase shows that what you're proposing is used consistently.
I'll change it.

> > +	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> > +
> > +	# Creation of the tag has failed, so ideally refs/heads/something
> > +	# should not exist. The fact that it does demonstrates that there is
> > +	# a bug in the `--atomic` flag.
> > +	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> > +'
> 
> Makes sense.
> 
> > +test_expect_success 'atomic fetch with backfill should use single transaction' '
> > +	git init clone4 &&
> > +
> > +	# Fetching with the `--atomic` flag should update all references in a
> > +	# single transaction, including backfilled tags. We thus expect to see
> > +	# a single reference transaction for the created branch and tags.
> > +	cat >expected <<-EOF &&
> > +		prepared
> > +		$ZERO_OID $B refs/heads/something
> > +		$ZERO_OID $S refs/tags/tag2
> > +		committed
> > +		$ZERO_OID $B refs/heads/something
> > +		$ZERO_OID $S refs/tags/tag2
> > +		prepared
> > +		$ZERO_OID $T refs/tags/tag1
> > +		committed
> > +		$ZERO_OID $T refs/tags/tag1
> > +	EOF
> 
> I think we see two transactions here, even though the comment says
> otherwise.  Is this expecting a known breakage?

Yes, it is. I've added a comment in this patch to document the breakage,
which is then removed when the bug is fixed.

> > +	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> > +		( echo "$*" && cat ) >>actual
> > +	EOF
> > +
> > +	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> > +	test_cmp expected clone4/actual
> 
> Nice way to make sure what is and is not in each transaction.  I
> feels a bit too rigid (e.g. in the first transaction, there is no
> inherent reason to expect that the update to head/something is
> mentioned before the update to tags/tag2, for example).
> 
> > +'
> > +
> > +test_expect_success 'backfill failure causes command to fail' '
> > +	git init clone5 &&
> > +
> > +	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> > +		while read oldrev newrev reference
> > +		do
> > +			if test "\$reference" = refs/tags/tag1
> > +			then
> > +				# Create a nested tag below the actual tag we
> > +				# wanted to write, which causes a D/F conflict
> > +				# later when we want to commit refs/tags/tag1.
> > +				# We cannot just `exit 1` here given that this
> > +				# would cause us to die immediately.
> 
> > +				git update-ref refs/tags/tag1/nested $B
> 
> I have been wondering if we need to do this from the hook?  If we
> have this ref before we start "fetch", would it have the same
> effect, or "fetch" notices that this interfering ref exists and
> removes it to make room for storing refs/tags/tag1, making the whole
> thing fail to fail?

No, it indeed is not, thanks!

Patrick

> > +				exit \$!
> 
> In any case, "exit 0" or "exit \$?" would be understandable, but
> exit with "$!", which is ...?  The process ID of the most recent
> background command?  Puzzled.
> 
> > +			fi
> > +		done
> > +	EOF

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> >> +				# would cause us to die immediately.
>> >> +				git update-ref refs/tags/tag1/nested $B
>> >> +				exit \$!
>> >> +			fi
>> >> +		done
>> >> +	EOF
>> >
>> > I think I've reviewed the previous round of these patches in
>> > detail.  I by mistake sent a comment for this step in v2, but I
>> > think the same puzzlement exists in this round, too.
>> 
>> Namely:
>> 
>> I have been wondering if we need to do this from the hook?  If we
>> have this ref before we start "fetch", would it have the same
>> effect, or "fetch" notices that this interfering ref exists and
>> removes it to make room for storing refs/tags/tag1, making the whole
>> thing fail to fail?
>> 
>> > +				exit \$!
>> 
>> In any case, "exit 0" or "exit \$?" would be understandable, but
>> exit with "$!", which is ...?  The process ID of the most recent
>> background command?  Puzzled.
>
> Oof, this was supposed to be `exit \$?`, thanks for catching this. But
> your above comment is right: we can indeed just create the D/F conflict
> outside of the hook and thus avoid the hook script altogether. Thanks!

I see.

As that shell does not send anything to background, at the point of
the reference $! would yield an empty string, and "exit" is
equivalent to "exit $?", it is doing the right thing, I presume.

The topic has been in 'next' for a while, so if you are inclined to
fix it up, please send an incremental patch.  If you do "exit" it
would be a one-liner change, or if you use a different "cause D/F
conflict outside the hook" approach, the change may become a bit
more involved.

Thanks.

Re: [PATCH v3 1/7] fetch: increase test coverage of fetches

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On Wed, Mar 02, 2022 at 10:49:05PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> >> +				# would cause us to die immediately.
> >> >> +				git update-ref refs/tags/tag1/nested $B
> >> >> +				exit \$!
> >> >> +			fi
> >> >> +		done
> >> >> +	EOF
> >> >
> >> > I think I've reviewed the previous round of these patches in
> >> > detail.  I by mistake sent a comment for this step in v2, but I
> >> > think the same puzzlement exists in this round, too.
> >> 
> >> Namely:
> >> 
> >> I have been wondering if we need to do this from the hook?  If we
> >> have this ref before we start "fetch", would it have the same
> >> effect, or "fetch" notices that this interfering ref exists and
> >> removes it to make room for storing refs/tags/tag1, making the whole
> >> thing fail to fail?
> >> 
> >> > +				exit \$!
> >> 
> >> In any case, "exit 0" or "exit \$?" would be understandable, but
> >> exit with "$!", which is ...?  The process ID of the most recent
> >> background command?  Puzzled.
> >
> > Oof, this was supposed to be `exit \$?`, thanks for catching this. But
> > your above comment is right: we can indeed just create the D/F conflict
> > outside of the hook and thus avoid the hook script altogether. Thanks!
> 
> I see.
> 
> As that shell does not send anything to background, at the point of
> the reference $! would yield an empty string, and "exit" is
> equivalent to "exit $?", it is doing the right thing, I presume.
> 
> The topic has been in 'next' for a while, so if you are inclined to
> fix it up, please send an incremental patch.  If you do "exit" it
> would be a one-liner change, or if you use a different "cause D/F
> conflict outside the hook" approach, the change may become a bit
> more involved.
> 
> Thanks.

Fair enough, thanks for clarifying the steps. Does it make sense to also
change indentantion of the heredocs as per your review of v2 or should I
just keep that as-is now?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]