| Message ID | 9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resend of GIT_TEST_PROTOCOL_VERSION patches | expand |
On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote: > Some tests check that fetching an unreachable object fails, but protocol > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these > tests are always run using protocol v0. I think this is reasonable, but just musing on a few things: 1. Are we sure going forward that we want to retain this behavior? I feel like we discussed this on the list recently with no real conclusion, but I'm having trouble digging it up in the archive. 2. If it does change, is there any way we could automatically find spots like this that would then need to be undone? I cannot think of a good solution, and I don't think it's a show-stopper not to have one, but I thought I'd put it forward as a concept. -Peff
> On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote: > > > Some tests check that fetching an unreachable object fails, but protocol > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these > > tests are always run using protocol v0. > > I think this is reasonable, but just musing on a few things: > > 1. Are we sure going forward that we want to retain this behavior? I > feel like we discussed this on the list recently with no real > conclusion, but I'm having trouble digging it up in the archive. One such discussion is here: https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/ > 2. If it does change, is there any way we could automatically find > spots like this that would then need to be undone? I cannot think > of a good solution, and I don't think it's a show-stopper not to > have one, but I thought I'd put it forward as a concept. We can do so now either by "blaming" one and finding the originating commit, or by searching for "support fetching unadvertised objects" (I used the same comment everywhere in the commit [1] so that people can do this), but I don't know how to enforce this for future work. (We can add a special variable, but I think it's unnecessary and we can't enforce that people use that new special variable instead of GIT_TEST_PROTOCOL_VERSION anyway.) [1] https://public-inbox.org/git/9b9061985202ec022cc562637d7f62ea599e7d8c.1549411880.git.jonathantanmy@google.com/
On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote: > > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote: > > > > > Some tests check that fetching an unreachable object fails, but protocol > > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these > > > tests are always run using protocol v0. > > > > I think this is reasonable, but just musing on a few things: > > > > 1. Are we sure going forward that we want to retain this behavior? I > > feel like we discussed this on the list recently with no real > > conclusion, but I'm having trouble digging it up in the archive. > > One such discussion is here: > https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/ Thanks, that was what I was thinking of, though there doesn't seem to have been much discussion in response. :) > > 2. If it does change, is there any way we could automatically find > > spots like this that would then need to be undone? I cannot think > > of a good solution, and I don't think it's a show-stopper not to > > have one, but I thought I'd put it forward as a concept. > > We can do so now either by "blaming" one and finding the originating > commit, or by searching for "support fetching unadvertised objects" (I > used the same comment everywhere in the commit [1] so that people can do > this), but I don't know how to enforce this for future work. (We can add > a special variable, but I think it's unnecessary and we can't enforce > that people use that new special variable instead of > GIT_TEST_PROTOCOL_VERSION anyway.) Yeah, I think we can find them once we know they need fixing. I was more wondering whether we would remember to do so. I.e., is there a way the test suite could remind us when our assumptions change. I can't think of an easy way to do so, though. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Feb 14, 2019 at 11:58:25AM -0800, Jonathan Tan wrote: > >> > On Tue, Feb 05, 2019 at 04:21:16PM -0800, Jonathan Tan wrote: >> > >> > > Some tests check that fetching an unreachable object fails, but protocol >> > > v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these >> > > tests are always run using protocol v0. >> > >> > I think this is reasonable, but just musing on a few things: >> > >> > 1. Are we sure going forward that we want to retain this behavior? I >> > feel like we discussed this on the list recently with no real >> > conclusion, but I'm having trouble digging it up in the archive. >> >> One such discussion is here: >> https://public-inbox.org/git/20181214101232.GC13465@sigill.intra.peff.net/ > > Thanks, that was what I was thinking of, though there doesn't seem to > have been much discussion in response. :) > >> > 2. If it does change, is there any way we could automatically find >> > spots like this that would then need to be undone? I cannot think >> > of a good solution, and I don't think it's a show-stopper not to >> > have one, but I thought I'd put it forward as a concept. >> >> We can do so now either by "blaming" one and finding the originating >> commit, or by searching for "support fetching unadvertised objects" (I >> used the same comment everywhere in the commit [1] so that people can do >> this), but I don't know how to enforce this for future work. (We can add >> a special variable, but I think it's unnecessary and we can't enforce >> that people use that new special variable instead of >> GIT_TEST_PROTOCOL_VERSION anyway.) > > Yeah, I think we can find them once we know they need fixing. I was more > wondering whether we would remember to do so. I.e., is there a way the > test suite could remind us when our assumptions change. I can't think of > an easy way to do so, though. Perhaps looking it a different way may help. Instead of saying "v2 will not protect unreachable objects, so this test must be run with v0", think of it like "Git ought to protect unreachable objects, so test with different versions of protocols to make sure all satisfy the requirement---for now, v2 is known to be broken, so write it with test_expect_failure". IOW, have one test for each version, some of them may document a known breakage.
On Fri, Feb 22, 2019 at 12:47:42PM -0800, Junio C Hamano wrote: > >> We can do so now either by "blaming" one and finding the originating > >> commit, or by searching for "support fetching unadvertised objects" (I > >> used the same comment everywhere in the commit [1] so that people can do > >> this), but I don't know how to enforce this for future work. (We can add > >> a special variable, but I think it's unnecessary and we can't enforce > >> that people use that new special variable instead of > >> GIT_TEST_PROTOCOL_VERSION anyway.) > > > > Yeah, I think we can find them once we know they need fixing. I was more > > wondering whether we would remember to do so. I.e., is there a way the > > test suite could remind us when our assumptions change. I can't think of > > an easy way to do so, though. > > Perhaps looking it a different way may help. Instead of saying "v2 > will not protect unreachable objects, so this test must be run with > v0", think of it like "Git ought to protect unreachable objects, so > test with different versions of protocols to make sure all satisfy > the requirement---for now, v2 is known to be broken, so write it > with test_expect_failure". IOW, have one test for each version, > some of them may document a known breakage. Ah, yeah, that would work. It means writing out the test twice, once for each protocol version. But if we're explicitly expecting two different behaviors, then that's exactly what we ought to be doing. I'm still not sure if there's consensus on whether the v2 behavior is a problem or not, though. -Peff
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 49c540b1e1..0ef4d6f20c 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -636,7 +636,9 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a test_commit -C server 6 && git init client && - test_must_fail git -C client fetch-pack ../server \ + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \ $(git -C server rev-parse refs/heads/master^) 2>err && test_i18ngrep "Server does not allow request for unadvertised object" err ' diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 37e8e80893..4bfbb79654 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1147,8 +1147,12 @@ test_expect_success 'fetch exact SHA1' ' git prune && test_must_fail git cat-file -t $the_commit && + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + # fetching the hidden object should fail by default - test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err && test_i18ngrep "Server does not allow request for unadvertised object" err && test_must_fail git rev-parse --verify refs/heads/copy && @@ -1204,7 +1208,10 @@ do mk_empty shallow && ( cd shallow && - test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 && + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git fetch --depth=1 ../testrepo/.git $SHA1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch --depth=1 ../testrepo/.git $SHA1 && git cat-file commit $SHA1 @@ -1232,15 +1239,20 @@ do mk_empty shallow && ( cd shallow && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 && + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_3 && + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch ../testrepo/.git $SHA1_1 && git cat-file commit $SHA1_1 && test_must_fail git cat-file commit $SHA1_2 && git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \ + git fetch ../testrepo/.git $SHA1_3 ) ' done diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 8f620e0a35..d2661eb338 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -308,7 +308,10 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' ' git init --bare test_reachable.git && git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && - test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" ' test_expect_success 'test allowanysha1inwant with unreachable' ' @@ -327,7 +330,10 @@ test_expect_success 'test allowanysha1inwant with unreachable' ' git init --bare test_reachable.git && git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" && - test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" && + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" && git -C "$server" config uploadpack.allowanysha1inwant 1 && git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index e87164aa8f..c973278300 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -943,7 +943,10 @@ test_expect_success 'submodule update clone shallow submodule outside of depth' cd super3 && sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp && mv -f .gitmodules.tmp .gitmodules && - test_must_fail git submodule update --init --depth=1 2>actual && + # Some protocol versions (e.g. 2) support fetching + # unadvertised objects, so restrict this test to v0. + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git submodule update --init --depth=1 2>actual && test_i18ngrep "Direct fetching of that commit failed." actual && git -C ../submodule config uploadpack.allowReachableSHA1InWant true && git submodule update --init --depth=1 >actual &&
Some tests check that fetching an unreachable object fails, but protocol v2 allows such fetches. Unset GIT_TEST_PROTOCOL_VERSION so that these tests are always run using protocol v0. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- t/t5500-fetch-pack.sh | 4 +++- t/t5516-fetch-push.sh | 22 +++++++++++++++++----- t/t5551-http-fetch-smart.sh | 10 ++++++++-- t/t7406-submodule-update.sh | 5 ++++- 4 files changed, 32 insertions(+), 9 deletions(-)