Document two partial clone bugs, fix one #556
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
changed the title
|
/submit |
|
Submitted as [email protected] |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -335,6 +335,7 @@ static void find_non_local_tags(const struct ref *refs, | |||
| struct string_list_item *remote_ref_item; | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
> From: Derrick Stolee <[email protected]>
>
> When using partial-clone, do_oid_object_info_extended() can trigger a
> fetch for missing objects. This can be extremely expensive when asking
> for a tag or commit, as we are completely removed from the context of
> the missing object and thus supply no "haves" in the request.
>
> 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a
> global variable that prevented these fetches in favor of a bitflag.
> However, some object existence checks were not updated to use this flag.
>
> Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in
> addition to OBJECT_INFO_QUICK. The _QUICK option only prevents
> repreparing the pack-file structures. We need to be extremely careful
> about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist
> due to updated refs.
>
> This resolves a broken test in t5616-partial-clone.sh.
>
> Signed-off-by: Derrick Stolee <[email protected]>
Thanks for catching this.
I wonder if the commit message in this patch could be better worded -
the first paragraph seems to say that fetching missing commits and tags
are expensive, but that is not the problem here; the problem is that the
client lazily fetches refs advertised by the server, thinking that it is
lacking them due to partial clone, even when there is no expectation
that the client have them (so the commits and tags are not truly
missing).
So I would reword the first paragraph as:
When using partial clone, find_non_local_tags() in builtin/fetch.c
checks each remote tag to see if its object also exists locally. There
is no expectation that the object exist locally, but this function
nevertheless triggers a lazy fetch if the object does not exist. This
can be extremely expensive when asking for a commit, as we are
completely removed from the context of the non-existent object and
thus supply no "haves" in the request.
All this rests on my thinking that "missing" has the connotation (or
actual meaning) that we expect the object to be there. If we think that
"missing" can also mean that the remote has it but the local doesn't,
then you can ignore what I just said :-)
Other than that, both patches look good to me.
| @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' ' | |||
| grep "want $(cat hash)" trace | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Wed, Feb 19, 2020 at 11:22 AM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
> [...]
> The tests are ordered in this way because if I swap the test order the
> tag test will succeed instead of fail. I believe this is because somehow
> we need the srv.bare repo to not have any tags when we clone, but then
> have tags in our next fetch.
This ordering requirement might deserve an in-code comment in the test
script itself.
More below...
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
> +test_expect_failure 'verify fetch succeeds when asking for new tags' '
> + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
> + for i in I J K
> + do
> + test_commit -C src $i &&
> + git -C src branch $i
> + done &&
If test_commit() or git-branch fail, those failures will go unnoticed.
You can fix this by bailing from the loop, like this:
for i in I J K
do
test_commit -C src $i &&
git -C src branch $i || return 1
done &&
Same comment applies to the other new test.
> + git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
> + git -C tag-test fetch --tags origin
> +'
> +
> +test_expect_failure 'verify fetch downloads only one pack when updating refs' '
> + git clone --filter=blob:none "file://$(pwd)/srv.bare" pack-test &&
> + ls pack-test/.git/objects/pack/*pack >pack-list &&
> + test_line_count = 2 pack-list &&
> + for i in A B C
> + do
> + test_commit -C src $i &&
> + git -C src branch $i
> + done &&
> + git -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&
> + git -C pack-test fetch origin &&
> + ls pack-test/.git/objects/pack/*pack >pack-list &&
> + test_line_count = 3 pack-list
> +'
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/19/2020 1:38 PM, Eric Sunshine wrote:
> On Wed, Feb 19, 2020 at 11:22 AM Derrick Stolee via GitGitGadget
> <[email protected]> wrote:
>> [...]
>> The tests are ordered in this way because if I swap the test order the
>> tag test will succeed instead of fail. I believe this is because somehow
>> we need the srv.bare repo to not have any tags when we clone, but then
>> have tags in our next fetch.
>
> This ordering requirement might deserve an in-code comment in the test
> script itself.
Can do.
> More below...
>
>> Signed-off-by: Derrick Stolee <[email protected]>
>> ---
>> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
>> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
>> +test_expect_failure 'verify fetch succeeds when asking for new tags' '
>> + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
>> + for i in I J K
>> + do
>> + test_commit -C src $i &&
>> + git -C src branch $i
>> + done &&
>
> If test_commit() or git-branch fail, those failures will go unnoticed.
> You can fix this by bailing from the loop, like this:
>
> for i in I J K
> do
> test_commit -C src $i &&
> git -C src branch $i || return 1
> done &&
>
> Same comment applies to the other new test.
Thanks!
-Stolee
| @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' ' | |||
| grep "want $(cat hash)" trace | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index fea56cda6d3..ed2ef45c37a 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
> grep "want $(cat hash)" trace
> '
>
> +test_expect_failure 'verify fetch succeeds when asking for new tags' '
> + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
> + for i in I J K
> + do
> + test_commit -C src $i &&
> + git -C src branch $i
> + done &&
> + git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
> + git -C tag-test fetch --tags origin
> +'
Is this about an ultra-recent regresssion? When applied directly on
top of v2.25.0, this one seems to pass already without any change.
> +test_expect_failure 'verify fetch downloads only one pack when updating refs' '
> + git clone --filter=blob:none "file://$(pwd)/srv.bare" pack-test &&
> + ls pack-test/.git/objects/pack/*pack >pack-list &&
> + test_line_count = 2 pack-list &&
> + for i in A B C
> + do
> + test_commit -C src $i &&
> + git -C src branch $i
> + done &&
> + git -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&
> + git -C pack-test fetch origin &&
> + ls pack-test/.git/objects/pack/*pack >pack-list &&
> + test_line_count = 3 pack-list
> +'
> +
> . "$TEST_DIRECTORY"/lib-httpd.sh
> start_httpd
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <[email protected]> wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> > +test_expect_failure 'verify fetch succeeds when asking for new tags' '
> > + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
> > + for i in I J K
> > + do
> > + test_commit -C src $i &&
> > + git -C src branch $i
> > + done &&
> > + git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
> > + git -C tag-test fetch --tags origin
> > +'
>
> Is this about an ultra-recent regresssion? When applied directly on
> top of v2.25.0, this one seems to pass already without any change.
True, although both fail when applied atop "master".
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <[email protected]> writes:
> On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <[email protected]> wrote:
>> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>> > +test_expect_failure 'verify fetch succeeds when asking for new tags' '
>> > + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
>> > + for i in I J K
>> > + do
>> > + test_commit -C src $i &&
>> > + git -C src branch $i
>> > + done &&
>> > + git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
>> > + git -C tag-test fetch --tags origin
>> > +'
>>
>> Is this about an ultra-recent regresssion? When applied directly on
>> top of v2.25.0, this one seems to pass already without any change.
>
> True, although both fail when applied atop "master".
I flipped the first one (i.e. test #24) to expect success and run
bisect between 3f7553ac ("Merge branch 'jt/t5616-robustify'",
2020-02-12) and the tip of 'master'.
Interesting that bisecting it points at 684ceae3 ("fetch: default to
protocol version 2", 2019-12-23).
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 9a9178fd28..099406c2f1 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -384,6 +384,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
grep "want $(cat hash)" trace
'
+test_expect_success 'verify fetch succeeds when asking for new tags' '
+ git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
+ for i in I J K
+ do
+ test_commit -C src $i &&
+ git -C src branch $i
+ done &&
+ git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
+ git -C tag-test fetch --tags origin
+'
+
+test_expect_failure 'verify fetch downloads only one pack when updating refs' '
+ git clone --filter=blob:none "file://$(pwd)/srv.bare" pack-test &&
+ ls pack-test/.git/objects/pack/*pack >pack-list &&
+ test_line_count = 2 pack-list &&
+ for i in A B C
+ do
+ test_commit -C src $i &&
+ git -C src branch $i
+ done &&
+ git -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&
+ git -C pack-test fetch origin &&
+ ls pack-test/.git/objects/pack/*pack >pack-list &&
+ test_line_count = 3 pack-list
+'
+
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
--
2.25.1-440-g39558b81cc
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/19/2020 4:17 PM, Junio C Hamano wrote:
> Eric Sunshine <[email protected]> writes:
>
>> On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <[email protected]> wrote:
>>> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>>>> +test_expect_failure 'verify fetch succeeds when asking for new tags' '
>>>> + git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
>>>> + for i in I J K
>>>> + do
>>>> + test_commit -C src $i &&
>>>> + git -C src branch $i
>>>> + done &&
>>>> + git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&
>>>> + git -C tag-test fetch --tags origin
>>>> +'
>>>
>>> Is this about an ultra-recent regresssion? When applied directly on
>>> top of v2.25.0, this one seems to pass already without any change.
>>
>> True, although both fail when applied atop "master".
>
> I flipped the first one (i.e. test #24) to expect success and run
> bisect between 3f7553ac ("Merge branch 'jt/t5616-robustify'",
> 2020-02-12) and the tip of 'master'.
>
> Interesting that bisecting it points at 684ceae3 ("fetch: default to
> protocol version 2", 2019-12-23).
Thanks for tracking this down. I had originally been working on
top of master, but then rebased onto v2.25.0 to test this on our
VFS for Git/Scalar fork [1]. I have since noticed that the test
passes in that case.
Thanks,
-Stolee
[1] https://github.com/microsoft/git/pull/247
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
|
This patch series was integrated into pu via git@c58cd7e. |
|
This branch is now known as |
|
This patch series was integrated into pu via git@65035e9. |
|
This patch series was integrated into pu via git@f80c967. |
While testing partial clone, I noticed some odd behavior. I was testing a way of running 'git init', followed by manually configuring the remote for partial clone, and then running 'git fetch'. Astonishingly, I saw the 'git fetch' process start asking the server for multiple rounds of pack-file downloads! When tweaking the situation a little more, I discovered that I could cause the remote to hang up with an error. Add two tests that demonstrate these two issues. In the first test, we find that when fetching with blob filters from a repository that previously did not have any tags, the 'git fetch --tags origin' command fails because the server sends "multiple filter-specs cannot be combined". This only happens when using protocol v2. In the second test, we see that a 'git fetch origin' request with several ref updates results in multiple pack-file downloads. This must be due to Git trying to fault-in the objects pointed by the refs. What makes this matter particularly nasty is that this goes through the do_oid_object_info_extended() method, so there are no "haves" in the negotiation. This leads the remote to send every reachable commit and tree from each new ref, providing a quadratic amount of data transfer! This test is fixed if we revert 6462d5e (fetch: remove fetch_if_missing=0, 2019-11-05), but that revert causes other test failures. The real fix will need more care. The tests are ordered in this way because if I swap the test order the tag test will succeed instead of fail. I believe this is because somehow we need the srv.bare repo to not have any tags when we clone, but then have tags in our next fetch. Signed-off-by: Derrick Stolee <[email protected]>
When using partial clone, find_non_local_tags() in builtin/fetch.c checks each remote tag to see if its object also exists locally. There is no expectation that the object exist locally, but this function nevertheless triggers a lazy fetch if the object does not exist. This can be extremely expensive when asking for a commit, as we are completely removed from the context of the non-existent object and thus supply no "haves" in the request. 6462d5e (fetch: remove fetch_if_missing=0, 2019-11-05) removed a global variable that prevented these fetches in favor of a bitflag. However, some object existence checks were not updated to use this flag. Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in addition to OBJECT_INFO_QUICK. The _QUICK option only prevents repreparing the pack-file structures. We need to be extremely careful about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist due to updated refs. This resolves a broken test in t5616-partial-clone.sh. Signed-off-by: Derrick Stolee <[email protected]>
derrickstolee
force-pushed
the
partial-clone-fix
branch
from
937a882 to
7c4c9f0
Compare
|
/submit |
|
Submitted as [email protected] |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into pu via git@0577552. |
|
This patch series was integrated into pu via git@ddebbbb. |
|
This patch series was integrated into next via git@a26434b. |
|
This patch series was integrated into pu via git@91b48d7. |
|
This patch series was integrated into pu via git@992e35d. |
|
This patch series was integrated into pu via git@444cff6. |
|
This patch series was integrated into next via git@444cff6. |
|
This patch series was integrated into master via git@444cff6. |
|
Closed via 444cff6. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While playing with partial clone, I discovered a few bugs and document them with tests in patch 1. One seems to be a server-side bug that happens in a somewhat rare situation, but not terribly unlikely. The other is a client-side bug that leads to quadratic amounts of data transfer; I fix this bug in patch 2.
UPDATES in V2:
Added "|| return 1" inside the for loops.
Added an in-test comment about the test ordering.
Required protocol.version=2 in the tags test due to the bisect Junio performed.
Updated the commit message via Jonathan Tan's suggestion.
You can ignore the stack traces I sent earlier, as those seem to be from states I cannot get into without being destructive to my .git directory.
Thanks,
-Stolee
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]