| Message ID | 6e0c6aa9a71d4192591ed406735684cd15a0e3b9.1549411880.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resend of GIT_TEST_PROTOCOL_VERSION patches | expand |
On Wed, Feb 06 2019, Jonathan Tan wrote: > Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used > from tests. When set, this ensures protocol.version is at least the > given value, allowing the entire test suite to be run as if this > configuration is in place for all repositories. > > As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is > unset, set to 0, or set to 1. Some tests fail when > GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in > subsequent patches. > > This is based on work by Ævar Arnfjörð Bjarmason. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > protocol.c | 17 +++++++++++++++-- > t/README | 3 +++ > t/t5400-send-pack.sh | 2 +- > t/t5551-http-fetch-smart.sh | 3 ++- > 4 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/protocol.c b/protocol.c > index 5664bd7a05..c7a735bfa2 100644 > --- a/protocol.c > +++ b/protocol.c > @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version) > enum protocol_version get_protocol_version_config(void) > { > const char *value; > + enum protocol_version retval = protocol_v0; > + const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; > + const char *git_test_v = getenv(git_test_k); > + > if (!git_config_get_string_const("protocol.version", &value)) { > enum protocol_version version = parse_protocol_version(value); > > @@ -49,10 +53,19 @@ enum protocol_version get_protocol_version_config(void) > die("unknown value for config 'protocol.version': %s", > value); > > - return version; > + retval = version; > + } > + > + if (git_test_v && strlen(git_test_v)) { > + enum protocol_version env = parse_protocol_version(git_test_v); > + > + if (env == protocol_unknown_version) > + die("unknown value for %s: %s", git_test_k, git_test_v); > + if (retval < env) > + retval = env; > } > > - return protocol_v0; > + return retval; > } > > void register_allowed_protocol_version(enum protocol_version version) > diff --git a/t/README b/t/README > index 25864ec883..21e941eb94 100644 > --- a/t/README > +++ b/t/README > @@ -327,6 +327,9 @@ marked strings" in po/README for details. > GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole > test suite. Accept any boolean values that are accepted by git-config. > > +GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the > +'protocol.version' setting to n if it is less than n. > + In my version (https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/) I didn't have this "if it is less than n" caveat. I expect that helped with making some tests that were setting e.g. protocol.version=2 Just Work, is that the reason for this? Mine also had more docs here, but maybe telling people that they can use "env" is too much... > GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon > pack-objects code path where there are more than 1024 packs even if > the actual number of packs in repository is below this limit. Accept > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh > index f1932ea431..571d620aed 100755 > --- a/t/t5400-send-pack.sh > +++ b/t/t5400-send-pack.sh > @@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' ' > $shared .have > EOF > > - GIT_TRACE_PACKET=$(pwd)/trace \ > + GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \ > git push \ > --receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \ > fork HEAD:foo && > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh > index a60dd907bd..8f620e0a35 100755 > --- a/t/t5551-http-fetch-smart.sh > +++ b/t/t5551-http-fetch-smart.sh > @@ -44,7 +44,8 @@ test_expect_success 'clone http repository' ' > < Cache-Control: no-cache, max-age=0, must-revalidate > < Content-Type: application/x-git-upload-pack-result > EOF > - GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && > + GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \ > + git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && > test_cmp file clone/file && > tr '\''\015'\'' Q <err | > sed -e "
> > @@ -327,6 +327,9 @@ marked strings" in po/README for details. > > GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole > > test suite. Accept any boolean values that are accepted by git-config. > > > > +GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the > > +'protocol.version' setting to n if it is less than n. > > + > > In my version > (https://public-inbox.org/git/20181213155817.27666-6-avarab@gmail.com/) > I didn't have this "if it is less than n" caveat. I expect that helped > with making some tests that were setting e.g. protocol.version=2 Just > Work, is that the reason for this? Yes, that's right. I thought that there is not much value in testing tests that are explicitly protocol v2 as another protocol, since there was a reason in the first place why the test writer wanted to test it with v2. > Mine also had more docs here, but maybe telling people that they can use > "env" is too much... With the ability to set =0 to effectively disable the option (because the minimum of 0 and 0/1/2 is 0/1/2), I thought it was less important.
On Tue, Feb 05, 2019 at 04:21:15PM -0800, Jonathan Tan wrote: > Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used > from tests. When set, this ensures protocol.version is at least the > given value, allowing the entire test suite to be run as if this > configuration is in place for all repositories. > > As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is > unset, set to 0, or set to 1. Some tests fail when > GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in > subsequent patches. Makes sense. The "at least" part made me scratch my head at first, but your explanation in response to Ævar made sense. Two minor nits: > diff --git a/protocol.c b/protocol.c > index 5664bd7a05..c7a735bfa2 100644 > --- a/protocol.c > +++ b/protocol.c > @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version) > enum protocol_version get_protocol_version_config(void) > { > const char *value; > + enum protocol_version retval = protocol_v0; > + const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; > + const char *git_test_v = getenv(git_test_k); We've discussed recently how the return value from getenv() isn't stable. It looks like we could assign it much closer to the point-of-use here (which still isn't 100% foolproof, but I think is something we could encourage as a general pattern, and mostly works due to our ring-buffer technique). I.e., right before this conditional: > + > + if (git_test_v && strlen(git_test_v)) { It's more idiomatic in our code base to check for a non-empty string as: if (git_test_v && *git_test_v) though obviously that's pretty minor. -Peff
diff --git a/protocol.c b/protocol.c index 5664bd7a05..c7a735bfa2 100644 --- a/protocol.c +++ b/protocol.c @@ -42,6 +42,10 @@ static const char *format_protocol_version(enum protocol_version version) enum protocol_version get_protocol_version_config(void) { const char *value; + enum protocol_version retval = protocol_v0; + const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; + const char *git_test_v = getenv(git_test_k); + if (!git_config_get_string_const("protocol.version", &value)) { enum protocol_version version = parse_protocol_version(value); @@ -49,10 +53,19 @@ enum protocol_version get_protocol_version_config(void) die("unknown value for config 'protocol.version': %s", value); - return version; + retval = version; + } + + if (git_test_v && strlen(git_test_v)) { + enum protocol_version env = parse_protocol_version(git_test_v); + + if (env == protocol_unknown_version) + die("unknown value for %s: %s", git_test_k, git_test_v); + if (retval < env) + retval = env; } - return protocol_v0; + return retval; } void register_allowed_protocol_version(enum protocol_version version) diff --git a/t/README b/t/README index 25864ec883..21e941eb94 100644 --- a/t/README +++ b/t/README @@ -327,6 +327,9 @@ marked strings" in po/README for details. GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole test suite. Accept any boolean values that are accepted by git-config. +GIT_TEST_PROTOCOL_VERSION=<n>, when set, overrides the +'protocol.version' setting to n if it is less than n. + GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon pack-objects code path where there are more than 1024 packs even if the actual number of packs in repository is below this limit. Accept diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh index f1932ea431..571d620aed 100755 --- a/t/t5400-send-pack.sh +++ b/t/t5400-send-pack.sh @@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' ' $shared .have EOF - GIT_TRACE_PACKET=$(pwd)/trace \ + GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \ git push \ --receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \ fork HEAD:foo && diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index a60dd907bd..8f620e0a35 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -44,7 +44,8 @@ test_expect_success 'clone http repository' ' < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-result EOF - GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && + GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \ + git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && test_cmp file clone/file && tr '\''\015'\'' Q <err | sed -e "
Define a GIT_TEST_PROTOCOL_VERSION environment variable meant to be used from tests. When set, this ensures protocol.version is at least the given value, allowing the entire test suite to be run as if this configuration is in place for all repositories. As of this patch, all tests pass whether GIT_TEST_PROTOCOL_VERSION is unset, set to 0, or set to 1. Some tests fail when GIT_TEST_PROTOCOL_VERSION is set to 2, but this will be dealt with in subsequent patches. This is based on work by Ævar Arnfjörð Bjarmason. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- protocol.c | 17 +++++++++++++++-- t/README | 3 +++ t/t5400-send-pack.sh | 2 +- t/t5551-http-fetch-smart.sh | 3 ++- 4 files changed, 21 insertions(+), 4 deletions(-)