upload-pack: clear flags before each v2 request
diff mbox series

Message ID 20181016215850.47821-1-jonathantanmy@google.com
State New
Headers show
Series
  • upload-pack: clear flags before each v2 request
Related show

Commit Message

Jonathan Tan Oct. 16, 2018, 9:58 p.m. UTC
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(+)

Comments

Arturas Moskvinas Oct. 17, 2018, 12:05 p.m. UTC | #1
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
>
Junio C Hamano Oct. 18, 2018, 5:16 a.m. UTC | #2
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.

Patch
diff mbox series

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