Re: [PATCH ALTERNATIVE v6.v3 4/6] config: parse http.<url>.<variable> using urlmatch
- From
Kyle J. McKay <mackyle@gmail.com>
- Date
- Aug 5, 2013, 23:57 UTC
- 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:
Show 13 quoted lines
> "Kyle J. McKay" <mackyle@gmail.com> writes: > >> Use the urlmatch_config_entry() to wrap the underlying >> http_options() two-level variable parser in order to set >> http.<variable> to the value with the most specific URL in the >> configuration. >> >> Signed-off-by: Jeff King <peff@peff.net> >> Signed-off-by: Kyle J. McKay <mackyle@gmail.com> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- > > 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:
Show 13 quoted lines
> * jc/url-match (2013-07-31) 6 commits > - config: "git config --get-urlmatch" parses section.<url>.key > - builtin/config: refactor collect_config() > - config: parse http.<url>.<variable> using urlmatch > - config: add generic callback wrapper to parse section.<url>.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.
Show 15 quoted lines
>> 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.
Show 23 quoted lines
> 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).
Show 8 quoted lines
> 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 ]
Show 6 quoted lines
>> +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.
Show 5 quoted lines
>> + 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 */
Show 50 quoted lines
>> + 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.