[PATCH 0/3] Push negotiation fixes

[PATCH 0/3] Push negotiation fixes

[Jonathan Tan]

Testing at $DAYJOB revealed some bugs in some fundamental scenarios, and
here are their fixes.

Jonathan Tan (3):
  send-pack: fix push.negotiate with remote helper
  send-pack: fix push nego. when remote has refs
  fetch: die on invalid --negotiation-tip hash

 builtin/fetch.c       |  4 +++-
 builtin/send-pack.c   |  1 +
 send-pack.c           |  6 +++--
 t/t5510-fetch.sh      |  9 ++++++++
 t/t5516-fetch-push.sh | 54 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 3 deletions(-)

-- 
2.32.0.288.g62a8d224e6-goog

[PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Jonathan Tan]

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 &&
+	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
-- 
2.32.0.288.g62a8d224e6-goog

Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Emily Shaffer]

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

Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Jonathan Tan]

> >  
> >  	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.

Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Jonathan Tan]

> > +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.)

Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

[Ævar Arnfjörð Bjarmason]

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.

[PATCH 2/3] send-pack: fix push nego. when remote has refs

[Jonathan Tan]

Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
did not test the case in which a remote advertises at least one ref. In
such a case, "remote_refs" in get_commons_through_negotiation() in
send-pack.c would also contain those refs with a zero ref->new_oid (in
addition to the refs being pushed with a nonzero ref->new_oid). Passing
them as negotiation tips to "git fetch" causes an error, so filter them
out.

(The exact error that would happen in "git fetch" in this case is a
segmentation fault, which is unwanted. This will be fixed in the
subsequent commit.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 send-pack.c           | 6 ++++--
 t/t5516-fetch-push.sh | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 9cb9f71650..85945becf0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,
 	child.no_stdin = 1;
 	child.out = -1;
 	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
-	for (ref = remote_refs; ref; ref = ref->next)
-		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (!is_null_oid(&ref->new_oid))
+			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
+	}
 	strvec_push(&child.args, url);
 
 	if (start_command(&child))
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 5ce32e531a..e383ba662f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '
 	# Without negotiation
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	echo now pushing without negotiation &&
 	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&
@@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '
 	rm event &&
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&
 	grep_wrote 2 event # 1 commit, 1 tree
@@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
 	rm event &&
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
 		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
@@ -1783,6 +1786,7 @@ test_expect_success 'http push with negotiation' '
 	# Without negotiation
 	test_create_repo "$SERVER" &&
 	test_config -C "$SERVER" http.receivepack true &&
+	test_commit -C "$SERVER" unrelated_commit &&
 	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 \
@@ -1794,6 +1798,7 @@ test_expect_success 'http push with negotiation' '
 	rm -rf "$SERVER" &&
 	test_create_repo "$SERVER" &&
 	test_config -C "$SERVER" http.receivepack true &&
+	test_commit -C "$SERVER" unrelated_commit &&
 	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 \
-- 
2.32.0.288.g62a8d224e6-goog

Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs

[Emily Shaffer]

On Wed, Jun 23, 2021 at 03:30:52PM -0700, Jonathan Tan wrote:
> 
> Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
> did not test the case in which a remote advertises at least one ref. In
> such a case, "remote_refs" in get_commons_through_negotiation() in
> send-pack.c would also contain those refs with a zero ref->new_oid (in
> addition to the refs being pushed with a nonzero ref->new_oid). Passing
> them as negotiation tips to "git fetch" causes an error, so filter them
> out.

So here we are filtering those redundant refs on the client side?

> 
> (The exact error that would happen in "git fetch" in this case is a
> segmentation fault, which is unwanted. This will be fixed in the
> subsequent commit.)
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  send-pack.c           | 6 ++++--
>  t/t5516-fetch-push.sh | 5 +++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/send-pack.c b/send-pack.c
> index 9cb9f71650..85945becf0 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,
>  	child.no_stdin = 1;
>  	child.out = -1;
>  	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> -	for (ref = remote_refs; ref; ref = ref->next)
> -		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> +	for (ref = remote_refs; ref; ref = ref->next) {
> +		if (!is_null_oid(&ref->new_oid))
> +			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> +	}
>  	strvec_push(&child.args, url);
>  
>  	if (start_command(&child))
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 5ce32e531a..e383ba662f 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '
>  	# Without negotiation
>  	mk_empty testrepo &&
>  	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
> +	test_commit -C testrepo unrelated_commit &&
>  	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
>  	echo now pushing without negotiation &&
>  	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&
> @@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '
>  	rm event &&
>  	mk_empty testrepo &&
>  	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
> +	test_commit -C testrepo unrelated_commit &&

So now we are asking 'testrepo' to initially advertise that it also has
unrelated_commit, which we don't care about, and expect to work fine
anyway. Ok.

>  	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
>  	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&
>  	grep_wrote 2 event # 1 commit, 1 tree
> @@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
>  	rm event &&
>  	mk_empty testrepo &&
>  	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
> +	test_commit -C testrepo unrelated_commit &&
>  	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
>  	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
>  		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
> @@ -1783,6 +1786,7 @@ test_expect_success 'http push with negotiation' '
>  	# Without negotiation
>  	test_create_repo "$SERVER" &&
>  	test_config -C "$SERVER" http.receivepack true &&
> +	test_commit -C "$SERVER" unrelated_commit &&
>  	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 \
> @@ -1794,6 +1798,7 @@ test_expect_success 'http push with negotiation' '
>  	rm -rf "$SERVER" &&
>  	test_create_repo "$SERVER" &&
>  	test_config -C "$SERVER" http.receivepack true &&
> +	test_commit -C "$SERVER" unrelated_commit &&
>  	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 \
> -- 
> 2.32.0.288.g62a8d224e6-goog


Seems reasonable enough to me.
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs

[Jonathan Tan]

> On Wed, Jun 23, 2021 at 03:30:52PM -0700, Jonathan Tan wrote:
> > 
> > Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
> > did not test the case in which a remote advertises at least one ref. In
> > such a case, "remote_refs" in get_commons_through_negotiation() in
> > send-pack.c would also contain those refs with a zero ref->new_oid (in
> > addition to the refs being pushed with a nonzero ref->new_oid). Passing
> > them as negotiation tips to "git fetch" causes an error, so filter them
> > out.
> 
> So here we are filtering those redundant refs on the client side?

Yes, but I don't know what you mean by "redundant".

> > @@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '
> >  	rm event &&
> >  	mk_empty testrepo &&
> >  	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
> > +	test_commit -C testrepo unrelated_commit &&
> 
> So now we are asking 'testrepo' to initially advertise that it also has
> unrelated_commit, which we don't care about, and expect to work fine
> anyway. Ok.

Yes.

> Seems reasonable enough to me.
> Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

Thanks.

[PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Jonathan Tan]

If a full hexadecimal hash is given as a --negotiation-tip to "git
fetch", and that hash does not correspond to an object, "git fetch" will
segfault if --negotiate-only is given and will silently ignore that hash
otherwise. Make these cases fatal errors, just like the case when an
invalid ref name or abbreviated hash is given.

While at it, mark the error messages as translatable.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch.c  | 4 +++-
 t/t5510-fetch.sh | 9 +++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9191620e50..2c50465cff 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
 		if (!has_glob_specials(s)) {
 			struct object_id oid;
 			if (get_oid(s, &oid))
-				die("%s is not a valid object", s);
+				die(_("%s is not a valid object"), s);
+			if (!has_object(the_repository, &oid, 0))
+				die(_("%s is not a valid object"), s);
 			oid_array_append(oids, &oid);
 			continue;
 		}
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e83b2a6506..5fc5750d8d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1214,6 +1214,15 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '
 	check_negotiation_tip
 '
 
+test_expect_success '--negotiation-tip rejects missing OIDs' '
+	setup_negotiation_tip server server 0 &&
+	test_must_fail git -C client fetch \
+		--negotiation-tip=alpha_1 \
+		--negotiation-tip=$(test_oid zero) \
+		origin alpha_s beta_s 2>err &&
+	test_i18ngrep "is not a valid object" err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.32.0.288.g62a8d224e6-goog

Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Emily Shaffer]

On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:
> 
> If a full hexadecimal hash is given as a --negotiation-tip to "git
> fetch", and that hash does not correspond to an object, "git fetch" will
> segfault if --negotiate-only is given and will silently ignore that hash
> otherwise. Make these cases fatal errors, just like the case when an
> invalid ref name or abbreviated hash is given.
> 
> While at it, mark the error messages as translatable.
I don't usually like this kind of "while we're at it" change, but in
this case it's very very small, so it doesn't bother me much.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fetch.c  | 4 +++-
>  t/t5510-fetch.sh | 9 +++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 9191620e50..2c50465cff 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
>  		if (!has_glob_specials(s)) {
>  			struct object_id oid;
>  			if (get_oid(s, &oid))
> -				die("%s is not a valid object", s);
> +				die(_("%s is not a valid object"), s);
> +			if (!has_object(the_repository, &oid, 0))
> +				die(_("%s is not a valid object"), s);
Any reason not to consolidate these, e.g. if (get_oid || !has_object)?
Then we wouldn't dup the err string.

>  			oid_array_append(oids, &oid);
>  			continue;
>  		}
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index e83b2a6506..5fc5750d8d 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1214,6 +1214,15 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '
>  	check_negotiation_tip
>  '
>  
> +test_expect_success '--negotiation-tip rejects missing OIDs' '
> +	setup_negotiation_tip server server 0 &&
> +	test_must_fail git -C client fetch \
> +		--negotiation-tip=alpha_1 \
This one's okay...

> +		--negotiation-tip=$(test_oid zero) \
...and this one's junk. Ok.

> +		origin alpha_s beta_s 2>err &&
> +	test_i18ngrep "is not a valid object" err
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>  
> -- 
> 2.32.0.288.g62a8d224e6-goog

Other than the little nit, this seems OK to me.

 - Emily

Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 13 2021, Emily Shaffer wrote:

> On Wed, Jun 23, 2021 at 03:30:53PM -0700, Jonathan Tan wrote:
>> 
>> If a full hexadecimal hash is given as a --negotiation-tip to "git
>> fetch", and that hash does not correspond to an object, "git fetch" will
>> segfault if --negotiate-only is given and will silently ignore that hash
>> otherwise. Make these cases fatal errors, just like the case when an
>> invalid ref name or abbreviated hash is given.
>> 
>> While at it, mark the error messages as translatable.
> I don't usually like this kind of "while we're at it" change, but in
> this case it's very very small, so it doesn't bother me much.
>> 
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
>>  builtin/fetch.c  | 4 +++-
>>  t/t5510-fetch.sh | 9 +++++++++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 9191620e50..2c50465cff 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
>>  		if (!has_glob_specials(s)) {
>>  			struct object_id oid;
>>  			if (get_oid(s, &oid))
>> -				die("%s is not a valid object", s);
>> +				die(_("%s is not a valid object"), s);
>> +			if (!has_object(the_repository, &oid, 0))
>> +				die(_("%s is not a valid object"), s);
> Any reason not to consolidate these, e.g. if (get_oid || !has_object)?
> Then we wouldn't dup the err string.

Generally I'd agree, but aren't we explicitly conflating cases where
something is a valid way no name an object v.s. being certain that such
an object does not exist? I.e. this should be something like:

    if can't get_get():
        error "couldn't get the OID of revision '%s'"
    if can't look up fully-qualified OID:
        error "the OID '%s' does not exist"

Or something...

Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Jonathan Tan]

> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index 9191620e50..2c50465cff 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
> >>  		if (!has_glob_specials(s)) {
> >>  			struct object_id oid;
> >>  			if (get_oid(s, &oid))
> >> -				die("%s is not a valid object", s);
> >> +				die(_("%s is not a valid object"), s);
> >> +			if (!has_object(the_repository, &oid, 0))
> >> +				die(_("%s is not a valid object"), s);
> > Any reason not to consolidate these, e.g. if (get_oid || !has_object)?
> > Then we wouldn't dup the err string.
> 
> Generally I'd agree, but aren't we explicitly conflating cases where
> something is a valid way no name an object v.s. being certain that such
> an object does not exist? I.e. this should be something like:
> 
>     if can't get_get():
>         error "couldn't get the OID of revision '%s'"
>     if can't look up fully-qualified OID:
>         error "the OID '%s' does not exist"
> 
> Or something...

Good point. I'll use wording similar to yours.

Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14 2021, Jonathan Tan wrote:

>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> >> index 9191620e50..2c50465cff 100644
>> >> --- a/builtin/fetch.c
>> >> +++ b/builtin/fetch.c
>> >> @@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
>> >>  		if (!has_glob_specials(s)) {
>> >>  			struct object_id oid;
>> >>  			if (get_oid(s, &oid))
>> >> -				die("%s is not a valid object", s);
>> >> +				die(_("%s is not a valid object"), s);
>> >> +			if (!has_object(the_repository, &oid, 0))
>> >> +				die(_("%s is not a valid object"), s);
>> > Any reason not to consolidate these, e.g. if (get_oid || !has_object)?
>> > Then we wouldn't dup the err string.
>> 
>> Generally I'd agree, but aren't we explicitly conflating cases where
>> something is a valid way no name an object v.s. being certain that such
>> an object does not exist? I.e. this should be something like:
>> 
>>     if can't get_get():
>>         error "couldn't get the OID of revision '%s'"
>>     if can't look up fully-qualified OID:
>>         error "the OID '%s' does not exist"
>> 
>> Or something...
>
> Good point. I'll use wording similar to yours.

I stole the former from a quick grepping around:

    builtin/bisect--helper.c: res = error(_("couldn't get the oid of the rev '%s'"), rev)

Then http-push.c suggests "perhaps you need to fetch" for
has_object_file(), but I don't know if it applies here...

[PATCH v2 0/3] Push negotiation fixes

[Jonathan Tan]

Thanks, Emily and Ævar for your reviews. There are no changes to C code
in this version of the patch set, but the tests have been updated
following both your comments.

I have ended up splitting up the HTTP tests away from t5516 into its own
file. I had to use the number 5549 because it was the next available
one. There is one Bash function that is duplicated - I don't foresee
many such functions being duplicated, but if that happens, I guess we
can always created a helper script somewhere else.

Jonathan Tan (3):
  send-pack: fix push.negotiate with remote helper
  send-pack: fix push nego. when remote has refs
  fetch: die on invalid --negotiation-tip hash

 builtin/fetch.c            |  4 ++-
 builtin/send-pack.c        |  1 +
 send-pack.c                |  6 ++--
 t/t5510-fetch.sh           | 13 +++++++
 t/t5516-fetch-push.sh      |  4 ++-
 t/t5549-fetch-push-http.sh | 72 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+), 4 deletions(-)
 create mode 100755 t/t5549-fetch-push-http.sh

Range-diff against v1:
1:  eb0dce9f49 < -:  ---------- send-pack: fix push.negotiate with remote helper
-:  ---------- > 1:  af40bee611 send-pack: fix push.negotiate with remote helper
2:  3bf6921d96 ! 2:  c841693303 send-pack: fix push nego. when remote has refs
    @@ t/t5516-fetch-push.sh: test_expect_success 'push with negotiation proceeds anywa
      	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
      	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
      		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
    -@@ t/t5516-fetch-push.sh: test_expect_success 'http push with negotiation' '
    - 	# Without negotiation
    - 	test_create_repo "$SERVER" &&
    - 	test_config -C "$SERVER" http.receivepack true &&
    -+	test_commit -C "$SERVER" unrelated_commit &&
    - 	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 \
    -@@ t/t5516-fetch-push.sh: test_expect_success 'http push with negotiation' '
    - 	rm -rf "$SERVER" &&
    - 	test_create_repo "$SERVER" &&
    +@@ t/t5516-fetch-push.sh: test_expect_success 'denyCurrentBranch and worktrees' '
    + 	git -C cloned push origin HEAD:new-wt &&
    + 	test_must_fail git -C cloned push --delete origin new-wt
    + '
    +-
    + test_done
    +
    + ## t/t5549-fetch-push-http.sh ##
    +@@ t/t5549-fetch-push-http.sh: setup_client_and_server () {
    + 	git init "$SERVER" &&
    + 	test_when_finished 'rm -rf "$SERVER"' &&
      	test_config -C "$SERVER" http.receivepack true &&
     +	test_commit -C "$SERVER" unrelated_commit &&
      	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 \
    + 	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit
    + }
3:  1453a1b4e5 ! 3:  476da85859 fetch: die on invalid --negotiation-tip hash
    @@ builtin/fetch.c: static void add_negotiation_tips(struct git_transport_options *
     -				die("%s is not a valid object", s);
     +				die(_("%s is not a valid object"), s);
     +			if (!has_object(the_repository, &oid, 0))
    -+				die(_("%s is not a valid object"), s);
    ++				die(_("the object %s does not exist"), s);
      			oid_array_append(oids, &oid);
      			continue;
      		}
    @@ t/t5510-fetch.sh: test_expect_success '--negotiation-tip understands abbreviated
     +		--negotiation-tip=alpha_1 \
     +		--negotiation-tip=$(test_oid zero) \
     +		origin alpha_s beta_s 2>err &&
    -+	test_i18ngrep "is not a valid object" err
    ++	cat >fatal-expect <<-EOF &&
    ++	fatal: the object $(test_oid zero) does not exist
    ++EOF
    ++	grep fatal: err >fatal-actual &&
    ++	test_cmp fatal-expect fatal-actual
     +'
     +
      . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.32.0.93.g670b81a890-goog

[PATCH v2 1/3] send-pack: fix push.negotiate with remote helper

[Jonathan Tan]

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/send-pack.c        |  1 +
 t/t5549-fetch-push-http.sh | 71 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)
 create mode 100755 t/t5549-fetch-push-http.sh

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/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh
new file mode 100755
index 0000000000..f50d584881
--- /dev/null
+++ b/t/t5549-fetch-push-http.sh
@@ -0,0 +1,71 @@
+#!/bin/sh
+
+test_description='fetch/push functionality using the HTTP protocol'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server"
+URI="$HTTPD_URL/smart/server"
+
+grep_wrote () {
+	object_count=$1
+	file_name=$2
+	grep 'write_pack_file/wrote.*"value":"'$1'"' $2
+}
+
+setup_client_and_server () {
+	git init client &&
+	test_when_finished 'rm -rf client' &&
+	test_commit -C client first_commit &&
+	test_commit -C client second_commit &&
+
+	git init "$SERVER" &&
+	test_when_finished 'rm -rf "$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
+}
+
+test_expect_success 'push without negotiation (for comparing object counts with the next test)' '
+	setup_client_and_server &&
+
+	GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 \
+		push "$URI" refs/heads/main:refs/remotes/origin/main &&
+	test_when_finished "rm -f event" &&
+	grep_wrote 6 event # 2 commits, 2 trees, 2 blobs
+'
+
+test_expect_success 'push with negotiation' '
+	setup_client_and_server &&
+
+	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 &&
+	test_when_finished "rm -f event" &&
+	grep_wrote 3 event # 1 commit, 1 tree, 1 blob
+'
+
+test_expect_success 'push with negotiation proceeds anyway even if negotiation fails' '
+	setup_client_and_server &&
+
+	# Use protocol v0 to make negotiation fail (because protocol v0 does
+	# not support the "wait-for-done" capability, which is required for
+	# push negotiation)
+	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 &&
+	test_when_finished "rm -f event" &&
+	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
+
+	cat >warning-expect <<-EOF &&
+	warning: --negotiate-only requires protocol v2
+	warning: push negotiation failed; proceeding anyway with push
+EOF
+	grep warning: err >warning-actual &&
+	test_cmp warning-expect warning-actual
+'
+
+test_done
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15 2021, Jonathan Tan wrote:


> +	git init client &&
> +	test_when_finished 'rm -rf client' &&

Nit (don't think this needs a re-roll): Better to do test_when_finished
before whatever creates the thing, in case the command has a bug/errors
out etc. during development. Ditto the below.

[PATCH v2 2/3] send-pack: fix push nego. when remote has refs

[Jonathan Tan]

Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
did not test the case in which a remote advertises at least one ref. In
such a case, "remote_refs" in get_commons_through_negotiation() in
send-pack.c would also contain those refs with a zero ref->new_oid (in
addition to the refs being pushed with a nonzero ref->new_oid). Passing
them as negotiation tips to "git fetch" causes an error, so filter them
out.

(The exact error that would happen in "git fetch" in this case is a
segmentation fault, which is unwanted. This will be fixed in the
subsequent commit.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 send-pack.c                | 6 ++++--
 t/t5516-fetch-push.sh      | 4 +++-
 t/t5549-fetch-push-http.sh | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 9cb9f71650..85945becf0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,
 	child.no_stdin = 1;
 	child.out = -1;
 	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
-	for (ref = remote_refs; ref; ref = ref->next)
-		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (!is_null_oid(&ref->new_oid))
+			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
+	}
 	strvec_push(&child.args, url);
 
 	if (start_command(&child))
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0916f76302..4db8edd9c8 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -201,6 +201,7 @@ test_expect_success 'push with negotiation' '
 	# Without negotiation
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	echo now pushing without negotiation &&
 	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&
@@ -210,6 +211,7 @@ test_expect_success 'push with negotiation' '
 	rm event &&
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&
 	grep_wrote 2 event # 1 commit, 1 tree
@@ -219,6 +221,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
 	rm event &&
 	mk_empty testrepo &&
 	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
+	test_commit -C testrepo unrelated_commit &&
 	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
 	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
 		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
@@ -1767,5 +1770,4 @@ test_expect_success 'denyCurrentBranch and worktrees' '
 	git -C cloned push origin HEAD:new-wt &&
 	test_must_fail git -C cloned push --delete origin new-wt
 '
-
 test_done
diff --git a/t/t5549-fetch-push-http.sh b/t/t5549-fetch-push-http.sh
index f50d584881..2cdebcb735 100755
--- a/t/t5549-fetch-push-http.sh
+++ b/t/t5549-fetch-push-http.sh
@@ -27,6 +27,7 @@ setup_client_and_server () {
 	git init "$SERVER" &&
 	test_when_finished 'rm -rf "$SERVER"' &&
 	test_config -C "$SERVER" http.receivepack true &&
+	test_commit -C "$SERVER" unrelated_commit &&
 	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
 	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit
 }
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15 2021, Jonathan Tan wrote:

> Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
> did not test the case in which a remote advertises at least one ref. In
> such a case, "remote_refs" in get_commons_through_negotiation() in
> send-pack.c would also contain those refs with a zero ref->new_oid (in
> addition to the refs being pushed with a nonzero ref->new_oid). Passing
> them as negotiation tips to "git fetch" causes an error, so filter them
> out.
>
> (The exact error that would happen in "git fetch" in this case is a
> segmentation fault, which is unwanted. This will be fixed in the
> subsequent commit.)

Let's add the test from the subsequent here as a test_expect_failure and
flip it to "success".

> @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,
>  	child.no_stdin = 1;
>  	child.out = -1;
>  	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> -	for (ref = remote_refs; ref; ref = ref->next)
> -		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> +	for (ref = remote_refs; ref; ref = ref->next) {
> +		if (!is_null_oid(&ref->new_oid))
> +			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> +	}
>  	strvec_push(&child.args, url);

This will run into my eff40457a4 (fetch: fix segfault in
--negotiate-only without --negotiation-tip=*, 2021-07-08) if we supply a
--negotiate-only without --negotiation-tip=, but trying it it looks like
even when you push to an empty repo and your repo is itself empty we'll
always add the tip you're pushing as the negotiation tip.

Let's add a test for that, i.e. I instrumented your test to check what
happens whe I do the push without any remote/local refs, both for
one/both cases (and both combinations), it seems to work...

For code that's doing a loop over "refs" testing that seems to be
worthwhile, i.e. we don't actually depend on "refs" in the sense that
they exist, but the refs we've constructed in-memory to be created on
the remote, correct?

I.e. this on top would be OK (not saying you need this, but I for one
would find it easier to follow with this):
	
	diff --git a/send-pack.c b/send-pack.c
	index b3a495b7b1..d1e231076c 100644
	--- a/send-pack.c
	+++ b/send-pack.c
	@@ -420,15 +420,20 @@ static void get_commons_through_negotiation(const char *url,
	 	struct child_process child = CHILD_PROCESS_INIT;
	 	const struct ref *ref;
	 	int len = the_hash_algo->hexsz + 1; /* hash + NL */
	+	int got_tip = 0;
	 
	 	child.git_cmd = 1;
	 	child.no_stdin = 1;
	 	child.out = -1;
	 	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
	 	for (ref = remote_refs; ref; ref = ref->next) {
	-		if (!is_null_oid(&ref->new_oid))
	-			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
	+		if (is_null_oid(&ref->new_oid))
	+			continue;
	+		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
	+		got_tip = 1;
	 	}
	+	if (!got_tip)
	+		BUG("should get at least one ref tip, even with no remote/local refs");
	 	strvec_push(&child.args, url);
	 
	 	if (start_command(&child))

But also: looking at the trace output we already have the ref
advertisement at this point, so in the case of an empty repo we'll see
it has no refs, but then we're going to provide a --negotiation-tip=*
pointing to our local OID anyway.

That seems like a fairly non-obvious edge case that should be called out
/ tested.

I.e. aren't we at least just going to engage in redundant work there in
trying to negotiate with empty repos, or is it going to noop anyway.

Or maybe we'll get lucky and they have the OID already, they just
recently deleted their reference(s), then we won't need to send as much
over? Is that what this is trying to do?

But hrm, won't that sort of thing increase the odds of repository
corruption?

I.e. now we make the implicit assumption that an OID we see in the
advertisement is one the server isn't going to aggressively prune while
our push is underday (Jeff King has a good E-Mail summarizing that
somewhere, not digging it up now, but I could...).

So such a remote will negotiate with us using that OID, but unlike with
advertised OIDs we can't safely assume that the OID won't be racily
deleted during our negotiation.

Or maybe I'm entirely wrong here....

Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs

[Jeff King]

On Tue, Jul 27, 2021 at 10:09:35AM +0200, Ævar Arnfjörð Bjarmason wrote:

> I.e. now we make the implicit assumption that an OID we see in the
> advertisement is one the server isn't going to aggressively prune while
> our push is underday (Jeff King has a good E-Mail summarizing that
> somewhere, not digging it up now, but I could...).
> 
> So such a remote will negotiate with us using that OID, but unlike with
> advertised OIDs we can't safely assume that the OID won't be racily
> deleted during our negotiation.

I haven't been following the push-negotiation stuff closely, nor do I
have a specific email in mind that summarizes this. So take my input
with a grain of salt. But...

Wouldn't this also be a problem for multi-round fetch negotiation? An
object may become unreachable or even go away entirely during the course
of a fetch. I'd expect that to be rare, but when it does happen, for the
fetch to end up barfing (the server says "hey, I don't know about that
object").

-Peff

Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs

[Jonathan Tan]

> On Thu, Jul 15 2021, Jonathan Tan wrote:
> 
> > Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
> > did not test the case in which a remote advertises at least one ref. In
> > such a case, "remote_refs" in get_commons_through_negotiation() in
> > send-pack.c would also contain those refs with a zero ref->new_oid (in
> > addition to the refs being pushed with a nonzero ref->new_oid). Passing
> > them as negotiation tips to "git fetch" causes an error, so filter them
> > out.
> >
> > (The exact error that would happen in "git fetch" in this case is a
> > segmentation fault, which is unwanted. This will be fixed in the
> > subsequent commit.)
> 
> Let's add the test from the subsequent here as a test_expect_failure and
> flip it to "success".

What is the subsequent?

> > @@ -425,8 +425,10 @@ static void get_commons_through_negotiation(const char *url,
> >  	child.no_stdin = 1;
> >  	child.out = -1;
> >  	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> > -	for (ref = remote_refs; ref; ref = ref->next)
> > -		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> > +	for (ref = remote_refs; ref; ref = ref->next) {
> > +		if (!is_null_oid(&ref->new_oid))
> > +			strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
> > +	}
> >  	strvec_push(&child.args, url);
> 
> This will run into my eff40457a4 (fetch: fix segfault in
> --negotiate-only without --negotiation-tip=*, 2021-07-08) if we supply a
> --negotiate-only without --negotiation-tip=, but trying it it looks like
> even when you push to an empty repo and your repo is itself empty we'll
> always add the tip you're pushing as the negotiation tip.
> 
> Let's add a test for that, i.e. I instrumented your test to check what
> happens whe I do the push without any remote/local refs, both for
> one/both cases (and both combinations), it seems to work...

I'm not sure how useful this no-ref test will be, because if my existing
tests are correct, the thing we're pushing is guaranteed to be in this
list (so the list will be non-empty).

> For code that's doing a loop over "refs" testing that seems to be
> worthwhile, i.e. we don't actually depend on "refs" in the sense that
> they exist, but the refs we've constructed in-memory to be created on
> the remote, correct?

Yes.

> But also: looking at the trace output we already have the ref
> advertisement at this point, so in the case of an empty repo we'll see
> it has no refs, but then we're going to provide a --negotiation-tip=*
> pointing to our local OID anyway.

Hmm...are you running under protocol v0? In protocol v2, there should be
no ref advertisement at this point.

> That seems like a fairly non-obvious edge case that should be called out
> / tested.
> 
> I.e. aren't we at least just going to engage in redundant work there in
> trying to negotiate with empty repos, or is it going to noop anyway.
> 
> Or maybe we'll get lucky and they have the OID already, they just
> recently deleted their reference(s), then we won't need to send as much
> over? Is that what this is trying to do?
> 
> But hrm, won't that sort of thing increase the odds of repository
> corruption?

No, trying to be lucky in finding an OID that the server has no plans of
advertising is not the aim.

> I.e. now we make the implicit assumption that an OID we see in the
> advertisement is one the server isn't going to aggressively prune while
> our push is underday (Jeff King has a good E-Mail summarizing that
> somewhere, not digging it up now, but I could...).
> 
> So such a remote will negotiate with us using that OID, but unlike with
> advertised OIDs we can't safely assume that the OID won't be racily
> deleted during our negotiation.
> 
> Or maybe I'm entirely wrong here....

There's always the risk that the server will say it has something and
then aggressively prune it, but I think that all fetch/push code has to
deal with it. A more realistic scenario is that one server in a
load-balanced arrangement advertises a commit that the other does not
have, but we are unlikely to be affected by that here because the ref
negotiation would usually concern old commits that the local user has
built upon, not the very latest commits that someone else just pushed.

[PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash

[Jonathan Tan]

If a full hexadecimal hash is given as a --negotiation-tip to "git
fetch", and that hash does not correspond to an object, "git fetch" will
segfault if --negotiate-only is given and will silently ignore that hash
otherwise. Make these cases fatal errors, just like the case when an
invalid ref name or abbreviated hash is given.

While at it, mark the error messages as translatable.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch.c  |  4 +++-
 t/t5510-fetch.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index dfde96a435..0aeb043840 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1428,7 +1428,9 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
 		if (!has_glob_specials(s)) {
 			struct object_id oid;
 			if (get_oid(s, &oid))
-				die("%s is not a valid object", s);
+				die(_("%s is not a valid object"), s);
+			if (!has_object(the_repository, &oid, 0))
+				die(_("the object %s does not exist"), s);
 			oid_array_append(oids, &oid);
 			continue;
 		}
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e83b2a6506..a0faf0dd94 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1214,6 +1214,19 @@ test_expect_success '--negotiation-tip understands abbreviated SHA-1' '
 	check_negotiation_tip
 '
 
+test_expect_success '--negotiation-tip rejects missing OIDs' '
+	setup_negotiation_tip server server 0 &&
+	test_must_fail git -C client fetch \
+		--negotiation-tip=alpha_1 \
+		--negotiation-tip=$(test_oid zero) \
+		origin alpha_s beta_s 2>err &&
+	cat >fatal-expect <<-EOF &&
+	fatal: the object $(test_oid zero) does not exist
+EOF
+	grep fatal: err >fatal-actual &&
+	test_cmp fatal-expect fatal-actual
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH v2 0/3] Push negotiation fixes

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Thanks, Emily and Ævar for your reviews. There are no changes to C code
> in this version of the patch set, but the tests have been updated
> following both your comments.

Thank you all.

Will queue.