{"thread":{"id":"55996","subject":"[PATCH 0/3] Push negotiation fixes","startedAt":"2021-06-23T22:31:03Z","lastAt":"2021-07-27T21:11:48Z","messageCount":24,"participants":["Jonathan Tan","Emily Shaffer","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"428338","messageId":"cover.1624486920.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":null,"subject":"[PATCH 0/3] Push negotiation fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-23T22:30:50Z","receivedAt":"2021-06-23T22:31:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Testing at $DAYJOB revealed some bugs in some fundamental scenarios, and\nhere are their fixes.\n\nJonathan Tan (3):\n send-pack: fix push.negotiate with remote helper\n send-pack: fix push nego. when remote has refs\n fetch: die on invalid --negotiation-tip hash\n\n builtin/fetch.c | 4 +++-\n builtin/send-pack.c | 1 +\n send-pack.c | 6 +++--\n t/t5510-fetch.sh | 9 ++++++++\n t/t5516-fetch-push.sh | 54 +++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 71 insertions(+), 3 deletions(-)\n\n-- \n2.32.0.288.g62a8d224e6-goog\n\n"},{"id":"428339","messageId":"175da5f02b319bb637700e4f6665ee346674e1b0.1624486920.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1624486920.git.jonathantanmy@google.com","subject":"[PATCH 2/3] send-pack: fix push nego. when remote has refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-23T22:30:52Z","receivedAt":"2021-06-23T22:31:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\ndid not test the case in which a remote advertises at least one ref. In\nsuch a case, \"remote_refs\" in get_commons_through_negotiation() in\nsend-pack.c would also contain those refs with a zero ref->new_oid (in\naddition to the refs being pushed with a nonzero ref->new_oid). Passing\nthem as negotiation tips to \"git fetch\" causes an error, so filter them\nout.\n\n(The exact error that would happen in \"git fetch\" in this case is a\nsegmentation fault, which is unwanted. This will be fixed in the\nsubsequent commit.)\n\nSigned-off-by: Jonathan Tan \n---\n send-pack.c | 6 ++++--\n t/t5516-fetch-push.sh | 5 +++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 9cb9f71650..85945becf0 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,\n \tchild.no_stdin = 1;\n \tchild.out = -1;\n \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n-\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\t}\n \tstrvec_push(&child.args, url);\n \n \tif (start_command(&child))\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 5ce32e531a..e383ba662f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '\n \t# Without negotiation\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \techo now pushing without negotiation &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&\n@@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '\n \trm event &&\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n \tgrep_wrote 2 event # 1 commit, 1 tree\n@@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \trm event &&\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n@@ -1783,6 +1786,7 @@ test_expect_success 'http push with negotiation' '\n \t# Without negotiation\n \ttest_create_repo \"$SERVER\" &&\n \ttest_config -C \"$SERVER\" http.receivepack true &&\n+\ttest_commit -C \"$SERVER\" unrelated_commit &&\n \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n@@ -1794,6 +1798,7 @@ test_expect_success 'http push with negotiation' '\n \trm -rf \"$SERVER\" &&\n \ttest_create_repo \"$SERVER\" &&\n \ttest_config -C \"$SERVER\" http.receivepack true &&\n+\ttest_commit -C \"$SERVER\" unrelated_commit &&\n \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n-- \n2.32.0.288.g62a8d224e6-goog\n\n"},{"id":"428340","messageId":"042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1624486920.git.jonathantanmy@google.com","subject":"[PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-23T22:30:51Z","receivedAt":"2021-06-23T22:31:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\nintroduced the push.negotiate config variable and included a test. The\ntest only covered pushing without a remote helper, so the fact that\npushing with a remote helper doesn't work went unnoticed.\n\nThis is ultimately caused by the \"url\" field not being set in the args\nstruct. This field being unset probably went unnoticed because besides\npush negotiation, this field is only used to generate a \"pushee\" line in\na push cert (and if not given, no such line is generated). Therefore,\nset this field.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/send-pack.c | 1 +\n t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex a7e01667b0..729dea1d25 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \targs.atomic = atomic;\n \targs.stateless_rpc = stateless_rpc;\n \targs.push_options = push_options.nr ? &push_options : NULL;\n+\targs.url = dest;\n \n \tif (from_stdin) {\n \t\tif (args.stateless_rpc) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 0916f76302..5ce32e531a 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'http push with negotiation' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\tURI=\"$HTTPD_URL/smart/server\" &&\n+\n+\trm -rf client &&\n+\tgit init client &&\n+\ttest_commit -C client first_commit &&\n+\ttest_commit -C client second_commit &&\n+\n+\t# Without negotiation\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_config -C \"$SERVER\" http.receivepack true &&\n+\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n+\n+\t# Same commands, but with negotiation\n+\trm event &&\n+\trm -rf \"$SERVER\" &&\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_config -C \"$SERVER\" http.receivepack true &&\n+\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n+'\n+\n+test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n+\trm event &&\n+\trm -rf \"$SERVER\" &&\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_config -C \"$SERVER\" http.receivepack true &&\n+\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n+\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n+\ttest_i18ngrep \"push negotiation failed\" err\n+'\n+\n+# DO NOT add non-httpd-specific tests here, because the last part of this\n+# test script is only executed when httpd is available and enabled.\n+\n test_done\n-- \n2.32.0.288.g62a8d224e6-goog\n\n"},{"id":"428341","messageId":"8df607f4cfb3064200bf4effc91726c92804e637.1624486920.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1624486920.git.jonathantanmy@google.com","subject":"[PATCH 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-23T22:30:53Z","receivedAt":"2021-06-23T22:31:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If a full hexadecimal hash is given as a --negotiation-tip to \"git\nfetch\", and that hash does not correspond to an object, \"git fetch\" will\nsegfault if --negotiate-only is given and will silently ignore that hash\notherwise. Make these cases fatal errors, just like the case when an\ninvalid ref name or abbreviated hash is given.\n\nWhile at it, mark the error messages as translatable.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 4 +++-\n t/t5510-fetch.sh | 9 +++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9191620e50..2c50465cff 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n \t\tif (!has_glob_specials(s)) {\n \t\t\tstruct object_id oid;\n \t\t\tif (get_oid(s, &oid))\n-\t\t\t\tdie(\"%s is not a valid object\", s);\n+\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n+\t\t\tif (!has_object(the_repository, &oid, 0))\n+\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n \t\t\toid_array_append(oids, &oid);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex e83b2a6506..5fc5750d8d 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1214,6 +1214,15 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '\n \tcheck_negotiation_tip\n '\n \n+test_expect_success '--negotiation-tip rejects missing OIDs' '\n+\tsetup_negotiation_tip server server 0 &&\n+\ttest_must_fail git -C client fetch \\\n+\t\t--negotiation-tip=alpha_1 \\\n+\t\t--negotiation-tip=$(test_oid zero) \\\n+\t\torigin alpha_s beta_s 2>err &&\n+\ttest_i18ngrep \"is not a valid object\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.32.0.288.g62a8d224e6-goog\n\n"},{"id":"429942","messageId":"YO4SZs40xzHEb6W7@google.com","threadId":"55996","inReplyTo":"042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-07-13T22:23:34Z","receivedAt":"2021-07-13T22:23:43Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 23, 2021 at 03:30:51PM -0700, Jonathan Tan wrote:\n> \n> Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> introduced the push.negotiate config variable and included a test. The\n> test only covered pushing without a remote helper, so the fact that\n> pushing with a remote helper doesn't work went unnoticed.\n> \n> This is ultimately caused by the \"url\" field not being set in the args\n> struct. This field being unset probably went unnoticed because besides\n> push negotiation, this field is only used to generate a \"pushee\" line in\n> a push cert (and if not given, no such line is generated). Therefore,\n> set this field.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/send-pack.c | 1 +\n> t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 50 insertions(+)\n> \n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index a7e01667b0..729dea1d25 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n> \targs.atomic = atomic;\n> \targs.stateless_rpc = stateless_rpc;\n> \targs.push_options = push_options.nr ? &push_options : NULL;\n> +\targs.url = dest;\nSure, the fix itself is small and inoffensive. And the rest of the patch\nis regression testing.\n> \n> \tif (from_stdin) {\n> \t\tif (args.stateless_rpc) {\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 0916f76302..5ce32e531a 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> \ttest_must_fail git -C cloned push --delete origin new-wt\n> '\n> \n> +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> +start_httpd\n\nAh, so fetch-push test wasn't doing any HTTP testing whatsoever. Does\nthat mean there is a better place for these to go? Or does it mean that\nfetch-push test was under-testing?\n\n> +\n> +test_expect_success 'http push with negotiation' '\n> +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> +\tURI=\"$HTTPD_URL/smart/server\" &&\n> +\n> +\trm -rf client &&\n> +\tgit init client &&\n> +\ttest_commit -C client first_commit &&\n> +\ttest_commit -C client second_commit &&\n> +\n> +\t# Without negotiation\n> +\ttest_create_repo \"$SERVER\" &&\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\nPushing a branch with just the first commit...\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\ntransfer.hideRefs (referenced by receive.hideRefs) says this ref will be\nomitted from advertisement, so we are forcing either an inefficient push\nor a negotiation to occur, by having the server initially claim not to\nknow about it. But it's only omitted from the *initial* advertisement,\nso it will be advertised in later rounds of negotiation, right?\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\nAnd then from 'main' we push first_commit and second_commit?\n> +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\nNice, I like the comment - this helps.\n> +\n> +\t# Same commands, but with negotiation\n> +\trm event &&\n> +\trm -rf \"$SERVER\" &&\nOk, clean up the trace and the server so we can start over, but we don't\nneed to recreate the client commits because the server doesn't know\nabout them anyway. Fine.\n> +\ttest_create_repo \"$SERVER\" &&\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\nAnd then here's the same set of commands with push negotiation, ok.\n> +\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n\nIs there any reason the event counts would change or be\nnon-deterministic outside of negotiation? Or, in other words, is this\npotentially flaky?\n\n> +'\n> +\n> +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n> +\trm event &&\n> +\trm -rf \"$SERVER\" &&\n> +\ttest_create_repo \"$SERVER\" &&\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\nHmm, this relies on 'client' being in the same state the above test left\nit. Probably better to recreate it or at least leave a loud warning\nabout it in a comment above this test definition...\n\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n\nAnd we're pushing with protocol v0 so no negotiation can occur here,\nright?\n\n> +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> +\ttest_i18ngrep \"push negotiation failed\" err\n> +'\n> +\n> +# DO NOT add non-httpd-specific tests here, because the last part of this\n> +# test script is only executed when httpd is available and enabled.\n> +\n> test_done\n> -- \n> 2.32.0.288.g62a8d224e6-goog\n> \n\nThanks for answering novice questions :)\n\n - Emily\n"},{"id":"429944","messageId":"YO4UIGQPugvHcYrw@google.com","threadId":"55996","inReplyTo":"175da5f02b319bb637700e4f6665ee346674e1b0.1624486920.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-07-13T22:30:56Z","receivedAt":"2021-07-13T22:31:06Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 23, 2021 at 03:30:52PM -0700, Jonathan Tan wrote:\n> \n> Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> did not test the case in which a remote advertises at least one ref. In\n> such a case, \"remote_refs\" in get_commons_through_negotiation() in\n> send-pack.c would also contain those refs with a zero ref->new_oid (in\n> addition to the refs being pushed with a nonzero ref->new_oid). Passing\n> them as negotiation tips to \"git fetch\" causes an error, so filter them\n> out.\n\nSo here we are filtering those redundant refs on the client side?\n\n> \n> (The exact error that would happen in \"git fetch\" in this case is a\n> segmentation fault, which is unwanted. This will be fixed in the\n> subsequent commit.)\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> send-pack.c | 6 ++++--\n> t/t5516-fetch-push.sh | 5 +++++\n> 2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/send-pack.c b/send-pack.c\n> index 9cb9f71650..85945becf0 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,\n> \tchild.no_stdin = 1;\n> \tchild.out = -1;\n> \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n> -\tfor (ref = remote_refs; ref; ref = ref->next)\n> -\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\tfor (ref = remote_refs; ref; ref = ref->next) {\n> +\t\tif (!is_null_oid(&ref->new_oid))\n> +\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\t}\n> \tstrvec_push(&child.args, url);\n> \n> \tif (start_command(&child))\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 5ce32e531a..e383ba662f 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '\n> \t# Without negotiation\n> \tmk_empty testrepo &&\n> \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n> +\ttest_commit -C testrepo unrelated_commit &&\n> \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n> \techo now pushing without negotiation &&\n> \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&\n> @@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '\n> \trm event &&\n> \tmk_empty testrepo &&\n> \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n> +\ttest_commit -C testrepo unrelated_commit &&\n\nSo now we are asking 'testrepo' to initially advertise that it also has\nunrelated_commit, which we don't care about, and expect to work fine\nanyway. Ok.\n\n> \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n> \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n> \tgrep_wrote 2 event # 1 commit, 1 tree\n> @@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n> \trm event &&\n> \tmk_empty testrepo &&\n> \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n> +\ttest_commit -C testrepo unrelated_commit &&\n> \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n> \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n> \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n> @@ -1783,6 +1786,7 @@ test_expect_success 'http push with negotiation' '\n> \t# Without negotiation\n> \ttest_create_repo \"$SERVER\" &&\n> \ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\ttest_commit -C \"$SERVER\" unrelated_commit &&\n> \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n> @@ -1794,6 +1798,7 @@ test_expect_success 'http push with negotiation' '\n> \trm -rf \"$SERVER\" &&\n> \ttest_create_repo \"$SERVER\" &&\n> \ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\ttest_commit -C \"$SERVER\" unrelated_commit &&\n> \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n> -- \n> 2.32.0.288.g62a8d224e6-goog\n\n\nSeems reasonable enough to me.\nReviewed-by: Emily Shaffer \n"},{"id":"429946","messageId":"YO4Vc58rl2Ble31u@google.com","threadId":"55996","inReplyTo":"8df607f4cfb3064200bf4effc91726c92804e637.1624486920.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-07-13T22:36:35Z","receivedAt":"2021-07-13T22:36:42Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:\n> \n> If a full hexadecimal hash is given as a --negotiation-tip to \"git\n> fetch\", and that hash does not correspond to an object, \"git fetch\" will\n> segfault if --negotiate-only is given and will silently ignore that hash\n> otherwise. Make these cases fatal errors, just like the case when an\n> invalid ref name or abbreviated hash is given.\n> \n> While at it, mark the error messages as translatable.\nI don't usually like this kind of \"while we're at it\" change, but in\nthis case it's very very small, so it doesn't bother me much.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/fetch.c | 4 +++-\n> t/t5510-fetch.sh | 9 +++++++++\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 9191620e50..2c50465cff 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n> \t\tif (!has_glob_specials(s)) {\n> \t\t\tstruct object_id oid;\n> \t\t\tif (get_oid(s, &oid))\n> -\t\t\t\tdie(\"%s is not a valid object\", s);\n> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n> +\t\t\tif (!has_object(the_repository, &oid, 0))\n> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\nAny reason not to consolidate these, e.g. if (get_oid || !has_object)?\nThen we wouldn't dup the err string.\n\n> \t\t\toid_array_append(oids, &oid);\n> \t\t\tcontinue;\n> \t\t}\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index e83b2a6506..5fc5750d8d 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1214,6 +1214,15 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '\n> \tcheck_negotiation_tip\n> '\n> \n> +test_expect_success '--negotiation-tip rejects missing OIDs' '\n> +\tsetup_negotiation_tip server server 0 &&\n> +\ttest_must_fail git -C client fetch \\\n> +\t\t--negotiation-tip=alpha_1 \\\nThis one's okay...\n\n> +\t\t--negotiation-tip=$(test_oid zero) \\\n...and this one's junk. Ok.\n\n> +\t\torigin alpha_s beta_s 2>err &&\n> +\ttest_i18ngrep \"is not a valid object\" err\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n> \n> -- \n> 2.32.0.288.g62a8d224e6-goog\n\nOther than the little nit, this seems OK to me.\n\n - Emily\n"},{"id":"429950","messageId":"87bl753i2p.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-13T23:11:48Z","receivedAt":"2021-07-13T23:33:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Jonathan Tan wrote:\n\n> Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> introduced the push.negotiate config variable and included a test. The\n> test only covered pushing without a remote helper, so the fact that\n> pushing with a remote helper doesn't work went unnoticed.\n>\n> This is ultimately caused by the \"url\" field not being set in the args\n> struct. This field being unset probably went unnoticed because besides\n> push negotiation, this field is only used to generate a \"pushee\" line in\n> a push cert (and if not given, no such line is generated). Therefore,\n> set this field.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/send-pack.c | 1 +\n> t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 50 insertions(+)\n>\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index a7e01667b0..729dea1d25 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n> \targs.atomic = atomic;\n> \targs.stateless_rpc = stateless_rpc;\n> \targs.push_options = push_options.nr ? &push_options : NULL;\n> +\targs.url = dest;\n> \n> \tif (from_stdin) {\n> \t\tif (args.stateless_rpc) {\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 0916f76302..5ce32e531a 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> \ttest_must_fail git -C cloned push --delete origin new-wt\n> '\n> \n> +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> +start_httpd\n> +\n> +test_expect_success 'http push with negotiation' '\n> +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> +\tURI=\"$HTTPD_URL/smart/server\" &&\n> +\n> +\trm -rf client &&\n\nNothing in this test file has created a \"client\" directory at this\npoint, so we shouldn't have this to remove it.\n\nI think the real reason for this is that you're just copy/pasting this\nfrom elsewhere. I've got an unsubmitted series where I fixed various\ncallsites in these t/t*http* tests to use test_when_finished instead.\n\nThis pattern of having the next test clean up after you leads to bad\ninter-dependencies, there were a few things broken e.g. if earlier tests\nwere skipped. Let's not continue that pattern.\n\n> +\tgit init client &&\n> +\ttest_commit -C client first_commit &&\n> +\ttest_commit -C client second_commit &&\n> +\n> +\t# Without negotiation\n> +\ttest_create_repo \"$SERVER\" &&\n\ns/test_create_repo/git init/g\n\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> +\n> +\t# Same commands, but with negotiation\n> +\trm event &&\n> +\trm -rf \"$SERVER\" &&\n\nDitto test_when_finished, or actually:\n\n> +\ttest_create_repo \"$SERVER\" &&\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n\nIf we're entirely setting up the server again shouldn't this just be\nanother test_expect_success block?\n\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> +\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n> +'\n> +\n> +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n> +\trm event &&\n> +\trm -rf \"$SERVER\" &&\n> +\ttest_create_repo \"$SERVER\" &&\n> +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> +\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n> +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n> +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> +\ttest_i18ngrep \"push negotiation failed\" err\n\ns/test_i18ngrep/grep/, or actually better yet is there a reason not to\ndo test_cmp here? I'm pretty sure there's not.\n\nThe reason I've got that unsubmitted series is because I found a bug\nthat was cleverly hidden away by an earlier such 'grep the output'\ncode/test you'd added recently.\n\nThere *are* cases where we e.g. get STDERR output from the OS itself\nabout bad connections, or races, but we should at least:\n\n grep error: error-lines.actual &&\n test_cmp error-lines.expect error-lines.actual\n\nTo check that we get the errors we expected, and *only* those.\n\n> +'\n> +\n> +# DO NOT add non-httpd-specific tests here, because the last part of this\n> +# test script is only executed when httpd is available and enabled.\n> +\n> test_done\n\nAlso, instead of this comment let's just create another\nt*-fetch-push-http.sh test. Because:\n\n * This test is already really slow, it takes me 13s to run it now. I\n haven't tested, but setting up apache will make it even slower.\n\n * It's also an anti-pattern to switch midway to needing an external\n daemon v.s. doing it in a dedicated test.\n\n E.g. if you have the relevant GIT_* settings to run http:// tests,\n but not git:// tests we'll skip the former entirely in\n t5700-protocol-v1.sh, because we'll \"skip all\" as soon as we see we\n can't start the git-daemon.\n\n That specifically isn't an issue here, but better to avoid setting up\n the pattern.\n\n * What *is* an issue with it here is that the \"skip all\" in TAP is only\n handled before you run any tests, e.g. compare:\n \n $ prove ./t5700-protocol-v1.sh\n ./t5700-protocol-v1.sh .. ok \n All tests successful.\n Files=1, Tests=21, 2 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr 0.80 csys = 1.62 CPU)\n Result: PASS\n \n $ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n ./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\n Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr 0.00 csys = 0.03 CPU)\n Result: NOTESTS\n \n $ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n ./t5700-protocol-v1.sh .. ok \n All tests successful.\n Files=1, Tests=16, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.63 cusr 0.59 csys = 1.23 CPU)\n Result: PASS\n \n I.e. if you stick an inclusion of lib-httpd.sh this late in a test\n file we won't get a prominent notice saying we're categorically\n skipping certain types of tests based on an external dependency.\n\n If you had your own dedicated test instead you'd get:\n \n $ GIT_TEST_HTTPD=false prove ./t5705-protocol-v2-http.sh \n ./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)\n Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.02 cusr 0.01 csys = 0.05 CPU)\n Result: NOTESTS\n"},{"id":"429952","messageId":"878s293htx.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"YO4Vc58rl2Ble31u@google.com","subject":"Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-13T23:34:41Z","receivedAt":"2021-07-13T23:38:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 13 2021, Emily Shaffer wrote:\n\n> On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:\n>> \n>> If a full hexadecimal hash is given as a --negotiation-tip to \"git\n>> fetch\", and that hash does not correspond to an object, \"git fetch\" will\n>> segfault if --negotiate-only is given and will silently ignore that hash\n>> otherwise. Make these cases fatal errors, just like the case when an\n>> invalid ref name or abbreviated hash is given.\n>> \n>> While at it, mark the error messages as translatable.\n> I don't usually like this kind of \"while we're at it\" change, but in\n> this case it's very very small, so it doesn't bother me much.\n>> \n>> Signed-off-by: Jonathan Tan \n>> ---\n>> builtin/fetch.c | 4 +++-\n>> t/t5510-fetch.sh | 9 +++++++++\n>> 2 files changed, 12 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 9191620e50..2c50465cff 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n>> \t\tif (!has_glob_specials(s)) {\n>> \t\t\tstruct object_id oid;\n>> \t\t\tif (get_oid(s, &oid))\n>> -\t\t\t\tdie(\"%s is not a valid object\", s);\n>> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n>> +\t\t\tif (!has_object(the_repository, &oid, 0))\n>> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n> Any reason not to consolidate these, e.g. if (get_oid || !has_object)?\n> Then we wouldn't dup the err string.\n\nGenerally I'd agree, but aren't we explicitly conflating cases where\nsomething is a valid way no name an object v.s. being certain that such\nan object does not exist? I.e. this should be something like:\n\n if can't get_get():\n error \"couldn't get the OID of revision '%s'\"\n if can't look up fully-qualified OID:\n error \"the OID '%s' does not exist\"\n\nOr something...\n"},{"id":"430105","messageId":"20210714192509.4082107-1-jonathantanmy@google.com","threadId":"55996","inReplyTo":"YO4SZs40xzHEb6W7@google.com","subject":"Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-14T19:25:09Z","receivedAt":"2021-07-14T19:25:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > \n> > \tif (from_stdin) {\n> > \t\tif (args.stateless_rpc) {\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index 0916f76302..5ce32e531a 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n> > \ttest_must_fail git -C cloned push --delete origin new-wt\n> > '\n> > \n> > +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> > +start_httpd\n> \n> Ah, so fetch-push test wasn't doing any HTTP testing whatsoever. Does\n> that mean there is a better place for these to go? Or does it mean that\n> fetch-push test was under-testing?\n\nThese are closely related to the non-HTTP push negotiation tests that I\npreviously added to this file, so I don't think there is a better place.\nAs for whether this test is under-testing, I also don't think so - most\nfetch/push processes would be the same regardless of protocol (but not\npush negotiation, apparently).\n\n> \n> > +\n> > +test_expect_success 'http push with negotiation' '\n> > +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> > +\tURI=\"$HTTPD_URL/smart/server\" &&\n> > +\n> > +\trm -rf client &&\n> > +\tgit init client &&\n> > +\ttest_commit -C client first_commit &&\n> > +\ttest_commit -C client second_commit &&\n> > +\n> > +\t# Without negotiation\n> > +\ttest_create_repo \"$SERVER\" &&\n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> Pushing a branch with just the first commit...\n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> transfer.hideRefs (referenced by receive.hideRefs) says this ref will be\n> omitted from advertisement, so we are forcing either an inefficient push\n> or a negotiation to occur, by having the server initially claim not to\n> know about it. \n\nCurrently, the client attempts the negotiation before the push ref\nadvertisement, so the ref advertisement does not influence whether the\nnegotiation happens or not. \n\n(One might ask, should the negotiation happen after the push ref\nadvertisement then? This is an interesting idea, because the push ref\nadvertisement could have information that shows us that negotiation is\nunnecessary, but I think that eventually it's best if we suppress the\npush ref advertisement.)\n\n> But it's only omitted from the *initial* advertisement,\n> so it will be advertised in later rounds of negotiation, right?\n\nThere is no ref advertisement in any round of negotiation (there is no\nls-refs call). It will be acknowledged if the client queries for it,\nthough. (In any case, this config is not meant to affect the negotiation\npart at all, because the negotiation part is using the \"fetch\" protocol\nand the \"receive\" in \"receive.hideRefs\" means that it only affects the\n\"push\" protocol.)\n\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> And then from 'main' we push first_commit and second_commit?\n\nDepending on the result of negotiation. Here, we push both commits (as\nyou can see from the next line you quoted below).\n\n> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> Nice, I like the comment - this helps.\n\nThanks.\n\n> > +\n> > +\t# Same commands, but with negotiation\n> > +\trm event &&\n> > +\trm -rf \"$SERVER\" &&\n> Ok, clean up the trace and the server so we can start over, but we don't\n> need to recreate the client commits because the server doesn't know\n> about them anyway. Fine.\n> > +\ttest_create_repo \"$SERVER\" &&\n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> And then here's the same set of commands with push negotiation, ok.\n> > +\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n> \n> Is there any reason the event counts would change or be\n> non-deterministic outside of negotiation? Or, in other words, is this\n> potentially flaky?\n\nIn general, no - the information that the server conveys to the client\nin the ref advertisement (which commits it has) is quite well-defined,\nso it can be precisely computed which objects the server has and hasn't.\nOne potential point of flakiness is if we change the client to push\nextra objects (say, if the client decided to store one packfile per\ncommit and all its trees and blobs, and then push the entire packfile\nwhenever it needs to push that commit), but I don't think that we're\nheading in that direction.\n\n> \n> > +'\n> > +\n> > +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n> > +\trm event &&\n> > +\trm -rf \"$SERVER\" &&\n> > +\ttest_create_repo \"$SERVER\" &&\n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> Hmm, this relies on 'client' being in the same state the above test left\n> it. Probably better to recreate it or at least leave a loud warning\n> about it in a comment above this test definition...\n\nI ended up recreating it.\n\n> \n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> > +\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n> \n> And we're pushing with protocol v0 so no negotiation can occur here,\n> right?\n\nYes, but this is worth adding a comment for, so I added it.\n> \n> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> > +\ttest_i18ngrep \"push negotiation failed\" err\n> > +'\n> > +\n> > +# DO NOT add non-httpd-specific tests here, because the last part of this\n> > +# test script is only executed when httpd is available and enabled.\n> > +\n> > test_done\n> > -- \n> > 2.32.0.288.g62a8d224e6-goog\n> > \n> \n> Thanks for answering novice questions :)\n\nThanks for taking a look.\n"},{"id":"430108","messageId":"20210714193220.4083070-1-jonathantanmy@google.com","threadId":"55996","inReplyTo":"87bl753i2p.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-14T19:32:20Z","receivedAt":"2021-07-14T19:32:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +test_expect_success 'http push with negotiation' '\n> > +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> > +\tURI=\"$HTTPD_URL/smart/server\" &&\n> > +\n> > +\trm -rf client &&\n> \n> Nothing in this test file has created a \"client\" directory at this\n> point, so we shouldn't have this to remove it.\n> \n> I think the real reason for this is that you're just copy/pasting this\n> from elsewhere. I've got an unsubmitted series where I fixed various\n> callsites in these t/t*http* tests to use test_when_finished instead.\n> \n> This pattern of having the next test clean up after you leads to bad\n> inter-dependencies, there were a few things broken e.g. if earlier tests\n> were skipped. Let's not continue that pattern.\n\nOK - I have changed it so that each test is independent and cleans up after\nitself.\n\n> \n> > +\tgit init client &&\n> > +\ttest_commit -C client first_commit &&\n> > +\ttest_commit -C client second_commit &&\n> > +\n> > +\t# Without negotiation\n> > +\ttest_create_repo \"$SERVER\" &&\n> \n> s/test_create_repo/git init/g\n\nDone.\n\n> \n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> > +\n> > +\t# Same commands, but with negotiation\n> > +\trm event &&\n> > +\trm -rf \"$SERVER\" &&\n> \n> Ditto test_when_finished, or actually:\n> \n> > +\ttest_create_repo \"$SERVER\" &&\n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n\nIn my current version, I have changed everything to \"git init\". Should I\nuse \"test_create_repo\" instead?\n\n> \n> If we're entirely setting up the server again shouldn't this just be\n> another test_expect_success block?\n\nI only made one block at first because the test without negotiation is\nthere just for comparing object counts, but OK, I have split it up into\n2 blocks.\n\n> \n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n> > +\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n> > +'\n> > +\n> > +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n> > +\trm event &&\n> > +\trm -rf \"$SERVER\" &&\n> > +\ttest_create_repo \"$SERVER\" &&\n> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n> > +\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n> > +\ttest_i18ngrep \"push negotiation failed\" err\n> \n> s/test_i18ngrep/grep/, or actually better yet is there a reason not to\n> do test_cmp here? I'm pretty sure there's not.\n> \n> The reason I've got that unsubmitted series is because I found a bug\n> that was cleverly hidden away by an earlier such 'grep the output'\n> code/test you'd added recently.\n> \n> There *are* cases where we e.g. get STDERR output from the OS itself\n> about bad connections, or races, but we should at least:\n> \n> grep error: error-lines.actual &&\n> test_cmp error-lines.expect error-lines.actual\n> \n> To check that we get the errors we expected, and *only* those.\n\nI didn't want to make the test prescribe unnecessary details, but good\npoint about hiding bugs. I have switched it to what you describe.\n\n> \n> > +'\n> > +\n> > +# DO NOT add non-httpd-specific tests here, because the last part of this\n> > +# test script is only executed when httpd is available and enabled.\n> > +\n> > test_done\n> \n> Also, instead of this comment let's just create another\n> t*-fetch-push-http.sh test. Because:\n> \n> * This test is already really slow, it takes me 13s to run it now. I\n> haven't tested, but setting up apache will make it even slower.\n> \n> * It's also an anti-pattern to switch midway to needing an external\n> daemon v.s. doing it in a dedicated test.\n> \n> E.g. if you have the relevant GIT_* settings to run http:// tests,\n> but not git:// tests we'll skip the former entirely in\n> t5700-protocol-v1.sh, because we'll \"skip all\" as soon as we see we\n> can't start the git-daemon.\n> \n> That specifically isn't an issue here, but better to avoid setting up\n> the pattern.\n\nI think this is a pattern that we need, though. Sometimes there are\nclosely related fetch/push tests (as in here, and as in t5700) that want\nto test similar things across different protocols.\n\n> \n> * What *is* an issue with it here is that the \"skip all\" in TAP is only\n> handled before you run any tests, e.g. compare:\n> \n> $ prove ./t5700-protocol-v1.sh\n> ./t5700-protocol-v1.sh .. ok \n> All tests successful.\n> Files=1, Tests=21, 2 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr 0.80 csys = 1.62 CPU)\n> Result: PASS\n> \n> $ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n> ./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\n> Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr 0.00 csys = 0.03 CPU)\n> Result: NOTESTS\n> \n> $ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n> ./t5700-protocol-v1.sh .. ok \n> All tests successful.\n> Files=1, Tests=16, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.63 cusr 0.59 csys = 1.23 CPU)\n> Result: PASS\n> \n> I.e. if you stick an inclusion of lib-httpd.sh this late in a test\n> file we won't get a prominent notice saying we're categorically\n> skipping certain types of tests based on an external dependency.\n> \n> If you had your own dedicated test instead you'd get:\n> \n> $ GIT_TEST_HTTPD=false prove ./t5705-protocol-v2-http.sh \n> ./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)\n> Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.02 cusr 0.01 csys = 0.05 CPU)\n> Result: NOTESTS\n\nIs it useful to know the count of tests that are skipped? (The user\npresumably already knows that some are skipped because they set the\nenvironment variable in the first place.)\n"},{"id":"430110","messageId":"20210714193359.4083725-1-jonathantanmy@google.com","threadId":"55996","inReplyTo":"YO4UIGQPugvHcYrw@google.com","subject":"Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-14T19:33:59Z","receivedAt":"2021-07-14T19:34:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Jun 23, 2021 at 03:30:52PM -0700, Jonathan Tan wrote:\n> > \n> > Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> > did not test the case in which a remote advertises at least one ref. In\n> > such a case, \"remote_refs\" in get_commons_through_negotiation() in\n> > send-pack.c would also contain those refs with a zero ref->new_oid (in\n> > addition to the refs being pushed with a nonzero ref->new_oid). Passing\n> > them as negotiation tips to \"git fetch\" causes an error, so filter them\n> > out.\n> \n> So here we are filtering those redundant refs on the client side?\n\nYes, but I don't know what you mean by \"redundant\".\n\n> > @@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '\n> > \trm event &&\n> > \tmk_empty testrepo &&\n> > \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n> > +\ttest_commit -C testrepo unrelated_commit &&\n> \n> So now we are asking 'testrepo' to initially advertise that it also has\n> unrelated_commit, which we don't care about, and expect to work fine\n> anyway. Ok.\n\nYes.\n\n> Seems reasonable enough to me.\n> Reviewed-by: Emily Shaffer \n\nThanks.\n"},{"id":"430111","messageId":"20210714193506.4084421-1-jonathantanmy@google.com","threadId":"55996","inReplyTo":"878s293htx.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-14T19:35:06Z","receivedAt":"2021-07-14T19:35:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> >> index 9191620e50..2c50465cff 100644\n> >> --- a/builtin/fetch.c\n> >> +++ b/builtin/fetch.c\n> >> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n> >> \t\tif (!has_glob_specials(s)) {\n> >> \t\t\tstruct object_id oid;\n> >> \t\t\tif (get_oid(s, &oid))\n> >> -\t\t\t\tdie(\"%s is not a valid object\", s);\n> >> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n> >> +\t\t\tif (!has_object(the_repository, &oid, 0))\n> >> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n> > Any reason not to consolidate these, e.g. if (get_oid || !has_object)?\n> > Then we wouldn't dup the err string.\n> \n> Generally I'd agree, but aren't we explicitly conflating cases where\n> something is a valid way no name an object v.s. being certain that such\n> an object does not exist? I.e. this should be something like:\n> \n> if can't get_get():\n> error \"couldn't get the OID of revision '%s'\"\n> if can't look up fully-qualified OID:\n> error \"the OID '%s' does not exist\"\n> \n> Or something...\n\nGood point. I'll use wording similar to yours.\n"},{"id":"430135","messageId":"87sg0gzhys.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"20210714193506.4084421-1-jonathantanmy@google.com","subject":"Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T21:45:24Z","receivedAt":"2021-07-14T21:46:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Jonathan Tan wrote:\n\n>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> >> index 9191620e50..2c50465cff 100644\n>> >> --- a/builtin/fetch.c\n>> >> +++ b/builtin/fetch.c\n>> >> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n>> >> \t\tif (!has_glob_specials(s)) {\n>> >> \t\t\tstruct object_id oid;\n>> >> \t\t\tif (get_oid(s, &oid))\n>> >> -\t\t\t\tdie(\"%s is not a valid object\", s);\n>> >> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n>> >> +\t\t\tif (!has_object(the_repository, &oid, 0))\n>> >> +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n>> > Any reason not to consolidate these, e.g. if (get_oid || !has_object)?\n>> > Then we wouldn't dup the err string.\n>> \n>> Generally I'd agree, but aren't we explicitly conflating cases where\n>> something is a valid way no name an object v.s. being certain that such\n>> an object does not exist? I.e. this should be something like:\n>> \n>> if can't get_get():\n>> error \"couldn't get the OID of revision '%s'\"\n>> if can't look up fully-qualified OID:\n>> error \"the OID '%s' does not exist\"\n>> \n>> Or something...\n>\n> Good point. I'll use wording similar to yours.\n\nI stole the former from a quick grepping around:\n\n builtin/bisect--helper.c: res = error(_(\"couldn't get the oid of the rev '%s'\"), rev)\n\nThen http-push.c suggests \"perhaps you need to fetch\" for\nhas_object_file(), but I don't know if it applies here...\n"},{"id":"430137","messageId":"87pmvkzh2g.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"20210714193220.4083070-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-14T21:51:05Z","receivedAt":"2021-07-14T22:06:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 14 2021, Jonathan Tan wrote:\n\n>> > +test_expect_success 'http push with negotiation' '\n>> > +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n>> > +\tURI=\"$HTTPD_URL/smart/server\" &&\n>> > +\n>> > +\trm -rf client &&\n>> \n>> Nothing in this test file has created a \"client\" directory at this\n>> point, so we shouldn't have this to remove it.\n>> \n>> I think the real reason for this is that you're just copy/pasting this\n>> from elsewhere. I've got an unsubmitted series where I fixed various\n>> callsites in these t/t*http* tests to use test_when_finished instead.\n>> \n>> This pattern of having the next test clean up after you leads to bad\n>> inter-dependencies, there were a few things broken e.g. if earlier tests\n>> were skipped. Let's not continue that pattern.\n>\n> OK - I have changed it so that each test is independent and cleans up after\n> itself.\n>\n>> \n>> > +\tgit init client &&\n>> > +\ttest_commit -C client first_commit &&\n>> > +\ttest_commit -C client second_commit &&\n>> > +\n>> > +\t# Without negotiation\n>> > +\ttest_create_repo \"$SERVER\" &&\n>> \n>> s/test_create_repo/git init/g\n>\n> Done.\n>\n>> \n>> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n>> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n>> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n>> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n>> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n>> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n>> > +\n>> > +\t# Same commands, but with negotiation\n>> > +\trm event &&\n>> > +\trm -rf \"$SERVER\" &&\n>> \n>> Ditto test_when_finished, or actually:\n>> \n>> > +\ttest_create_repo \"$SERVER\" &&\n>> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n>\n> In my current version, I have changed everything to \"git init\". Should I\n> use \"test_create_repo\" instead?\n\nSorry I just copy/pasted that from your version, yes indeed\ns/test_create_repo/git init/ (a trivial thing, but they're 1=1\nequivalent these days after a recent change of mine in\ntest-lib-functions.sh).\n\n>> \n>> If we're entirely setting up the server again shouldn't this just be\n>> another test_expect_success block?\n>\n> I only made one block at first because the test without negotiation is\n> there just for comparing object counts, but OK, I have split it up into\n> 2 blocks.\n>\n>> \n>> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n>> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n>> > +\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n>> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n>> > +\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n>> > +'\n>> > +\n>> > +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '\n>> > +\trm event &&\n>> > +\trm -rf \"$SERVER\" &&\n>> > +\ttest_create_repo \"$SERVER\" &&\n>> > +\ttest_config -C \"$SERVER\" http.receivepack true &&\n>> > +\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n>> > +\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n>> > +\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n>> > +\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n>> > +\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n>> > +\ttest_i18ngrep \"push negotiation failed\" err\n>> \n>> s/test_i18ngrep/grep/, or actually better yet is there a reason not to\n>> do test_cmp here? I'm pretty sure there's not.\n>> \n>> The reason I've got that unsubmitted series is because I found a bug\n>> that was cleverly hidden away by an earlier such 'grep the output'\n>> code/test you'd added recently.\n>> \n>> There *are* cases where we e.g. get STDERR output from the OS itself\n>> about bad connections, or races, but we should at least:\n>> \n>> grep error: error-lines.actual &&\n>> test_cmp error-lines.expect error-lines.actual\n>> \n>> To check that we get the errors we expected, and *only* those.\n>\n> I didn't want to make the test prescribe unnecessary details, but good\n> point about hiding bugs. I have switched it to what you describe.\n>\n>> \n>> > +'\n>> > +\n>> > +# DO NOT add non-httpd-specific tests here, because the last part of this\n>> > +# test script is only executed when httpd is available and enabled.\n>> > +\n>> > test_done\n>> \n>> Also, instead of this comment let's just create another\n>> t*-fetch-push-http.sh test. Because:\n>> \n>> * This test is already really slow, it takes me 13s to run it now. I\n>> haven't tested, but setting up apache will make it even slower.\n>> \n>> * It's also an anti-pattern to switch midway to needing an external\n>> daemon v.s. doing it in a dedicated test.\n>> \n>> E.g. if you have the relevant GIT_* settings to run http:// tests,\n>> but not git:// tests we'll skip the former entirely in\n>> t5700-protocol-v1.sh, because we'll \"skip all\" as soon as we see we\n>> can't start the git-daemon.\n>> \n>> That specifically isn't an issue here, but better to avoid setting up\n>> the pattern.\n>\n> I think this is a pattern that we need, though. Sometimes there are\n> closely related fetch/push tests (as in here, and as in t5700) that want\n> to test similar things across different protocols.\n\nYeah, in my split-up WIP there's a bit of that, in my split-up of them I\ndidn't find the end result harder to read, e.g. in\nt/t5702-protocol-v2.sh it's mostly in different sections of the file,\nfirst git://, then file:// and then http://, with some misc in-between,\nhaving a t/t57*-protocol-{git,file,http}.sh didn't make things harder to\nread.\n\n>> \n>> * What *is* an issue with it here is that the \"skip all\" in TAP is only\n>> handled before you run any tests, e.g. compare:\n>> \n>> $ prove ./t5700-protocol-v1.sh\n>> ./t5700-protocol-v1.sh .. ok \n>> All tests successful.\n>> Files=1, Tests=21, 2 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr 0.80 csys = 1.62 CPU)\n>> Result: PASS\n>> \n>> $ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n>> ./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)\n>> Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr 0.00 csys = 0.03 CPU)\n>> Result: NOTESTS\n>> \n>> $ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh\n>> ./t5700-protocol-v1.sh .. ok \n>> All tests successful.\n>> Files=1, Tests=16, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.63 cusr 0.59 csys = 1.23 CPU)\n>> Result: PASS\n>> \n>> I.e. if you stick an inclusion of lib-httpd.sh this late in a test\n>> file we won't get a prominent notice saying we're categorically\n>> skipping certain types of tests based on an external dependency.\n>> \n>> If you had your own dedicated test instead you'd get:\n>> \n>> $ GIT_TEST_HTTPD=false prove ./t5705-protocol-v2-http.sh \n>> ./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)\n>> Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.02 cusr 0.01 csys = 0.05 CPU)\n>> Result: NOTESTS\n>\n> Is it useful to know the count of tests that are skipped? (The user\n> presumably already knows that some are skipped because they set the\n> environment variable in the first place.)\n\nA thing I do really often is to run some glob of tests, say I'm working\non git-fetch, and run *fetch*, I'd get this in the output:\n \n [23:58:15] t5527-fetch-odd-refs.sh ......................... ok 121 ms ( 0.01 usr 0.00 sys + 0.72 cusr 0.38 csys = 1.11 CPU)\n [23:58:15] t5535-fetch-push-symref.sh ...................... ok 90 ms ( 0.01 usr 0.00 sys + 0.14 cusr 0.02 csys = 0.17 CPU)\n [23:58:16] t5536-fetch-conflicts.sh ........................ ok 180 ms ( 0.01 usr 0.01 sys + 0.28 cusr 0.07 csys = 0.37 CPU)\n [23:58:16] t5539-fetch-http-shallow.sh ..................... skipped: no web server found at '/usr/sbin/apache'\n [23:58:16] t5550-http-fetch-dumb.sh ........................ skipped: no web server found at '/usr/sbin/apache'\n [23:58:16] t5551-http-fetch-smart.sh ....................... skipped: no web server found at '/usr/sbin/apache'\n [23:58:16] t5554-noop-fetch-negotiator.sh .................. ok 3 ms ( 0.00 usr 0.00 sys + 0.07 cusr 0.02 csys = 0.09 CPU)\n [23:58:16] t5537-fetch-shallow.sh .......................... ok 641 ms ( 0.03 usr 0.01 sys + 0.87 cusr 0.31 csys = 1.22 CPU)\n [23:58:16] t5582-fetch-negative-refspec.sh ................. ok 564 ms ( 0.02 usr 0.01 sys + 0.77 cusr 0.32 csys = 1.12 CPU)\n\nIt's useful to know that a portion of the tests was skipped entirely,\nwhereas if we do git:// tests first, and then later http:// the loss of\ncoverage is silent (unless you run it individually under --verbose) or\nwhatever.\n\nThis is really useful for the http tests iin particular, it's really\ncommon not to have apache installed.\n"},{"id":"430252","messageId":"cover.1626370766.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1624486920.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] Push negotiation fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-15T17:44:29Z","receivedAt":"2021-07-15T17:44:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, Emily and Ævar for your reviews. There are no changes to C code\nin this version of the patch set, but the tests have been updated\nfollowing both your comments.\n\nI have ended up splitting up the HTTP tests away from t5516 into its own\nfile. I had to use the number 5549 because it was the next available\none. There is one Bash function that is duplicated - I don't foresee\nmany such functions being duplicated, but if that happens, I guess we\ncan always created a helper script somewhere else.\n\nJonathan Tan (3):\n send-pack: fix push.negotiate with remote helper\n send-pack: fix push nego. when remote has refs\n fetch: die on invalid --negotiation-tip hash\n\n builtin/fetch.c | 4 ++-\n builtin/send-pack.c | 1 +\n send-pack.c | 6 ++--\n t/t5510-fetch.sh | 13 +++++++\n t/t5516-fetch-push.sh | 4 ++-\n t/t5549-fetch-push-http.sh | 72 ++++++++++++++++++++++++++++++++++++++\n 6 files changed, 96 insertions(+), 4 deletions(-)\n create mode 100755 t/t5549-fetch-push-http.sh\n\nRange-diff against v1:\n1: eb0dce9f49 < -: ---------- send-pack: fix push.negotiate with remote helper\n-: ---------- > 1: af40bee611 send-pack: fix push.negotiate with remote helper\n2: 3bf6921d96 ! 2: c841693303 send-pack: fix push nego. when remote has refs\n @@ t/t5516-fetch-push.sh: test_expect_success 'push with negotiation proceeds anywa\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n -@@ t/t5516-fetch-push.sh: test_expect_success 'http push with negotiation' '\n - \t# Without negotiation\n - \ttest_create_repo \"$SERVER\" &&\n - \ttest_config -C \"$SERVER\" http.receivepack true &&\n -+\ttest_commit -C \"$SERVER\" unrelated_commit &&\n - \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n - \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n - \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n -@@ t/t5516-fetch-push.sh: test_expect_success 'http push with negotiation' '\n - \trm -rf \"$SERVER\" &&\n - \ttest_create_repo \"$SERVER\" &&\n +@@ t/t5516-fetch-push.sh: test_expect_success 'denyCurrentBranch and worktrees' '\n + \tgit -C cloned push origin HEAD:new-wt &&\n + \ttest_must_fail git -C cloned push --delete origin new-wt\n + '\n +-\n + test_done\n +\n + ## t/t5549-fetch-push-http.sh ##\n +@@ t/t5549-fetch-push-http.sh: setup_client_and_server () {\n + \tgit init \"$SERVER\" &&\n + \ttest_when_finished 'rm -rf \"$SERVER\"' &&\n \ttest_config -C \"$SERVER\" http.receivepack true &&\n +\ttest_commit -C \"$SERVER\" unrelated_commit &&\n \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n - \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit &&\n - \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n + \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit\n + }\n3: 1453a1b4e5 ! 3: 476da85859 fetch: die on invalid --negotiation-tip hash\n @@ builtin/fetch.c: static void add_negotiation_tips(struct git_transport_options *\n -\t\t\t\tdie(\"%s is not a valid object\", s);\n +\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n +\t\t\tif (!has_object(the_repository, &oid, 0))\n -+\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n ++\t\t\t\tdie(_(\"the object %s does not exist\"), s);\n \t\t\toid_array_append(oids, &oid);\n \t\t\tcontinue;\n \t\t}\n @@ t/t5510-fetch.sh: test_expect_success '--negotiation-tip understands abbreviated\n +\t\t--negotiation-tip=alpha_1 \\\n +\t\t--negotiation-tip=$(test_oid zero) \\\n +\t\torigin alpha_s beta_s 2>err &&\n -+\ttest_i18ngrep \"is not a valid object\" err\n ++\tcat >fatal-expect <<-EOF &&\n ++\tfatal: the object $(test_oid zero) does not exist\n ++EOF\n ++\tgrep fatal: err >fatal-actual &&\n ++\ttest_cmp fatal-expect fatal-actual\n +'\n +\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.32.0.93.g670b81a890-goog\n\n"},{"id":"430253","messageId":"af40bee611d4dbbd9bb5f1466bec0be118b74165.1626370766.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1626370766.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-15T17:44:30Z","receivedAt":"2021-07-15T17:44:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\nintroduced the push.negotiate config variable and included a test. The\ntest only covered pushing without a remote helper, so the fact that\npushing with a remote helper doesn't work went unnoticed.\n\nThis is ultimately caused by the \"url\" field not being set in the args\nstruct. This field being unset probably went unnoticed because besides\npush negotiation, this field is only used to generate a \"pushee\" line in\na push cert (and if not given, no such line is generated). Therefore,\nset this field.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n builtin/send-pack.c | 1 +\n t/t5549-fetch-push-http.sh | 71 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 72 insertions(+)\n create mode 100755 t/t5549-fetch-push-http.sh\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex a7e01667b0..729dea1d25 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \targs.atomic = atomic;\n \targs.stateless_rpc = stateless_rpc;\n \targs.push_options = push_options.nr ? &push_options : NULL;\n+\targs.url = dest;\n \n \tif (from_stdin) {\n \t\tif (args.stateless_rpc) {\ndiff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh\nnew file mode 100755\nindex 0000000000..f50d584881\n--- /dev/null\n+++ b/t/t5549-fetch-push-http.sh\n@@ -0,0 +1,71 @@\n+#!/bin/sh\n+\n+test_description='fetch/push functionality using the HTTP protocol'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+SERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\"\n+URI=\"$HTTPD_URL/smart/server\"\n+\n+grep_wrote () {\n+\tobject_count=$1\n+\tfile_name=$2\n+\tgrep 'write_pack_file/wrote.*\"value\":\"'$1'\"' $2\n+}\n+\n+setup_client_and_server () {\n+\tgit init client &&\n+\ttest_when_finished 'rm -rf client' &&\n+\ttest_commit -C client first_commit &&\n+\ttest_commit -C client second_commit &&\n+\n+\tgit init \"$SERVER\" &&\n+\ttest_when_finished 'rm -rf \"$SERVER\"' &&\n+\ttest_config -C \"$SERVER\" http.receivepack true &&\n+\tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit\n+}\n+\n+test_expect_success 'push without negotiation (for comparing object counts with the next test)' '\n+\tsetup_client_and_server &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n+\ttest_when_finished \"rm -f event\" &&\n+\tgrep_wrote 6 event # 2 commits, 2 trees, 2 blobs\n+'\n+\n+test_expect_success 'push with negotiation' '\n+\tsetup_client_and_server &&\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c protocol.version=2 -c push.negotiate=1 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main &&\n+\ttest_when_finished \"rm -f event\" &&\n+\tgrep_wrote 3 event # 1 commit, 1 tree, 1 blob\n+'\n+\n+test_expect_success 'push with negotiation proceeds anyway even if negotiation fails' '\n+\tsetup_client_and_server &&\n+\n+\t# Use protocol v0 to make negotiation fail (because protocol v0 does\n+\t# not support the \"wait-for-done\" capability, which is required for\n+\t# push negotiation)\n+\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" git -C client -c push.negotiate=1 \\\n+\t\tpush \"$URI\" refs/heads/main:refs/remotes/origin/main 2>err &&\n+\ttest_when_finished \"rm -f event\" &&\n+\tgrep_wrote 6 event && # 2 commits, 2 trees, 2 blobs\n+\n+\tcat >warning-expect <<-EOF &&\n+\twarning: --negotiate-only requires protocol v2\n+\twarning: push negotiation failed; proceeding anyway with push\n+EOF\n+\tgrep warning: err >warning-actual &&\n+\ttest_cmp warning-expect warning-actual\n+'\n+\n+test_done\n-- \n2.32.0.93.g670b81a890-goog\n\n"},{"id":"430254","messageId":"c8416933035849e40b88c29f1d5fa91064ca0c8a.1626370766.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1626370766.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] send-pack: fix push nego. when remote has refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-15T17:44:31Z","receivedAt":"2021-07-15T17:44:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\ndid not test the case in which a remote advertises at least one ref. In\nsuch a case, \"remote_refs\" in get_commons_through_negotiation() in\nsend-pack.c would also contain those refs with a zero ref->new_oid (in\naddition to the refs being pushed with a nonzero ref->new_oid). Passing\nthem as negotiation tips to \"git fetch\" causes an error, so filter them\nout.\n\n(The exact error that would happen in \"git fetch\" in this case is a\nsegmentation fault, which is unwanted. This will be fixed in the\nsubsequent commit.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n send-pack.c | 6 ++++--\n t/t5516-fetch-push.sh | 4 +++-\n t/t5549-fetch-push-http.sh | 1 +\n 3 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 9cb9f71650..85945becf0 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,\n \tchild.no_stdin = 1;\n \tchild.out = -1;\n \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n-\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\t}\n \tstrvec_push(&child.args, url);\n \n \tif (start_command(&child))\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 0916f76302..4db8edd9c8 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '\n \t# Without negotiation\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \techo now pushing without negotiation &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&\n@@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '\n \trm event &&\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n \tgrep_wrote 2 event # 1 commit, 1 tree\n@@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f\n \trm event &&\n \tmk_empty testrepo &&\n \tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\ttest_commit -C testrepo unrelated_commit &&\n \tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n \tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n \t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n@@ -1767,5 +1770,4 @@ test_expect_success 'denyCurrentBranch and worktrees' '\n \tgit -C cloned push origin HEAD:new-wt &&\n \ttest_must_fail git -C cloned push --delete origin new-wt\n '\n-\n test_done\ndiff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh\nindex f50d584881..2cdebcb735 100755\n--- a/t/t5549-fetch-push-http.sh\n+++ b/t/t5549-fetch-push-http.sh\n@@ -27,6 +27,7 @@ setup_client_and_server () {\n \tgit init \"$SERVER\" &&\n \ttest_when_finished 'rm -rf \"$SERVER\"' &&\n \ttest_config -C \"$SERVER\" http.receivepack true &&\n+\ttest_commit -C \"$SERVER\" unrelated_commit &&\n \tgit -C client push \"$URI\" first_commit:refs/remotes/origin/first_commit &&\n \tgit -C \"$SERVER\" config receive.hideRefs refs/remotes/origin/first_commit\n }\n-- \n2.32.0.93.g670b81a890-goog\n\n"},{"id":"430255","messageId":"476da85859eb333573c3f87cc7284b8b28032214.1626370766.git.jonathantanmy@google.com","threadId":"55996","inReplyTo":"cover.1626370766.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-15T17:44:32Z","receivedAt":"2021-07-15T17:44:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If a full hexadecimal hash is given as a --negotiation-tip to \"git\nfetch\", and that hash does not correspond to an object, \"git fetch\" will\nsegfault if --negotiate-only is given and will silently ignore that hash\notherwise. Make these cases fatal errors, just like the case when an\ninvalid ref name or abbreviated hash is given.\n\nWhile at it, mark the error messages as translatable.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n builtin/fetch.c | 4 +++-\n t/t5510-fetch.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex dfde96a435..0aeb043840 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)\n \t\tif (!has_glob_specials(s)) {\n \t\t\tstruct object_id oid;\n \t\t\tif (get_oid(s, &oid))\n-\t\t\t\tdie(\"%s is not a valid object\", s);\n+\t\t\t\tdie(_(\"%s is not a valid object\"), s);\n+\t\t\tif (!has_object(the_repository, &oid, 0))\n+\t\t\t\tdie(_(\"the object %s does not exist\"), s);\n \t\t\toid_array_append(oids, &oid);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex e83b2a6506..a0faf0dd94 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1214,6 +1214,19 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '\n \tcheck_negotiation_tip\n '\n \n+test_expect_success '--negotiation-tip rejects missing OIDs' '\n+\tsetup_negotiation_tip server server 0 &&\n+\ttest_must_fail git -C client fetch \\\n+\t\t--negotiation-tip=alpha_1 \\\n+\t\t--negotiation-tip=$(test_oid zero) \\\n+\t\torigin alpha_s beta_s 2>err &&\n+\tcat >fatal-expect <<-EOF &&\n+\tfatal: the object $(test_oid zero) does not exist\n+EOF\n+\tgrep fatal: err >fatal-actual &&\n+\ttest_cmp fatal-expect fatal-actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.32.0.93.g670b81a890-goog\n\n"},{"id":"430260","messageId":"xmqqtukvs8ks.fsf@gitster.g","threadId":"55996","inReplyTo":"cover.1626370766.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/3] Push negotiation fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T19:03:47Z","receivedAt":"2021-07-15T19:04:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks, Emily and Ævar for your reviews. There are no changes to C code\n> in this version of the patch set, but the tests have been updated\n> following both your comments.\n\nThank you all.\n\nWill queue.\n"},{"id":"431289","messageId":"87y29sqjd1.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"af40bee611d4dbbd9bb5f1466bec0be118b74165.1626370766.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T07:56:38Z","receivedAt":"2021-07-27T07:57:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 15 2021, Jonathan Tan wrote:\n\n\n> +\tgit init client &&\n> +\ttest_when_finished 'rm -rf client' &&\n\nNit (don't think this needs a re-roll): Better to do test_when_finished\nbefore whatever creates the thing, in case the command has a bug/errors\nout etc. during development. Ditto the below.\n"},{"id":"431292","messageId":"87v94wqi6k.fsf@evledraar.gmail.com","threadId":"55996","inReplyTo":"c8416933035849e40b88c29f1d5fa91064ca0c8a.1626370766.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-27T08:09:35Z","receivedAt":"2021-07-27T08:23:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 15 2021, Jonathan Tan wrote:\n\n> Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> did not test the case in which a remote advertises at least one ref. In\n> such a case, \"remote_refs\" in get_commons_through_negotiation() in\n> send-pack.c would also contain those refs with a zero ref->new_oid (in\n> addition to the refs being pushed with a nonzero ref->new_oid). Passing\n> them as negotiation tips to \"git fetch\" causes an error, so filter them\n> out.\n>\n> (The exact error that would happen in \"git fetch\" in this case is a\n> segmentation fault, which is unwanted. This will be fixed in the\n> subsequent commit.)\n\nLet's add the test from the subsequent here as a test_expect_failure and\nflip it to \"success\".\n\n> @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,\n> \tchild.no_stdin = 1;\n> \tchild.out = -1;\n> \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n> -\tfor (ref = remote_refs; ref; ref = ref->next)\n> -\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\tfor (ref = remote_refs; ref; ref = ref->next) {\n> +\t\tif (!is_null_oid(&ref->new_oid))\n> +\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\t}\n> \tstrvec_push(&child.args, url);\n\nThis will run into my eff40457a4 (fetch: fix segfault in\n--negotiate-only without --negotiation-tip=*, 2021-07-08) if we supply a\n--negotiate-only without --negotiation-tip=, but trying it it looks like\neven when you push to an empty repo and your repo is itself empty we'll\nalways add the tip you're pushing as the negotiation tip.\n\nLet's add a test for that, i.e. I instrumented your test to check what\nhappens whe I do the push without any remote/local refs, both for\none/both cases (and both combinations), it seems to work...\n\nFor code that's doing a loop over \"refs\" testing that seems to be\nworthwhile, i.e. we don't actually depend on \"refs\" in the sense that\nthey exist, but the refs we've constructed in-memory to be created on\nthe remote, correct?\n\nI.e. this on top would be OK (not saying you need this, but I for one\nwould find it easier to follow with this):\n\t\n\tdiff --git a/send-pack.c b/send-pack.c\n\tindex b3a495b7b1..d1e231076c 100644\n\t--- a/send-pack.c\n\t+++ b/send-pack.c\n\t@@ -420,15 +420,20 @@ static void get_commons_through_negotiation(const char *url,\n\t \tstruct child_process child = CHILD_PROCESS_INIT;\n\t \tconst struct ref *ref;\n\t \tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n\t+\tint got_tip = 0;\n\t \n\t \tchild.git_cmd = 1;\n\t \tchild.no_stdin = 1;\n\t \tchild.out = -1;\n\t \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n\t \tfor (ref = remote_refs; ref; ref = ref->next) {\n\t-\t\tif (!is_null_oid(&ref->new_oid))\n\t-\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n\t+\t\tif (is_null_oid(&ref->new_oid))\n\t+\t\t\tcontinue;\n\t+\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n\t+\t\tgot_tip = 1;\n\t \t}\n\t+\tif (!got_tip)\n\t+\t\tBUG(\"should get at least one ref tip, even with no remote/local refs\");\n\t \tstrvec_push(&child.args, url);\n\t \n\t \tif (start_command(&child))\n\nBut also: looking at the trace output we already have the ref\nadvertisement at this point, so in the case of an empty repo we'll see\nit has no refs, but then we're going to provide a --negotiation-tip=*\npointing to our local OID anyway.\n\nThat seems like a fairly non-obvious edge case that should be called out\n/ tested.\n\nI.e. aren't we at least just going to engage in redundant work there in\ntrying to negotiate with empty repos, or is it going to noop anyway.\n\nOr maybe we'll get lucky and they have the OID already, they just\nrecently deleted their reference(s), then we won't need to send as much\nover? Is that what this is trying to do?\n\nBut hrm, won't that sort of thing increase the odds of repository\ncorruption?\n\nI.e. now we make the implicit assumption that an OID we see in the\nadvertisement is one the server isn't going to aggressively prune while\nour push is underday (Jeff King has a good E-Mail summarizing that\nsomewhere, not digging it up now, but I could...).\n\nSo such a remote will negotiate with us using that OID, but unlike with\nadvertised OIDs we can't safely assume that the OID won't be racily\ndeleted during our negotiation.\n\nOr maybe I'm entirely wrong here....\n"},{"id":"431315","messageId":"YQA4bzFxdOHSQva7@coredump.intra.peff.net","threadId":"55996","inReplyTo":"87v94wqi6k.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-27T16:46:39Z","receivedAt":"2021-07-27T16:46:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 27, 2021 at 10:09:35AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I.e. now we make the implicit assumption that an OID we see in the\n> advertisement is one the server isn't going to aggressively prune while\n> our push is underday (Jeff King has a good E-Mail summarizing that\n> somewhere, not digging it up now, but I could...).\n> \n> So such a remote will negotiate with us using that OID, but unlike with\n> advertised OIDs we can't safely assume that the OID won't be racily\n> deleted during our negotiation.\n\nI haven't been following the push-negotiation stuff closely, nor do I\nhave a specific email in mind that summarizes this. So take my input\nwith a grain of salt. But...\n\nWouldn't this also be a problem for multi-round fetch negotiation? An\nobject may become unreachable or even go away entirely during the course\nof a fetch. I'd expect that to be rare, but when it does happen, for the\nfetch to end up barfing (the server says \"hey, I don't know about that\nobject\").\n\n-Peff\n"},{"id":"431340","messageId":"20210727211142.2570867-1-jonathantanmy@google.com","threadId":"55996","inReplyTo":"87v94wqi6k.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-27T21:11:42Z","receivedAt":"2021-07-27T21:11:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Thu, Jul 15 2021, Jonathan Tan wrote:\n> \n> > Commit 477673d6f3 (\"send-pack: support push negotiation\", 2021-05-05)\n> > did not test the case in which a remote advertises at least one ref. In\n> > such a case, \"remote_refs\" in get_commons_through_negotiation() in\n> > send-pack.c would also contain those refs with a zero ref->new_oid (in\n> > addition to the refs being pushed with a nonzero ref->new_oid). Passing\n> > them as negotiation tips to \"git fetch\" causes an error, so filter them\n> > out.\n> >\n> > (The exact error that would happen in \"git fetch\" in this case is a\n> > segmentation fault, which is unwanted. This will be fixed in the\n> > subsequent commit.)\n> \n> Let's add the test from the subsequent here as a test_expect_failure and\n> flip it to \"success\".\n\nWhat is the subsequent?\n\n> > @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,\n> > \tchild.no_stdin = 1;\n> > \tchild.out = -1;\n> > \tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n> > -\tfor (ref = remote_refs; ref; ref = ref->next)\n> > -\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> > +\tfor (ref = remote_refs; ref; ref = ref->next) {\n> > +\t\tif (!is_null_oid(&ref->new_oid))\n> > +\t\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> > +\t}\n> > \tstrvec_push(&child.args, url);\n> \n> This will run into my eff40457a4 (fetch: fix segfault in\n> --negotiate-only without --negotiation-tip=*, 2021-07-08) if we supply a\n> --negotiate-only without --negotiation-tip=, but trying it it looks like\n> even when you push to an empty repo and your repo is itself empty we'll\n> always add the tip you're pushing as the negotiation tip.\n> \n> Let's add a test for that, i.e. I instrumented your test to check what\n> happens whe I do the push without any remote/local refs, both for\n> one/both cases (and both combinations), it seems to work...\n\nI'm not sure how useful this no-ref test will be, because if my existing\ntests are correct, the thing we're pushing is guaranteed to be in this\nlist (so the list will be non-empty).\n\n> For code that's doing a loop over \"refs\" testing that seems to be\n> worthwhile, i.e. we don't actually depend on \"refs\" in the sense that\n> they exist, but the refs we've constructed in-memory to be created on\n> the remote, correct?\n\nYes.\n\n> But also: looking at the trace output we already have the ref\n> advertisement at this point, so in the case of an empty repo we'll see\n> it has no refs, but then we're going to provide a --negotiation-tip=*\n> pointing to our local OID anyway.\n\nHmm...are you running under protocol v0? In protocol v2, there should be\nno ref advertisement at this point.\n\n> That seems like a fairly non-obvious edge case that should be called out\n> / tested.\n> \n> I.e. aren't we at least just going to engage in redundant work there in\n> trying to negotiate with empty repos, or is it going to noop anyway.\n> \n> Or maybe we'll get lucky and they have the OID already, they just\n> recently deleted their reference(s), then we won't need to send as much\n> over? Is that what this is trying to do?\n> \n> But hrm, won't that sort of thing increase the odds of repository\n> corruption?\n\nNo, trying to be lucky in finding an OID that the server has no plans of\nadvertising is not the aim.\n\n> I.e. now we make the implicit assumption that an OID we see in the\n> advertisement is one the server isn't going to aggressively prune while\n> our push is underday (Jeff King has a good E-Mail summarizing that\n> somewhere, not digging it up now, but I could...).\n> \n> So such a remote will negotiate with us using that OID, but unlike with\n> advertised OIDs we can't safely assume that the OID won't be racily\n> deleted during our negotiation.\n> \n> Or maybe I'm entirely wrong here....\n\nThere's always the risk that the server will say it has something and\nthen aggressively prune it, but I think that all fetch/push code has to\ndeal with it. A more realistic scenario is that one server in a\nload-balanced arrangement advertises a commit that the other does not\nhave, but we are unlikely to be affected by that here because the ref\nnegotiation would usually concern old commits that the local user has\nbuilt upon, not the very latest commits that someone else just pushed.\n"}]}