mbox series

[WIP,0/8] Trying to revive GIT_TEST_PROTOCOL_VERSION

Message ID cover.1547677183.git.jonathantanmy@google.com (mailing list archive)
Headers show
Series Trying to revive GIT_TEST_PROTOCOL_VERSION | expand

Message

Jonathan Tan Jan. 16, 2019, 10:42 p.m. UTC
Ævar, are you planning to revive GIT_TEST_PROTOCOL_VERSION? I have
updated the patchset in light of some new branches that have appeared.

This is on master merged with:
 - jk/proto-v2-hidden-refs-fix
 - tg/t5570-drop-racy-test
 - js/protocol-advertise-multi
 - jt/upload-pack-deepen-relative-proto-v2
 - jt/fetch-pack-v2

One notable change I made is that I made this envvar determine the
minimum version. So, if GIT_TEST_PROTOCOL_VERSION=1 and the test
explicitly states v2, v2 is used (but if GIT_TEST_PROTOCOL_VERISON=2,
all use v2). I think this reduces the number of false negatives, since
there are quite a few tests that use v2 specific features, and that are
already marked as v2.

I included one genuine bug fix (the last patch) and the rest are either
overspecifications (which I didn't investigate too deeply) or false
negatives. There are still some errors when run with
GIT_TEST_PROTOCOL_VERSION=2 which I don't think are false negatives -
I'll continue to look into them.

Jonathan Tan (8):
  tests: define GIT_TEST_PROTOCOL_VERSION
  tests: always test fetch of unreachable with v0
  t5503: fix overspecification of trace expectation
  t5512: compensate for v0 only sending HEAD symrefs
  t5700: only run with protocol version 1
  tests: fix protocol version for overspecifications
  t5552: compensate for v2 filtering ref adv.
  remote-curl: in v2, fill credentials if needed

 protocol.c                           | 17 ++++++++--
 remote-curl.c                        |  9 +++++-
 t/README                             |  3 ++
 t/t5400-send-pack.sh                 |  2 +-
 t/t5500-fetch-pack.sh                |  4 ++-
 t/t5503-tagfollow.sh                 |  2 +-
 t/t5512-ls-remote.sh                 | 18 ++++++++---
 t/t5515-fetch-merge-logic.sh         |  4 +++
 t/t5516-fetch-push.sh                | 22 ++++++++++---
 t/t5539-fetch-http-shallow.sh        |  5 ++-
 t/t5541-http-push-smart.sh           | 14 +++++++--
 t/t5551-http-fetch-smart.sh          | 47 +++++++++++++++++++++-------
 t/t5552-skipping-fetch-negotiator.sh |  5 ++-
 t/t5700-protocol-v1.sh               |  3 ++
 t/t7406-submodule-update.sh          |  5 ++-
 15 files changed, 128 insertions(+), 32 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Jan. 17, 2019, 9:31 a.m. UTC | #1
On Wed, Jan 16 2019, Jonathan Tan wrote:

> Ævar, are you planning to revive GIT_TEST_PROTOCOL_VERSION? I have
> updated the patchset in light of some new branches that have appeared.
>
> This is on master merged with:
>  - jk/proto-v2-hidden-refs-fix
>  - tg/t5570-drop-racy-test
>  - js/protocol-advertise-multi
>  - jt/upload-pack-deepen-relative-proto-v2
>  - jt/fetch-pack-v2

I'm happy to have you pick that up as you've done here, especially since
you're actually working on v2 and I'm not, so you can more easily know
what it conflicts with etc. I just wanted to have it in one way or
another, i.e. be able to deploy v2 and assert that "next + custom
patches" doesn't break something for v2.

I think [CC: Junio] that we shouldn't be concerned about an addition of
GIT_TEST_PROTOCOL_VERSION patches in any form breaking the test suite
under GIT_TEST_PROTOCOL_VERSION=2, and just be concerned about the
default GIT_TEST_PROTOCOL_VERSION= case. I.e. if we have v2 patches
in-flight that break things no big deal, we can always circle back and
fix those things or annotate the tests.
Jonathan Tan Jan. 17, 2019, 6:37 p.m. UTC | #2
> I'm happy to have you pick that up as you've done here, especially since
> you're actually working on v2 and I'm not, so you can more easily know
> what it conflicts with etc. I just wanted to have it in one way or
> another, i.e. be able to deploy v2 and assert that "next + custom
> patches" doesn't break something for v2.
> 
> I think [CC: Junio] that we shouldn't be concerned about an addition of
> GIT_TEST_PROTOCOL_VERSION patches in any form breaking the test suite
> under GIT_TEST_PROTOCOL_VERSION=2, and just be concerned about the
> default GIT_TEST_PROTOCOL_VERSION= case. I.e. if we have v2 patches
> in-flight that break things no big deal, we can always circle back and
> fix those things or annotate the tests.

That sounds good to me. My main concern is that this will end up being
dead code (if we have too many tests that fail with
GIT_TEST_PROTOCOL_VERSION=2 and no one bothers with it anymore), but I
don't think that will happen - in this patch set, I have eliminated a
lot of false failures and strove to give reasons for the
GIT_TEST_PROTOCOL_VERSION= annotations, and I think there's interest
(well, at least from me) in investigating the remaining apparent bugs.
Junio C Hamano Jan. 17, 2019, 9:18 p.m. UTC | #3
Jonathan Tan <jonathantanmy@google.com> writes:

>> I'm happy to have you pick that up as you've done here, especially since
>> you're actually working on v2 and I'm not, so you can more easily know
>> what it conflicts with etc. I just wanted to have it in one way or
>> another, i.e. be able to deploy v2 and assert that "next + custom
>> patches" doesn't break something for v2.
>> 
>> I think [CC: Junio] that we shouldn't be concerned about an addition of
>> GIT_TEST_PROTOCOL_VERSION patches in any form breaking the test suite
>> under GIT_TEST_PROTOCOL_VERSION=2, and just be concerned about the
>> default GIT_TEST_PROTOCOL_VERSION= case. I.e. if we have v2 patches
>> in-flight that break things no big deal, we can always circle back and
>> fix those things or annotate the tests.
>
> That sounds good to me. My main concern is that this will end up being
> dead code (if we have too many tests that fail with
> GIT_TEST_PROTOCOL_VERSION=2 and no one bothers with it anymore), but I
> don't think that will happen - in this patch set, I have eliminated a
> lot of false failures and strove to give reasons for the
> GIT_TEST_PROTOCOL_VERSION= annotations, and I think there's interest
> (well, at least from me) in investigating the remaining apparent bugs.

Yup, sounds good to me, too.