* [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 15:18 ` Christian Couder
` (2 more replies)
2022-02-17 13:04 ` [PATCH v2 2/7] fetch: backfill tags before setting upstream Patrick Steinhardt
` (7 subsequent siblings)
8 siblings, 3 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 5687 bytes --]
When using git-fetch(1) with the `--atomic` flag the expectation is that
either all of the references are updated, or alternatively none are in
case the fetch fails. While we already have tests for this, we do not
have any tests which exercise atomicity either when pruning deleted refs
or when backfilling tags. This gap in test coverage hides that we indeed
don't handle atomicity correctly for both of these cases.
Add test cases which cover these testing gaps to demonstrate the broken
behaviour. Note that tests are not marked as `test_expect_failure`: this
is done to explicitly demonstrate the current known-wrong behaviour, and
they will be fixed up as soon as we fix the underlying bugs.
While at it this commit also adds another test case which demonstrates
that backfilling of tags does not return an error code in case the
backfill fails. This bug will also be fixed by a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
t/t5510-fetch.sh | 33 ++++++++++++++++++
2 files changed, 114 insertions(+)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd4..6ffe2a5719 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -160,4 +160,85 @@ test_expect_success 'new clone fetch main and tags' '
test_cmp expect actual
'
+test_expect_success 'atomic fetch with failing backfill' '
+ git init clone3 &&
+
+ # We want to test whether a failure when backfilling tags correctly
+ # aborts the complete transaction when `--atomic` is passed: we should
+ # neither create the branch nor should we create the tag when either
+ # one of both fails to update correctly.
+ #
+ # To trigger failure we simply abort when backfilling a tag.
+ write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
+ while read oldrev newrev reference
+ do
+ if test "$reference" = refs/tags/tag1
+ then
+ exit 1
+ fi
+ done
+ EOF
+
+ test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
+
+ # Creation of the tag has failed, so ideally refs/heads/something
+ # should not exist. The fact that it does demonstrates that there is
+ # a bug in the `--atomic` flag.
+ test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+'
+
+test_expect_success 'atomic fetch with backfill should use single transaction' '
+ git init clone4 &&
+
+ # Fetching with the `--atomic` flag should update all references in a
+ # single transaction, including backfilled tags. We thus expect to see
+ # a single reference transaction for the created branch and tags.
+ cat >expected <<-EOF &&
+ prepared
+ $ZERO_OID $B refs/heads/something
+ $ZERO_OID $S refs/tags/tag2
+ committed
+ $ZERO_OID $B refs/heads/something
+ $ZERO_OID $S refs/tags/tag2
+ prepared
+ $ZERO_OID $T refs/tags/tag1
+ committed
+ $ZERO_OID $T refs/tags/tag1
+ EOF
+
+ write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
+ ( echo "$*" && cat ) >>actual
+ EOF
+
+ git -C clone4 fetch --atomic .. $B:refs/heads/something &&
+ test_cmp expected clone4/actual
+'
+
+test_expect_success 'backfill failure causes command to fail' '
+ git init clone5 &&
+
+ write_script clone5/.git/hooks/reference-transaction <<-EOF &&
+ while read oldrev newrev reference
+ do
+ if test "\$reference" = refs/tags/tag1
+ then
+ # Create a nested tag below the actual tag we
+ # wanted to write, which causes a D/F conflict
+ # later when we want to commit refs/tags/tag1.
+ # We cannot just `exit 1` here given that this
+ # would cause us to die immediately.
+ git update-ref refs/tags/tag1/nested $B
+ exit \$!
+ fi
+ done
+ EOF
+
+ # Even though we fail to create refs/tags/tag1 the below command
+ # unexpectedly succeeds.
+ git -C clone5 fetch .. $B:refs/heads/something &&
+ test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
+ test $S = $(git -C clone5 rev-parse --verify tag2) &&
+ test_must_fail git -C clone5 rev-parse --verify tag1
+'
+
test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ef0da0a63b..70d51f343b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -343,6 +343,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
test_cmp expected atomic/.git/FETCH_HEAD
'
+test_expect_success 'fetch --atomic --prune executes a single reference transaction only' '
+ test_when_finished "rm -rf \"$D\"/atomic" &&
+
+ cd "$D" &&
+ git branch scheduled-for-deletion &&
+ git clone . atomic &&
+ git branch -D scheduled-for-deletion &&
+ git branch new-branch &&
+ head_oid=$(git rev-parse HEAD) &&
+
+ # Fetching with the `--atomic` flag should update all references in a
+ # single transaction. It is currently missing coverage of pruned
+ # references though, and as a result those may be committed to disk
+ # even if updating references fails later.
+ cat >expected <<-EOF &&
+ prepared
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+ committed
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+ prepared
+ $ZERO_OID $head_oid refs/remotes/origin/new-branch
+ committed
+ $ZERO_OID $head_oid refs/remotes/origin/new-branch
+ EOF
+
+ write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+ ( echo "$*" && cat ) >>actual
+ EOF
+
+ git -C atomic fetch --atomic --prune origin &&
+ test_cmp expected atomic/actual
+'
+
test_expect_success '--refmap="" ignores configured refspec' '
cd "$TRASH_DIRECTORY" &&
git clone "$D" remote-refs &&
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 13:04 ` [PATCH v2 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
@ 2022-02-17 15:18 ` Christian Couder
2022-02-21 7:57 ` Patrick Steinhardt
2022-02-17 20:41 ` Junio C Hamano
2022-03-03 0:25 ` Junio C Hamano
2 siblings, 1 reply; 35+ messages in thread
From: Christian Couder @ 2022-02-17 15:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jonathan Tan, Elijah Newren
On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:
> +test_expect_success 'atomic fetch with failing backfill' '
> + git init clone3 &&
> +
> + # We want to test whether a failure when backfilling tags correctly
> + # aborts the complete transaction when `--atomic` is passed: we should
> + # neither create the branch nor should we create the tag when either
> + # one of both fails to update correctly.
> + #
> + # To trigger failure we simply abort when backfilling a tag.
> + write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> + while read oldrev newrev reference
> + do
> + if test "$reference" = refs/tags/tag1
> + then
> + exit 1
> + fi
Maybe the following could save a few lines:
test "$reference" = refs/tags/tag1 && exit 1
It would make the code look a bit different than in another hook
script written below though, so not a big deal.
> + done
> + EOF
Overall it looks good, and I like the improved commit message!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 15:18 ` Christian Couder
@ 2022-02-21 7:57 ` Patrick Steinhardt
0 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 7:57 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]
On Thu, Feb 17, 2022 at 04:18:07PM +0100, Christian Couder wrote:
> On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> > +test_expect_success 'atomic fetch with failing backfill' '
> > + git init clone3 &&
> > +
> > + # We want to test whether a failure when backfilling tags correctly
> > + # aborts the complete transaction when `--atomic` is passed: we should
> > + # neither create the branch nor should we create the tag when either
> > + # one of both fails to update correctly.
> > + #
> > + # To trigger failure we simply abort when backfilling a tag.
> > + write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> > + while read oldrev newrev reference
> > + do
> > + if test "$reference" = refs/tags/tag1
> > + then
> > + exit 1
> > + fi
>
> Maybe the following could save a few lines:
>
> test "$reference" = refs/tags/tag1 && exit 1
>
> It would make the code look a bit different than in another hook
> script written below though, so not a big deal.
If `$reference` does not match the tag we want to continue and
eventually return successfully from this hook. But:
$ while read foo; do test "$foo" = bar && echo match; done < <(echo bar)
match
$ while read foo; do test "$foo" = bar && echo match; done < <(echo foo)
$ echo $?
1
So with the proposed change we'd now exit the hook with an error code
instead of returning successfully.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 13:04 ` [PATCH v2 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
2022-02-17 15:18 ` Christian Couder
@ 2022-02-17 20:41 ` Junio C Hamano
2022-02-17 22:43 ` Junio C Hamano
2022-02-18 6:49 ` Patrick Steinhardt
2022-03-03 0:25 ` Junio C Hamano
2 siblings, 2 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 20:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> When using git-fetch(1) with the `--atomic` flag the expectation is that
> either all of the references are updated, or alternatively none are in
> case the fetch fails. While we already have tests for this, we do not
> have any tests which exercise atomicity either when pruning deleted refs
> or when backfilling tags. This gap in test coverage hides that we indeed
> don't handle atomicity correctly for both of these cases.
>
> Add test cases which cover these testing gaps to demonstrate the broken
> behaviour. Note that tests are not marked as `test_expect_failure`: this
> is done to explicitly demonstrate the current known-wrong behaviour, and
> they will be fixed up as soon as we fix the underlying bugs.
>
> While at it this commit also adds another test case which demonstrates
> that backfilling of tags does not return an error code in case the
> backfill fails. This bug will also be fixed by a subsequent commit.
No need to reorganize the series just to fix this, but in general,
adding new tests that are designed to fail in an early commit and
then give fixes in a separate commit is to be avoided for two
reasons.
One is obviously that it breaks bisectability. And another is that
it makes it unclear what the end-user visible problem was when
reading the actual fix, and makes reviewing the series harder.
If a test that demonstrates an existing bug comes with the code to
fix the bug in the same commit, it obviously will not break
bisectability, and the new test serves also as a documentation of
what the end-user visible symptoms of the bug being fixed are.
Sometimes people work around the bisectability issue by starting
these tests that demonstrates known-to-be-unfixed-yet bugs with
test_expect_failure, but that is not a good solution for the
reviewability issue, as the step that fixes the bug will show the
code fix plus only the first few lines of the test being changed
to expect success and it is not readily visible what the symptom
actually were.
A series of patches, each one of which fixes one issue and
demonstrates the issue with tests that expects success when the
issue is resolved, is much more pleasant to read than a series where
it has tests for multiple issues, followed by a patch or two to
handle each issue.
And in such a series, it is much easier to see what breaks if the
fix weren't there. Reviewers can apply one step of the patch and
tentatively revert everything outside t/ to see how the added test
breaks without the fix. It helps those who may want to cherry-pick
a particular fix, together with its associated test, if a series is
done that way.
Thanks.
> +test_expect_success 'atomic fetch with failing backfill' '
> + git init clone3 &&
> +
> + # We want to test whether a failure when backfilling tags correctly
> + # aborts the complete transaction when `--atomic` is passed: we should
> + # neither create the branch nor should we create the tag when either
> + # one of both fails to update correctly.
> + #
> + # To trigger failure we simply abort when backfilling a tag.
What does this paragraph want the phrase `backfilling tags` to mean?
Just from end-user's perspective, there is only one (i.e. if an
object that is tagged gets fetched and that tag is not here, fetch
it too), but at the mechanism level, there are two distinct code
paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
call backfill_tags()). Which failure does this talk about, or it
does not matter which code path gave us the tag?
> + write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> + while read oldrev newrev reference
> + do
> + if test "$reference" = refs/tags/tag1
> + then
> + exit 1
> + fi
> + done
> + EOF
> +
> + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> +
> + # Creation of the tag has failed, so ideally refs/heads/something
> + # should not exist. The fact that it does demonstrates that there is
> + # a bug in the `--atomic` flag.
> + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> +'
> +
> +test_expect_success 'atomic fetch with backfill should use single transaction' '
> + git init clone4 &&
> +
> + # Fetching with the `--atomic` flag should update all references in a
> + # single transaction, including backfilled tags. We thus expect to see
> + # a single reference transaction for the created branch and tags.
> + cat >expected <<-EOF &&
> + prepared
> + $ZERO_OID $B refs/heads/something
> + $ZERO_OID $S refs/tags/tag2
> + committed
> + $ZERO_OID $B refs/heads/something
> + $ZERO_OID $S refs/tags/tag2
> + prepared
> + $ZERO_OID $T refs/tags/tag1
> + committed
> + $ZERO_OID $T refs/tags/tag1
> + EOF
> +
> + write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> + ( echo "$*" && cat ) >>actual
> + EOF
Nicely done.
> +
> + git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> + test_cmp expected clone4/actual
> +'
> +
> +test_expect_success 'backfill failure causes command to fail' '
> + git init clone5 &&
> +
> + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
Is there a reason why we cannot do <<-\EOF here to lose the
backslashes in the here-text?
> + while read oldrev newrev reference
> + do
> + if test "\$reference" = refs/tags/tag1
> + then
> + # Create a nested tag below the actual tag we
> + # wanted to write, which causes a D/F conflict
> + # later when we want to commit refs/tags/tag1.
> + # We cannot just `exit 1` here given that this
> + # would cause us to die immediately.
> + git update-ref refs/tags/tag1/nested $B
Wow, nasty ;-)
> + exit \$!
> + fi
> + done
> + EOF
> +
> + # Even though we fail to create refs/tags/tag1 the below command
> + # unexpectedly succeeds.
> + git -C clone5 fetch .. $B:refs/heads/something &&
> + test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
> + test $S = $(git -C clone5 rev-parse --verify tag2) &&
> + test_must_fail git -C clone5 rev-parse --verify tag1
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 20:41 ` Junio C Hamano
@ 2022-02-17 22:43 ` Junio C Hamano
2022-02-18 6:49 ` Patrick Steinhardt
1 sibling, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 22:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Junio C Hamano <gitster@pobox.com> writes:
> Is there a reason why we cannot do <<-\EOF here to lose the
> backslashes in the here-text?
Yes. We refer to "$B".
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 20:41 ` Junio C Hamano
2022-02-17 22:43 ` Junio C Hamano
@ 2022-02-18 6:49 ` Patrick Steinhardt
2022-02-18 16:59 ` Junio C Hamano
1 sibling, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-18 6:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > +test_expect_success 'atomic fetch with failing backfill' '
> > + git init clone3 &&
> > +
> > + # We want to test whether a failure when backfilling tags correctly
> > + # aborts the complete transaction when `--atomic` is passed: we should
> > + # neither create the branch nor should we create the tag when either
> > + # one of both fails to update correctly.
> > + #
> > + # To trigger failure we simply abort when backfilling a tag.
>
> What does this paragraph want the phrase `backfilling tags` to mean?
> Just from end-user's perspective, there is only one (i.e. if an
> object that is tagged gets fetched and that tag is not here, fetch
> it too), but at the mechanism level, there are two distinct code
> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
> call backfill_tags()). Which failure does this talk about, or it
> does not matter which code path gave us the tag?
It refers to `backfill_tags()`. Should I update this comment to clarify?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-18 6:49 ` Patrick Steinhardt
@ 2022-02-18 16:59 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-18 16:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
> [snip]
>> > +test_expect_success 'atomic fetch with failing backfill' '
>> > + git init clone3 &&
>> > +
>> > + # We want to test whether a failure when backfilling tags correctly
>> > + # aborts the complete transaction when `--atomic` is passed: we should
>> > + # neither create the branch nor should we create the tag when either
>> > + # one of both fails to update correctly.
>> > + #
>> > + # To trigger failure we simply abort when backfilling a tag.
>>
>> What does this paragraph want the phrase `backfilling tags` to mean?
>> Just from end-user's perspective, there is only one (i.e. if an
>> object that is tagged gets fetched and that tag is not here, fetch
>> it too), but at the mechanism level, there are two distinct code
>> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
>> call backfill_tags()). Which failure does this talk about, or it
>> does not matter which code path gave us the tag?
>
> It refers to `backfill_tags()`. Should I update this comment to clarify?
After reading the patch, the issue is only about the case where we
perform the second fetch and the case where OPT_FOLLOWTAGS does
everything necessary is not relevant. So hinting that we are
interested to see what a failure in the follow-on fetch does to the
atomicity would be a good idea, and mentioning backfill_tags() would
have been a good way to do so. Perhaps like "whether a failure in
backfill_tags() correctly aborts..." or something?
Thanks.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-02-17 13:04 ` [PATCH v2 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
2022-02-17 15:18 ` Christian Couder
2022-02-17 20:41 ` Junio C Hamano
@ 2022-03-03 0:25 ` Junio C Hamano
2022-03-03 6:47 ` Patrick Steinhardt
2 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2022-03-03 0:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> +test_expect_success 'atomic fetch with failing backfill' '
> + git init clone3 &&
> +
> + # We want to test whether a failure when backfilling tags correctly
> + # aborts the complete transaction when `--atomic` is passed: we should
> + # neither create the branch nor should we create the tag when either
> + # one of both fails to update correctly.
> + #
> + # To trigger failure we simply abort when backfilling a tag.
> + write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> + while read oldrev newrev reference
> + do
> + if test "$reference" = refs/tags/tag1
> + then
> + exit 1
> + fi
> + done
> + EOF
Without the extra indentation level, all your <<here-doc would
become easier to read. You are consistently doing this in your
patches, which it is better than random mixes of indentation style,
though.
> + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> +
> + # Creation of the tag has failed, so ideally refs/heads/something
> + # should not exist. The fact that it does demonstrates that there is
> + # a bug in the `--atomic` flag.
> + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> +'
Makes sense.
> +test_expect_success 'atomic fetch with backfill should use single transaction' '
> + git init clone4 &&
> +
> + # Fetching with the `--atomic` flag should update all references in a
> + # single transaction, including backfilled tags. We thus expect to see
> + # a single reference transaction for the created branch and tags.
> + cat >expected <<-EOF &&
> + prepared
> + $ZERO_OID $B refs/heads/something
> + $ZERO_OID $S refs/tags/tag2
> + committed
> + $ZERO_OID $B refs/heads/something
> + $ZERO_OID $S refs/tags/tag2
> + prepared
> + $ZERO_OID $T refs/tags/tag1
> + committed
> + $ZERO_OID $T refs/tags/tag1
> + EOF
I think we see two transactions here, even though the comment says
otherwise. Is this expecting a known breakage?
> + write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> + ( echo "$*" && cat ) >>actual
> + EOF
> +
> + git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> + test_cmp expected clone4/actual
Nice way to make sure what is and is not in each transaction. I
feels a bit too rigid (e.g. in the first transaction, there is no
inherent reason to expect that the update to head/something is
mentioned before the update to tags/tag2, for example).
> +'
> +
> +test_expect_success 'backfill failure causes command to fail' '
> + git init clone5 &&
> +
> + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> + while read oldrev newrev reference
> + do
> + if test "\$reference" = refs/tags/tag1
> + then
> + # Create a nested tag below the actual tag we
> + # wanted to write, which causes a D/F conflict
> + # later when we want to commit refs/tags/tag1.
> + # We cannot just `exit 1` here given that this
> + # would cause us to die immediately.
> + git update-ref refs/tags/tag1/nested $B
I have been wondering if we need to do this from the hook? If we
have this ref before we start "fetch", would it have the same
effect, or "fetch" notices that this interfering ref exists and
removes it to make room for storing refs/tags/tag1, making the whole
thing fail to fail?
> + exit \$!
In any case, "exit 0" or "exit \$?" would be understandable, but
exit with "$!", which is ...? The process ID of the most recent
background command? Puzzled.
> + fi
> + done
> + EOF
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/7] fetch: increase test coverage of fetches
2022-03-03 0:25 ` Junio C Hamano
@ 2022-03-03 6:47 ` Patrick Steinhardt
0 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-03-03 6:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]
On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > +test_expect_success 'atomic fetch with failing backfill' '
> > + git init clone3 &&
> > +
> > + # We want to test whether a failure when backfilling tags correctly
> > + # aborts the complete transaction when `--atomic` is passed: we should
> > + # neither create the branch nor should we create the tag when either
> > + # one of both fails to update correctly.
> > + #
> > + # To trigger failure we simply abort when backfilling a tag.
> > + write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> > + while read oldrev newrev reference
> > + do
> > + if test "$reference" = refs/tags/tag1
> > + then
> > + exit 1
> > + fi
> > + done
> > + EOF
>
> Without the extra indentation level, all your <<here-doc would
> become easier to read. You are consistently doing this in your
> patches, which it is better than random mixes of indentation style,
> though.
Personally I think it's easier to read this way, but grepping through
the codebase shows that what you're proposing is used consistently.
I'll change it.
> > + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> > +
> > + # Creation of the tag has failed, so ideally refs/heads/something
> > + # should not exist. The fact that it does demonstrates that there is
> > + # a bug in the `--atomic` flag.
> > + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> > +'
>
> Makes sense.
>
> > +test_expect_success 'atomic fetch with backfill should use single transaction' '
> > + git init clone4 &&
> > +
> > + # Fetching with the `--atomic` flag should update all references in a
> > + # single transaction, including backfilled tags. We thus expect to see
> > + # a single reference transaction for the created branch and tags.
> > + cat >expected <<-EOF &&
> > + prepared
> > + $ZERO_OID $B refs/heads/something
> > + $ZERO_OID $S refs/tags/tag2
> > + committed
> > + $ZERO_OID $B refs/heads/something
> > + $ZERO_OID $S refs/tags/tag2
> > + prepared
> > + $ZERO_OID $T refs/tags/tag1
> > + committed
> > + $ZERO_OID $T refs/tags/tag1
> > + EOF
>
> I think we see two transactions here, even though the comment says
> otherwise. Is this expecting a known breakage?
Yes, it is. I've added a comment in this patch to document the breakage,
which is then removed when the bug is fixed.
> > + write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> > + ( echo "$*" && cat ) >>actual
> > + EOF
> > +
> > + git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> > + test_cmp expected clone4/actual
>
> Nice way to make sure what is and is not in each transaction. I
> feels a bit too rigid (e.g. in the first transaction, there is no
> inherent reason to expect that the update to head/something is
> mentioned before the update to tags/tag2, for example).
>
> > +'
> > +
> > +test_expect_success 'backfill failure causes command to fail' '
> > + git init clone5 &&
> > +
> > + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> > + while read oldrev newrev reference
> > + do
> > + if test "\$reference" = refs/tags/tag1
> > + then
> > + # Create a nested tag below the actual tag we
> > + # wanted to write, which causes a D/F conflict
> > + # later when we want to commit refs/tags/tag1.
> > + # We cannot just `exit 1` here given that this
> > + # would cause us to die immediately.
>
> > + git update-ref refs/tags/tag1/nested $B
>
> I have been wondering if we need to do this from the hook? If we
> have this ref before we start "fetch", would it have the same
> effect, or "fetch" notices that this interfering ref exists and
> removes it to make room for storing refs/tags/tag1, making the whole
> thing fail to fail?
No, it indeed is not, thanks!
Patrick
> > + exit \$!
>
> In any case, "exit 0" or "exit \$?" would be understandable, but
> exit with "$!", which is ...? The process ID of the most recent
> background command? Puzzled.
>
> > + fi
> > + done
> > + EOF
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 2/7] fetch: backfill tags before setting upstream
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
2022-02-17 13:04 ` [PATCH v2 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 22:07 ` Junio C Hamano
2022-02-17 13:04 ` [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place Patrick Steinhardt
` (6 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]
The fetch code flow is a bit hard to understand right now:
1. We optionally prune all references which have vanished on the
remote side.
2. We fetch and update all other references locally.
3. We update the upstream branch in the gitconfig.
4. We backfill tags pointing into the history we have just fetched.
It is quite confusing that we fetch objects and update references in
both (2) and (4), which is further stressed by the point that we use a
`skip` goto label to jump from (3) to (4) in case we fail to update the
gitconfig as expected.
Reorder the code to first update all local references, and only after we
have done so update the upstream branch information. This improves the
code flow and furthermore makes it easier to refactor the way we update
references together.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6f5e157863..904ca9f1ca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct ref *ref_map;
+ struct ref *ref_map = NULL;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
retcode = 1;
}
if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
- free_refs(ref_map);
retcode = 1;
goto cleanup;
}
+ /*
+ * If neither --no-tags nor --tags was specified, do automated tag
+ * following.
+ */
+ if (tags == TAGS_DEFAULT && autotags) {
+ struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
+
+ find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+ if (tags_ref_map)
+ backfill_tags(transport, tags_ref_map, worktrees);
+
+ free_refs(tags_ref_map);
+ }
+
if (set_upstream) {
struct branch *branch = branch_get("HEAD");
struct ref *rm;
@@ -1644,7 +1657,7 @@ static int do_fetch(struct transport *transport,
if (!rm->peer_ref) {
if (source_ref) {
warning(_("multiple branches detected, incompatible with --set-upstream"));
- goto skip;
+ goto cleanup;
} else {
source_ref = rm;
}
@@ -1658,7 +1671,7 @@ static int do_fetch(struct transport *transport,
warning(_("could not set upstream of HEAD to '%s' from '%s' when "
"it does not point to any branch."),
shortname, transport->remote->name);
- goto skip;
+ goto cleanup;
}
if (!strcmp(source_ref->name, "HEAD") ||
@@ -1678,21 +1691,9 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
-skip:
- free_refs(ref_map);
-
- /* if neither --no-tags nor --tags was specified, do automated tag
- * following ... */
- if (tags == TAGS_DEFAULT && autotags) {
- struct ref **tail = &ref_map;
- ref_map = NULL;
- find_non_local_tags(remote_refs, &ref_map, &tail);
- if (ref_map)
- backfill_tags(transport, ref_map, worktrees);
- free_refs(ref_map);
- }
cleanup:
+ free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
}
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/7] fetch: backfill tags before setting upstream
2022-02-17 13:04 ` [PATCH v2 2/7] fetch: backfill tags before setting upstream Patrick Steinhardt
@ 2022-02-17 22:07 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 22:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> The fetch code flow is a bit hard to understand right now:
>
> 1. We optionally prune all references which have vanished on the
> remote side.
> 2. We fetch and update all other references locally.
> 3. We update the upstream branch in the gitconfig.
> 4. We backfill tags pointing into the history we have just fetched.
>
> It is quite confusing that we fetch objects and update references in
> both (2) and (4), which is further stressed by the point that we use a
> `skip` goto label to jump from (3) to (4) in case we fail to update the
> gitconfig as expected.
>
> Reorder the code to first update all local references, and only after we
> have done so update the upstream branch information. This improves the
> code flow and furthermore makes it easier to refactor the way we update
> references together.
OK, as "setting upsream" is more or less unrelated to the act of
actual fetching the refs and objects reachable from them, moving it
outside the main code that is about fetching does make sense.
> static int do_fetch(struct transport *transport,
> struct refspec *rs)
> {
> - struct ref *ref_map;
> + struct ref *ref_map = NULL;
This is needed because we will always do free_refs() on the variable
after this patch, and one early "goto cleanup" happens even before
we touch ref_map, when truncate_fetch_head() fails.
> @@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
> retcode = 1;
> }
> if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
> - free_refs(ref_map);
> retcode = 1;
> goto cleanup;
And because we always free_refs(ref_map), we can lose a call here.
OK.
> + /*
> + * If neither --no-tags nor --tags was specified, do automated tag
> + * following.
> + */
> + if (tags == TAGS_DEFAULT && autotags) {
> + struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
> +
> + find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> + if (tags_ref_map)
> + backfill_tags(transport, tags_ref_map, worktrees);
> +
> + free_refs(tags_ref_map);
> + }
Here, the new code uses a local and separete tags_ref_map variable
and free it before we leave, instead of reusing ref_map variable.
OK.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
2022-02-17 13:04 ` [PATCH v2 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
2022-02-17 13:04 ` [PATCH v2 2/7] fetch: backfill tags before setting upstream Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 22:12 ` Junio C Hamano
2022-02-17 13:04 ` [PATCH v2 4/7] fetch: report errors when backfilling tags fails Patrick Steinhardt
` (5 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 5634 bytes --]
There are two different locations where we're appending to FETCH_HEAD:
first when storing updated references, and second when backfilling tags.
Both times we open the file, append to it and then commit it into place,
which is essentially duplicate work.
Improve the lifecycle of updating FETCH_HEAD by opening and committing
it once in `do_fetch()`, where we pass the structure down to the code
which wants to append to it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 904ca9f1ca..f8adb40b45 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1084,9 +1084,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
static int store_updated_refs(const char *raw_url, const char *remote_name,
int connectivity_checked, struct ref *ref_map,
- struct worktree **worktrees)
+ struct fetch_head *fetch_head, struct worktree **worktrees)
{
- struct fetch_head fetch_head;
int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
struct ref_transaction *transaction = NULL;
@@ -1096,10 +1095,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
int want_status;
int summary_width = transport_summary_width(ref_map);
- rc = open_fetch_head(&fetch_head);
- if (rc)
- return -1;
-
if (raw_url)
url = transport_anonymize_url(raw_url);
else
@@ -1206,7 +1201,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_addf(¬e, "'%s' of ", what);
}
- append_fetch_head(&fetch_head, &rm->old_oid,
+ append_fetch_head(fetch_head, &rm->old_oid,
rm->fetch_head_status,
note.buf, url, url_len);
@@ -1246,9 +1241,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (!rc)
- commit_fetch_head(&fetch_head);
-
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
" 'git remote prune %s' to remove any old, conflicting "
@@ -1268,7 +1260,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_release(&err);
ref_transaction_free(transaction);
free(url);
- close_fetch_head(&fetch_head);
return rc;
}
@@ -1309,6 +1300,7 @@ static int check_exist_and_connected(struct ref *ref_map)
static int fetch_and_consume_refs(struct transport *transport,
struct ref *ref_map,
+ struct fetch_head *fetch_head,
struct worktree **worktrees)
{
int connectivity_checked = 1;
@@ -1331,7 +1323,7 @@ static int fetch_and_consume_refs(struct transport *transport,
trace2_region_enter("fetch", "consume_refs", the_repository);
ret = store_updated_refs(transport->url, transport->remote->name,
- connectivity_checked, ref_map, worktrees);
+ connectivity_checked, ref_map, fetch_head, worktrees);
trace2_region_leave("fetch", "consume_refs", the_repository);
out:
@@ -1503,7 +1495,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
return transport;
}
-static void backfill_tags(struct transport *transport, struct ref *ref_map,
+static void backfill_tags(struct transport *transport,
+ struct ref *ref_map,
+ struct fetch_head *fetch_head,
struct worktree **worktrees)
{
int cannot_reuse;
@@ -1525,7 +1519,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_and_consume_refs(transport, ref_map, worktrees);
+ fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1544,6 +1538,7 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
int must_list_refs = 1;
struct worktree **worktrees = get_worktrees();
+ struct fetch_head fetch_head = { 0 };
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map, worktrees);
+ retcode = open_fetch_head(&fetch_head);
+ if (retcode)
+ goto cleanup;
+
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1619,7 +1618,8 @@ static int do_fetch(struct transport *transport,
if (retcode != 0)
retcode = 1;
}
- if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
+
+ if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
retcode = 1;
goto cleanup;
}
@@ -1633,11 +1633,13 @@ static int do_fetch(struct transport *transport,
find_non_local_tags(remote_refs, &tags_ref_map, &tail);
if (tags_ref_map)
- backfill_tags(transport, tags_ref_map, worktrees);
+ backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
free_refs(tags_ref_map);
}
+ commit_fetch_head(&fetch_head);
+
if (set_upstream) {
struct branch *branch = branch_get("HEAD");
struct ref *rm;
@@ -1693,6 +1695,7 @@ static int do_fetch(struct transport *transport,
}
cleanup:
+ close_fetch_head(&fetch_head);
free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place
2022-02-17 13:04 ` [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place Patrick Steinhardt
@ 2022-02-17 22:12 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 22:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> There are two different locations where we're appending to FETCH_HEAD:
> first when storing updated references, and second when backfilling tags.
> Both times we open the file, append to it and then commit it into place,
> which is essentially duplicate work.
>
> Improve the lifecycle of updating FETCH_HEAD by opening and committing
> it once in `do_fetch()`, where we pass the structure down to the code
> which wants to append to it.
OK. Each call to store_updated_refs() used to be responsible for
opening, appending, committing, and closing FETCH_HEAD. We now make
do_fetch() responsible for opening, committing, and closing, and let
the direct and indirect callers of fetch_and_consume_refs() only
worry about appending to it. Makes sense.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 4/7] fetch: report errors when backfilling tags fails
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (2 preceding siblings ...)
2022-02-17 13:04 ` [PATCH v2 3/7] fetch: control lifecycle of FETCH_HEAD in a single place Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 22:16 ` Junio C Hamano
2022-02-17 13:04 ` [PATCH v2 5/7] refs: add interface to iterate over queued transactional updates Patrick Steinhardt
` (4 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]
When the backfilling of tags fails we do not report this error to the
caller, but only report it implicitly at a later point when reporting
updated references. This leaves callers unable to act upon the
information of whether the backfilling succeeded or not.
Refactor the function to return an error code and pass it up the
callstack. This causes us to correctly propagate the error back to the
user of git-fetch(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 26 ++++++++++++++++++--------
t/t5503-tagfollow.sh | 4 +---
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f8adb40b45..d304314f16 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
return transport;
}
-static void backfill_tags(struct transport *transport,
- struct ref *ref_map,
- struct fetch_head *fetch_head,
- struct worktree **worktrees)
+static int backfill_tags(struct transport *transport,
+ struct ref *ref_map,
+ struct fetch_head *fetch_head,
+ struct worktree **worktrees)
{
- int cannot_reuse;
+ int retcode, cannot_reuse;
/*
* Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it
@@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+ retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
gsecondary = NULL;
}
+
+ return retcode;
}
static int do_fetch(struct transport *transport,
@@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
find_non_local_tags(remote_refs, &tags_ref_map, &tail);
- if (tags_ref_map)
- backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
+ if (tags_ref_map) {
+ /*
+ * If backfilling of tags fails then we want to tell
+ * the user so, but we have to continue regardless to
+ * populate upstream information of the references we
+ * have already fetched above.
+ */
+ if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+ retcode = 1;
+ }
free_refs(tags_ref_map);
}
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 6ffe2a5719..c057c49e80 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
done
EOF
- # Even though we fail to create refs/tags/tag1 the below command
- # unexpectedly succeeds.
- git -C clone5 fetch .. $B:refs/heads/something &&
+ test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
test $S = $(git -C clone5 rev-parse --verify tag2) &&
test_must_fail git -C clone5 rev-parse --verify tag1
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/7] fetch: report errors when backfilling tags fails
2022-02-17 13:04 ` [PATCH v2 4/7] fetch: report errors when backfilling tags fails Patrick Steinhardt
@ 2022-02-17 22:16 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 22:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> +static int backfill_tags(struct transport *transport,
> ...
> }
The change to this function is quite straight-forward.
> static int do_fetch(struct transport *transport,
> @@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
> struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>
> find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> - if (tags_ref_map)
> - backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
> + if (tags_ref_map) {
> + /*
> + * If backfilling of tags fails then we want to tell
> + * the user so, but we have to continue regardless to
> + * populate upstream information of the references we
> + * have already fetched above.
> + */
OK. Unless atomic, using the available information on successfully
updated ones would be fine. Perhaps under --atomic mode, a failure
to commit the ref transaction will also prevent the upstream
information from getting updated? We'll see soon enough.
> + if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
> + retcode = 1;
> + }
> free_refs(tags_ref_map);
> }
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 6ffe2a5719..c057c49e80 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
> done
> EOF
>
> - # Even though we fail to create refs/tags/tag1 the below command
> - # unexpectedly succeeds.
> - git -C clone5 fetch .. $B:refs/heads/something &&
> + test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
OK.
> test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
> test $S = $(git -C clone5 rev-parse --verify tag2) &&
> test_must_fail git -C clone5 rev-parse --verify tag1
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 5/7] refs: add interface to iterate over queued transactional updates
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (3 preceding siblings ...)
2022-02-17 13:04 ` [PATCH v2 4/7] fetch: report errors when backfilling tags fails Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 13:04 ` [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags Patrick Steinhardt
` (3 subsequent siblings)
8 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]
There is no way for a caller to see whether a reference update has
already been queued up for a given reference transaction. There are
multiple alternatives to provide this functionality:
- We may add a function that simply tells us whether a specific
reference has already been queued. If implemented naively then
this would potentially be quadratic in runtime behaviour if this
question is asked repeatedly because we have to iterate over all
references every time. The alternative would be to add a hashmap
of all queued reference updates to speed up the lookup, but this
adds overhead to all callers.
- We may add a flag to `ref_transaction_add_update()` that causes it
to skip duplicates, but this has the same runtime concerns as the
first alternative.
- We may add an interface which lets callers collect all updates
which have already been queued such that he can avoid re-adding
them. This is the most flexible approach and puts the burden on
the caller, but also allows us to not impact any of the existing
callsites which don't need this information.
This commit implements the last approach: it allows us to compute the
map of already-queued updates once up front such that we can then skip
all subsequent references which are already part of this map.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 16 ++++++++++++++++
refs.h | 14 ++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/refs.c b/refs.c
index d680de3bc0..35d4e69687 100644
--- a/refs.c
+++ b/refs.c
@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
return refs->be->initial_transaction_commit(refs, transaction, err);
}
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+ ref_transaction_for_each_queued_update_fn cb,
+ void *cb_data)
+{
+ int i;
+
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+
+ cb(update->refname,
+ (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,
+ (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,
+ cb_data);
+ }
+}
+
int refs_delete_refs(struct ref_store *refs, const char *logmsg,
struct string_list *refnames, unsigned int flags)
{
diff --git a/refs.h b/refs.h
index ff859d5951..1ae12c410a 100644
--- a/refs.h
+++ b/refs.h
@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,
int initial_ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
+/*
+ * Execute the given callback function for each of the reference updates which
+ * have been queued in the given transaction. `old_oid` and `new_oid` may be
+ * `NULL` pointers depending on whether the update has these object IDs set or
+ * not.
+ */
+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ void *cb_data);
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+ ref_transaction_for_each_queued_update_fn cb,
+ void *cb_data);
+
/*
* Free `*transaction` and all associated data.
*/
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (4 preceding siblings ...)
2022-02-17 13:04 ` [PATCH v2 5/7] refs: add interface to iterate over queued transactional updates Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 22:27 ` Junio C Hamano
2022-02-17 13:04 ` [PATCH v2 7/7] fetch: make `--atomic` flag cover pruning of refs Patrick Steinhardt
` (2 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 10780 bytes --]
When fetching references from a remote we by default also fetch all tags
which point into the history we have fetched. This is a separate step
performed after updating local references because it requires us to walk
over the history on the client-side to determine whether the remote has
announced any tags which point to one of the fetched commits.
This backfilling of tags isn't covered by the `--atomic` flag: right
now, it only applies to the step where we update our local references.
This is an oversight at the time the flag was introduced: its purpose is
to either update all references or none, but right now we happily update
local references even in the case where backfilling failed.
Fix this by pulling up creation of the reference transaction such that
we can pass the same transaction to both the code which updates local
references and to the code which backfills tags. This allows us to only
commit the transaction in case both actions succeed.
Note that we also have to start passing the transaction into
`find_non_local_tags()`: this function is responsible for finding all
tags which we need to backfill. Right now, it will happily return tags
which have already been updated with our local references. But when we
use a single transaction for both local references and backfilling then
it may happen that we try to queue the same reference update twice to
the transaction, which consequently triggers a bug. We thus have to skip
over any tags which have already been queued.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 92 +++++++++++++++++++++++++++++++-------------
t/t5503-tagfollow.sh | 11 ++----
2 files changed, 69 insertions(+), 34 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d304314f16..67af842091 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)
item->ignore = 1;
}
+
+static void add_already_queued_tags(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ void *cb_data)
+{
+ struct hashmap *queued_tags = cb_data;
+ if (starts_with(refname, "refs/tags/") && new_oid)
+ (void) refname_hash_add(queued_tags, refname, new_oid);
+}
+
static void find_non_local_tags(const struct ref *refs,
+ struct ref_transaction *transaction,
struct ref **head,
struct ref ***tail)
{
@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *refs,
create_fetch_oidset(head, &fetch_oids);
for_each_ref(add_one_refname, &existing_refs);
+
+ /*
+ * If we already have a transaction, then we need to filter out all
+ * tags which have already been queued up.
+ */
+ if (transaction)
+ ref_transaction_for_each_queued_update(transaction,
+ add_already_queued_tags,
+ &existing_refs);
+
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -600,7 +622,7 @@ static struct ref *get_ref_map(struct remote *remote,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, &tail, 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(remote_refs, &ref_map, &tail);
+ find_non_local_tags(remote_refs, NULL, &ref_map, &tail);
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1083,12 +1105,12 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
"to avoid this check\n");
static int store_updated_refs(const char *raw_url, const char *remote_name,
- int connectivity_checked, struct ref *ref_map,
+ int connectivity_checked,
+ struct ref_transaction *transaction, struct ref *ref_map,
struct fetch_head *fetch_head, struct worktree **worktrees)
{
int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
- struct ref_transaction *transaction = NULL;
const char *what, *kind;
struct ref *rm;
char *url;
@@ -1110,14 +1132,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (atomic_fetch) {
- transaction = ref_transaction_begin(&err);
- if (!transaction) {
- error("%s", err.buf);
- goto abort;
- }
- }
-
prepare_format_display(ref_map);
/*
@@ -1233,14 +1247,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (!rc && transaction) {
- rc = ref_transaction_commit(transaction, &err);
- if (rc) {
- error("%s", err.buf);
- goto abort;
- }
- }
-
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
" 'git remote prune %s' to remove any old, conflicting "
@@ -1258,7 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
abort:
strbuf_release(¬e);
strbuf_release(&err);
- ref_transaction_free(transaction);
free(url);
return rc;
}
@@ -1299,6 +1304,7 @@ static int check_exist_and_connected(struct ref *ref_map)
}
static int fetch_and_consume_refs(struct transport *transport,
+ struct ref_transaction *transaction,
struct ref *ref_map,
struct fetch_head *fetch_head,
struct worktree **worktrees)
@@ -1323,7 +1329,8 @@ static int fetch_and_consume_refs(struct transport *transport,
trace2_region_enter("fetch", "consume_refs", the_repository);
ret = store_updated_refs(transport->url, transport->remote->name,
- connectivity_checked, ref_map, fetch_head, worktrees);
+ connectivity_checked, transaction, ref_map,
+ fetch_head, worktrees);
trace2_region_leave("fetch", "consume_refs", the_repository);
out:
@@ -1496,6 +1503,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
}
static int backfill_tags(struct transport *transport,
+ struct ref_transaction *transaction,
struct ref *ref_map,
struct fetch_head *fetch_head,
struct worktree **worktrees)
@@ -1519,7 +1527,7 @@ static int backfill_tags(struct transport *transport,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+ retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1532,6 +1540,7 @@ static int backfill_tags(struct transport *transport,
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
+ struct ref_transaction *transaction = NULL;
struct ref *ref_map = NULL;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
@@ -1541,6 +1550,7 @@ static int do_fetch(struct transport *transport,
int must_list_refs = 1;
struct worktree **worktrees = get_worktrees();
struct fetch_head fetch_head = { 0 };
+ struct strbuf err = STRBUF_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1602,6 +1612,14 @@ static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
+ if (atomic_fetch) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ retcode = error("%s", err.buf);
+ goto cleanup;
+ }
+ }
+
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,
retcode = 1;
}
- if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
+ if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
retcode = 1;
goto cleanup;
}
@@ -1633,21 +1651,37 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT && autotags) {
struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
- find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+ find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
if (tags_ref_map) {
/*
* If backfilling of tags fails then we want to tell
* the user so, but we have to continue regardless to
* populate upstream information of the references we
- * have already fetched above.
+ * have already fetched above. The exception though is
+ * when `--atomic` is passed: in that case we'll abort
+ * the transaction and don't commit anything.
*/
- if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+ if (backfill_tags(transport, transaction, tags_ref_map,
+ &fetch_head, worktrees))
retcode = 1;
}
free_refs(tags_ref_map);
}
+ if (transaction) {
+ if (retcode)
+ goto cleanup;
+
+ retcode = ref_transaction_commit(transaction, &err);
+ if (retcode) {
+ error("%s", err.buf);
+ ref_transaction_free(transaction);
+ transaction = NULL;
+ goto cleanup;
+ }
+ }
+
commit_fetch_head(&fetch_head);
if (set_upstream) {
@@ -1705,7 +1739,13 @@ static int do_fetch(struct transport *transport,
}
cleanup:
+ if (retcode && transaction) {
+ ref_transaction_abort(transaction, &err);
+ error("%s", err.buf);
+ }
+
close_fetch_head(&fetch_head);
+ strbuf_release(&err);
free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index c057c49e80..e72fdc2534 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -180,11 +180,8 @@ test_expect_success 'atomic fetch with failing backfill' '
EOF
test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
-
- # Creation of the tag has failed, so ideally refs/heads/something
- # should not exist. The fact that it does demonstrates that there is
- # a bug in the `--atomic` flag.
- test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+ test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
+ test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
'
test_expect_success 'atomic fetch with backfill should use single transaction' '
@@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
prepared
$ZERO_OID $B refs/heads/something
$ZERO_OID $S refs/tags/tag2
+ $ZERO_OID $T refs/tags/tag1
committed
$ZERO_OID $B refs/heads/something
$ZERO_OID $S refs/tags/tag2
- prepared
- $ZERO_OID $T refs/tags/tag1
- committed
$ZERO_OID $T refs/tags/tag1
EOF
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags
2022-02-17 13:04 ` [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags Patrick Steinhardt
@ 2022-02-17 22:27 ` Junio C Hamano
0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2022-02-17 22:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> Fix this by pulling up creation of the reference transaction such that
> we can pass the same transaction to both the code which updates local
> references and to the code which backfills tags. This allows us to only
> commit the transaction in case both actions succeed.
OK, having done the FETCH_HEAD thing, the idea is quite similar.
Instead of letting two invocations to store_updated_refs() to
independently open and close separate transactions, we control
everything centrally in do_fetch().
Makes sense.
> @@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
> prepared
> $ZERO_OID $B refs/heads/something
> $ZERO_OID $S refs/tags/tag2
> + $ZERO_OID $T refs/tags/tag1
> committed
> $ZERO_OID $B refs/heads/something
> $ZERO_OID $S refs/tags/tag2
> - prepared
> - $ZERO_OID $T refs/tags/tag1
> - committed
> $ZERO_OID $T refs/tags/tag1
> EOF
OK.
Unlike the "expect the behaviour at the end of the series from the
beginning with known-to-fail tests" pattern I cautioned against in
an earlier step, this is a good way to show how the behaviour
changes.
Thanks.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 7/7] fetch: make `--atomic` flag cover pruning of refs
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (5 preceding siblings ...)
2022-02-17 13:04 ` [PATCH v2 6/7] fetch: make `--atomic` flag cover backfilling of tags Patrick Steinhardt
@ 2022-02-17 13:04 ` Patrick Steinhardt
2022-02-17 15:50 ` [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Christian Couder
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
8 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-17 13:04 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 5171 bytes --]
When fetching with the `--prune` flag we will delete any local
references matching the fetch refspec which have disappeared on the
remote. This step is not currently covered by the `--atomic` flag: we
delete branches even though updating of local references has failed,
which means that the fetch is not an all-or-nothing operation.
Fix this bug by passing in the global transaction into `prune_refs()`:
if one is given, then we'll only queue up deletions and not commit them
right away.
This change also improves performance when pruning many branches in a
repository with a big packed-refs file: every references is pruned in
its own transaction, which means that we potentially have to rewrite
the packed-refs files for every single reference we're about to prune.
The following benchmark demonstrates this: it performs a pruning fetch
from a repository with a single reference into a repository with 100k
references, which causes us to prune all but one reference. This is of
course a very artificial setup, but serves to demonstrate the impact of
only having to write the packed-refs file once:
Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)
Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]
Range (min … max): 2.328 s … 2.407 s 10 runs
Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)
Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]
Range (min … max): 1.346 s … 1.400 s 10 runs
Summary
'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran
1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 30 ++++++++++++++++++++++--------
t/t5510-fetch.sh | 8 ++------
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67af842091..9a2b5c03a4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1338,11 +1338,14 @@ static int fetch_and_consume_refs(struct transport *transport,
return ret;
}
-static int prune_refs(struct refspec *rs, struct ref *ref_map,
+static int prune_refs(struct refspec *rs,
+ struct ref_transaction *transaction,
+ struct ref *ref_map,
const char *raw_url)
{
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
+ struct strbuf err = STRBUF_INIT;
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
@@ -1363,13 +1366,22 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
url_len = i - 3;
if (!dry_run) {
- struct string_list refnames = STRING_LIST_INIT_NODUP;
+ if (transaction) {
+ for (ref = stale_refs; ref; ref = ref->next) {
+ result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+ "fetch: prune", &err);
+ if (result)
+ goto cleanup;
+ }
+ } else {
+ struct string_list refnames = STRING_LIST_INIT_NODUP;
- for (ref = stale_refs; ref; ref = ref->next)
- string_list_append(&refnames, ref->name);
+ for (ref = stale_refs; ref; ref = ref->next)
+ string_list_append(&refnames, ref->name);
- result = delete_refs("fetch: prune", &refnames, 0);
- string_list_clear(&refnames, 0);
+ result = delete_refs("fetch: prune", &refnames, 0);
+ string_list_clear(&refnames, 0);
+ }
}
if (verbosity >= 0) {
@@ -1388,6 +1400,8 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
}
}
+cleanup:
+ strbuf_release(&err);
free(url);
free_refs(stale_refs);
return result;
@@ -1629,10 +1643,10 @@ static int do_fetch(struct transport *transport,
* don't care whether --tags was specified.
*/
if (rs->nr) {
- retcode = prune_refs(rs, ref_map, transport->url);
+ retcode = prune_refs(rs, transaction, ref_map, transport->url);
} else {
retcode = prune_refs(&transport->remote->fetch,
- ref_map,
+ transaction, ref_map,
transport->url);
}
if (retcode != 0)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 70d51f343b..48e14e2dab 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact
head_oid=$(git rev-parse HEAD) &&
# Fetching with the `--atomic` flag should update all references in a
- # single transaction. It is currently missing coverage of pruned
- # references though, and as a result those may be committed to disk
- # even if updating references fails later.
+ # single transaction.
cat >expected <<-EOF &&
prepared
$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
- committed
- $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
- prepared
$ZERO_OID $head_oid refs/remotes/origin/new-branch
committed
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
$ZERO_OID $head_oid refs/remotes/origin/new-branch
EOF
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (6 preceding siblings ...)
2022-02-17 13:04 ` [PATCH v2 7/7] fetch: make `--atomic` flag cover pruning of refs Patrick Steinhardt
@ 2022-02-17 15:50 ` Christian Couder
2022-02-17 22:30 ` Junio C Hamano
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
8 siblings, 1 reply; 35+ messages in thread
From: Christian Couder @ 2022-02-17 15:50 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jonathan Tan, Elijah Newren
On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote:
> This is the second version of my patch series which fixes these
> problems.
I took another look and found only a very minor improvement in one of
the tests in patch 1/7.
Thanks!
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 0/7] fetch: improve atomicity of `--atomic` flag
2022-02-17 13:04 [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Patrick Steinhardt
` (7 preceding siblings ...)
2022-02-17 15:50 ` [PATCH v2 0/7] fetch: improve atomicity of `--atomic` flag Christian Couder
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
` (6 more replies)
8 siblings, 7 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]
Hi,
in c7b190dabd (fetch: implement support for atomic reference updates,
2021-01-12), I have added a new `--atomic` flag to git-fetch(1) which is
supposed to make it an all-or-nothing operation: either all refs are
updated, or none is. I have recently discovered though that there were
two oversights: neither pruning of refs via `--prune` nor the tag
backfill mechanism are currently covered by this flag, which breaks the
promise.
This is the third version of this patch series fixing these issues. The
only change compared to v2 is a clarification in a comment to better
point out which type of backfilling is being tested.
Patrick
Patrick Steinhardt (7):
fetch: increase test coverage of fetches
fetch: backfill tags before setting upstream
fetch: control lifecycle of FETCH_HEAD in a single place
fetch: report errors when backfilling tags fails
refs: add interface to iterate over queued transactional updates
fetch: make `--atomic` flag cover backfilling of tags
fetch: make `--atomic` flag cover pruning of refs
builtin/fetch.c | 192 +++++++++++++++++++++++++++++--------------
refs.c | 16 ++++
refs.h | 14 ++++
t/t5503-tagfollow.sh | 74 +++++++++++++++++
t/t5510-fetch.sh | 29 +++++++
5 files changed, 263 insertions(+), 62 deletions(-)
Range-diff against v2:
1: b4ca3f1f3b ! 1: 081174b7a0 fetch: increase test coverage of fetches
@@ t/t5503-tagfollow.sh: test_expect_success 'new clone fetch main and tags' '
+test_expect_success 'atomic fetch with failing backfill' '
+ git init clone3 &&
+
-+ # We want to test whether a failure when backfilling tags correctly
++ # We want to test whether a failure in `backfill_tags()` correctly
+ # aborts the complete transaction when `--atomic` is passed: we should
+ # neither create the branch nor should we create the tag when either
+ # one of both fails to update correctly.
2: b0a067bbc1 = 2: 9867e76ac7 fetch: backfill tags before setting upstream
3: 0b9d04622d = 3: 7c36ecbcf4 fetch: control lifecycle of FETCH_HEAD in a single place
4: bc1e396ae0 = 4: a7e005dd48 fetch: report errors when backfilling tags fails
5: fac2e3555a = 5: 0316d770e9 refs: add interface to iterate over queued transactional updates
6: 331ee40e57 = 6: 2f98e34c84 fetch: make `--atomic` flag cover backfilling of tags
7: 2ad16530e5 = 7: 7292de826b fetch: make `--atomic` flag cover pruning of refs
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-03-03 0:30 ` Junio C Hamano
2022-02-21 8:02 ` [PATCH v3 2/7] fetch: backfill tags before setting upstream Patrick Steinhardt
` (5 subsequent siblings)
6 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 5686 bytes --]
When using git-fetch(1) with the `--atomic` flag the expectation is that
either all of the references are updated, or alternatively none are in
case the fetch fails. While we already have tests for this, we do not
have any tests which exercise atomicity either when pruning deleted refs
or when backfilling tags. This gap in test coverage hides that we indeed
don't handle atomicity correctly for both of these cases.
Add test cases which cover these testing gaps to demonstrate the broken
behaviour. Note that tests are not marked as `test_expect_failure`: this
is done to explicitly demonstrate the current known-wrong behaviour, and
they will be fixed up as soon as we fix the underlying bugs.
While at it this commit also adds another test case which demonstrates
that backfilling of tags does not return an error code in case the
backfill fails. This bug will also be fixed by a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++
t/t5510-fetch.sh | 33 ++++++++++++++++++
2 files changed, 114 insertions(+)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd4..7dadaafd4b 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -160,4 +160,85 @@ test_expect_success 'new clone fetch main and tags' '
test_cmp expect actual
'
+test_expect_success 'atomic fetch with failing backfill' '
+ git init clone3 &&
+
+ # We want to test whether a failure in `backfill_tags()` correctly
+ # aborts the complete transaction when `--atomic` is passed: we should
+ # neither create the branch nor should we create the tag when either
+ # one of both fails to update correctly.
+ #
+ # To trigger failure we simply abort when backfilling a tag.
+ write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
+ while read oldrev newrev reference
+ do
+ if test "$reference" = refs/tags/tag1
+ then
+ exit 1
+ fi
+ done
+ EOF
+
+ test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
+
+ # Creation of the tag has failed, so ideally refs/heads/something
+ # should not exist. The fact that it does demonstrates that there is
+ # a bug in the `--atomic` flag.
+ test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+'
+
+test_expect_success 'atomic fetch with backfill should use single transaction' '
+ git init clone4 &&
+
+ # Fetching with the `--atomic` flag should update all references in a
+ # single transaction, including backfilled tags. We thus expect to see
+ # a single reference transaction for the created branch and tags.
+ cat >expected <<-EOF &&
+ prepared
+ $ZERO_OID $B refs/heads/something
+ $ZERO_OID $S refs/tags/tag2
+ committed
+ $ZERO_OID $B refs/heads/something
+ $ZERO_OID $S refs/tags/tag2
+ prepared
+ $ZERO_OID $T refs/tags/tag1
+ committed
+ $ZERO_OID $T refs/tags/tag1
+ EOF
+
+ write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
+ ( echo "$*" && cat ) >>actual
+ EOF
+
+ git -C clone4 fetch --atomic .. $B:refs/heads/something &&
+ test_cmp expected clone4/actual
+'
+
+test_expect_success 'backfill failure causes command to fail' '
+ git init clone5 &&
+
+ write_script clone5/.git/hooks/reference-transaction <<-EOF &&
+ while read oldrev newrev reference
+ do
+ if test "\$reference" = refs/tags/tag1
+ then
+ # Create a nested tag below the actual tag we
+ # wanted to write, which causes a D/F conflict
+ # later when we want to commit refs/tags/tag1.
+ # We cannot just `exit 1` here given that this
+ # would cause us to die immediately.
+ git update-ref refs/tags/tag1/nested $B
+ exit \$!
+ fi
+ done
+ EOF
+
+ # Even though we fail to create refs/tags/tag1 the below command
+ # unexpectedly succeeds.
+ git -C clone5 fetch .. $B:refs/heads/something &&
+ test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
+ test $S = $(git -C clone5 rev-parse --verify tag2) &&
+ test_must_fail git -C clone5 rev-parse --verify tag1
+'
+
test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ef0da0a63b..70d51f343b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -343,6 +343,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' '
test_cmp expected atomic/.git/FETCH_HEAD
'
+test_expect_success 'fetch --atomic --prune executes a single reference transaction only' '
+ test_when_finished "rm -rf \"$D\"/atomic" &&
+
+ cd "$D" &&
+ git branch scheduled-for-deletion &&
+ git clone . atomic &&
+ git branch -D scheduled-for-deletion &&
+ git branch new-branch &&
+ head_oid=$(git rev-parse HEAD) &&
+
+ # Fetching with the `--atomic` flag should update all references in a
+ # single transaction. It is currently missing coverage of pruned
+ # references though, and as a result those may be committed to disk
+ # even if updating references fails later.
+ cat >expected <<-EOF &&
+ prepared
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+ committed
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
+ prepared
+ $ZERO_OID $head_oid refs/remotes/origin/new-branch
+ committed
+ $ZERO_OID $head_oid refs/remotes/origin/new-branch
+ EOF
+
+ write_script atomic/.git/hooks/reference-transaction <<-\EOF &&
+ ( echo "$*" && cat ) >>actual
+ EOF
+
+ git -C atomic fetch --atomic --prune origin &&
+ test_cmp expected atomic/actual
+'
+
test_expect_success '--refmap="" ignores configured refspec' '
cd "$TRASH_DIRECTORY" &&
git clone "$D" remote-refs &&
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-02-21 8:02 ` [PATCH v3 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
@ 2022-03-03 0:30 ` Junio C Hamano
2022-03-03 0:32 ` Junio C Hamano
0 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2022-03-03 0:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
> +test_expect_success 'backfill failure causes command to fail' '
> + git init clone5 &&
> +
> + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> + while read oldrev newrev reference
> + do
> + if test "\$reference" = refs/tags/tag1
> + then
> + # Create a nested tag below the actual tag we
> + # wanted to write, which causes a D/F conflict
> + # later when we want to commit refs/tags/tag1.
> + # We cannot just `exit 1` here given that this
> + # would cause us to die immediately.
> + git update-ref refs/tags/tag1/nested $B
> + exit \$!
> + fi
> + done
> + EOF
I think I've reviewed the previous round of these patches in
detail. I by mistake sent a comment for this step in v2, but I
think the same puzzlement exists in this round, too.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-03-03 0:30 ` Junio C Hamano
@ 2022-03-03 0:32 ` Junio C Hamano
2022-03-03 6:43 ` Patrick Steinhardt
0 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2022-03-03 0:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> +test_expect_success 'backfill failure causes command to fail' '
>> + git init clone5 &&
>> +
>> + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
>> + while read oldrev newrev reference
>> + do
>> + if test "\$reference" = refs/tags/tag1
>> + then
>> + # Create a nested tag below the actual tag we
>> + # wanted to write, which causes a D/F conflict
>> + # later when we want to commit refs/tags/tag1.
>> + # We cannot just `exit 1` here given that this
>> + # would cause us to die immediately.
>> + git update-ref refs/tags/tag1/nested $B
>> + exit \$!
>> + fi
>> + done
>> + EOF
>
> I think I've reviewed the previous round of these patches in
> detail. I by mistake sent a comment for this step in v2, but I
> think the same puzzlement exists in this round, too.
Namely:
I have been wondering if we need to do this from the hook? If we
have this ref before we start "fetch", would it have the same
effect, or "fetch" notices that this interfering ref exists and
removes it to make room for storing refs/tags/tag1, making the whole
thing fail to fail?
> + exit \$!
In any case, "exit 0" or "exit \$?" would be understandable, but
exit with "$!", which is ...? The process ID of the most recent
background command? Puzzled.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-03-03 0:32 ` Junio C Hamano
@ 2022-03-03 6:43 ` Patrick Steinhardt
2022-03-03 6:49 ` Junio C Hamano
0 siblings, 1 reply; 35+ messages in thread
From: Patrick Steinhardt @ 2022-03-03 6:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
On Wed, Mar 02, 2022 at 04:32:48PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> +test_expect_success 'backfill failure causes command to fail' '
> >> + git init clone5 &&
> >> +
> >> + write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> >> + while read oldrev newrev reference
> >> + do
> >> + if test "\$reference" = refs/tags/tag1
> >> + then
> >> + # Create a nested tag below the actual tag we
> >> + # wanted to write, which causes a D/F conflict
> >> + # later when we want to commit refs/tags/tag1.
> >> + # We cannot just `exit 1` here given that this
> >> + # would cause us to die immediately.
> >> + git update-ref refs/tags/tag1/nested $B
> >> + exit \$!
> >> + fi
> >> + done
> >> + EOF
> >
> > I think I've reviewed the previous round of these patches in
> > detail. I by mistake sent a comment for this step in v2, but I
> > think the same puzzlement exists in this round, too.
>
> Namely:
>
> I have been wondering if we need to do this from the hook? If we
> have this ref before we start "fetch", would it have the same
> effect, or "fetch" notices that this interfering ref exists and
> removes it to make room for storing refs/tags/tag1, making the whole
> thing fail to fail?
>
> > + exit \$!
>
> In any case, "exit 0" or "exit \$?" would be understandable, but
> exit with "$!", which is ...? The process ID of the most recent
> background command? Puzzled.
Oof, this was supposed to be `exit \$?`, thanks for catching this. But
your above comment is right: we can indeed just create the D/F conflict
outside of the hook and thus avoid the hook script altogether. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-03-03 6:43 ` Patrick Steinhardt
@ 2022-03-03 6:49 ` Junio C Hamano
2022-03-03 6:51 ` Patrick Steinhardt
0 siblings, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2022-03-03 6:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
Patrick Steinhardt <ps@pks.im> writes:
>> >> + # would cause us to die immediately.
>> >> + git update-ref refs/tags/tag1/nested $B
>> >> + exit \$!
>> >> + fi
>> >> + done
>> >> + EOF
>> >
>> > I think I've reviewed the previous round of these patches in
>> > detail. I by mistake sent a comment for this step in v2, but I
>> > think the same puzzlement exists in this round, too.
>>
>> Namely:
>>
>> I have been wondering if we need to do this from the hook? If we
>> have this ref before we start "fetch", would it have the same
>> effect, or "fetch" notices that this interfering ref exists and
>> removes it to make room for storing refs/tags/tag1, making the whole
>> thing fail to fail?
>>
>> > + exit \$!
>>
>> In any case, "exit 0" or "exit \$?" would be understandable, but
>> exit with "$!", which is ...? The process ID of the most recent
>> background command? Puzzled.
>
> Oof, this was supposed to be `exit \$?`, thanks for catching this. But
> your above comment is right: we can indeed just create the D/F conflict
> outside of the hook and thus avoid the hook script altogether. Thanks!
I see.
As that shell does not send anything to background, at the point of
the reference $! would yield an empty string, and "exit" is
equivalent to "exit $?", it is doing the right thing, I presume.
The topic has been in 'next' for a while, so if you are inclined to
fix it up, please send an incremental patch. If you do "exit" it
would be a one-liner change, or if you use a different "cause D/F
conflict outside the hook" approach, the change may become a bit
more involved.
Thanks.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/7] fetch: increase test coverage of fetches
2022-03-03 6:49 ` Junio C Hamano
@ 2022-03-03 6:51 ` Patrick Steinhardt
0 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-03-03 6:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Jonathan Tan, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]
On Wed, Mar 02, 2022 at 10:49:05PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> >> + # would cause us to die immediately.
> >> >> + git update-ref refs/tags/tag1/nested $B
> >> >> + exit \$!
> >> >> + fi
> >> >> + done
> >> >> + EOF
> >> >
> >> > I think I've reviewed the previous round of these patches in
> >> > detail. I by mistake sent a comment for this step in v2, but I
> >> > think the same puzzlement exists in this round, too.
> >>
> >> Namely:
> >>
> >> I have been wondering if we need to do this from the hook? If we
> >> have this ref before we start "fetch", would it have the same
> >> effect, or "fetch" notices that this interfering ref exists and
> >> removes it to make room for storing refs/tags/tag1, making the whole
> >> thing fail to fail?
> >>
> >> > + exit \$!
> >>
> >> In any case, "exit 0" or "exit \$?" would be understandable, but
> >> exit with "$!", which is ...? The process ID of the most recent
> >> background command? Puzzled.
> >
> > Oof, this was supposed to be `exit \$?`, thanks for catching this. But
> > your above comment is right: we can indeed just create the D/F conflict
> > outside of the hook and thus avoid the hook script altogether. Thanks!
>
> I see.
>
> As that shell does not send anything to background, at the point of
> the reference $! would yield an empty string, and "exit" is
> equivalent to "exit $?", it is doing the right thing, I presume.
>
> The topic has been in 'next' for a while, so if you are inclined to
> fix it up, please send an incremental patch. If you do "exit" it
> would be a one-liner change, or if you use a different "cause D/F
> conflict outside the hook" approach, the change may become a bit
> more involved.
>
> Thanks.
Fair enough, thanks for clarifying the steps. Does it make sense to also
change indentantion of the heredocs as per your review of v2 or should I
just keep that as-is now?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 2/7] fetch: backfill tags before setting upstream
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 3/7] fetch: control lifecycle of FETCH_HEAD in a single place Patrick Steinhardt
` (4 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]
The fetch code flow is a bit hard to understand right now:
1. We optionally prune all references which have vanished on the
remote side.
2. We fetch and update all other references locally.
3. We update the upstream branch in the gitconfig.
4. We backfill tags pointing into the history we have just fetched.
It is quite confusing that we fetch objects and update references in
both (2) and (4), which is further stressed by the point that we use a
`skip` goto label to jump from (3) to (4) in case we fail to update the
gitconfig as expected.
Reorder the code to first update all local references, and only after we
have done so update the upstream branch information. This improves the
code flow and furthermore makes it easier to refactor the way we update
references together.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6f5e157863..904ca9f1ca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1536,7 +1536,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct ref *ref_map;
+ struct ref *ref_map = NULL;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
@@ -1620,11 +1620,24 @@ static int do_fetch(struct transport *transport,
retcode = 1;
}
if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
- free_refs(ref_map);
retcode = 1;
goto cleanup;
}
+ /*
+ * If neither --no-tags nor --tags was specified, do automated tag
+ * following.
+ */
+ if (tags == TAGS_DEFAULT && autotags) {
+ struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
+
+ find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+ if (tags_ref_map)
+ backfill_tags(transport, tags_ref_map, worktrees);
+
+ free_refs(tags_ref_map);
+ }
+
if (set_upstream) {
struct branch *branch = branch_get("HEAD");
struct ref *rm;
@@ -1644,7 +1657,7 @@ static int do_fetch(struct transport *transport,
if (!rm->peer_ref) {
if (source_ref) {
warning(_("multiple branches detected, incompatible with --set-upstream"));
- goto skip;
+ goto cleanup;
} else {
source_ref = rm;
}
@@ -1658,7 +1671,7 @@ static int do_fetch(struct transport *transport,
warning(_("could not set upstream of HEAD to '%s' from '%s' when "
"it does not point to any branch."),
shortname, transport->remote->name);
- goto skip;
+ goto cleanup;
}
if (!strcmp(source_ref->name, "HEAD") ||
@@ -1678,21 +1691,9 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
-skip:
- free_refs(ref_map);
-
- /* if neither --no-tags nor --tags was specified, do automated tag
- * following ... */
- if (tags == TAGS_DEFAULT && autotags) {
- struct ref **tail = &ref_map;
- ref_map = NULL;
- find_non_local_tags(remote_refs, &ref_map, &tail);
- if (ref_map)
- backfill_tags(transport, ref_map, worktrees);
- free_refs(ref_map);
- }
cleanup:
+ free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
}
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 3/7] fetch: control lifecycle of FETCH_HEAD in a single place
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 1/7] fetch: increase test coverage of fetches Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 2/7] fetch: backfill tags before setting upstream Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 4/7] fetch: report errors when backfilling tags fails Patrick Steinhardt
` (3 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 5634 bytes --]
There are two different locations where we're appending to FETCH_HEAD:
first when storing updated references, and second when backfilling tags.
Both times we open the file, append to it and then commit it into place,
which is essentially duplicate work.
Improve the lifecycle of updating FETCH_HEAD by opening and committing
it once in `do_fetch()`, where we pass the structure down to the code
which wants to append to it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 904ca9f1ca..f8adb40b45 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1084,9 +1084,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
static int store_updated_refs(const char *raw_url, const char *remote_name,
int connectivity_checked, struct ref *ref_map,
- struct worktree **worktrees)
+ struct fetch_head *fetch_head, struct worktree **worktrees)
{
- struct fetch_head fetch_head;
int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
struct ref_transaction *transaction = NULL;
@@ -1096,10 +1095,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
int want_status;
int summary_width = transport_summary_width(ref_map);
- rc = open_fetch_head(&fetch_head);
- if (rc)
- return -1;
-
if (raw_url)
url = transport_anonymize_url(raw_url);
else
@@ -1206,7 +1201,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_addf(¬e, "'%s' of ", what);
}
- append_fetch_head(&fetch_head, &rm->old_oid,
+ append_fetch_head(fetch_head, &rm->old_oid,
rm->fetch_head_status,
note.buf, url, url_len);
@@ -1246,9 +1241,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (!rc)
- commit_fetch_head(&fetch_head);
-
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
" 'git remote prune %s' to remove any old, conflicting "
@@ -1268,7 +1260,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_release(&err);
ref_transaction_free(transaction);
free(url);
- close_fetch_head(&fetch_head);
return rc;
}
@@ -1309,6 +1300,7 @@ static int check_exist_and_connected(struct ref *ref_map)
static int fetch_and_consume_refs(struct transport *transport,
struct ref *ref_map,
+ struct fetch_head *fetch_head,
struct worktree **worktrees)
{
int connectivity_checked = 1;
@@ -1331,7 +1323,7 @@ static int fetch_and_consume_refs(struct transport *transport,
trace2_region_enter("fetch", "consume_refs", the_repository);
ret = store_updated_refs(transport->url, transport->remote->name,
- connectivity_checked, ref_map, worktrees);
+ connectivity_checked, ref_map, fetch_head, worktrees);
trace2_region_leave("fetch", "consume_refs", the_repository);
out:
@@ -1503,7 +1495,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
return transport;
}
-static void backfill_tags(struct transport *transport, struct ref *ref_map,
+static void backfill_tags(struct transport *transport,
+ struct ref *ref_map,
+ struct fetch_head *fetch_head,
struct worktree **worktrees)
{
int cannot_reuse;
@@ -1525,7 +1519,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_and_consume_refs(transport, ref_map, worktrees);
+ fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1544,6 +1538,7 @@ static int do_fetch(struct transport *transport,
TRANSPORT_LS_REFS_OPTIONS_INIT;
int must_list_refs = 1;
struct worktree **worktrees = get_worktrees();
+ struct fetch_head fetch_head = { 0 };
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1601,6 +1596,10 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map, worktrees);
+ retcode = open_fetch_head(&fetch_head);
+ if (retcode)
+ goto cleanup;
+
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1619,7 +1618,8 @@ static int do_fetch(struct transport *transport,
if (retcode != 0)
retcode = 1;
}
- if (fetch_and_consume_refs(transport, ref_map, worktrees)) {
+
+ if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
retcode = 1;
goto cleanup;
}
@@ -1633,11 +1633,13 @@ static int do_fetch(struct transport *transport,
find_non_local_tags(remote_refs, &tags_ref_map, &tail);
if (tags_ref_map)
- backfill_tags(transport, tags_ref_map, worktrees);
+ backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
free_refs(tags_ref_map);
}
+ commit_fetch_head(&fetch_head);
+
if (set_upstream) {
struct branch *branch = branch_get("HEAD");
struct ref *rm;
@@ -1693,6 +1695,7 @@ static int do_fetch(struct transport *transport,
}
cleanup:
+ close_fetch_head(&fetch_head);
free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 4/7] fetch: report errors when backfilling tags fails
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
` (2 preceding siblings ...)
2022-02-21 8:02 ` [PATCH v3 3/7] fetch: control lifecycle of FETCH_HEAD in a single place Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 5/7] refs: add interface to iterate over queued transactional updates Patrick Steinhardt
` (2 subsequent siblings)
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]
When the backfilling of tags fails we do not report this error to the
caller, but only report it implicitly at a later point when reporting
updated references. This leaves callers unable to act upon the
information of whether the backfilling succeeded or not.
Refactor the function to return an error code and pass it up the
callstack. This causes us to correctly propagate the error back to the
user of git-fetch(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 26 ++++++++++++++++++--------
t/t5503-tagfollow.sh | 4 +---
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f8adb40b45..d304314f16 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
return transport;
}
-static void backfill_tags(struct transport *transport,
- struct ref *ref_map,
- struct fetch_head *fetch_head,
- struct worktree **worktrees)
+static int backfill_tags(struct transport *transport,
+ struct ref *ref_map,
+ struct fetch_head *fetch_head,
+ struct worktree **worktrees)
{
- int cannot_reuse;
+ int retcode, cannot_reuse;
/*
* Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it
@@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+ retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
gsecondary = NULL;
}
+
+ return retcode;
}
static int do_fetch(struct transport *transport,
@@ -1632,8 +1634,16 @@ static int do_fetch(struct transport *transport,
struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
find_non_local_tags(remote_refs, &tags_ref_map, &tail);
- if (tags_ref_map)
- backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
+ if (tags_ref_map) {
+ /*
+ * If backfilling of tags fails then we want to tell
+ * the user so, but we have to continue regardless to
+ * populate upstream information of the references we
+ * have already fetched above.
+ */
+ if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+ retcode = 1;
+ }
free_refs(tags_ref_map);
}
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 7dadaafd4b..27a1dfa3c0 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -233,9 +233,7 @@ test_expect_success 'backfill failure causes command to fail' '
done
EOF
- # Even though we fail to create refs/tags/tag1 the below command
- # unexpectedly succeeds.
- git -C clone5 fetch .. $B:refs/heads/something &&
+ test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
test $S = $(git -C clone5 rev-parse --verify tag2) &&
test_must_fail git -C clone5 rev-parse --verify tag1
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 5/7] refs: add interface to iterate over queued transactional updates
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
` (3 preceding siblings ...)
2022-02-21 8:02 ` [PATCH v3 4/7] fetch: report errors when backfilling tags fails Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 6/7] fetch: make `--atomic` flag cover backfilling of tags Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 7/7] fetch: make `--atomic` flag cover pruning of refs Patrick Steinhardt
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]
There is no way for a caller to see whether a reference update has
already been queued up for a given reference transaction. There are
multiple alternatives to provide this functionality:
- We may add a function that simply tells us whether a specific
reference has already been queued. If implemented naively then
this would potentially be quadratic in runtime behaviour if this
question is asked repeatedly because we have to iterate over all
references every time. The alternative would be to add a hashmap
of all queued reference updates to speed up the lookup, but this
adds overhead to all callers.
- We may add a flag to `ref_transaction_add_update()` that causes it
to skip duplicates, but this has the same runtime concerns as the
first alternative.
- We may add an interface which lets callers collect all updates
which have already been queued such that he can avoid re-adding
them. This is the most flexible approach and puts the burden on
the caller, but also allows us to not impact any of the existing
callsites which don't need this information.
This commit implements the last approach: it allows us to compute the
map of already-queued updates once up front such that we can then skip
all subsequent references which are already part of this map.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 16 ++++++++++++++++
refs.h | 14 ++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/refs.c b/refs.c
index d680de3bc0..35d4e69687 100644
--- a/refs.c
+++ b/refs.c
@@ -2417,6 +2417,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
return refs->be->initial_transaction_commit(refs, transaction, err);
}
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+ ref_transaction_for_each_queued_update_fn cb,
+ void *cb_data)
+{
+ int i;
+
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+
+ cb(update->refname,
+ (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,
+ (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,
+ cb_data);
+ }
+}
+
int refs_delete_refs(struct ref_store *refs, const char *logmsg,
struct string_list *refnames, unsigned int flags)
{
diff --git a/refs.h b/refs.h
index ff859d5951..1ae12c410a 100644
--- a/refs.h
+++ b/refs.h
@@ -776,6 +776,20 @@ int ref_transaction_abort(struct ref_transaction *transaction,
int initial_ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
+/*
+ * Execute the given callback function for each of the reference updates which
+ * have been queued in the given transaction. `old_oid` and `new_oid` may be
+ * `NULL` pointers depending on whether the update has these object IDs set or
+ * not.
+ */
+typedef void ref_transaction_for_each_queued_update_fn(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ void *cb_data);
+void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
+ ref_transaction_for_each_queued_update_fn cb,
+ void *cb_data);
+
/*
* Free `*transaction` and all associated data.
*/
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 6/7] fetch: make `--atomic` flag cover backfilling of tags
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
` (4 preceding siblings ...)
2022-02-21 8:02 ` [PATCH v3 5/7] refs: add interface to iterate over queued transactional updates Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
2022-02-21 8:02 ` [PATCH v3 7/7] fetch: make `--atomic` flag cover pruning of refs Patrick Steinhardt
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 10780 bytes --]
When fetching references from a remote we by default also fetch all tags
which point into the history we have fetched. This is a separate step
performed after updating local references because it requires us to walk
over the history on the client-side to determine whether the remote has
announced any tags which point to one of the fetched commits.
This backfilling of tags isn't covered by the `--atomic` flag: right
now, it only applies to the step where we update our local references.
This is an oversight at the time the flag was introduced: its purpose is
to either update all references or none, but right now we happily update
local references even in the case where backfilling failed.
Fix this by pulling up creation of the reference transaction such that
we can pass the same transaction to both the code which updates local
references and to the code which backfills tags. This allows us to only
commit the transaction in case both actions succeed.
Note that we also have to start passing the transaction into
`find_non_local_tags()`: this function is responsible for finding all
tags which we need to backfill. Right now, it will happily return tags
which have already been updated with our local references. But when we
use a single transaction for both local references and backfilling then
it may happen that we try to queue the same reference update twice to
the transaction, which consequently triggers a bug. We thus have to skip
over any tags which have already been queued.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 92 +++++++++++++++++++++++++++++++-------------
t/t5503-tagfollow.sh | 11 ++----
2 files changed, 69 insertions(+), 34 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index d304314f16..67af842091 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)
item->ignore = 1;
}
+
+static void add_already_queued_tags(const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid,
+ void *cb_data)
+{
+ struct hashmap *queued_tags = cb_data;
+ if (starts_with(refname, "refs/tags/") && new_oid)
+ (void) refname_hash_add(queued_tags, refname, new_oid);
+}
+
static void find_non_local_tags(const struct ref *refs,
+ struct ref_transaction *transaction,
struct ref **head,
struct ref ***tail)
{
@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *refs,
create_fetch_oidset(head, &fetch_oids);
for_each_ref(add_one_refname, &existing_refs);
+
+ /*
+ * If we already have a transaction, then we need to filter out all
+ * tags which have already been queued up.
+ */
+ if (transaction)
+ ref_transaction_for_each_queued_update(transaction,
+ add_already_queued_tags,
+ &existing_refs);
+
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -600,7 +622,7 @@ static struct ref *get_ref_map(struct remote *remote,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, &tail, 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(remote_refs, &ref_map, &tail);
+ find_non_local_tags(remote_refs, NULL, &ref_map, &tail);
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1083,12 +1105,12 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
"to avoid this check\n");
static int store_updated_refs(const char *raw_url, const char *remote_name,
- int connectivity_checked, struct ref *ref_map,
+ int connectivity_checked,
+ struct ref_transaction *transaction, struct ref *ref_map,
struct fetch_head *fetch_head, struct worktree **worktrees)
{
int url_len, i, rc = 0;
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
- struct ref_transaction *transaction = NULL;
const char *what, *kind;
struct ref *rm;
char *url;
@@ -1110,14 +1132,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (atomic_fetch) {
- transaction = ref_transaction_begin(&err);
- if (!transaction) {
- error("%s", err.buf);
- goto abort;
- }
- }
-
prepare_format_display(ref_map);
/*
@@ -1233,14 +1247,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
}
}
- if (!rc && transaction) {
- rc = ref_transaction_commit(transaction, &err);
- if (rc) {
- error("%s", err.buf);
- goto abort;
- }
- }
-
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
" 'git remote prune %s' to remove any old, conflicting "
@@ -1258,7 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
abort:
strbuf_release(¬e);
strbuf_release(&err);
- ref_transaction_free(transaction);
free(url);
return rc;
}
@@ -1299,6 +1304,7 @@ static int check_exist_and_connected(struct ref *ref_map)
}
static int fetch_and_consume_refs(struct transport *transport,
+ struct ref_transaction *transaction,
struct ref *ref_map,
struct fetch_head *fetch_head,
struct worktree **worktrees)
@@ -1323,7 +1329,8 @@ static int fetch_and_consume_refs(struct transport *transport,
trace2_region_enter("fetch", "consume_refs", the_repository);
ret = store_updated_refs(transport->url, transport->remote->name,
- connectivity_checked, ref_map, fetch_head, worktrees);
+ connectivity_checked, transaction, ref_map,
+ fetch_head, worktrees);
trace2_region_leave("fetch", "consume_refs", the_repository);
out:
@@ -1496,6 +1503,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
}
static int backfill_tags(struct transport *transport,
+ struct ref_transaction *transaction,
struct ref *ref_map,
struct fetch_head *fetch_head,
struct worktree **worktrees)
@@ -1519,7 +1527,7 @@ static int backfill_tags(struct transport *transport,
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
+ retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1532,6 +1540,7 @@ static int backfill_tags(struct transport *transport,
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
+ struct ref_transaction *transaction = NULL;
struct ref *ref_map = NULL;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
@@ -1541,6 +1550,7 @@ static int do_fetch(struct transport *transport,
int must_list_refs = 1;
struct worktree **worktrees = get_worktrees();
struct fetch_head fetch_head = { 0 };
+ struct strbuf err = STRBUF_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1602,6 +1612,14 @@ static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
+ if (atomic_fetch) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ retcode = error("%s", err.buf);
+ goto cleanup;
+ }
+ }
+
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,
retcode = 1;
}
- if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
+ if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
retcode = 1;
goto cleanup;
}
@@ -1633,21 +1651,37 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT && autotags) {
struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
- find_non_local_tags(remote_refs, &tags_ref_map, &tail);
+ find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
if (tags_ref_map) {
/*
* If backfilling of tags fails then we want to tell
* the user so, but we have to continue regardless to
* populate upstream information of the references we
- * have already fetched above.
+ * have already fetched above. The exception though is
+ * when `--atomic` is passed: in that case we'll abort
+ * the transaction and don't commit anything.
*/
- if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
+ if (backfill_tags(transport, transaction, tags_ref_map,
+ &fetch_head, worktrees))
retcode = 1;
}
free_refs(tags_ref_map);
}
+ if (transaction) {
+ if (retcode)
+ goto cleanup;
+
+ retcode = ref_transaction_commit(transaction, &err);
+ if (retcode) {
+ error("%s", err.buf);
+ ref_transaction_free(transaction);
+ transaction = NULL;
+ goto cleanup;
+ }
+ }
+
commit_fetch_head(&fetch_head);
if (set_upstream) {
@@ -1705,7 +1739,13 @@ static int do_fetch(struct transport *transport,
}
cleanup:
+ if (retcode && transaction) {
+ ref_transaction_abort(transaction, &err);
+ error("%s", err.buf);
+ }
+
close_fetch_head(&fetch_head);
+ strbuf_release(&err);
free_refs(ref_map);
free_worktrees(worktrees);
return retcode;
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 27a1dfa3c0..fe23cad4ce 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -180,11 +180,8 @@ test_expect_success 'atomic fetch with failing backfill' '
EOF
test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
-
- # Creation of the tag has failed, so ideally refs/heads/something
- # should not exist. The fact that it does demonstrates that there is
- # a bug in the `--atomic` flag.
- test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
+ test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
+ test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
'
test_expect_success 'atomic fetch with backfill should use single transaction' '
@@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
prepared
$ZERO_OID $B refs/heads/something
$ZERO_OID $S refs/tags/tag2
+ $ZERO_OID $T refs/tags/tag1
committed
$ZERO_OID $B refs/heads/something
$ZERO_OID $S refs/tags/tag2
- prepared
- $ZERO_OID $T refs/tags/tag1
- committed
$ZERO_OID $T refs/tags/tag1
EOF
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 7/7] fetch: make `--atomic` flag cover pruning of refs
2022-02-21 8:02 ` [PATCH v3 " Patrick Steinhardt
` (5 preceding siblings ...)
2022-02-21 8:02 ` [PATCH v3 6/7] fetch: make `--atomic` flag cover backfilling of tags Patrick Steinhardt
@ 2022-02-21 8:02 ` Patrick Steinhardt
6 siblings, 0 replies; 35+ messages in thread
From: Patrick Steinhardt @ 2022-02-21 8:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Jonathan Tan, Elijah Newren, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 5171 bytes --]
When fetching with the `--prune` flag we will delete any local
references matching the fetch refspec which have disappeared on the
remote. This step is not currently covered by the `--atomic` flag: we
delete branches even though updating of local references has failed,
which means that the fetch is not an all-or-nothing operation.
Fix this bug by passing in the global transaction into `prune_refs()`:
if one is given, then we'll only queue up deletions and not commit them
right away.
This change also improves performance when pruning many branches in a
repository with a big packed-refs file: every references is pruned in
its own transaction, which means that we potentially have to rewrite
the packed-refs files for every single reference we're about to prune.
The following benchmark demonstrates this: it performs a pruning fetch
from a repository with a single reference into a repository with 100k
references, which causes us to prune all but one reference. This is of
course a very artificial setup, but serves to demonstrate the impact of
only having to write the packed-refs file once:
Benchmark 1: git fetch --prune --atomic +refs/*:refs/* (HEAD~)
Time (mean ± σ): 2.366 s ± 0.021 s [User: 0.858 s, System: 1.508 s]
Range (min … max): 2.328 s … 2.407 s 10 runs
Benchmark 2: git fetch --prune --atomic +refs/*:refs/* (HEAD)
Time (mean ± σ): 1.369 s ± 0.017 s [User: 0.715 s, System: 0.641 s]
Range (min … max): 1.346 s … 1.400 s 10 runs
Summary
'git fetch --prune --atomic +refs/*:refs/* (HEAD)' ran
1.73 ± 0.03 times faster than 'git fetch --prune --atomic +refs/*:refs/* (HEAD~)'
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fetch.c | 30 ++++++++++++++++++++++--------
t/t5510-fetch.sh | 8 ++------
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67af842091..9a2b5c03a4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1338,11 +1338,14 @@ static int fetch_and_consume_refs(struct transport *transport,
return ret;
}
-static int prune_refs(struct refspec *rs, struct ref *ref_map,
+static int prune_refs(struct refspec *rs,
+ struct ref_transaction *transaction,
+ struct ref *ref_map,
const char *raw_url)
{
int url_len, i, result = 0;
struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
+ struct strbuf err = STRBUF_INIT;
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
@@ -1363,13 +1366,22 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
url_len = i - 3;
if (!dry_run) {
- struct string_list refnames = STRING_LIST_INIT_NODUP;
+ if (transaction) {
+ for (ref = stale_refs; ref; ref = ref->next) {
+ result = ref_transaction_delete(transaction, ref->name, NULL, 0,
+ "fetch: prune", &err);
+ if (result)
+ goto cleanup;
+ }
+ } else {
+ struct string_list refnames = STRING_LIST_INIT_NODUP;
- for (ref = stale_refs; ref; ref = ref->next)
- string_list_append(&refnames, ref->name);
+ for (ref = stale_refs; ref; ref = ref->next)
+ string_list_append(&refnames, ref->name);
- result = delete_refs("fetch: prune", &refnames, 0);
- string_list_clear(&refnames, 0);
+ result = delete_refs("fetch: prune", &refnames, 0);
+ string_list_clear(&refnames, 0);
+ }
}
if (verbosity >= 0) {
@@ -1388,6 +1400,8 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
}
}
+cleanup:
+ strbuf_release(&err);
free(url);
free_refs(stale_refs);
return result;
@@ -1629,10 +1643,10 @@ static int do_fetch(struct transport *transport,
* don't care whether --tags was specified.
*/
if (rs->nr) {
- retcode = prune_refs(rs, ref_map, transport->url);
+ retcode = prune_refs(rs, transaction, ref_map, transport->url);
} else {
retcode = prune_refs(&transport->remote->fetch,
- ref_map,
+ transaction, ref_map,
transport->url);
}
if (retcode != 0)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 70d51f343b..48e14e2dab 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -354,17 +354,13 @@ test_expect_success 'fetch --atomic --prune executes a single reference transact
head_oid=$(git rev-parse HEAD) &&
# Fetching with the `--atomic` flag should update all references in a
- # single transaction. It is currently missing coverage of pruned
- # references though, and as a result those may be committed to disk
- # even if updating references fails later.
+ # single transaction.
cat >expected <<-EOF &&
prepared
$ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
- committed
- $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
- prepared
$ZERO_OID $head_oid refs/remotes/origin/new-branch
committed
+ $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion
$ZERO_OID $head_oid refs/remotes/origin/new-branch
EOF
--
2.35.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 35+ messages in thread