Re: [PATCH 2/6] connect: uniformize and group CONNECT_DIAG_URL handling code

On 01.05.16 08:02, Mike Hommey wrote:

Yes, and there (historical) reasons for that. The first implementation did support IPV6 with SSH:

commit 5ba884483fe1a5f9ce1ce5e3c5e1c37c0fd296c4
    [PATCH] GIT: Try all addresses for given remote name
   
    Try all addresses for given remote name until it succeeds.  Also
    supports IPv6.

This lead to a regression, which was fixed here:
commit ce6f8e7ec2bbebe2472e23b684cae0a4adf325ad
    Fix git protocol connection 'port' override
   
    It was broken by the IPv6 patches - we need to remove the ":" part from
    the hostname for a successful name lookup.

Later the ssh:// syntax was added:

commit c05186cc38ca4605bff1f275619d7d0faeaf2fa5
    Support git+ssh:// and ssh+git:// URL
   
Later support for [] was added:
commit 356bece0a2725818191b12f6e991490d52baa1d1
    GIT: Support [address] in URLs
    Allow IPv6address/IPvFuture enclosed by [] in URLs, like:
       git push '[3ffe:ffff:...:1]:GIT/git'
    or
       git push 'ssh://[3ffe:ffff:...:1]/GIT/git'

Later, support for a trailing ':' was added:
commit 608d48b2207a6152839a9762c7a66f217bceb440
    Fix "getaddrinfo()" buglet
    So when somebody passes me a "please pull" request pointing to something
    like the following
    	git://git.kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git
    (note the extraneous colon at the end of the host name), git would happily
    try to connect to port 0, which would generally just cause the remote to
    not even answer, and the "connect()" will take a long time to time out.

Then it was possible to use ssh:// with a port number:
commit 2e7766655abd0312a6bf4db6a6ff141e7e4ac8b6
    URL: allow port specification in ssh:// URLs

Parsing of the port number was improved here:
commit 8f1482536ad680fcd738158e76e254a534f2e690
    connect.c: stricter port validation, silence compiler warning

Then, it was possible to pass the port number to putty or plink:
commit 36ad53ffee6ed5b7c277cde660f526fd8ce3d68f
    connect.c: Support PuTTY plink and TortoisePlink as SSH on Windows

This caused a regression, see below.

Later, there was a fix for [] together with a port number:
commit 7acf438215d1b0e8e47a707f3585de8486a2e5fe
    git: Wrong parsing of ssh urls with IPv6 literals ignores port

A better way to distinguish "local file system" URLS from scp like URLs,
(None of them has a scheme, but they are nice to use)
commit 60003340cda05f5ecd79ee8522b21eda038b994b
    clone: allow cloning local paths with colons in them

This fix lead to another regression, which was partly resolved here
(And that's where test cases came in)
commit 8d3d28f5dba94a15a79975e4adc909c295c80d80
    clone: tighten "local paths with colons" check a bit
   

 In order to allow diagnostics in the parser, the diag-url feature was added:
commit 5610b7c0c6957cf0b236b6fac087c1f4dc209376
    git fetch-pack: add --diag-url
 
The the parser was improved:
commit 6a59974869c0a6e9399101bc02223b4c00a8aff2
    git fetch: support host:/~repo

and refactored:
commit 83b058752707a6ba4af51ebc98c47913bc7d2d25
    git_connect(): refactor the port handling for ssh

And more refactored:
commit c59ab2e52a64abd7fded97e0983a9b7f3d0508a0
    connect.c: refactor url parsing

And more refactored:
commit a2036d7e00ad8aa16ba010a80078e10f0e4568a3
    git_connect(): use common return point

Later the parser was improved:
commit 86ceb337ec340c7db9b060b90bfab05a08b8251b
    connect.c: allow ssh://user@[2001:db8::1]/repo.git
commit 3f55ccab8e0fec73c8e38b909e9bb4963bfb8f6a
    t5500: show user name and host in diag-url

However, one of the refactoring broke a use case, which was valid before, leading to regressions around the world:

commit 6b6c5f7a2f66751a93afce54277a1f30ab0dc521
    connect.c: ignore extra colon after hostname

Now back to the regression introduced by supporting plink/putty with ports, it was improved by a preparing commit, followed by the fix:

commit baaf233755f71c057d28b9e8692e24d4fca7d22f
    connect: improve check for plink to reduce false positives

commit 37ee646e72d7f39d61a538e21a4c2721e32cb444
    connect: simplify SSH connection code path

Later IPV6/IPV4 only connections had been supported:
commit c915f11eb4922e154e29cf62d3b549d65c06a170
    connect & http: support -4 and -6 switches for remote operations

I skipped the dates and names (I was responsible for one regression) I hope this gives a half-correct overview, why I am reluctant to change any code in connect.c unless there is a fix for a real world problem.

And even here, the test cases should be changed first (and reviewed) in an own commit. Marked as test_must_failure. The c-code can be changed in the next commit, and the TC are marked as "test_expect_success"

Back to another topic: If you want to support the native Git-support for native HG via hg-serve, I will be happy to assist with reviews. Please, if possible, don't touch connect.c at all. (Beside the memory leak fix).

It may be better to start with a real remote-helper and copy the code from connect.c And, later, if there are common code paths, refactor stuff.