{"thread":{"id":"50060","subject":"[PATCH] upload-pack: teach deepen-relative in protocol v2","startedAt":"2018-12-18T21:24:42Z","lastAt":"2019-01-10T22:54:34Z","messageCount":7,"participants":["Jonathan Tan","Josh Steadmon","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365553","messageId":"20181218212435.201641-1-jonathantanmy@google.com","threadId":"50060","inReplyTo":null,"subject":"[PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-18T21:24:35Z","receivedAt":"2018-12-18T21:24:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 685fbd3291 (\"fetch-pack: perform a fetch using v2\", 2018-03-15)\nattempted to teach Git deepen-relative in protocol v2 (among other\nthings), but it didn't work:\n\n (1) fetch-pack.c needs to emit \"deepen-relative\".\n (2) upload-pack.c needs to ensure that the correct deepen_relative\n     variable is passed to deepen() (there are two - the static variable\n     and the one in struct upload_pack_data).\n (3) Before deepen() computes the list of reachable shallows, it first\n     needs to mark all \"our\" refs as OUR_REF. v2 currently does not do\n     this, because unlike v0, it is not needed otherwise.\n\nFix all this and include a test demonstrating that it works now. For\n(2), the static variable deepen_relative is also eliminated, removing a\nsource of confusion.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nSimilar to my first fix [1], another fix for an issue discovered by\nAevar's GIT_TEST_PROTOCOL_VERSION patches. This one is more\nstraightforward.\n\nWith this fix and my first fix [1], t5500 no longer reveals any more\nbugs. (There might be more in other test files.)\n\n[1] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/\n---\n fetch-pack.c           |  2 ++\n t/t5702-protocol-v2.sh | 29 +++++++++++++++++++++++++++++\n upload-pack.c          | 17 +++++++++++++++--\n 3 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9691046e64..c383ea46b3 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1042,6 +1042,8 @@ static void add_shallow_requests(struct strbuf *req_buf,\n \t\t\tpacket_buf_write(req_buf, \"deepen-not %s\", s->string);\n \t\t}\n \t}\n+\tif (args->deepen_relative)\n+\t\tpacket_buf_write(req_buf, \"deepen-relative\\n\");\n }\n \n static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..340953f01c 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -471,6 +471,35 @@ test_expect_success 'upload-pack respects client shallows' '\n \tgrep \"fetch< version 2\" trace\n '\n \n+test_expect_success 'deepen-relative' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\ttest_commit -C server two &&\n+\ttest_commit -C server three &&\n+\tgit clone --depth 1 \"file://$(pwd)/server\" client &&\n+\ttest_commit -C server four &&\n+\n+\t# Sanity check that only \"three\" is downloaded\n+\tgit -C client log --pretty=tformat:%s master >actual &&\n+\techo three >expected &&\n+\ttest_cmp expected actual &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch --deepen=1 origin &&\n+\t# Ensure that protocol v2 is used\n+\tgrep \"fetch< version 2\" trace &&\n+\n+\tgit -C client log --pretty=tformat:%s origin/master >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tfour\n+\tthree\n+\ttwo\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24..9df27b55a0 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -43,7 +43,6 @@\n \n static timestamp_t oldest_have;\n \n-static int deepen_relative;\n static int multi_ack;\n static int no_done;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n@@ -662,6 +661,9 @@ static void send_unshallow(const struct object_array *shallows,\n \t}\n }\n \n+static int check_ref(const char *refname_full, const struct object_id *oid,\n+\t\t     int flag, void *cb_data);\n+\n static void deepen(int depth, int deepen_relative,\n \t\t   struct object_array *shallows, struct object_array *want_obj)\n {\n@@ -676,6 +678,13 @@ static void deepen(int depth, int deepen_relative,\n \t\tstruct object_array reachable_shallows = OBJECT_ARRAY_INIT;\n \t\tstruct commit_list *result;\n \n+\t\t/*\n+\t\t * Checking for reachable shallows requires that our refs be\n+\t\t * marked with OUR_REF.\n+\t\t */\n+\t\thead_ref_namespaced(check_ref, NULL);\n+\t\tfor_each_namespaced_ref(check_ref, NULL);\n+\n \t\tget_reachable_list(shallows, &reachable_shallows);\n \t\tresult = get_shallow_commits(&reachable_shallows,\n \t\t\t\t\t     depth + 1,\n@@ -712,6 +721,7 @@ static void deepen_by_rev_list(int ac, const char **av,\n static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\t     timestamp_t deepen_since,\n \t\t\t     struct string_list *deepen_not,\n+\t\t\t     int deepen_relative,\n \t\t\t     struct object_array *shallows,\n \t\t\t     struct object_array *want_obj)\n {\n@@ -834,6 +844,7 @@ static void receive_needs(struct object_array *want_obj)\n \tint has_non_tip = 0;\n \ttimestamp_t deepen_since = 0;\n \tint deepen_rev_list = 0;\n+\tint deepen_relative = 0;\n \n \tshallow_nr = 0;\n \tfor (;;) {\n@@ -925,7 +936,8 @@ static void receive_needs(struct object_array *want_obj)\n \t\treturn;\n \n \tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n-\t\t\t      &deepen_not, &shallows, want_obj))\n+\t\t\t      &deepen_not, deepen_relative, &shallows,\n+\t\t\t      want_obj))\n \t\tpacket_flush(1);\n \tobject_array_clear(&shallows);\n }\n@@ -1398,6 +1410,7 @@ static void send_shallow_info(struct upload_pack_data *data,\n \n \tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n \t\t\t       data->deepen_since, &data->deepen_not,\n+\t\t\t       data->deepen_relative,\n \t\t\t       &data->shallows, want_obj) &&\n \t    is_repository_shallow(the_repository))\n \t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366441","messageId":"20190109193510.GE54613@google.com","threadId":"50060","inReplyTo":"20181218212435.201641-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-01-09T19:35:10Z","receivedAt":"2019-01-09T19:35:20Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.18 13:24, Jonathan Tan wrote:\n> Commit 685fbd3291 (\"fetch-pack: perform a fetch using v2\", 2018-03-15)\n> attempted to teach Git deepen-relative in protocol v2 (among other\n> things), but it didn't work:\n> \n>  (1) fetch-pack.c needs to emit \"deepen-relative\".\n>  (2) upload-pack.c needs to ensure that the correct deepen_relative\n>      variable is passed to deepen() (there are two - the static variable\n>      and the one in struct upload_pack_data).\n>  (3) Before deepen() computes the list of reachable shallows, it first\n>      needs to mark all \"our\" refs as OUR_REF. v2 currently does not do\n>      this, because unlike v0, it is not needed otherwise.\n> \n> Fix all this and include a test demonstrating that it works now. For\n> (2), the static variable deepen_relative is also eliminated, removing a\n> source of confusion.\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Similar to my first fix [1], another fix for an issue discovered by\n> Aevar's GIT_TEST_PROTOCOL_VERSION patches. This one is more\n> straightforward.\n> \n> With this fix and my first fix [1], t5500 no longer reveals any more\n> bugs. (There might be more in other test files.)\n> \n> [1] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/\n> ---\n>  fetch-pack.c           |  2 ++\n>  t/t5702-protocol-v2.sh | 29 +++++++++++++++++++++++++++++\n>  upload-pack.c          | 17 +++++++++++++++--\n>  3 files changed, 46 insertions(+), 2 deletions(-)\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 9691046e64..c383ea46b3 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1042,6 +1042,8 @@ static void add_shallow_requests(struct strbuf *req_buf,\n>  \t\t\tpacket_buf_write(req_buf, \"deepen-not %s\", s->string);\n>  \t\t}\n>  \t}\n> +\tif (args->deepen_relative)\n> +\t\tpacket_buf_write(req_buf, \"deepen-relative\\n\");\n>  }\n>  \n>  static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 0f2b09ebb8..340953f01c 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -471,6 +471,35 @@ test_expect_success 'upload-pack respects client shallows' '\n>  \tgrep \"fetch< version 2\" trace\n>  '\n>  \n> +test_expect_success 'deepen-relative' '\n> +\trm -rf server client trace &&\n> +\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server one &&\n> +\ttest_commit -C server two &&\n> +\ttest_commit -C server three &&\n> +\tgit clone --depth 1 \"file://$(pwd)/server\" client &&\n> +\ttest_commit -C server four &&\n> +\n> +\t# Sanity check that only \"three\" is downloaded\n> +\tgit -C client log --pretty=tformat:%s master >actual &&\n> +\techo three >expected &&\n> +\ttest_cmp expected actual &&\n> +\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> +\t\tfetch --deepen=1 origin &&\n> +\t# Ensure that protocol v2 is used\n> +\tgrep \"fetch< version 2\" trace &&\n> +\n> +\tgit -C client log --pretty=tformat:%s origin/master >actual &&\n> +\tcat >expected <<-\\EOF &&\n> +\tfour\n> +\tthree\n> +\ttwo\n> +\tEOF\n> +\ttest_cmp expected actual\n> +'\n> +\n>  # Test protocol v2 with 'http://' transport\n>  #\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 5e81f1ff24..9df27b55a0 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -43,7 +43,6 @@\n>  \n>  static timestamp_t oldest_have;\n>  \n> -static int deepen_relative;\n>  static int multi_ack;\n>  static int no_done;\n>  static int use_thin_pack, use_ofs_delta, use_include_tag;\n> @@ -662,6 +661,9 @@ static void send_unshallow(const struct object_array *shallows,\n>  \t}\n>  }\n>  \n> +static int check_ref(const char *refname_full, const struct object_id *oid,\n> +\t\t     int flag, void *cb_data);\n> +\n>  static void deepen(int depth, int deepen_relative,\n>  \t\t   struct object_array *shallows, struct object_array *want_obj)\n>  {\n> @@ -676,6 +678,13 @@ static void deepen(int depth, int deepen_relative,\n>  \t\tstruct object_array reachable_shallows = OBJECT_ARRAY_INIT;\n>  \t\tstruct commit_list *result;\n>  \n> +\t\t/*\n> +\t\t * Checking for reachable shallows requires that our refs be\n> +\t\t * marked with OUR_REF.\n> +\t\t */\n> +\t\thead_ref_namespaced(check_ref, NULL);\n> +\t\tfor_each_namespaced_ref(check_ref, NULL);\n> +\n>  \t\tget_reachable_list(shallows, &reachable_shallows);\n>  \t\tresult = get_shallow_commits(&reachable_shallows,\n>  \t\t\t\t\t     depth + 1,\n> @@ -712,6 +721,7 @@ static void deepen_by_rev_list(int ac, const char **av,\n>  static int send_shallow_list(int depth, int deepen_rev_list,\n>  \t\t\t     timestamp_t deepen_since,\n>  \t\t\t     struct string_list *deepen_not,\n> +\t\t\t     int deepen_relative,\n>  \t\t\t     struct object_array *shallows,\n>  \t\t\t     struct object_array *want_obj)\n>  {\n> @@ -834,6 +844,7 @@ static void receive_needs(struct object_array *want_obj)\n>  \tint has_non_tip = 0;\n>  \ttimestamp_t deepen_since = 0;\n>  \tint deepen_rev_list = 0;\n> +\tint deepen_relative = 0;\n>  \n>  \tshallow_nr = 0;\n>  \tfor (;;) {\n> @@ -925,7 +936,8 @@ static void receive_needs(struct object_array *want_obj)\n>  \t\treturn;\n>  \n>  \tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n> -\t\t\t      &deepen_not, &shallows, want_obj))\n> +\t\t\t      &deepen_not, deepen_relative, &shallows,\n> +\t\t\t      want_obj))\n>  \t\tpacket_flush(1);\n>  \tobject_array_clear(&shallows);\n>  }\n> @@ -1398,6 +1410,7 @@ static void send_shallow_info(struct upload_pack_data *data,\n>  \n>  \tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n>  \t\t\t       data->deepen_since, &data->deepen_not,\n> +\t\t\t       data->deepen_relative,\n>  \t\t\t       &data->shallows, want_obj) &&\n>  \t    is_repository_shallow(the_repository))\n>  \t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n> -- \n> 2.19.0.271.gfe8321ec05.dirty\n> \n\nLooks good to me.\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"},{"id":"366515","messageId":"xmqqa7k82wnr.fsf@gitster-ct.c.googlers.com","threadId":"50060","inReplyTo":"20190109193510.GE54613@google.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T18:30:00Z","receivedAt":"2019-01-10T18:30:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> Looks good to me.\n> Reviewed-by: Josh Steadmon <steadmon@google.com>\n\nThanks, both.\n"},{"id":"366531","messageId":"xmqqr2dk1bxf.fsf@gitster-ct.c.googlers.com","threadId":"50060","inReplyTo":"xmqqa7k82wnr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T20:43:08Z","receivedAt":"2019-01-10T20:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Josh Steadmon <steadmon@google.com> writes:\n>\n>> Looks good to me.\n>> Reviewed-by: Josh Steadmon <steadmon@google.com>\n>\n> Thanks, both.\n\nThis, when applied on top of ecbdaf08991, seems to break t5702.30,\nat least for me.\n\nTip of 'pu' in today's push-out will be broken as it has this patch\nmerged.\n\nCan somebody take a look?\n\n\nexpecting success:\n        rm -rf server client trace &&\n\n        test_create_repo server &&\n        test_commit -C server one &&\n        test_commit -C server two &&\n        test_commit -C server three &&\n        git clone --depth 1 \"file://$(pwd)/server\" client &&\n        test_commit -C server four &&\n\n        # Sanity check that only \"three\" is downloaded\n        git -C client log --pretty=tformat:%s master >actual &&\n        echo three >expected &&\n        test_cmp expected actual &&\n\n        GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n                fetch --deepen=1 origin &&\n        # Ensure that protocol v2 is used\n        grep \"fetch< version 2\" trace &&\n\n        git -C client log --pretty=tformat:%s origin/master >actual &&\n        cat >expected <<-\\EOF &&\n        four\n        three\n        two\n        EOF\n        test_cmp expected actual\n\nInitialized empty Git repository in /home/gitster/w/git.git/t/trash directory.t5702-protocol-v2/server/.git/\n[master (root-commit) 1581e3e] one\n Author: A U Thor <author@example.com>\n 1 file changed, 1 insertion(+)\n create mode 100644 one.t\n[master 5680d21] two\n Author: A U Thor <author@example.com>\n 1 file changed, 1 insertion(+)\n create mode 100644 two.t\n[master 94705d7] three\n Author: A U Thor <author@example.com>\n 1 file changed, 1 insertion(+)\n create mode 100644 three.t\nCloning into 'client'...\nremote: Enumerating objects: 5, done.\nremote: Counting objects: 100% (5/5), done.\nremote: Compressing objects: 100% (2/2), done.\nremote: Total 5 (delta 0), reused 0 (delta 0)\nReceiving objects: 100% (5/5), done.\n[master 0b417c7] four\n Author: A U Thor <author@example.com>\n 1 file changed, 1 insertion(+)\n create mode 100644 four.t\nremote: Enumerating objects: 6, done.\nremote: Counting objects: 100% (6/6), done.\nremote: Compressing objects: 100% (4/4), done.\nremote: Total 5 (delta 0), reused 0 (delta 0)\nUnpacking objects: 100% (5/5), done.\nFrom file:///home/gitster/w/git.git/t/trash directory.t5702-protocol-v2/server\n   94705d7..0b417c7  master     -> origin/master\n * [new tag]         four       -> four\nfatal: shallow file has changed since we read it\nnot ok 30 - deepen-relative\n"},{"id":"366535","messageId":"20190110212857.58772-1-jonathantanmy@google.com","threadId":"50060","inReplyTo":"xmqqr2dk1bxf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-10T21:28:57Z","receivedAt":"2019-01-10T21:29:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Josh Steadmon <steadmon@google.com> writes:\n> >\n> >> Looks good to me.\n> >> Reviewed-by: Josh Steadmon <steadmon@google.com>\n> >\n> > Thanks, both.\n> \n> This, when applied on top of ecbdaf08991, seems to break t5702.30,\n> at least for me.\n> \n> Tip of 'pu' in today's push-out will be broken as it has this patch\n> merged.\n> \n> Can somebody take a look?\n\nI can reproduce this. I'll investigate this further. In the meantime,\nfeel free to remove this from pu.\n"},{"id":"366540","messageId":"20190110221132.52443-1-jonathantanmy@google.com","threadId":"50060","inReplyTo":"20190110212857.58772-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-10T22:11:32Z","receivedAt":"2019-01-10T22:11:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > This, when applied on top of ecbdaf08991, seems to break t5702.30,\n> > at least for me.\n> > \n> > Tip of 'pu' in today's push-out will be broken as it has this patch\n> > merged.\n> > \n> > Can somebody take a look?\n> \n> I can reproduce this. I'll investigate this further. In the meantime,\n> feel free to remove this from pu.\n\nThe test failure is caused by a bug I fixed in [1]. I've verified that\nif I apply [1] first, then this, all tests pass. If you do include this\npatch in some branch, it should be dependent on [1], but I understand if\nyou'd like reviewers to look at both this and [1] first.\n\nI thought I tested this without something that fixes the issue fixed by\n[1] and it passed, but apparently that's not true.\n\n[1] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/\n"},{"id":"366543","messageId":"xmqqwoncyvh5.fsf@gitster-ct.c.googlers.com","threadId":"50060","inReplyTo":"20190110221132.52443-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: teach deepen-relative in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T22:54:30Z","receivedAt":"2019-01-10T22:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> > This, when applied on top of ecbdaf08991, seems to break t5702.30,\n>> > at least for me.\n>> > \n>> > Tip of 'pu' in today's push-out will be broken as it has this patch\n>> > merged.\n>> > \n>> > Can somebody take a look?\n>> \n>> I can reproduce this. I'll investigate this further. In the meantime,\n>> feel free to remove this from pu.\n>\n> The test failure is caused by a bug I fixed in [1]. I've verified that\n> if I apply [1] first, then this, all tests pass. If you do include this\n> patch in some branch, it should be dependent on [1], but I understand if\n> you'd like reviewers to look at both this and [1] first.\n>\n> I thought I tested this without something that fixes the issue fixed by\n> [1] and it passed, but apparently that's not true.\n>\n> [1] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/\n\nOK, that means that it would be the cleanest to make them into\ntwo-patch series, I guess.\n\nLet's requeue these two and see what happens.\n\nThanks.\n\n"}]}