| Message ID | 20181016215850.47821-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | upload-pack: clear flags before each v2 request | expand |
Hello, I can confirm that shallow clones are no longer failing. -- Arturas Moskvinas On Wed, Oct 17, 2018 at 12:58 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > Suppose a server has the following commit graph: > > A B > \ / > O > > We create a client by cloning A from the server with depth 1, and add > many commits to it (so that future fetches span multiple requests due to > lengthy negotiation). If it then fetches B using protocol v2, the fetch > spanning multiple requests, the resulting packfile does not contain O > even though the client did report that A is shallow. > > This is because upload_pack_v2() can be called multiple times while > processing the same session. During the 2nd and all subsequent > invocations, some object flags remain from the previous invocations. In > particular, CLIENT_SHALLOW remains, preventing process_shallow() from > adding client-reported shallows to the "shallows" array, and hence > pack-objects not knowing about these client-reported shallows. > > Therefore, teach upload_pack_v2() to clear object flags at the start of > each invocation. > > (One alternative is to reduce or eliminate usage of object flags in > protocol v2, but that doesn't seem feasible because almost all 8 flags > are used pervasively in v2 code.) > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This was noticed by Arturas Moskvinas <arturas@uber.com> in [1]. The > reproduction steps given were to repeat a shallow fetch twice in > succession, but I found it easier to write a more understandable test if > I made the 2nd fetch an ordinary fetch. In any case, I also reran the > original reproduction steps, and the fetch completes without error. > > This patch doesn't cover the negotiation issue that I mentioned in my > previous reply [2]. > > [1] https://public-inbox.org/git/CAGY-PBgsG-T3JY=awszwGMpFx+jDx-a1FcV0s6vR067bSqgZaA@mail.gmail.com/ > [2] https://public-inbox.org/git/20181013004356.257709-1-jonathantanmy@google.com/ > --- > t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++ > upload-pack.c | 5 +++++ > 2 files changed, 30 insertions(+) > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 88a886975d..70b88385ba 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' ' > git -C client cat-file -e $(git -C client rev-parse annotated_tag) > ' > > +test_expect_success 'upload-pack respects client shallows' ' > + rm -rf server client trace && > + > + git init server && > + test_commit -C server base && > + test_commit -C server client_has && > + > + git clone --depth=1 "file://$(pwd)/server" client && > + > + # Add extra commits to the client so that the whole fetch takes more > + # than 1 request (due to negotiation) > + for i in $(seq 1 32) > + do > + test_commit -C client c$i > + done && > + > + git -C server checkout -b newbranch base && > + test_commit -C server client_wants && > + > + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \ > + fetch origin newbranch && > + # Ensure that protocol v2 is used > + grep "git< version 2" trace > +' > + > # Test protocol v2 with 'http://' transport > # > . "$TEST_DIRECTORY"/lib-httpd.sh > diff --git a/upload-pack.c b/upload-pack.c > index 62a1000f44..de7de1de38 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -37,6 +37,9 @@ > #define CLIENT_SHALLOW (1u << 18) > #define HIDDEN_REF (1u << 19) > > +#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \ > + NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF) > + > static timestamp_t oldest_have; > > static int deepen_relative; > @@ -1393,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys, > enum fetch_state state = FETCH_PROCESS_ARGS; > struct upload_pack_data data; > > + clear_object_flags(ALL_FLAGS); > + > git_config(upload_pack_config, NULL); > > upload_pack_data_init(&data); > -- > 2.19.0.271.gfe8321ec05.dirty >
Jonathan Tan <jonathantanmy@google.com> writes: > Suppose a server has the following commit graph: > > A B > \ / > O > > We create a client by cloning A from the server with depth 1, and add > many commits to it (so that future fetches span multiple requests due to > lengthy negotiation). If it then fetches B using protocol v2, the fetch > spanning multiple requests, the resulting packfile does not contain O > even though the client did report that A is shallow. Hmph, what if commit O had a long history behind it? Should fetching of B result in fetching the whole history? Would we notice that now all of A's parents are available locally and declare that the repository is no longer shallow? I am trying to figure out if "does not contain O when we fetch B, even though we earlier fetched A shallowly, excluding its parents" is unconditionally a bad thing. The change to the code itself sort-of makes sense (I say sort-of because I didn't carefully look at the callers to see if they mind getting all these flags cleared, but the ones that are cleared are the ones that are involved mostly in the negotiation and shold be OK). > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 88a886975d..70b88385ba 100755 > --- a/t/t5702-protocol-v2.sh > +++ b/t/t5702-protocol-v2.sh > @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' ' > git -C client cat-file -e $(git -C client rev-parse annotated_tag) > ' > > +test_expect_success 'upload-pack respects client shallows' ' > + rm -rf server client trace && > + > + git init server && > + test_commit -C server base && > + test_commit -C server client_has && > + > + git clone --depth=1 "file://$(pwd)/server" client && > + > + # Add extra commits to the client so that the whole fetch takes more > + # than 1 request (due to negotiation) > + for i in $(seq 1 32) Use test_seq instead, or you'll get hit by test-lint? Applied on 'master' or 'maint', this new test does not pass even with s/seq/test_&/, so there may be something else wrong with it, though.
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 88a886975d..70b88385ba 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' ' git -C client cat-file -e $(git -C client rev-parse annotated_tag) ' +test_expect_success 'upload-pack respects client shallows' ' + rm -rf server client trace && + + git init server && + test_commit -C server base && + test_commit -C server client_has && + + git clone --depth=1 "file://$(pwd)/server" client && + + # Add extra commits to the client so that the whole fetch takes more + # than 1 request (due to negotiation) + for i in $(seq 1 32) + do + test_commit -C client c$i + done && + + git -C server checkout -b newbranch base && + test_commit -C server client_wants && + + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \ + fetch origin newbranch && + # Ensure that protocol v2 is used + grep "git< version 2" trace +' + # Test protocol v2 with 'http://' transport # . "$TEST_DIRECTORY"/lib-httpd.sh diff --git a/upload-pack.c b/upload-pack.c index 62a1000f44..de7de1de38 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -37,6 +37,9 @@ #define CLIENT_SHALLOW (1u << 18) #define HIDDEN_REF (1u << 19) +#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \ + NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF) + static timestamp_t oldest_have; static int deepen_relative; @@ -1393,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys, enum fetch_state state = FETCH_PROCESS_ARGS; struct upload_pack_data data; + clear_object_flags(ALL_FLAGS); + git_config(upload_pack_config, NULL); upload_pack_data_init(&data);
Suppose a server has the following commit graph: A B \ / O We create a client by cloning A from the server with depth 1, and add many commits to it (so that future fetches span multiple requests due to lengthy negotiation). If it then fetches B using protocol v2, the fetch spanning multiple requests, the resulting packfile does not contain O even though the client did report that A is shallow. This is because upload_pack_v2() can be called multiple times while processing the same session. During the 2nd and all subsequent invocations, some object flags remain from the previous invocations. In particular, CLIENT_SHALLOW remains, preventing process_shallow() from adding client-reported shallows to the "shallows" array, and hence pack-objects not knowing about these client-reported shallows. Therefore, teach upload_pack_v2() to clear object flags at the start of each invocation. (One alternative is to reduce or eliminate usage of object flags in protocol v2, but that doesn't seem feasible because almost all 8 flags are used pervasively in v2 code.) Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This was noticed by Arturas Moskvinas <arturas@uber.com> in [1]. The reproduction steps given were to repeat a shallow fetch twice in succession, but I found it easier to write a more understandable test if I made the 2nd fetch an ordinary fetch. In any case, I also reran the original reproduction steps, and the fetch completes without error. This patch doesn't cover the negotiation issue that I mentioned in my previous reply [2]. [1] https://public-inbox.org/git/CAGY-PBgsG-T3JY=awszwGMpFx+jDx-a1FcV0s6vR067bSqgZaA@mail.gmail.com/ [2] https://public-inbox.org/git/20181013004356.257709-1-jonathantanmy@google.com/ --- t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++ upload-pack.c | 5 +++++ 2 files changed, 30 insertions(+)