fetch: improve atomicity of `--atomic` flag

PATCH Superseded Patrick Steinhardt 3 participants 35 messages Feb 17, 2022 – Mar 3, 2022

lore

Share & tools

lore

Patrick Steinhardt

author

2022-02-17 13:04 UTC #0 lore raw

v2 cover fetch: improve atomicity of `--atomic` flag

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 <[email protected]>

@@ 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 <[email protected]>

@@ 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 <[email protected]>

## 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

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-17 13:04 UTC #1 lore raw

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

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 <[email protected]>

---
 t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh     | 33 ++++++++++++++++++
 2 files changed, 114 insertions(+)

t/t5503-tagfollow.sh +81 -0

index 195fc64dd4..6ffe2a5719 100755
@@ -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

t/t5510-fetch.sh +33 -0

index ef0da0a63b..70d51f343b 100755
@@ -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 &&

Signature · 1 line

2.35.1

Christian Couder

2022-02-17 15:18 UTC #2 lore raw

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

Quoted 6 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +6 -0

+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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 178

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+       # 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.

Review

Quoted 2 lines

(quoted diff) +2 -0
+               done
+       EOF

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

Patrick Steinhardt

author

2022-02-21 07:57 UTC #3 lore raw

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

Quoted 21 lines · Christian Couder

Quoted 6 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +6 -0

+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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 178

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+       # 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:

Quoted 4 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

Junio C Hamano

2022-02-17 20:41 UTC #4 lore raw

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

In reply to Patrick Steinhardt

Quoted 7 lines · Patrick Steinhardt

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.

Quoted 8 lines

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.

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 171

Quoted 9 lines

t/t5503-tagfollow.sh +9 -0

+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?

Quoted 30 lines

t/t5503-tagfollow.sh +30 -0

+	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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 211

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+		$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.

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 220

Quoted 9 lines

t/t5503-tagfollow.sh +9 -0

+
+	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?

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 230

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+		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 ;-)

Quoted 14 lines

t/t5503-tagfollow.sh +13 -0

+				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

Junio C Hamano

2022-02-17 22:43 UTC #5 lore raw

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

Quoted 2 lines · Junio C Hamano

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

Yes. We refer to "$B".

Patrick Steinhardt

author

2022-02-18 06:50 UTC #6 lore raw

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

In reply to Junio C Hamano

Quoted 1 line · Junio C Hamano

Patrick Steinhardt <[email protected]> writes:

[snip]

Quoted 10 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 171

Quoted 9 lines

t/t5503-tagfollow.sh +9 -0

+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.

Quoted 7 lines

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

Junio C Hamano

2022-02-18 16:59 UTC #7 lore raw

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

Quoted 21 lines · Patrick Steinhardt

Quoted 1 line · Junio C Hamano
Patrick Steinhardt <[email protected]> writes:
[snip]
Quoted 10 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 171
Quoted 9 lines
t/t5503-tagfollow.sh +9 -0
+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.
Quoted 7 lines
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?

Quoted 1 line

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.

Junio C Hamano

2022-03-03 00:25 UTC #8 lore raw

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

In reply to Patrick Steinhardt

Quoted 8 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +8 -0

+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.
+	#

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 180

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+	# 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.

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 188

Quoted 7 lines

t/t5503-tagfollow.sh +7 -0

+	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.

Quoted 8 lines

t/t5503-tagfollow.sh +8 -0

+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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 207

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+		$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?

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 214

Quoted 6 lines

t/t5503-tagfollow.sh +6 -0

+	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).

Quoted 7 lines

t/t5503-tagfollow.sh +6 -0

+
+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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 230

Quoted 10 lines

t/t5503-tagfollow.sh +9 -0

+		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?

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231

Quoted 1 line

t/t5503-tagfollow.sh +1 -0
+				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.

Quoted 3 lines

t/t5503-tagfollow.sh +3 -0
+			fi
+		done
+	EOF

Patrick Steinhardt

author

2022-03-03 06:47 UTC #9 lore raw

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

Quoted 17 lines · Junio C Hamano

Quoted 15 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +15 -0

+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

Quoted 8 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 180
Quoted 3 lines
t/t5503-tagfollow.sh +3 -0
+			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.

Quoted 23 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 188

Quoted 7 lines

t/t5503-tagfollow.sh +7 -0

+	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.

Quoted 13 lines

t/t5503-tagfollow.sh +13 -0

+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

Quoted 8 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 207
Quoted 5 lines
t/t5503-tagfollow.sh +5 -0
+		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.

Quoted 28 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 214
Quoted 6 lines
t/t5503-tagfollow.sh +6 -0
+	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).
Quoted 15 lines
+'
t/t5503-tagfollow.sh +14 -0
+
+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.

Quoted 7 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 230
Quoted 1 line
t/t5503-tagfollow.sh +1 -0
+				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

Quoted 9 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231
Quoted 1 line
t/t5503-tagfollow.sh +1 -0
+				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.
Quoted 3 lines
t/t5503-tagfollow.sh +3 -0
+			fi
+		done
+	EOF

Patrick Steinhardt

author

2022-02-17 13:04 UTC #10 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

builtin/fetch.c +18 ~~-17~~

index 6f5e157863..904ca9f1ca 100644
@@ -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;
 }

Signature · 1 line

2.35.1

Junio C Hamano

2022-02-17 22:07 UTC #11 lore raw

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

Quoted 13 lines · Patrick Steinhardt

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.

Quoted 4 lines

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.

builtin/fetch.c static void backfill_tags(struct transport *transport, struct ref *ref_map, · line 1539

Quoted 5 lines

static int do_fetch(struct transport *transport,
builtin/fetch.c +1 -1
 		    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.

builtin/fetch.c static int do_fetch(struct transport *transport, · line 1624

Quoted 7 lines

builtin/fetch.c +0 -1

@@ -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.

builtin/fetch.c static int do_fetch(struct transport *transport, · line 1639

Quoted 13 lines

builtin/fetch.c +13 -0

+	/*
+	 * 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.

Patrick Steinhardt

author

2022-02-17 13:04 UTC #12 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

builtin/fetch.c +19 ~~-16~~

index 904ca9f1ca..f8adb40b45 100644
@@ -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;

Signature · 1 line

2.35.1

Junio C Hamano

2022-02-17 22:12 UTC #13 lore raw

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

Quoted 8 lines · Patrick Steinhardt

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.

Patrick Steinhardt

author

2022-02-17 13:04 UTC #14 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c      | 26 ++++++++++++++++++--------
 t/t5503-tagfollow.sh |  4 +---
 2 files changed, 19 insertions(+), 11 deletions(-)

builtin/fetch.c +18 -8

index f8adb40b45..d304314f16 100644
@@ -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);
 	}

t/t5503-tagfollow.sh +1 -3

index 6ffe2a5719..c057c49e80 100755
@@ -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

Signature · 1 line

2.35.1

Junio C Hamano

2022-02-17 22:16 UTC #15 lore raw

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

Quoted 3 lines · Patrick Steinhardt

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

The change to this function is quite straight-forward.

Quoted 4 lines

static int do_fetch(struct transport *transport,

builtin/fetch.c +0 -0

@@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;

builtin/fetch.c static int do_fetch(struct transport *transport, · line 1643

Quoted 10 lines

builtin/fetch.c +7 -2

 		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.

Quoted 11 lines

builtin/fetch.c +3 -0

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

t/t5503-tagfollow.sh +0 -0

index 6ffe2a5719..c057c49e80 100755

t/t5503-tagfollow.sh test_expect_success 'backfill failure causes command to fail' ' · line 236

Quoted 7 lines

t/t5503-tagfollow.sh +1 -3

@@ -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.

Quoted 3 lines

t/t5503-tagfollow.sh +0 -0

 	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

Patrick Steinhardt

author

2022-02-17 13:04 UTC #16 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 refs.c | 16 ++++++++++++++++
 refs.h | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

refs.c +16 -0

index d680de3bc0..35d4e69687 100644
@@ -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)
 {

refs.h +14 -0

index ff859d5951..1ae12c410a 100644
@@ -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.
  */

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-17 13:04 UTC #17 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c      | 92 +++++++++++++++++++++++++++++++-------------
 t/t5503-tagfollow.sh | 11 ++----
 2 files changed, 69 insertions(+), 34 deletions(-)

builtin/fetch.c +66 ~~-26~~

index d304314f16..67af842091 100644
@@ -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;

t/t5503-tagfollow.sh +3 -8

index c057c49e80..e72fdc2534 100755
@@ -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

Signature · 1 line

2.35.1

Junio C Hamano

2022-02-17 22:27 UTC #18 lore raw

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

Quoted 4 lines · Patrick Steinhardt

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.

t/t5503-tagfollow.sh test_expect_success 'atomic fetch with backfill should use single transaction' ' · line 202

Quoted 13 lines

t/t5503-tagfollow.sh +1 -3

@@ -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.

Patrick Steinhardt

author

2022-02-17 13:04 UTC #19 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c  | 30 ++++++++++++++++++++++--------
 t/t5510-fetch.sh |  8 ++------
 2 files changed, 24 insertions(+), 14 deletions(-)

builtin/fetch.c +22 -8

index 67af842091..9a2b5c03a4 100644
@@ -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)

t/t5510-fetch.sh +2 -6

index 70d51f343b..48e14e2dab 100755
@@ -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

Signature · 1 line

2.35.1

Christian Couder

2022-02-17 15:51 UTC #20 lore raw

In reply to Patrick Steinhardt

Quoted 2 lines · Patrick Steinhardt

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!

Junio C Hamano

2022-02-17 22:30 UTC #21 lore raw

Quoted 7 lines · Christian Couder

Quoted 2 lines · Patrick Steinhardt
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.

Patrick Steinhardt

author

2022-02-21 08:02 UTC #22 lore raw

v3 cover fetch: improve atomicity of `--atomic` flag

In reply to Patrick Steinhardt

Hi,

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

 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

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:02 UTC #23 lore raw

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

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh     | 33 ++++++++++++++++++
 2 files changed, 114 insertions(+)

t/t5503-tagfollow.sh +81 -0

index 195fc64dd4..7dadaafd4b 100755
@@ -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

t/t5510-fetch.sh +33 -0

index ef0da0a63b..70d51f343b 100755
@@ -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 &&

Signature · 1 line

2.35.1

Junio C Hamano

2022-03-03 00:31 UTC #24 lore raw

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

Quoted 8 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +8 -0

+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

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 234

Quoted 10 lines

t/t5503-tagfollow.sh +10 -0

+				# 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.

Junio C Hamano

2022-03-03 00:32 UTC #25 lore raw

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

Quoted 16 lines · Junio C Hamano

Quoted 14 lines · Patrick Steinhardt

t/t5503-tagfollow.sh +14 -0

+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

Quoted 8 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 234
Quoted 4 lines
t/t5503-tagfollow.sh +4 -0
+				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:

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231

Quoted 1 line

t/t5503-tagfollow.sh +1 -0
+				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.

Patrick Steinhardt

author

2022-03-03 06:43 UTC #26 lore raw

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

Quoted 35 lines · Junio C Hamano

Quoted 16 lines · Junio C Hamano
Quoted 14 lines · Patrick Steinhardt
t/t5503-tagfollow.sh +14 -0
+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
Quoted 8 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 234
Quoted 4 lines
t/t5503-tagfollow.sh +4 -0
+				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?

Quoted 5 lines

t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231
Quoted 1 line
t/t5503-tagfollow.sh +1 -0
+				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

Junio C Hamano

2022-03-03 06:49 UTC #27 lore raw

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

Quoted 25 lines · Patrick Steinhardt

Quoted 19 lines
Quoted 10 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 234
Quoted 6 lines
t/t5503-tagfollow.sh +6 -0
+				# 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?
Quoted 5 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231
Quoted 1 line
t/t5503-tagfollow.sh +1 -0
+				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.

Quoted 3 lines

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.

Patrick Steinhardt

author

2022-03-03 06:51 UTC #28 lore raw

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

Quoted 37 lines · Junio C Hamano

Quoted 25 lines · Patrick Steinhardt
Quoted 19 lines
Quoted 10 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 234
Quoted 6 lines
t/t5503-tagfollow.sh +6 -0
+				# 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?
Quoted 5 lines
t/t5503-tagfollow.sh test_expect_success 'new clone fetch main and tags' ' · line 231
Quoted 1 line
t/t5503-tagfollow.sh +1 -0
+				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.
Quoted 3 lines
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.

Quoted 7 lines

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

Patrick Steinhardt

author

2022-02-21 08:02 UTC #29 lore raw

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

In reply to Patrick Steinhardt

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

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/fetch.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

builtin/fetch.c +18 ~~-17~~

index 6f5e157863..904ca9f1ca 100644
@@ -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;
 }

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:02 UTC #30 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

builtin/fetch.c +19 ~~-16~~

index 904ca9f1ca..f8adb40b45 100644
@@ -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;

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:02 UTC #31 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>

---
 builtin/fetch.c      | 26 ++++++++++++++++++--------
 t/t5503-tagfollow.sh |  4 +---
 2 files changed, 19 insertions(+), 11 deletions(-)

builtin/fetch.c +18 -8

index f8adb40b45..d304314f16 100644
@@ -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);
 	}

t/t5503-tagfollow.sh +1 -3

index 7dadaafd4b..27a1dfa3c0 100755
@@ -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

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:02 UTC #32 lore raw

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

In reply to Patrick Steinhardt

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 flag to ref_transaction_add_update() that causes it to skip duplicates, but this has the same runtime concerns as the first alternative.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 refs.c | 16 ++++++++++++++++
 refs.h | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

refs.c +16 -0

index d680de3bc0..35d4e69687 100644
@@ -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)
 {

refs.h +14 -0

index ff859d5951..1ae12c410a 100644
@@ -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.
  */

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:03 UTC #33 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 builtin/fetch.c      | 92 +++++++++++++++++++++++++++++++-------------
 t/t5503-tagfollow.sh | 11 ++----
 2 files changed, 69 insertions(+), 34 deletions(-)

builtin/fetch.c +66 ~~-26~~

index d304314f16..67af842091 100644
@@ -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;

t/t5503-tagfollow.sh +3 -8

index 27a1dfa3c0..fe23cad4ce 100755
@@ -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

Signature · 1 line

2.35.1

Patrick Steinhardt

author

2022-02-21 08:03 UTC #34 lore raw

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

In reply to Patrick Steinhardt

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.

    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 <[email protected]>

---
 builtin/fetch.c  | 30 ++++++++++++++++++++++--------
 t/t5510-fetch.sh |  8 ++------
 2 files changed, 24 insertions(+), 14 deletions(-)

builtin/fetch.c +22 -8

index 67af842091..9a2b5c03a4 100644
@@ -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)

t/t5510-fetch.sh +2 -6

index 70d51f343b..48e14e2dab 100755
@@ -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

Signature · 1 line

2.35.1