On Fri, Jun 03, 2016 at 08:59:46AM -0700, Junio C Hamano wrote: > Torsten Bögershausen <tboegi@xxxxxx> writes: > > > There where 2 comments in the review. > > The most important thing is that now > > git://[example.com:123]/path/to/repo is valid, but it shouldn't. > > This patch fixes it: > > > > @@ -673,7 +669,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_user, > > * "host:port" and NULL. > > * To support this undocumented legacy we still need to split the port. > > */ > > - if (!port) > > + if (!port && protocol == PROTO_SSH) > > Hmph, which one of these (if any) is valid, which ones aren't and > why? > > git://[example.com:123]/path/to/repo > ssh://[example.com:123]/path/to/repo > [example.com:123]:/path/to/repo > > I am wondering about the latter two, because both of them would > become PROTO_SSH at some point in the codepath. And I am wondering > about the first two, because they look the same at the syntactic > level and if one is allowed the users would expect the other would > also be (or vice versa). In fact, the parser doesn't even reject the one that is considered invalid (the first). It just happens to not work because example.com:123 is not a valid hostname, and can't be resolved, which is then the error presented to the user. Which brings me to: On Fri, Jun 03, 2016 at 07:07:18AM +0200, Torsten Bögershausen wrote: > The other thing is that I asked for a test case for > git://[example.com:123]/path/to/repo > which shouldn't be hard to do. Since the parser doesn't error out, the only way to test this is to check that the parser thinks the output for it is host=example.com:123 port=NONE. Which sounds like an awful way to check for this. (Also, the discussion back then was about git://[example.com:123]:/path/to/repo, not git://[example.com:123]/path/to/repo) Mike -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html