Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-07-13 23:33:17
On Wed, Jun 23 2021, 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; 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 + +test_expect_success 'http push with negotiation' ' + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && + URI="$HTTPD_URL/smart/server" && + + rm -rf client &&
Nothing in this test file has created a "client" directory at this point, so we shouldn't have this to remove it. I think the real reason for this is that you're just copy/pasting this from elsewhere. I've got an unsubmitted series where I fixed various callsites in these t/t*http* tests to use test_when_finished instead. This pattern of having the next test clean up after you leads to bad inter-dependencies, there were a few things broken e.g. if earlier tests were skipped. Let's not continue that pattern.
+ git init client && + test_commit -C client first_commit && + test_commit -C client second_commit && + + # Without negotiation + test_create_repo "$SERVER" &&
s/test_create_repo/git init/g
+ 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 \ + push "$URI" refs/heads/main:refs/remotes/origin/main && + grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs + + # Same commands, but with negotiation + rm event && + rm -rf "$SERVER" &&
Ditto test_when_finished, or actually:
+ test_create_repo "$SERVER" && + test_config -C "$SERVER" http.receivepack true &&
If we're entirely setting up the server again shouldn't this just be another test_expect_success block?
+ 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 && + grep_wrote 3 event # 1 commit, 1 tree, 1 blob +' + +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 && + 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 && + grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs + test_i18ngrep "push negotiation failed" err
s/test_i18ngrep/grep/, or actually better yet is there a reason not to
do test_cmp here? I'm pretty sure there's not.
The reason I've got that unsubmitted series is because I found a bug
that was cleverly hidden away by an earlier such 'grep the output'
code/test you'd added recently.
There *are* cases where we e.g. get STDERR output from the OS itself
about bad connections, or races, but we should at least:
grep error: <stderr >error-lines.actual &&
test_cmp error-lines.expect error-lines.actual
To check that we get the errors we expected, and *only* those.
+' + +# 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
Also, instead of this comment let's just create another
t*-fetch-push-http.sh test. Because:
* This test is already really slow, it takes me 13s to run it now. I
haven't tested, but setting up apache will make it even slower.
* It's also an anti-pattern to switch midway to needing an external
daemon v.s. doing it in a dedicated test.
E.g. if you have the relevant GIT_* settings to run http:// tests,
but not git:// tests we'll skip the former entirely in
t5700-protocol-v1.sh, because we'll "skip all" as soon as we see we
can't start the git-daemon.
That specifically isn't an issue here, but better to avoid setting up
the pattern.
* What *is* an issue with it here is that the "skip all" in TAP is only
handled before you run any tests, e.g. compare:
$ prove ./t5700-protocol-v1.sh
./t5700-protocol-v1.sh .. ok
All tests successful.
Files=1, Tests=21, 2 wallclock secs ( 0.02 usr 0.00 sys + 0.80 cusr 0.80 csys = 1.62 CPU)
Result: PASS
$ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)
Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr 0.00 csys = 0.03 CPU)
Result: NOTESTS
$ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
./t5700-protocol-v1.sh .. ok
All tests successful.
Files=1, Tests=16, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.63 cusr 0.59 csys = 1.23 CPU)
Result: PASS
I.e. if you stick an inclusion of lib-httpd.sh this late in a test
file we won't get a prominent notice saying we're categorically
skipping certain types of tests based on an external dependency.
If you had your own dedicated test instead you'd get:
$ GIT_TEST_HTTPD=false prove ./t5705-protocol-v2-http.sh
./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)
Files=1, Tests=0, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.02 cusr 0.01 csys = 0.05 CPU)
Result: NOTESTS