[PATCH] connect: display connection progress

[PATCH] connect: display connection progress

[Michael S. Tsirkin]

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 <mst@dev.mellanox.co.il>

---

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?

diff --git a/connect.c b/connect.c
index da89c9c..f026713 100644
--- a/connect.c
+++ b/connect.c
@@ -425,9 +425,11 @@ static int git_tcp_connect_sock(char *host)
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_protocol = IPPROTO_TCP;
 
+	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));
+	fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
 
 	for (ai0 = ai; ai; ai = ai->ai_next) {
 		sockfd = socket(ai->ai_family,
@@ -450,6 +452,8 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 
-- 
MST

Re: [PATCH] connect: display connection progress

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> 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 <mst@dev.mellanox.co.il>
>
> ---
>
> 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.

Re: [PATCH] connect: display connection progress

[Alex Riesen]

Michael S. Tsirkin, Sun, May 06, 2007 21:52:30 +0200:
> 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.

If there DNS problems, it is usually useful to show which address was
used. And what errors were, exactly (not just errno, but h_errno too).
And which one of many, BTW. Maybe this below could be of help:

diff --git a/connect.c b/connect.c
index da89c9c..34c26bb 100644
--- a/connect.c
+++ b/connect.c
@@ -391,6 +391,23 @@ static enum protocol get_protocol(const char *name)
 
 #ifndef NO_IPV6
 
+static const char *ai_name(const struct addrinfo *ai)
+{
+	static char addr[INET_ADDRSTRLEN];
+	if ( AF_INET == ai->ai_family ) {
+		struct sockaddr_in *in;
+		in = (struct sockaddr_in *)ai->ai_addr;
+		inet_ntop(ai->ai_family, &in->sin_addr, addr, sizeof(addr));
+	} else if ( AF_INET6 == ai->ai_family ) {
+		struct sockaddr_in6 *in;
+		in = (struct sockaddr_in6 *)ai->ai_addr;
+		inet_ntop(ai->ai_family, &in->sin6_addr, addr, sizeof(addr));
+	} else {
+		strcpy(addr, "(unknown)");
+	}
+	return addr;
+}
+
 /*
  * Returns a connected socket() fd, or else die()s.
  */
@@ -401,6 +418,7 @@ static int git_tcp_connect_sock(char *host)
 	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
+	int cnt = 0;
 
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
@@ -438,10 +456,17 @@ static int git_tcp_connect_sock(char *host)
 		}
 		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			saved_errno = errno;
+			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
+				host,
+				cnt,
+				ai_name(ai),
+				hstrerror(h_errno),
+				strerror(saved_errno));
 			close(sockfd);
 			sockfd = -1;
 			continue;
 		}
+		fprintf(stderr, "using %s[%s]\n", host, ai_name(ai));
 		break;
 	}
 
@@ -467,6 +492,7 @@ static int git_tcp_connect_sock(char *host)
 	struct sockaddr_in sa;
 	char **ap;
 	unsigned int nport;
+	int cnt;
 
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
@@ -497,7 +523,7 @@ static int git_tcp_connect_sock(char *host)
 		nport = se->s_port;
 	}
 
-	for (ap = he->h_addr_list; *ap; ap++) {
+	for (cnt = 0, ap = he->h_addr_list; *ap; ap++, cnt++) {
 		sockfd = socket(he->h_addrtype, SOCK_STREAM, 0);
 		if (sockfd < 0) {
 			saved_errno = errno;
@@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)
 
 		if (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {
 			saved_errno = errno;
+			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
+				host,
+				cnt,
+				inet_ntoa(*(struct in_addr *)&sa.sin_addr),
+				hstrerror(h_errno),
+				strerror(saved_errno));
 			close(sockfd);
 			sockfd = -1;
 			continue;
 		}
+		fprintf(stderr, "using %s[%s]\n",
+			host,
+			inet_ntoa(*(struct in_addr *)&sa.sin_addr));
 		break;
 	}
 

Re: [PATCH] connect: display connection progress

[Michael S. Tsirkin]

> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: [PATCH] connect: display connection progress
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> 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 <mst@dev.mellanox.co.il>
> >
> > ---
> >
> > 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

Re: [PATCH] connect: display connection progress

[Michael S. Tsirkin]

> @@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)
>  
>  		if (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {
>  			saved_errno = errno;
> +			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
> +				host,
> +				cnt,
> +				inet_ntoa(*(struct in_addr *)&sa.sin_addr),
> +				hstrerror(h_errno),
> +				strerror(saved_errno));
>  			close(sockfd);
>  			sockfd = -1;
>  			continue;
>  		}
> +		fprintf(stderr, "using %s[%s]\n",
> +			host,
> +			inet_ntoa(*(struct in_addr *)&sa.sin_addr));
>  		break;
>  	}

My manual says:
	The inet_ntoa() function shall convert the Internet host address
	specified by in to a string in the Internet standard dot notation.

does it work for IPv6?

-- 
MST

Re: [PATCH] connect: display connection progress

[Alex Riesen]

On 5/7/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> My manual says:
>         The inet_ntoa() function shall convert the Internet host address
>         specified by in to a string in the Internet standard dot notation.
>
> does it work for IPv6?

does it have to if NO_IPV6 is set?

[PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

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 <mst@dev.mellanox.co.il>

---

> > 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 <junkio@cox.net>:
> Subject: Re: [PATCH] connect: display connection progress
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> 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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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?

Re: [PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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?

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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> > Quoting Alex Riesen <raa.lkml@gmail.com>:
> > Subject: Re: [PATCHv2] connect: display connection progress
> >
> > On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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?
>
> 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.

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

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

Re: [PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >> Quoting Alex Riesen <raa.lkml@gmail.com>:
> >> Subject: Re: [PATCHv2] connect: display connection progress
> >>
> >> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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?
> >
> >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.
> 
> >> 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.
> 
> >> 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.

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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> > >> 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

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

Re: [PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >> >> 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

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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> > >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?

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

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

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

Re: [PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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).

Re: [PATCHv2] connect: display connection progress

[Michael S. Tsirkin]

> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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).

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

Re: [PATCHv2] connect: display connection progress

[Linus Torvalds]

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

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 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.

Re: [PATCHv2] connect: display connection progress

[Alex Riesen]

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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

Re: [PATCHv2] connect: display connection progress

[Junio C Hamano]

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:

>> Quoting Alex Riesen <raa.lkml@gmail.com>:
>> Subject: Re: [PATCHv2] connect: display connection progress
>> 
>> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> 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?
>
> 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"?

I would really feel this extra verbosity should not be the
default.   Thanks.