Re: [PATCH v8 3/4] tests: add new test for the url_normalize function
On Wed, Jul 24, 2013 at 12:01:26PM -0700, Kyle J. McKay wrote: > Right now, the values are only available as various strings, ints, > longs etc. which have to be formatted differently for output. The > original string value they were converted from is gone. The snippet > shown above only shows some of the "%s" formatters. > > Either the original value will have to be kept around or a > reconstituted string depending on what: > > git config --file=foo --url http noepsv $URL > > should output. If the original value was 0 or 1, should it output > that or "false" or "true"? The test-url-normalize code for "-c" > normalizes the output to "false" or "true" for all boolean values and > reconverts ints/longs to strings. I think it would be the responsibility of the caller to specify what they are looking for. I.e., add "--bool" to the git-config command line as appropriate. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/4] tests: add new test for the url_normalize function
On Jul 23, 2013, at 23:59, Jeff King wrote:
diff --git a/test-url-normalize.c b/test-url-normalize.c
new file mode 100644
index 000..f18bd88
--- /dev/null
+++ b/test-url-normalize.c
[...]
+ if (!strcmp("sslverify", opt_lc.buf))
+ printf("%s\n", curl_ssl_verify ? "true" : "false");
+ else if (!strcmp("sslcert", opt_lc.buf))
+ printf("%s\n", ssl_cert);
+#if LIBCURL_VERSION_NUM >= 0x070903
+ else if (!strcmp("sslkey", opt_lc.buf))
+ printf("%s\n", ssl_key);
+#endif
+#if LIBCURL_VERSION_NUM >= 0x070908
+ else if (!strcmp("sslcapath", opt_lc.buf))
+ printf("%s\n", ssl_capath);
+#endif
[...]
Do we need to have the complete list of options here, including curl
version limitations? It seems like this will eventually get out of
date
with the list of options. Would it be sufficient to test just one (or
even just handle a fake "http.$URL.foo" variable)?
Right now, the values are only available as various strings, ints,
longs etc. which have to be formatted differently for output. The
original string value they were converted from is gone. The snippet
shown above only shows some of the "%s" formatters.
Either the original value will have to be kept around or a
reconstituted string depending on what:
git config --file=foo --url http noepsv $URL
should output. If the original value was 0 or 1, should it output
that or "false" or "true"? The test-url-normalize code for "-c"
normalizes the output to "false" or "true" for all boolean values and
reconverts ints/longs to strings.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/4] tests: add new test for the url_normalize function
On Jul 24, 2013, at 10:14, Junio C Hamano wrote:
Jeff King writes:
How hard would it be to convert the "-c" option of test-url-normalize
into something like:
git config --file=foo --url http noepsv $URL
which would look for http.$URL.noepsv matches.
Lovely.
[snip]
Another thing we may want to consider is to see if we can
restructure the http_options interface a bit, so that the caller can
be agonistic to the actual meaning of the key. For example,
"git config --url http notknownyet $URL"
may want to be able to show the value for http..notknownyet
for the most matching for a given $URL, without knowing
what the variable means, just like any other configuration that is
queried via the "git config" program. The caller may want to pass
further type information like --bool, --int and --path as needed.
+#define url_normalize(u) http_options_url_normalize(u)
Does this macro do anything besides shorten the name? Is the extra
level of indirection to the reader worth it?
Probably not.
It's a hint that the name of the function might change. If it's going
to be used in a more generic fashion it may not belong in http.c
anymore in which case the name will likely change along with the
location. Nothing about the normalization function requires CURL or
http/https, so it would work equally well on rsync, ftp, smtp etc.
urls and could just move into url.{h,c} as url_normalize.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/4] tests: add new test for the url_normalize function
Jeff King writes:
> That is, would a shell script ever want to say "what is the value of
> this config variable for url $X"? Certainly our test scripts want to,
> and having a test-* program covers that, but might user scripts want to
> do the same? Or even to introduce its own URL-matched config options?
>
> How hard would it be to convert the "-c" option of test-url-normalize
> into something like:
>
> git config --file=foo --url http noepsv $URL
>
> which would look for http.$URL.noepsv matches.
Lovely.
>> +#if LIBCURL_VERSION_NUM >= 0x070903
>> +else if (!strcmp("sslkey", opt_lc.buf))
>> +printf("%s\n", ssl_key);
>> +#endif
>> +#if LIBCURL_VERSION_NUM >= 0x070908
>> +else if (!strcmp("sslcapath", opt_lc.buf))
>> +printf("%s\n", ssl_capath);
>> +#endif
>> [...]
>
> Do we need to have the complete list of options here, including curl
> version limitations? It seems like this will eventually get out of date
> with the list of options. Would it be sufficient to test just one (or
> even just handle a fake "http.$URL.foo" variable)?
Yeah, and that will be in line with "git config --url" direction.
Another thing we may want to consider is to see if we can
restructure the http_options interface a bit, so that the caller can
be agonistic to the actual meaning of the key. For example,
"git config --url http notknownyet $URL"
may want to be able to show the value for http..notknownyet
for the most matching for a given $URL, without knowing
what the variable means, just like any other configuration that is
queried via the "git config" program. The caller may want to pass
further type information like --bool, --int and --path as needed.
>> +#define url_normalize(u) http_options_url_normalize(u)
>
> Does this macro do anything besides shorten the name? Is the extra
> level of indirection to the reader worth it?
Probably not.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/4] tests: add new test for the url_normalize function
On Mon, Jul 22, 2013 at 05:56:43AM -0700, Kyle J. McKay wrote:
> In order to perform sane URL matching for http..* options,
> http.c normalizes URLs before performing matches.
>
> A new test-url-normalize test program is introduced along with
> a new t5200-url-normalize.sh script to run the tests.
While looking at test-url-normalize (and wanting to experiment some with
your series to see how it handled a few behaviors), I wondered if we
shouldn't be exposing this selection process to the user somehow via
git-config.
That is, would a shell script ever want to say "what is the value of
this config variable for url $X"? Certainly our test scripts want to,
and having a test-* program covers that, but might user scripts want to
do the same? Or even to introduce its own URL-matched config options?
How hard would it be to convert the "-c" option of test-url-normalize
into something like:
git config --file=foo --url http noepsv $URL
which would look for http.$URL.noepsv matches.
If we want to go that route, we don't have to do it now (the test-*
interface is unpublished, and the git-config interface can simply come
later than the internal feature). But it may be worth thinking about
now while it is in your head.
> diff --git a/test-url-normalize.c b/test-url-normalize.c
> new file mode 100644
> index 000..f18bd88
> --- /dev/null
> +++ b/test-url-normalize.c
> [...]
> + if (!strcmp("sslverify", opt_lc.buf))
> + printf("%s\n", curl_ssl_verify ? "true" : "false");
> + else if (!strcmp("sslcert", opt_lc.buf))
> + printf("%s\n", ssl_cert);
> +#if LIBCURL_VERSION_NUM >= 0x070903
> + else if (!strcmp("sslkey", opt_lc.buf))
> + printf("%s\n", ssl_key);
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070908
> + else if (!strcmp("sslcapath", opt_lc.buf))
> + printf("%s\n", ssl_capath);
> +#endif
> [...]
Do we need to have the complete list of options here, including curl
version limitations? It seems like this will eventually get out of date
with the list of options. Would it be sufficient to test just one (or
even just handle a fake "http.$URL.foo" variable)?
> +#define url_normalize(u) http_options_url_normalize(u)
Does this macro do anything besides shorten the name? Is the extra
level of indirection to the reader worth it?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 3/4] tests: add new test for the url_normalize function
In order to perform sane URL matching for http..* options, http.c normalizes URLs before performing matches. A new test-url-normalize test program is introduced along with a new t5200-url-normalize.sh script to run the tests. Since the url_normalize function currently lives in http.c this test will be skipped if NO_CURL is defined since http.c is skipped in that case. Signed-off-by: Kyle J. McKay --- .gitignore | 1 + Makefile | 5 ++ t/.gitattributes | 1 + t/t5200-url-normalize.sh | 198 +++ t/t5200/README | 18 + t/t5200/config-1 | 8 ++ t/t5200/config-2 | 3 + t/t5200/url-1| Bin 0 -> 20 bytes t/t5200/url-10 | Bin 0 -> 23 bytes t/t5200/url-11 | Bin 0 -> 25 bytes t/t5200/url-2| Bin 0 -> 20 bytes t/t5200/url-3| Bin 0 -> 23 bytes t/t5200/url-4| Bin 0 -> 23 bytes t/t5200/url-5| Bin 0 -> 23 bytes t/t5200/url-6| Bin 0 -> 23 bytes t/t5200/url-7| Bin 0 -> 23 bytes t/t5200/url-8| Bin 0 -> 23 bytes t/t5200/url-9| Bin 0 -> 23 bytes test-url-normalize.c | 131 +++ 19 files changed, 365 insertions(+) create mode 100755 t/t5200-url-normalize.sh create mode 100644 t/t5200/README create mode 100644 t/t5200/config-1 create mode 100644 t/t5200/config-2 create mode 100644 t/t5200/url-1 create mode 100644 t/t5200/url-10 create mode 100644 t/t5200/url-11 create mode 100644 t/t5200/url-2 create mode 100644 t/t5200/url-3 create mode 100644 t/t5200/url-4 create mode 100644 t/t5200/url-5 create mode 100644 t/t5200/url-6 create mode 100644 t/t5200/url-7 create mode 100644 t/t5200/url-8 create mode 100644 t/t5200/url-9 create mode 100644 test-url-normalize.c diff --git a/.gitignore b/.gitignore index 6669bf0..cd97e16 100644 --- a/.gitignore +++ b/.gitignore @@ -198,6 +198,7 @@ /test-string-list /test-subprocess /test-svn-fe +/test-url-normalize /test-wildmatch /common-cmds.h *.tar.gz diff --git a/Makefile b/Makefile index 0f931a2..f83879c 100644 --- a/Makefile +++ b/Makefile @@ -567,6 +567,7 @@ TEST_PROGRAMS_NEED_X += test-sigchain TEST_PROGRAMS_NEED_X += test-string-list TEST_PROGRAMS_NEED_X += test-subprocess TEST_PROGRAMS_NEED_X += test-svn-fe +TEST_PROGRAMS_NEED_X += test-url-normalize TEST_PROGRAMS_NEED_X += test-wildmatch TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) @@ -2235,6 +2236,10 @@ test-parse-options$X: parse-options.o parse-options-cb.o test-svn-fe$X: vcs-svn/lib.a +test-url-normalize$X: test-url-normalize.o GIT-LDFLAGS $(GITLIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) + .PRECIOUS: $(TEST_OBJS) test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS) diff --git a/t/.gitattributes b/t/.gitattributes index 1b97c54..f6f1df3 100644 --- a/t/.gitattributes +++ b/t/.gitattributes @@ -1 +1,2 @@ t[0-9][0-9][0-9][0-9]/* -whitespace +t5200/url-* binary diff --git a/t/t5200-url-normalize.sh b/t/t5200-url-normalize.sh new file mode 100755 index 000..d2dd886 --- /dev/null +++ b/t/t5200-url-normalize.sh @@ -0,0 +1,198 @@ +#!/bin/sh + +test_description='url normalization' +. ./test-lib.sh + +if test -n "$NO_CURL"; then + skip_all='skipping test, git built without http support' + test_done +fi + +# The base name of the test url files +tu="$TEST_DIRECTORY/t5200/url" + +# The base name of the test config files +tc="$TEST_DIRECTORY/t5200/config" + +# Note that only file: URLs should be allowed without a host + +test_expect_success 'url scheme' ' + ! test-url-normalize "" && + ! test-url-normalize "_" && + ! test-url-normalize "scheme" && + ! test-url-normalize "scheme:" && + ! test-url-normalize "scheme:/" && + ! test-url-normalize "scheme://" && + ! test-url-normalize "file" && + ! test-url-normalize "file:" && + ! test-url-normalize "file:/" && + test-url-normalize "file://" && + ! test-url-normalize "://acme.co" && + ! test-url-normalize "x_test://acme.co" && + ! test-url-normalize "-test://acme.co" && + ! test-url-normalize "0test://acme.co" && + ! test-url-normalize "+test://acme.co" && + ! test-url-normalize ".test://acme.co" && + ! test-url-normalize "schem%6e://" && + test-url-normalize "x-Test+v1.0://acme.co" && + test "$(test-url-normalize -p "AbCdeF://x.Y")" = "abcdef://x.y/" +' + +test_expect_success 'url authority' ' + ! test-url-normalize "scheme://user:pass@" && + ! test-url-normalize "scheme://?" && + ! test-url-normalize "scheme://#" && + ! test-url-normalize "scheme:///" && + ! test-url-normalize "scheme://:" && + ! test-url-normalize "scheme://:555" && + test-url-normalize "file://user:pass@" && + test-url-normali
