* [PATCH 0/3] Push negotiation fixes @ 2021-06-23 22:30 Jonathan Tan 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan ` (3 more replies) 0 siblings, 4 replies; 24+ messages in thread From: Jonathan Tan @ 2021-06-23 22:30 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-06-23 22:30 [PATCH 0/3] Push negotiation fixes Jonathan Tan @ 2021-06-23 22:30 ` Jonathan Tan 2021-07-13 22:23 ` Emily Shaffer 2021-07-13 23:11 ` Ævar Arnfjörð Bjarmason 2021-06-23 22:30 ` [PATCH 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan ` (2 subsequent siblings) 3 siblings, 2 replies; 24+ messages in thread From: Jonathan Tan @ 2021-06-23 22:30 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan @ 2021-07-13 22:23 ` Emily Shaffer 2021-07-14 19:25 ` Jonathan Tan 2021-07-13 23:11 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 24+ messages in thread From: Emily Shaffer @ 2021-07-13 22:23 UTC (permalink / raw) To: Jonathan Tan; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-07-13 22:23 ` Emily Shaffer @ 2021-07-14 19:25 ` Jonathan Tan 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Tan @ 2021-07-14 19:25 UTC (permalink / raw) To: emilyshaffer; +Cc: jonathantanmy, git > > > > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan 2021-07-13 22:23 ` Emily Shaffer @ 2021-07-13 23:11 ` Ævar Arnfjörð Bjarmason 2021-07-14 19:32 ` Jonathan Tan 1 sibling, 1 reply; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-13 23:11 UTC (permalink / raw) To: Jonathan Tan; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-07-13 23:11 ` Ævar Arnfjörð Bjarmason @ 2021-07-14 19:32 ` Jonathan Tan 2021-07-14 21:51 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-07-14 19:32 UTC (permalink / raw) To: avarab; +Cc: jonathantanmy, git > > +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.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper 2021-07-14 19:32 ` Jonathan Tan @ 2021-07-14 21:51 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 21:51 UTC (permalink / raw) To: Jonathan Tan; +Cc: git 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] send-pack: fix push nego. when remote has refs 2021-06-23 22:30 [PATCH 0/3] Push negotiation fixes Jonathan Tan 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan @ 2021-06-23 22:30 ` Jonathan Tan 2021-07-13 22:30 ` Emily Shaffer 2021-06-23 22:30 ` [PATCH 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan 3 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-06-23 22:30 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs 2021-06-23 22:30 ` [PATCH 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan @ 2021-07-13 22:30 ` Emily Shaffer 2021-07-14 19:33 ` Jonathan Tan 0 siblings, 1 reply; 24+ messages in thread From: Emily Shaffer @ 2021-07-13 22:30 UTC (permalink / raw) To: Jonathan Tan; +Cc: git 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> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] send-pack: fix push nego. when remote has refs 2021-07-13 22:30 ` Emily Shaffer @ 2021-07-14 19:33 ` Jonathan Tan 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Tan @ 2021-07-14 19:33 UTC (permalink / raw) To: emilyshaffer; +Cc: jonathantanmy, git > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] fetch: die on invalid --negotiation-tip hash 2021-06-23 22:30 [PATCH 0/3] Push negotiation fixes Jonathan Tan 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan 2021-06-23 22:30 ` [PATCH 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan @ 2021-06-23 22:30 ` Jonathan Tan 2021-07-13 22:36 ` Emily Shaffer 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan 3 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-06-23 22:30 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash 2021-06-23 22:30 ` [PATCH 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan @ 2021-07-13 22:36 ` Emily Shaffer 2021-07-13 23:34 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 24+ messages in thread From: Emily Shaffer @ 2021-07-13 22:36 UTC (permalink / raw) To: Jonathan Tan; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash 2021-07-13 22:36 ` Emily Shaffer @ 2021-07-13 23:34 ` Ævar Arnfjörð Bjarmason 2021-07-14 19:35 ` Jonathan Tan 0 siblings, 1 reply; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-13 23:34 UTC (permalink / raw) To: Emily Shaffer; +Cc: Jonathan Tan, git 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... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash 2021-07-13 23:34 ` Ævar Arnfjörð Bjarmason @ 2021-07-14 19:35 ` Jonathan Tan 2021-07-14 21:45 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-07-14 19:35 UTC (permalink / raw) To: avarab; +Cc: emilyshaffer, jonathantanmy, git > >> 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] fetch: die on invalid --negotiation-tip hash 2021-07-14 19:35 ` Jonathan Tan @ 2021-07-14 21:45 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-14 21:45 UTC (permalink / raw) To: Jonathan Tan; +Cc: emilyshaffer, git 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... ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 0/3] Push negotiation fixes 2021-06-23 22:30 [PATCH 0/3] Push negotiation fixes Jonathan Tan ` (2 preceding siblings ...) 2021-06-23 22:30 ` [PATCH 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan @ 2021-07-15 17:44 ` Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan ` (3 more replies) 3 siblings, 4 replies; 24+ messages in thread From: Jonathan Tan @ 2021-07-15 17:44 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, avarab, emilyshaffer 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan @ 2021-07-15 17:44 ` Jonathan Tan 2021-07-27 7:56 ` Ævar Arnfjörð Bjarmason 2021-07-15 17:44 ` [PATCH v2 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan ` (2 subsequent siblings) 3 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-07-15 17:44 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, avarab, emilyshaffer, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper 2021-07-15 17:44 ` [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan @ 2021-07-27 7:56 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-27 7:56 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, emilyshaffer, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 2/3] send-pack: fix push nego. when remote has refs 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan @ 2021-07-15 17:44 ` Jonathan Tan 2021-07-27 8:09 ` Ævar Arnfjörð Bjarmason 2021-07-15 17:44 ` [PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan 2021-07-15 19:03 ` [PATCH v2 0/3] Push negotiation fixes Junio C Hamano 3 siblings, 1 reply; 24+ messages in thread From: Jonathan Tan @ 2021-07-15 17:44 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, avarab, emilyshaffer, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs 2021-07-15 17:44 ` [PATCH v2 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan @ 2021-07-27 8:09 ` Ævar Arnfjörð Bjarmason 2021-07-27 16:46 ` Jeff King 2021-07-27 21:11 ` Jonathan Tan 0 siblings, 2 replies; 24+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-07-27 8:09 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, emilyshaffer, Junio C Hamano, Jeff King 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.... ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs 2021-07-27 8:09 ` Ævar Arnfjörð Bjarmason @ 2021-07-27 16:46 ` Jeff King 2021-07-27 21:11 ` Jonathan Tan 1 sibling, 0 replies; 24+ messages in thread From: Jeff King @ 2021-07-27 16:46 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jonathan Tan, git, emilyshaffer, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] send-pack: fix push nego. when remote has refs 2021-07-27 8:09 ` Ævar Arnfjörð Bjarmason 2021-07-27 16:46 ` Jeff King @ 2021-07-27 21:11 ` Jonathan Tan 1 sibling, 0 replies; 24+ messages in thread From: Jonathan Tan @ 2021-07-27 21:11 UTC (permalink / raw) To: avarab; +Cc: jonathantanmy, git, emilyshaffer, gitster, peff > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan @ 2021-07-15 17:44 ` Jonathan Tan 2021-07-15 19:03 ` [PATCH v2 0/3] Push negotiation fixes Junio C Hamano 3 siblings, 0 replies; 24+ messages in thread From: Jonathan Tan @ 2021-07-15 17:44 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, avarab, emilyshaffer, Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3] Push negotiation fixes 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan ` (2 preceding siblings ...) 2021-07-15 17:44 ` [PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan @ 2021-07-15 19:03 ` Junio C Hamano 3 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2021-07-15 19:03 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, avarab, emilyshaffer 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-07-27 21:11 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-23 22:30 [PATCH 0/3] Push negotiation fixes Jonathan Tan 2021-06-23 22:30 ` [PATCH 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan 2021-07-13 22:23 ` Emily Shaffer 2021-07-14 19:25 ` Jonathan Tan 2021-07-13 23:11 ` Ævar Arnfjörð Bjarmason 2021-07-14 19:32 ` Jonathan Tan 2021-07-14 21:51 ` Ævar Arnfjörð Bjarmason 2021-06-23 22:30 ` [PATCH 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan 2021-07-13 22:30 ` Emily Shaffer 2021-07-14 19:33 ` Jonathan Tan 2021-06-23 22:30 ` [PATCH 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan 2021-07-13 22:36 ` Emily Shaffer 2021-07-13 23:34 ` Ævar Arnfjörð Bjarmason 2021-07-14 19:35 ` Jonathan Tan 2021-07-14 21:45 ` Ævar Arnfjörð Bjarmason 2021-07-15 17:44 ` [PATCH v2 0/3] Push negotiation fixes Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 1/3] send-pack: fix push.negotiate with remote helper Jonathan Tan 2021-07-27 7:56 ` Ævar Arnfjörð Bjarmason 2021-07-15 17:44 ` [PATCH v2 2/3] send-pack: fix push nego. when remote has refs Jonathan Tan 2021-07-27 8:09 ` Ævar Arnfjörð Bjarmason 2021-07-27 16:46 ` Jeff King 2021-07-27 21:11 ` Jonathan Tan 2021-07-15 17:44 ` [PATCH v2 3/3] fetch: die on invalid --negotiation-tip hash Jonathan Tan 2021-07-15 19:03 ` [PATCH v2 0/3] Push negotiation fixes Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).