Re: [PATCH ALTERNATIVE v6.v3 4/6] config: parse http.<url>.<variable> using urlmatch

["Kyle J. McKay" <mackyle@gmail.com>]

On Aug 5, 2013, at 15:56, Junio C Hamano wrote:
> "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:

> * 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.

>> 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.