From: Kyle J. McKay Date: Mon, 05 Aug 2013 23:57:23 GMT Subject: Re: [PATCH ALTERNATIVE v6.v3 4/6] config: parse http.. using urlmatch Message-ID: <503E9E2D-93E5-43CB-BB61-499052B35F4F@gmail.com> In-Reply-To: <7vsiyniw63.fsf@alter.siamese.dyndns.org> On Aug 5, 2013, at 15:56, Junio C Hamano wrote: > "Kyle J. McKay" writes: > >> Use the urlmatch_config_entry() to wrap the underlying >> http_options() two-level variable parser in order to set >> http. to the value with the most specific URL in the >> configuration. >> >> Signed-off-by: Jeff King >> Signed-off-by: Kyle J. McKay >> Signed-off-by: Junio C Hamano >> --- > > Oops, what did we sign-off? Some code removal. No new additions. Actually this: On Aug 1, 2013, at 14:44, Junio C Hamano wrote: > * jc/url-match (2013-07-31) 6 commits > - config: "git config --get-urlmatch" parses section..key > - builtin/config: refactor collect_config() > - config: parse http.. using urlmatch > - config: add generic callback wrapper to parse section..key > - config: add helper to normalize and match URLs > - http.c: fix parsing of http.sslCertPasswordProtected variable > > Reroll of km/http-curl-config-per-url topic. Peff raises many good > points about the tests for http.* variables. > > Waiting for the discussion to conclude, hopefully with a replacement > test. As requested. >> This version of 4/6 moves the tests to t0110 since urlmatch is now >> global. >> The config tests are removed since part 6/6 already has those and >> they no >> longer belong with the urlmatch normalization tests. >> >> The Makefile rule has been removed since it's no longer needed to >> build >> correctly as the test program no longer includes http.c. >> >> Other than those changes (and a minor rename to reflect the new >> location), >> this patch is identical to the previous v6.v2 4/6. > > Ahh, figures. Thanks. The remaining tests, by the way, have not changed. They are identical to previous versions. > Peff, any comments? > >> diff --git a/t/t0110-urlmatch-normalization.sh b/t/t0110-urlmatch- >> normalization.sh >> new file mode 100755 >> index 00000000..8d6096d4 >> --- /dev/null >> +++ b/t/t0110-urlmatch-normalization.sh >> @@ -0,0 +1,177 @@ >> +#!/bin/sh >> + >> +test_description='urlmatch URL normalization' >> +. ./test-lib.sh >> + >> +# The base name of the test url files >> +tu="$TEST_DIRECTORY/t0110/url" >> + >> +# Note that only file: URLs should be allowed without a host > > It is somewhat unfortunate that the form most commonly used for > pushing is not supported at all, i.e. > > host:path That is an SSH extension and they are certainly not URLs according to RFC 3986 because that would require every host to be its own scheme. Also, host:path cannot in the general case, be unambiguously translated to a URL. For example, repo.or.cz:srv/git/alt-git, has no translation. It is different from repo.or.cz:/srv/git/alt-git which does have a translation. There's no guarantee that inserting a '/' will not change the meaning of the URL (that only happens to be the case on repo.or.cz because all the ssh git users in the chroot jail have a '/' home directory). > Current configuration set may not have anything interesting to > affect the git-over-ssh push codepath, so in practice it may not > matter, though. > >> +test_expect_success 'url authority' ' > > "authority" refers to the host part? (not a complaint, but is a > question) It refers to this production from RFC 3986 Section "3.2 Authority": authority = [ userinfo "@" ] host [ ":" port ] >> +test_expect_success 'url port checks' ' >> + test-urlmatch-normalization "xyz://q@some.host:" && > > This is presumably replaced by a default port for xyz:// scheme, > whatever the default port is, in other words, it is as if no colon > is given at the end? Yes. The "port" production above is: port = *DIGIT which means 0 or more digits. >> + test-urlmatch-normalization "xyz://q@some.host:456/" && >> + ! test-urlmatch-normalization "xyz://q@some.host:0" && >> + ! test-urlmatch-normalization "xyz://q@some.host:0000000" && > > Port #0 is disallowed? Intentionally so. The comments from urlmatch.c talk about this: /* * Port number must be all digits with leading 0s removed * and since all the protocols we deal with have a 16-bit * port number it must also be in the range 1..65535 * 0 is not allowed because that means "next available" * on just about every system and therefore cannot be used */ >> + test-urlmatch-normalization "xyz://q@some.host:0000001?" && > > Is it the same as specifying "xyz://q@some.host:1?" and does it > match "xyz://q@some.host:1"? > >> + test-urlmatch-normalization "xyz://q@some.host:065535#" && > > Ditto, for 65535 and without #-fragment at the end? > >> +test_expect_success 'url port normalization' ' >> + test "$(test-urlmatch-normalization -p "http://x:800")" = "http:// >> x:800/" && >> + test "$(test-urlmatch-normalization -p "http://x:0800")" = >> "http://x:800/" && >> + test "$(test-urlmatch-normalization -p "http://x:00000800")" = >> "http://x:800/" && >> + test "$(test-urlmatch-normalization -p "http://x:065535")" = >> "http://x:65535/" && >> + test "$(test-urlmatch-normalization -p "http://x:1")" = "http://x: >> 1/" && >> + test "$(test-urlmatch-normalization -p "http://x:80")" = "http:// >> x/" && >> + test "$(test-urlmatch-normalization -p "http://x:080")" = "http:// >> x/" && >> + test "$(test-urlmatch-normalization -p "http://x:000000080")" = >> "http://x/" && >> + test "$(test-urlmatch-normalization -p "https://x:443")" = "https://x/ >> " && >> + test "$(test-urlmatch-normalization -p "https://x:0443")" = "https://x/ >> " && >> + test "$(test-urlmatch-normalization -p "https://x:000000443")" = "https://x/ >> " >> +' > > OK, these answer most of the previous questions. > >> +# http://@foo specifies an empty user name but does not specify a >> password >> +# http://foo specifies neither a user name nor a password >> +# So they should not be equivalent >> +test_expect_success 'url equivalents' ' >> + test-urlmatch-normalization "httP://x" "Http://X/" && >> + test-urlmatch-normalization "Http://%4d%65:%4d^%70@The.Host" "hTTP://Me >> :%4D^p@the.HOST:80/" && >> + ! test-urlmatch-normalization "https://@x.y/^" "httpS://x.y:443/ >> ^" && > > The comment is about this test, which seems to make sense. What is > "^"? Just a random valid character that can appear in the path? > (not a complaint, but is a question). The character '^' is one of the always-unsafe characters that must always be escaped. It's also one of the always-unsafe characters that's easy to include in the tests as it doesn't require escaping or backslashing or binary includes. It doesn't otherwise have any special meaning.