Re: [PATCH v5 4/9] connect: make parse_connect_url() return separated host and port

Mike Hommey [off-list ref] writes:

+	get_host_and_port(&host, &port);
+
+	if (*host && !port) {
+		/* The host might contain a user:password string, ignore it
+		 * when searching for the port again */
+		char *end_user = strrchr(host, '@');
+		port = get_port(end_user ? end_user : host);

Scanning from the right because host part would never have '@', but
there could be an invalid URL with an unquoted '@' in userinfo part?
Then this makes sense.

A single process:

	... | grep -v -e '^host=' -e '^port='

perhaps?

This makes the diag output simpler and allows the caller to expect
the same set of variables, which is good.

On Mon, May 16, 2016 at 03:39:08PM -0700, Junio C Hamano wrote:

Mike Hommey [off-list ref] writes:

quoted

+	get_host_and_port(&host, &port);
+
+	if (*host && !port) {
+		/* The host might contain a user:password string, ignore it
+		 * when searching for the port again */
+		char *end_user = strrchr(host, '@');
+		port = get_port(end_user ? end_user : host);

Scanning from the right because host part would never have '@', but
there could be an invalid URL with an unquoted '@' in userinfo part?
Then this makes sense.

Indeed, I forgot to update the comment after the discussion on this
list.

quoted

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 91a69fc..9acba2b 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh

@@ -553,7 +553,7 @@ check_prot_path () {
 	Diag: protocol=$2
 	Diag: path=$3
 	EOF
-	git fetch-pack --diag-url "$1" | grep -v hostandport= >actual &&
+	git fetch-pack --diag-url "$1" | grep -v host= | grep -v port= >actual &&

A single process:

	... | grep -v -e '^host=' -e '^port='

heh, I'm actually replacing it with a single egrep in a subsequent
patch, following feedback from previous round, but missed that there was
this intermediate step with multiple greps still. Do you want an update
anyways, or does only the final result really matter? I guess I can
update it in any case, since I'll have to update the patch for the
comment above anyways.

Mike