Re: [PATCH v3 0/6] fix repo name when cloning a server's root
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 5, 2015, 17:34 UTC
- Message-ID
- <xmqq4mkd4p1x.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1438687797-14254-1-git-send-email-ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 17 quoted lines
> - The naive way of just adding '@' as path separator would break > cloning repositories like '/foo/bar@baz.git' (which would > currently become 'bar@baz' but would become 'baz' only). > > - Skipping the scheme initially is required because without it we > wouldn't be able to scan until the next dir separator in the > host part when stripping authentication information. > > - First checking for '/' in the current stripped URI when we > want to remove the port is required because we do not want to > strip port numbers when cloning from something like > '/foo/bar:2222.git' (which would currently become '2222' but > would then be stripped of the ':2222' part and instead become > 'bar'). Still, this breaks on cloning a bare repository in the > current dir (e.g. cloning 'bar:2222.git', which should become > '2222' because it is not a port number but would become > 'bar').
This is a very good write-up.
Please make sure all of the above appears in the commit log message somewhere.
> As you can see, there is a lot of complexity in there and I'm not > convinced this is better than just exposing > 'parse_connect_url()', which already handles everything for us.
If the function "handles everything for us", that's fine, but the primary reason I am hesitant is because parse_connect_url() was designed specifically not to have to worry about some protocols (e.g. I think feeding it a "http://" would fail, and more importantly, its current callers want such a call to fail). Also it is meant to handle some non-protocols (e.g. scp style host:path that does not follow <scheme>://...).
Also does it handle the "2222" case above? I do not think parse_connect_url() even calls get_host_and_port() to be able to tell what "2222" means in these examples.
Show 13 quoted lines
> Maybe I'm just being blind for the obvious solution here, though. > > Patrick Steinhardt (6): > tests: fix broken && chains in t1509-root-worktree > tests: fix cleanup after tests in t1509-root-worktree > clone: do not include authentication data in guessed dir > clone: do not use port number as dir name > clone: abort if no dir name could be guessed > clone: add tests for cloning with empty path > > builtin/clone.c | 67 ++++++++++++++++++++++++++++++++++++++++-------- > t/t1509-root-worktree.sh | 51 +++++++++++++++++++++++++++++++++--- > 2 files changed, 103 insertions(+), 15 deletions(-)