| Message ID | 042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Push negotiation fixes | expand |
On Wed, Jun 23, 2021 at 03:30:51PM -0700, Jonathan Tan wrote: > > 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 <jonathantanmy@google.com> > --- > 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. > > 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? > + > +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. > + > + # 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. > + 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? > +' > + > +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... > + 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
On Wed, Jun 23 2021, Jonathan Tan wrote: > 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 <jonathantanmy@google.com> > --- > 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
> > > > 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? These are closely related to the non-HTTP push negotiation tests that I previously added to this file, so I don't think there is a better place. As for whether this test is under-testing, I also don't think so - most fetch/push processes would be the same regardless of protocol (but not push negotiation, apparently). > > > + > > +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. Currently, the client attempts the negotiation before the push ref advertisement, so the ref advertisement does not influence whether the negotiation happens or not. (One might ask, should the negotiation happen after the push ref advertisement then? This is an interesting idea, because the push ref advertisement could have information that shows us that negotiation is unnecessary, but I think that eventually it's best if we suppress the push ref advertisement.) > But it's only omitted from the *initial* advertisement, > so it will be advertised in later rounds of negotiation, right? There is no ref advertisement in any round of negotiation (there is no ls-refs call). It will be acknowledged if the client queries for it, though. (In any case, this config is not meant to affect the negotiation part at all, because the negotiation part is using the "fetch" protocol and the "receive" in "receive.hideRefs" means that it only affects the "push" protocol.) > > + 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? Depending on the result of negotiation. Here, we push both commits (as you can see from the next line you quoted below). > > + grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs > Nice, I like the comment - this helps. Thanks. > > + > > + # 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. > > + 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? In general, no - the information that the server conveys to the client in the ref advertisement (which commits it has) is quite well-defined, so it can be precisely computed which objects the server has and hasn't. One potential point of flakiness is if we change the client to push extra objects (say, if the client decided to store one packfile per commit and all its trees and blobs, and then push the entire packfile whenever it needs to push that commit), but I don't think that we're heading in that direction. > > > +' > > + > > +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... I ended up recreating it. > > > + 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? Yes, but this is worth adding a comment for, so I added it. > > > + 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 :) Thanks for taking a look.
> > +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. OK - I have changed it so that each test is independent and cleans up after itself. > > > + 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 Done. > > > + 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 && In my current version, I have changed everything to "git init". Should I use "test_create_repo" instead? > > If we're entirely setting up the server again shouldn't this just be > another test_expect_success block? I only made one block at first because the test without negotiation is there just for comparing object counts, but OK, I have split it up into 2 blocks. > > > + 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. I didn't want to make the test prescribe unnecessary details, but good point about hiding bugs. I have switched it to what you describe. > > > +' > > + > > +# 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. I think this is a pattern that we need, though. Sometimes there are closely related fetch/push tests (as in here, and as in t5700) that want to test similar things across different protocols. > > * 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 Is it useful to know the count of tests that are skipped? (The user presumably already knows that some are skipped because they set the environment variable in the first place.)
On Wed, Jul 14 2021, Jonathan Tan wrote: >> > +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. > > OK - I have changed it so that each test is independent and cleans up after > itself. > >> >> > + 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 > > Done. > >> >> > + 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 && > > In my current version, I have changed everything to "git init". Should I > use "test_create_repo" instead? Sorry I just copy/pasted that from your version, yes indeed s/test_create_repo/git init/ (a trivial thing, but they're 1=1 equivalent these days after a recent change of mine in test-lib-functions.sh). >> >> If we're entirely setting up the server again shouldn't this just be >> another test_expect_success block? > > I only made one block at first because the test without negotiation is > there just for comparing object counts, but OK, I have split it up into > 2 blocks. > >> >> > + 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. > > I didn't want to make the test prescribe unnecessary details, but good > point about hiding bugs. I have switched it to what you describe. > >> >> > +' >> > + >> > +# 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. > > I think this is a pattern that we need, though. Sometimes there are > closely related fetch/push tests (as in here, and as in t5700) that want > to test similar things across different protocols. Yeah, in my split-up WIP there's a bit of that, in my split-up of them I didn't find the end result harder to read, e.g. in t/t5702-protocol-v2.sh it's mostly in different sections of the file, first git://, then file:// and then http://, with some misc in-between, having a t/t57*-protocol-{git,file,http}.sh didn't make things harder to read. >> >> * 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 > > Is it useful to know the count of tests that are skipped? (The user > presumably already knows that some are skipped because they set the > environment variable in the first place.) A thing I do really often is to run some glob of tests, say I'm working on git-fetch, and run *fetch*, I'd get this in the output: [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) [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) [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) [23:58:16] t5539-fetch-http-shallow.sh ..................... skipped: no web server found at '/usr/sbin/apache' [23:58:16] t5550-http-fetch-dumb.sh ........................ skipped: no web server found at '/usr/sbin/apache' [23:58:16] t5551-http-fetch-smart.sh ....................... skipped: no web server found at '/usr/sbin/apache' [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) [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) [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) It's useful to know that a portion of the tests was skipped entirely, whereas if we do git:// tests first, and then later http:// the loss of coverage is silent (unless you run it individually under --verbose) or whatever. This is really useful for the http tests iin particular, it's really common not to have apache installed.
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 && + 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 && + 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" && + 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 && + 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 +' + +# 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
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 <jonathantanmy@google.com> --- builtin/send-pack.c | 1 + t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)