Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper
flat view
From: Emily Shaffer <hidden>
Date: 2021-07-13 22:23:43
On Wed, Jun 23, 2021 at 03:30:51PM -0700, Jonathan Tan wrote:
quoted hunk ↗ jump to hunk
Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05) introduced the push.negotiate config variable and included a test. The test only covered pushing without a remote helper, so the fact that pushing with a remote helper doesn't work went unnoticed. This is ultimately caused by the "url" field not being set in the args struct. This field being unset probably went unnoticed because besides push negotiation, this field is only used to generate a "pushee" line in a push cert (and if not given, no such line is generated). Therefore, set this field. Signed-off-by: Jonathan Tan <redacted> --- builtin/send-pack.c | 1 + t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)diff --git a/builtin/send-pack.c b/builtin/send-pack.c index a7e01667b0..729dea1d25 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c@@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) args.atomic = atomic; args.stateless_rpc = stateless_rpc; args.push_options = push_options.nr ? &push_options : NULL; + args.url = dest;
Sure, the fix itself is small and inoffensive. And the rest of the patch is regression testing.
quoted hunk ↗ jump to hunk
if (from_stdin) { if (args.stateless_rpc) {diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 0916f76302..5ce32e531a 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh@@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' ' test_must_fail git -C cloned push --delete origin new-wt ' +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd
Ah, so fetch-push test wasn't doing any HTTP testing whatsoever. Does that mean there is a better place for these to go? Or does it mean that fetch-push test was under-testing?
quoted hunk ↗ jump to hunk
+ +test_expect_success 'http push with negotiation' ' + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && + URI="$HTTPD_URL/smart/server" && + + rm -rf client && + git init client && + test_commit -C client first_commit && + test_commit -C client second_commit && + + # Without negotiation + test_create_repo "$SERVER" && + test_config -C "$SERVER" http.receivepack true && + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
Pushing a branch with just the first commit...
+ git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
transfer.hideRefs (referenced by receive.hideRefs) says this ref will be omitted from advertisement, so we are forcing either an inefficient push or a negotiation to occur, by having the server initially claim not to know about it. But it's only omitted from the *initial* advertisement, so it will be advertised in later rounds of negotiation, right?
+ GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 \ + push "$URI" refs/heads/main:refs/remotes/origin/main &&
And then from 'main' we push first_commit and second_commit?
+ grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
Nice, I like the comment - this helps.
quoted hunk ↗ jump to hunk
+ + # Same commands, but with negotiation + rm event && + rm -rf "$SERVER" &&
Ok, clean up the trace and the server so we can start over, but we don't need to recreate the client commits because the server doesn't know about them anyway. Fine.
quoted hunk ↗ jump to hunk
+ test_create_repo "$SERVER" && + test_config -C "$SERVER" http.receivepack true && + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit && + git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit && + GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 -c push.negotiate=1 \ + push "$URI" refs/heads/main:refs/remotes/origin/main &&
And then here's the same set of commands with push negotiation, ok.
+ grep_wrote 3 event # 1 commit, 1 tree, 1 blob
Is there any reason the event counts would change or be non-deterministic outside of negotiation? Or, in other words, is this potentially flaky?
quoted hunk ↗ jump to hunk
+' + +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' ' + rm event && + rm -rf "$SERVER" && + test_create_repo "$SERVER" && + test_config -C "$SERVER" http.receivepack true && + git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
Hmm, this relies on 'client' being in the same state the above test left it. Probably better to recreate it or at least leave a loud warning about it in a comment above this test definition...
quoted hunk ↗ jump to hunk
+ git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit && + GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c push.negotiate=1 \ + push "$URI" refs/heads/main:refs/remotes/origin/main 2>err &&
And we're pushing with protocol v0 so no negotiation can occur here, right?
+ grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs + test_i18ngrep "push negotiation failed" err +' + +# DO NOT add non-httpd-specific tests here, because the last part of this +# test script is only executed when httpd is available and enabled. + test_done -- 2.32.0.288.g62a8d224e6-goog
Thanks for answering novice questions :) - Emily