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

From: Mike Hommey <mh@glandium.org>
Date: May 1, 2016, 23:20 UTC
Message-ID: <20160501232007.GA18006@glandium.org>
In-Reply-To: <ee3d1928-38c5-372c-223f-bd50bfb14930@web.de>

On Sun, May 01, 2016 at 03:37:24PM +0200, Torsten Bögershausen wrote:

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

I don't agree that connect.c should be left untouched because it's fragile. On the contrary, if anything, it should be made less fragile.

And the more I look at it, the more I'm tempted to just change the parse_connect_url function itself to do _all_ the parsing, instead of having some code paths invoke get_host_and_port and some others invoke get_port on top of that, for some effect that, when you look at the code, you can't know what it's supposed to be. If anything, my attempts at cleaning up the code, and partially failing are a demonstration that the code *does* need some clean up.

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

What is the benefit from doing so?

Show 8 quoted lines

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

There *are* common code paths. Which is exactly why I'm trying to reuse connect.c without copying it.

Mike