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