Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases

Mike Hommey [off-list ref] writes:

t5603-clone-dirname uses url patterns that are not tested with
fetch-pack --diag-url, and it would be useful if they were.

Interestingly, some of those tests, involving both a port and a
user:password pair, don't currently pass. Note that even if a
user:password pair is actually not supported by git, the values used
could be valid user names (user names can actually contain colons
and at signs), and are still worth testing the url parser for.

I am not sure about the last part of this (and the tests in the
patch for them).  When you are constrained by the Common Internet
Scheme Syntax, i.e.

    <scheme>://<user>:<password>@<host>:<port>/<url-path>

you cannot have arbitrary characters in these parts; within the user
and password field, any ":", "@", or "/" must be encoded.

Which maens that for the purpose of the parser you are modifying,
you can rely on these three special characters to parse things out
(decoding after the code determines which part is user and which
part is password is a separate issue).

That is way too confusing a name when reading the caller of it by
being too close to generic test helpers like test_might_fail,
test_must_fail, etc.

+	host=$1; shift
+	case $host in
+		git=*)

Dedent this line by one level.

+		test_expect_success "$@"
+		;;

These two lines are indented correctly.

On 2016-05-03 18.07, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

t5603-clone-dirname uses url patterns that are not tested with
fetch-pack --diag-url, and it would be useful if they were.

Interestingly, some of those tests, involving both a port and a
user:password pair, don't currently pass. Note that even if a
user:password pair is actually not supported by git, the values used
could be valid user names (user names can actually contain colons
and at signs), and are still worth testing the url parser for.

I am not sure about the last part of this (and the tests in the
patch for them).  When you are constrained by the Common Internet
Scheme Syntax, i.e.

    <scheme>://<user>:<password>@<host>:<port>/<url-path>

you cannot have arbitrary characters in these parts; within the user
and password field, any ":", "@", or "/" must be encoded.

I thinnk we have an old bug here:
	if (is_url(url_orig))
		url = url_decode(url_orig);
	else
		url = xstrdup(url_orig);

The the url should be separated into the components first,
and afther that url-path should got into url_decode,
and may be password, username....
(That's out of my head)

On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

t5603-clone-dirname uses url patterns that are not tested with
fetch-pack --diag-url, and it would be useful if they were.

Interestingly, some of those tests, involving both a port and a
user:password pair, don't currently pass. Note that even if a
user:password pair is actually not supported by git, the values used
could be valid user names (user names can actually contain colons
and at signs), and are still worth testing the url parser for.

I am not sure about the last part of this (and the tests in the
patch for them).  When you are constrained by the Common Internet
Scheme Syntax, i.e.

    <scheme>://<user>:<password>@<host>:<port>/<url-path>

you cannot have arbitrary characters in these parts; within the user
and password field, any ":", "@", or "/" must be encoded.

Which maens that for the purpose of the parser you are modifying,
you can rely on these three special characters to parse things out
(decoding after the code determines which part is user and which
part is password is a separate issue).

t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/
That's the basis for these additions. Whether that should work or not is
besides what I was interested in, which was to have a single test file to
run to test my changes, instead of several.

Strictly speaking, this patch is not necessary, because it only covers
things that I found while breaking other tests.

So, there are multiple possible ways forward here:
- Completely remove this patch for v5 of the series.
- Remove the user:passw@rd cases because of the @.
- Remove the user:password cases because we do nothing with the password
  anyways.
- A combination of both of the above.

I don't really care which is picked, at this point I just want to get
over with this series ;)

Mike

On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote:

On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:

quoted

Mike Hommey [off-list ref] writes:

quoted

t5603-clone-dirname uses url patterns that are not tested with
fetch-pack --diag-url, and it would be useful if they were.

Interestingly, some of those tests, involving both a port and a
user:password pair, don't currently pass. Note that even if a
user:password pair is actually not supported by git, the values used
could be valid user names (user names can actually contain colons
and at signs), and are still worth testing the url parser for.

I am not sure about the last part of this (and the tests in the
patch for them).  When you are constrained by the Common Internet
Scheme Syntax, i.e.

    <scheme>://<user>:<password>@<host>:<port>/<url-path>

you cannot have arbitrary characters in these parts; within the user
and password field, any ":", "@", or "/" must be encoded.

Which maens that for the purpose of the parser you are modifying,
you can rely on these three special characters to parse things out
(decoding after the code determines which part is user and which
part is password is a separate issue).

t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/
That's the basis for these additions. Whether that should work or not is
besides what I was interested in, which was to have a single test file to
run to test my changes, instead of several.

Strictly speaking, this patch is not necessary, because it only covers
things that I found while breaking other tests.

So, there are multiple possible ways forward here:
- Completely remove this patch for v5 of the series.
- Remove the user:passw@rd cases because of the @.
- Remove the user:password cases because we do nothing with the password
  anyways.
- A combination of both of the above.

Any opinions on this?

Mike

On 05.05.16 23:52, Mike Hommey wrote:

On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote:

quoted

On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote:

quoted

Mike Hommey [off-list ref] writes:

quoted

t5603-clone-dirname uses url patterns that are not tested with
fetch-pack --diag-url, and it would be useful if they were.

Interestingly, some of those tests, involving both a port and a
user:password pair, don't currently pass. Note that even if a
user:password pair is actually not supported by git, the values used
could be valid user names (user names can actually contain colons
and at signs), and are still worth testing the url parser for.

I am not sure about the last part of this (and the tests in the
patch for them).  When you are constrained by the Common Internet
Scheme Syntax, i.e.

    <scheme>://<user>:<password>@<host>:<port>/<url-path>

you cannot have arbitrary characters in these parts; within the user
and password field, any ":", "@", or "/" must be encoded.

Which maens that for the purpose of the parser you are modifying,
you can rely on these three special characters to parse things out
(decoding after the code determines which part is user and which
part is password is a separate issue).

t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/
That's the basis for these additions. Whether that should work or not is
besides what I was interested in, which was to have a single test file to
run to test my changes, instead of several.

Strictly speaking, this patch is not necessary, because it only covers
things that I found while breaking other tests.

So, there are multiple possible ways forward here:
- Completely remove this patch for v5 of the series.
- Remove the user:passw@rd cases because of the @.
- Remove the user:password cases because we do nothing with the password
  anyways.
- A combination of both of the above.

Any opinions on this?

ssh itself does not use a password:

SSH(1)                    BSD General Commands Manual                   SSH(1)

NAME
     ssh -- OpenSSH SSH client (remote login program)

SYNOPSIS
     ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
         [-D [bind_address:]port] [-e escape_char] [-F configfile] [-I pkcs11]
         [-i identity_file] [-L [bind_address:]port:host:hostport]
         [-l login_name] [-m mac_spec] [-O ctl_cmd] [-o option] [-p port]
         [-R [bind_address:]port:host:hostport] [-S ctl_path] [-W host:port]
         [-w local_tun[:remote_tun]] [user@]hostname [command]


Neither does Git.
A user name must not include a ':'

The user:password came in here:
Commit 92722efec01f67a54b
clone: do not use port number as dir name

Actually, looking back, it may have been better to say
git clone ssh://aaaa:bbbb@host:/path
is illegal and simply die() out.

Back to your question and looking at the offered alternatives. I would vote for
"Completely remove this patch for v5 of the series."