Re: [PATCH] connect: display connection progress

"Michael S. Tsirkin" [off-list ref] writes:

Make git notify the user about host resolution/connection attempts.  This
is useful both as a progress indicator on slow links, and helps reassure the
user there are no DNS/firewall problems.

Signed-off-by: Michael S. Tsirkin <redacted>

---

I find the following useful.
This currently only covers native git protocol. I expect it would
be easy to extend this to other protocols, if there's interest.
Opinions?

I think giving this kind of feedback makes a lot of sense, from
both the "assurance" point of view and also debuggability.

But please do this only under verbose, or squelch it if "quiet"
is asked.

Quoting Junio C Hamano [off-list ref]:
Subject: Re: [PATCH] connect: display connection progress

"Michael S. Tsirkin" [off-list ref] writes:

quoted

Make git notify the user about host resolution/connection attempts.  This
is useful both as a progress indicator on slow links, and helps reassure the
user there are no DNS/firewall problems.

Signed-off-by: Michael S. Tsirkin <redacted>

---

I find the following useful.
This currently only covers native git protocol. I expect it would
be easy to extend this to other protocols, if there's interest.
Opinions?

I think giving this kind of feedback makes a lot of sense, from
both the "assurance" point of view and also debuggability.

But please do this only under verbose, or squelch it if "quiet"
is asked.

Squelching it if quiet is set makes more sense to me.
I'll do that.

-- 
MST

Make git notify the user about host resolution/connection attempts.  This
is useful both as a progress indicator on slow links, and helps reassure the
user there are no DNS/firewall problems.

Signed-off-by: Michael S. Tsirkin <redacted>

---

quoted

I find the following useful.
This currently only covers native git protocol. I expect it would
be easy to extend this to other protocols, if there's interest.
Opinions?

Quoting Junio C Hamano [off-list ref]:
Subject: Re: [PATCH] connect: display connection progress

"Michael S. Tsirkin" [off-list ref] writes:

I think giving this kind of feedback makes a lot of sense, from
both the "assurance" point of view and also debuggability.

But please do this only under verbose, or squelch it if "quiet"
is asked.

Here's an updated patch. Please comment.

diff --git a/builtin-archive.c b/builtin-archive.c
index 7f4e409..5312e89 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c

@@ -45,7 +45,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	}
 
 	url = xstrdup(remote);
-	pid = git_connect(fd, url, exec);
+	pid = git_connect(fd, url, exec, NET_QUIET);
 	if (pid < 0)
 		return pid;
 

diff --git a/cache.h b/cache.h
index 8e76152..232faa7 100644
--- a/cache.h
+++ b/cache.h

@@ -462,7 +462,8 @@ struct ref {
 #define REF_HEADS	(1u << 1)
 #define REF_TAGS	(1u << 2)
 
-extern pid_t git_connect(int fd[2], char *url, const char *prog);
+#define NET_QUIET       (1u << 0)
+extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
 extern int finish_connect(pid_t pid);
 extern int path_match(const char *path, int nr, char **match);
 extern int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,

diff --git a/connect.c b/connect.c
index da89c9c..fd4718a 100644
--- a/connect.c
+++ b/connect.c

@@ -394,7 +394,7 @@ static enum protocol get_protocol(const char *name)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
 	char *colon, *end;

@@ -425,10 +425,16 @@ static int git_tcp_connect_sock(char *host)
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_protocol = IPPROTO_TCP;
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "Looking up %s ... ", host);
+
 	gai = getaddrinfo(host, port, &hints, &ai);
 	if (gai)
 		die("Unable to look up %s (port %s) (%s)", host, port, gai_strerror(gai));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
+
 	for (ai0 = ai; ai; ai = ai->ai_next) {
 		sockfd = socket(ai->ai_family,
 				ai->ai_socktype, ai->ai_protocol);

@@ -450,6 +456,9 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 

@@ -458,7 +467,7 @@ static int git_tcp_connect_sock(char *host)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
 	char *colon, *end;

@@ -485,6 +494,9 @@ static int git_tcp_connect_sock(char *host)
 		port = colon + 1;
 	}
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "Looking up %s ... ", host);
+
 	he = gethostbyname(host);
 	if (!he)
 		die("Unable to look up %s (%s)", host, hstrerror(h_errno));

@@ -497,6 +509,9 @@ static int git_tcp_connect_sock(char *host)
 		nport = se->s_port;
 	}
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
+
 	for (ap = he->h_addr_list; *ap; ap++) {
 		sockfd = socket(he->h_addrtype, SOCK_STREAM, 0);
 		if (sockfd < 0) {

@@ -521,15 +536,18 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host)
+static void git_tcp_connect(int fd[2], char *host, int flags)
 {
-	int sockfd = git_tcp_connect_sock(host);
+	int sockfd = git_tcp_connect_sock(host, flags);
 
 	fd[0] = sockfd;
 	fd[1] = dup(sockfd);

@@ -646,7 +664,7 @@ static void git_proxy_connect(int fd[2], char *host)
  *
  * Does not return a negative value on error; it just dies.
  */
-pid_t git_connect(int fd[2], char *url, const char *prog)
+pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 {
 	char *host, *path = url;
 	char *end;

@@ -719,7 +737,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog)
 		if (git_use_proxy(host))
 			git_proxy_connect(fd, host);
 		else
-			git_tcp_connect(fd, host);
+			git_tcp_connect(fd, host, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended components with a NUL byte.

diff --git a/fetch-pack.c b/fetch-pack.c
index 06f4aec..050b01d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -733,7 +733,7 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	pid = git_connect(fd, dest, uploadpack);
+	pid = git_connect(fd, dest, uploadpack, quiet ? NET_QUIET : 0);
 	if (pid < 0)
 		return 1;
 	if (heads && nr_heads)

diff --git a/peek-remote.c b/peek-remote.c
index 96bfac4..a5e0fc1 100644
--- a/peek-remote.c
+++ b/peek-remote.c

@@ -64,7 +64,7 @@ int main(int argc, char **argv)
 	if (!dest || i != argc - 1)
 		usage(peek_remote_usage);
 
-	pid = git_connect(fd, dest, uploadpack);
+	pid = git_connect(fd, dest, uploadpack, NET_QUIET);
 	if (pid < 0)
 		return 1;
 	ret = peek_remote(fd, flags);

diff --git a/send-pack.c b/send-pack.c
index d5b5162..5d99b25 100644
--- a/send-pack.c
+++ b/send-pack.c

@@ -393,7 +393,7 @@ int main(int argc, char **argv)
 		usage(send_pack_usage);
 	verify_remote_names(nr_heads, heads);
 
-	pid = git_connect(fd, dest, receivepack);
+	pid = git_connect(fd, dest, receivepack, verbose ? 0 : NET_QUIET);
 	if (pid < 0)
 		return 1;
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);

-- 
MST

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)

There is only one bit of flags ever used. What are the others for?
Why use negative logic?
What was wrong with plain "int verbose"?
What addresses were tried by connect?

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)

There is only one bit of flags ever used. What are the others for?

Hmm, I thought it's easier to read 
git_tcp_connect_sock(host, NET_QUIET)
	than
git_tcp_connect_sock(host, 1)

but maybe that's overdesign.

Why use negative logic?
What was wrong with plain "int verbose"?

I want the default to report connections, and -q
to silence them. Maybe "int quiet"?

What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure?
I think it makes sense, but it's a separate issue, isn't it?

-- 
MST

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)

There is only one bit of flags ever used. What are the others for?

Hmm, I thought it's easier to read
git_tcp_connect_sock(host, NET_QUIET)

It is easier to read. "int flags" isn't easier to understand.

quoted

Why use negative logic?
What was wrong with plain "int verbose"?

I want the default to report connections, and -q
to silence them. Maybe "int quiet"?

It depends. "Quiet" is negative, which automatically
makes the logic harder to follow (for humans, at least),
and you had to put negations all over git_tcp_connect,
exactly because the meaning is exactly the opposite to
what you need.

quoted

What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure?

Yes. Not on failure (not only). Every time an address is tried
to connect.

I think it makes sense, but it's a separate issue, isn't it?

You are just about to make git_tcp_connect verbose,
are you not?

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

quoted

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)

There is only one bit of flags ever used. What are the others for?

Hmm, I thought it's easier to read
git_tcp_connect_sock(host, NET_QUIET)

It is easier to read. "int flags" isn't easier to understand.

quoted

quoted

Why use negative logic?
What was wrong with plain "int verbose"?

I want the default to report connections, and -q
to silence them. Maybe "int quiet"?

It depends. "Quiet" is negative, which automatically
makes the logic harder to follow (for humans, at least),
and you had to put negations all over git_tcp_connect,
exactly because the meaning is exactly the opposite to
what you need.

quoted

quoted

What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure?

Yes. Not on failure (not only). Every time an address is tried
to connect.

Why not only on failure? IP addresses look ugly.

quoted

I think it makes sense, but it's a separate issue, isn't it?

You are just about to make git_tcp_connect verbose,
are you not?

Only if the flag is set. So git-fetch without -q qill be more verbose -
but it already spits out a fair amount of data on screen.

-- 
MST

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

quoted

quoted

What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure?

Yes. Not on failure (not only). Every time an address is tried
to connect.

Why not only on failure? IP addresses look ugly.

So you can see DNS problems you wanted to uncover.
DNS is all about mapping names to that ugly IP.
And DNS _problems_ often manifest themselves
by mapping the name to an unexpected IP.
Now that's really ugly

quoted

quoted

I think it makes sense, but it's a separate issue, isn't it?

You are just about to make git_tcp_connect verbose,
are you not?

Only if the flag is set. So git-fetch without -q qill be more verbose -
but it already spits out a fair amount of data on screen.

And so you added some more? Does not sound logical.

How about cleaning up this (reduce the amount of date
on screen) and adding another verbosity level (with your
messages and IP) instead?

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

quoted

quoted

quoted

What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure?

Yes. Not on failure (not only). Every time an address is tried
to connect.

Why not only on failure? IP addresses look ugly.

So you can see DNS problems you wanted to uncover.

I really just wanted git to tell me what it's doing,
so that I know it's not actually blocked on network,
not doing any work.

DNS is all about mapping names to that ugly IP.

Yes, but so far git port does not seem to be commonly open
on random IPs ;).

And DNS _problems_ often manifest themselves
by mapping the name to an unexpected IP.

Now that's really ugly

So, let's print the IP if -v is set?
Oh, look, now we'll have
NET_QUIET
NET_VERBOSE

quoted

quoted

quoted

I think it makes sense, but it's a separate issue, isn't it?

You are just about to make git_tcp_connect verbose,
are you not?

Only if the flag is set. So git-fetch without -q qill be more verbose -
but it already spits out a fair amount of data on screen.

And so you added some more? Does not sound logical.

How about cleaning up this (reduce the amount of date
on screen)

Isn't this why we have -q?

and adding another verbosity level (with your
messages and IP) instead?

What, yet another flag? Nooooooo

-- 
MST

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

quoted

Why not only on failure? IP addresses look ugly.

So you can see DNS problems you wanted to uncover.

I really just wanted git to tell me what it's doing,
so that I know it's not actually blocked on network,
not doing any work.

Aren't you interested in _what_ work is it doing?

quoted

DNS is all about mapping names to that ugly IP.

Yes, but so far git port does not seem to be commonly open
on random IPs ;).

Well, it does. It happened.

quoted

And DNS _problems_ often manifest themselves
by mapping the name to an unexpected IP.
Now that's really ugly

So, let's print the IP if -v is set?
Oh, look, now we'll have
NET_QUIET
NET_VERBOSE

No. All you have is QUIET and !QUIET (or VERBOSE and
!VERBOSE which is the same).

quoted

How about cleaning up this (reduce the amount of date
on screen)

Isn't this why we have -q?

Only fetch-pack has -q (and -v, which is confusing to
say the least).

quoted

quoted

How about cleaning up this (reduce the amount of date
on screen)

Isn't this why we have -q?

Only fetch-pack has -q (and -v, which is confusing to
say the least).

I find it quite proper to have both.
I would expect the following:

- git fetch normally displays progress meter, possibly
  tells me what stage it's in (connecting/downloading ....)
  so I know it's not hung.
- git fetch -q only tells me about errors/exceptional events
  good e.g. for scripts.
- git fetch -v gives a lot of detail useful for debugging
  only used if I see problems and want to debug.

Isn't this what's going on?

So, that's why the "connecting" message belongs in the default setup
(it can hang there for minutes), IP and such technicalia
belong with -v, and -q would only print data on connection error.
 
-- 
MST

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

So, that's why the "connecting" message belongs in the default setup
(it can hang there for minutes), IP and such technicalia
belong with -v, and -q would only print data on connection error.

How about a config option? So that people working with
repos connected through fast links (say, in local network or
even locally on the same system) are not bothered by the
"connecting" messages (they're is useless then, local networks
usually work).

Quoting Alex Riesen [off-list ref]:
Subject: Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

quoted

So, that's why the "connecting" message belongs in the default setup
(it can hang there for minutes), IP and such technicalia
belong with -v, and -q would only print data on connection error.

How about a config option? So that people working with
repos connected through fast links (say, in local network or
even locally on the same system) are not bothered by the
"connecting" messages (they're is useless then, local networks
usually work).

Do you really have git servers accessed over a local lan or on local system?
I just use ssh in this case, and I think that's the common case ...

I think making it possible to make -q a config option would be useful, though.

We *could* try doing something smart with non-blocking connect + select,
and only print the message if it takes > 1 second. Are you
sure it's worth the complication?

-- 
MST

On Thu, 10 May 2007, Alex Riesen wrote:

There is only one bit of flags ever used. What are the others for?

I actually think it tends to be better to have a "flags" field rather than 
a boolean, even if it only ends up having one flag.

Why use negative logic?

This one I agree with. Ity would be nicer with CONNECT_VERBOSE than with 
NET_QUIET, and having the tests be

	if (flags & CONNECT_VERBOSE)
		..

instead.

What was wrong with plain "int verbose"?

I could see wanting to add flags to do things like disable insecure 
connections etc, so there's certainly nothing saying that "verbose" is the 
only valid way to do things.

What addresses were tried by connect?

That would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?

		Linus

On 5/10/07, Linus Torvalds [off-list ref] wrote:

quoted

What addresses were tried by connect?

That would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?

It's nice to have. That's how I discovered which one of kernel.org
addresses is more stable.

On 5/10/07, Michael S. Tsirkin [off-list ref] wrote:

Do you really have git servers accessed over a local lan or on local system?

Yes and yes.

I just use ssh in this case, and I think that's the common case ...

not on windows

We *could* try doing something smart with non-blocking connect + select,
and only print the message if it takes > 1 second. Are you
sure it's worth the complication?

I'm not sure this suggestion of yours is worth the complication.
Besides, it's hard to get portably