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@xxxxxxxxxx>
> ---
> 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
