{"thread":{"id":"8005","subject":"[PATCH] connect: display connection progress","startedAt":"2007-05-06T19:52:30Z","lastAt":"2007-05-10T19:29:20Z","messageCount":21,"participants":["Michael S. Tsirkin","Junio C Hamano","Alex Riesen","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"41244","messageId":"20070506195230.GA30339@mellanox.co.il","threadId":"8005","inReplyTo":null,"subject":"[PATCH] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-06T19:52:30Z","receivedAt":"2007-05-06T19:52:30Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Make git notify the user about host resolution/connection attempts. This\nis useful both as a progress indicator on slow links, and helps reassure the\nuser there are no DNS/firewall problems.\n\nSigned-off-by: Michael S. Tsirkin \n\n---\n\nI find the following useful.\nThis currently only covers native git protocol. I expect it would\nbe easy to extend this to other protocols, if there's interest.\nOpinions?\n\ndiff --git a/connect.c b/connect.c\nindex da89c9c..f026713 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -425,9 +425,11 @@ static int git_tcp_connect_sock(char *host)\n \thints.ai_socktype = SOCK_STREAM;\n \thints.ai_protocol = IPPROTO_TCP;\n \n+\tfprintf(stderr, \"Looking up %s ... \", host);\n \tgai = getaddrinfo(host, port, &hints, &ai);\n \tif (gai)\n \t\tdie(\"Unable to look up %s (port %s) (%s)\", host, port, gai_strerror(gai));\n+\tfprintf(stderr, \"done.\\nConnecting to %s (port %s) ... \", host, port);\n \n \tfor (ai0 = ai; ai; ai = ai->ai_next) {\n \t\tsockfd = socket(ai->ai_family,\n@@ -450,6 +452,8 @@ static int git_tcp_connect_sock(char *host)\n \tif (sockfd < 0)\n \t\tdie(\"unable to connect a socket (%s)\", strerror(saved_errno));\n \n+\tfprintf(stderr, \"done.\\n\");\n+\n \treturn sockfd;\n }\n \n-- \nMST\n"},{"id":"41248","messageId":"7vhcqpbs78.fsf@assigned-by-dhcp.cox.net","threadId":"8005","inReplyTo":"20070506195230.GA30339@mellanox.co.il","subject":"Re: [PATCH] connect: display connection progress","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-06T20:41:31Z","receivedAt":"2007-05-06T20:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> Make git notify the user about host resolution/connection attempts. This\n> is useful both as a progress indicator on slow links, and helps reassure the\n> user there are no DNS/firewall problems.\n>\n> Signed-off-by: Michael S. Tsirkin \n>\n> ---\n>\n> I find the following useful.\n> This currently only covers native git protocol. I expect it would\n> be easy to extend this to other protocols, if there's interest.\n> Opinions?\n\nI think giving this kind of feedback makes a lot of sense, from\nboth the \"assurance\" point of view and also debuggability.\n\nBut please do this only under verbose, or squelch it if \"quiet\"\nis asked.\n"},{"id":"41258","messageId":"20070506222123.GB2439@steel.home","threadId":"8005","inReplyTo":"20070506195230.GA30339@mellanox.co.il","subject":"Re: [PATCH] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-06T22:21:23Z","receivedAt":"2007-05-06T22:21:23Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Michael S. Tsirkin, Sun, May 06, 2007 21:52:30 +0200:\n> Make git notify the user about host resolution/connection attempts. This\n> is useful both as a progress indicator on slow links, and helps reassure the\n> user there are no DNS/firewall problems.\n\nIf there DNS problems, it is usually useful to show which address was\nused. And what errors were, exactly (not just errno, but h_errno too).\nAnd which one of many, BTW. Maybe this below could be of help:\n\ndiff --git a/connect.c b/connect.c\nindex da89c9c..34c26bb 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -391,6 +391,23 @@ static enum protocol get_protocol(const char *name)\n \n #ifndef NO_IPV6\n \n+static const char *ai_name(const struct addrinfo *ai)\n+{\n+\tstatic char addr[INET_ADDRSTRLEN];\n+\tif ( AF_INET == ai->ai_family ) {\n+\t\tstruct sockaddr_in *in;\n+\t\tin = (struct sockaddr_in *)ai->ai_addr;\n+\t\tinet_ntop(ai->ai_family, &in->sin_addr, addr, sizeof(addr));\n+\t} else if ( AF_INET6 == ai->ai_family ) {\n+\t\tstruct sockaddr_in6 *in;\n+\t\tin = (struct sockaddr_in6 *)ai->ai_addr;\n+\t\tinet_ntop(ai->ai_family, &in->sin6_addr, addr, sizeof(addr));\n+\t} else {\n+\t\tstrcpy(addr, \"(unknown)\");\n+\t}\n+\treturn addr;\n+}\n+\n /*\n * Returns a connected socket() fd, or else die()s.\n */\n@@ -401,6 +418,7 @@ static int git_tcp_connect_sock(char *host)\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n \tstruct addrinfo hints, *ai0, *ai;\n \tint gai;\n+\tint cnt = 0;\n \n \tif (host[0] == '[') {\n \t\tend = strchr(host + 1, ']');\n@@ -438,10 +456,17 @@ static int git_tcp_connect_sock(char *host)\n \t\t}\n \t\tif (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n \t\t\tsaved_errno = errno;\n+\t\t\tfprintf(stderr, \"%s[%d: %s]: net=%s, errno=%s\\n\",\n+\t\t\t\thost,\n+\t\t\t\tcnt,\n+\t\t\t\tai_name(ai),\n+\t\t\t\thstrerror(h_errno),\n+\t\t\t\tstrerror(saved_errno));\n \t\t\tclose(sockfd);\n \t\t\tsockfd = -1;\n \t\t\tcontinue;\n \t\t}\n+\t\tfprintf(stderr, \"using %s[%s]\\n\", host, ai_name(ai));\n \t\tbreak;\n \t}\n \n@@ -467,6 +492,7 @@ static int git_tcp_connect_sock(char *host)\n \tstruct sockaddr_in sa;\n \tchar **ap;\n \tunsigned int nport;\n+\tint cnt;\n \n \tif (host[0] == '[') {\n \t\tend = strchr(host + 1, ']');\n@@ -497,7 +523,7 @@ static int git_tcp_connect_sock(char *host)\n \t\tnport = se->s_port;\n \t}\n \n-\tfor (ap = he->h_addr_list; *ap; ap++) {\n+\tfor (cnt = 0, ap = he->h_addr_list; *ap; ap++, cnt++) {\n \t\tsockfd = socket(he->h_addrtype, SOCK_STREAM, 0);\n \t\tif (sockfd < 0) {\n \t\t\tsaved_errno = errno;\n@@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)\n \n \t\tif (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {\n \t\t\tsaved_errno = errno;\n+\t\t\tfprintf(stderr, \"%s[%d: %s]: net=%s, errno=%s\\n\",\n+\t\t\t\thost,\n+\t\t\t\tcnt,\n+\t\t\t\tinet_ntoa(*(struct in_addr *)&sa.sin_addr),\n+\t\t\t\thstrerror(h_errno),\n+\t\t\t\tstrerror(saved_errno));\n \t\t\tclose(sockfd);\n \t\t\tsockfd = -1;\n \t\t\tcontinue;\n \t\t}\n+\t\tfprintf(stderr, \"using %s[%s]\\n\",\n+\t\t\thost,\n+\t\t\tinet_ntoa(*(struct in_addr *)&sa.sin_addr));\n \t\tbreak;\n \t}\n \n"},{"id":"41281","messageId":"20070507042039.GD30339@mellanox.co.il","threadId":"8005","inReplyTo":"7vhcqpbs78.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-07T04:20:39Z","receivedAt":"2007-05-07T04:20:39Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> Quoting Junio C Hamano :\n> Subject: Re: [PATCH] connect: display connection progress\n> \n> \"Michael S. Tsirkin\" writes:\n> \n> > Make git notify the user about host resolution/connection attempts. This\n> > is useful both as a progress indicator on slow links, and helps reassure the\n> > user there are no DNS/firewall problems.\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> >\n> > ---\n> >\n> > I find the following useful.\n> > This currently only covers native git protocol. I expect it would\n> > be easy to extend this to other protocols, if there's interest.\n> > Opinions?\n> \n> I think giving this kind of feedback makes a lot of sense, from\n> both the \"assurance\" point of view and also debuggability.\n> \n> But please do this only under verbose, or squelch it if \"quiet\"\n> is asked.\n\nSquelching it if quiet is set makes more sense to me.\nI'll do that.\n\n-- \nMST\n"},{"id":"41282","messageId":"20070507045451.GE30339@mellanox.co.il","threadId":"8005","inReplyTo":"20070506222123.GB2439@steel.home","subject":"Re: [PATCH] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-07T04:54:52Z","receivedAt":"2007-05-07T04:54:52Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> @@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)\n> \n> \t\tif (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {\n> \t\t\tsaved_errno = errno;\n> +\t\t\tfprintf(stderr, \"%s[%d: %s]: net=%s, errno=%s\\n\",\n> +\t\t\t\thost,\n> +\t\t\t\tcnt,\n> +\t\t\t\tinet_ntoa(*(struct in_addr *)&sa.sin_addr),\n> +\t\t\t\thstrerror(h_errno),\n> +\t\t\t\tstrerror(saved_errno));\n> \t\t\tclose(sockfd);\n> \t\t\tsockfd = -1;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tfprintf(stderr, \"using %s[%s]\\n\",\n> +\t\t\thost,\n> +\t\t\tinet_ntoa(*(struct in_addr *)&sa.sin_addr));\n> \t\tbreak;\n> \t}\n\nMy manual says:\n\tThe inet_ntoa() function shall convert the Internet host address\n\tspecified by in to a string in the Internet standard dot notation.\n\ndoes it work for IPv6?\n\n-- \nMST\n"},{"id":"41300","messageId":"81b0412b0705070051j4ff9b2c9wf3471143cfb0bea0@mail.gmail.com","threadId":"8005","inReplyTo":"20070507045451.GE30339@mellanox.co.il","subject":"Re: [PATCH] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-07T07:51:45Z","receivedAt":"2007-05-07T07:51:45Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/7/07, Michael S. Tsirkin wrote:\n> My manual says:\n> The inet_ntoa() function shall convert the Internet host address\n> specified by in to a string in the Internet standard dot notation.\n>\n> does it work for IPv6?\n\ndoes it have to if NO_IPV6 is set?\n"},{"id":"41727","messageId":"20070510095156.GC13655@mellanox.co.il","threadId":"8005","inReplyTo":"7vhcqpbs78.fsf@assigned-by-dhcp.cox.net","subject":"[PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T09:51:56Z","receivedAt":"2007-05-10T09:51:56Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Make git notify the user about host resolution/connection attempts. This\nis useful both as a progress indicator on slow links, and helps reassure the\nuser there are no DNS/firewall problems.\n\nSigned-off-by: Michael S. Tsirkin \n\n---\n\n> > I find the following useful.\n> > This currently only covers native git protocol. I expect it would\n> > be easy to extend this to other protocols, if there's interest.\n> > Opinions?\n> \n> Quoting Junio C Hamano :\n> Subject: Re: [PATCH] connect: display connection progress\n> \n> \"Michael S. Tsirkin\" writes:\n> \n> I think giving this kind of feedback makes a lot of sense, from\n> both the \"assurance\" point of view and also debuggability.\n> \n> But please do this only under verbose, or squelch it if \"quiet\"\n> is asked.\n\nHere's an updated patch. Please comment.\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 7f4e409..5312e89 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -45,7 +45,7 @@ static int run_remote_archiver(const char *remote, int argc,\n \t}\n \n \turl = xstrdup(remote);\n-\tpid = git_connect(fd, url, exec);\n+\tpid = git_connect(fd, url, exec, NET_QUIET);\n \tif (pid < 0)\n \t\treturn pid;\n \ndiff --git a/cache.h b/cache.h\nindex 8e76152..232faa7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -462,7 +462,8 @@ struct ref {\n #define REF_HEADS\t(1u << 1)\n #define REF_TAGS\t(1u << 2)\n \n-extern pid_t git_connect(int fd[2], char *url, const char *prog);\n+#define NET_QUIET (1u << 0)\n+extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);\n extern int finish_connect(pid_t pid);\n extern int path_match(const char *path, int nr, char **match);\n extern int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\ndiff --git a/connect.c b/connect.c\nindex da89c9c..fd4718a 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -394,7 +394,7 @@ static enum protocol get_protocol(const char *name)\n /*\n * Returns a connected socket() fd, or else die()s.\n */\n-static int git_tcp_connect_sock(char *host)\n+static int git_tcp_connect_sock(char *host, int flags)\n {\n \tint sockfd = -1, saved_errno = 0;\n \tchar *colon, *end;\n@@ -425,10 +425,16 @@ static int git_tcp_connect_sock(char *host)\n \thints.ai_socktype = SOCK_STREAM;\n \thints.ai_protocol = IPPROTO_TCP;\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"Looking up %s ... \", host);\n+\n \tgai = getaddrinfo(host, port, &hints, &ai);\n \tif (gai)\n \t\tdie(\"Unable to look up %s (port %s) (%s)\", host, port, gai_strerror(gai));\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"done.\\nConnecting to %s (port %s) ... \", host, port);\n+\n \tfor (ai0 = ai; ai; ai = ai->ai_next) {\n \t\tsockfd = socket(ai->ai_family,\n \t\t\t\tai->ai_socktype, ai->ai_protocol);\n@@ -450,6 +456,9 @@ static int git_tcp_connect_sock(char *host)\n \tif (sockfd < 0)\n \t\tdie(\"unable to connect a socket (%s)\", strerror(saved_errno));\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"done.\\n\");\n+\n \treturn sockfd;\n }\n \n@@ -458,7 +467,7 @@ static int git_tcp_connect_sock(char *host)\n /*\n * Returns a connected socket() fd, or else die()s.\n */\n-static int git_tcp_connect_sock(char *host)\n+static int git_tcp_connect_sock(char *host, int flags)\n {\n \tint sockfd = -1, saved_errno = 0;\n \tchar *colon, *end;\n@@ -485,6 +494,9 @@ static int git_tcp_connect_sock(char *host)\n \t\tport = colon + 1;\n \t}\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"Looking up %s ... \", host);\n+\n \the = gethostbyname(host);\n \tif (!he)\n \t\tdie(\"Unable to look up %s (%s)\", host, hstrerror(h_errno));\n@@ -497,6 +509,9 @@ static int git_tcp_connect_sock(char *host)\n \t\tnport = se->s_port;\n \t}\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"done.\\nConnecting to %s (port %s) ... \", host, port);\n+\n \tfor (ap = he->h_addr_list; *ap; ap++) {\n \t\tsockfd = socket(he->h_addrtype, SOCK_STREAM, 0);\n \t\tif (sockfd < 0) {\n@@ -521,15 +536,18 @@ static int git_tcp_connect_sock(char *host)\n \tif (sockfd < 0)\n \t\tdie(\"unable to connect a socket (%s)\", strerror(saved_errno));\n \n+\tif (!(flags & NET_QUIET))\n+\t\tfprintf(stderr, \"done.\\n\");\n+\n \treturn sockfd;\n }\n \n #endif /* NO_IPV6 */\n \n \n-static void git_tcp_connect(int fd[2], char *host)\n+static void git_tcp_connect(int fd[2], char *host, int flags)\n {\n-\tint sockfd = git_tcp_connect_sock(host);\n+\tint sockfd = git_tcp_connect_sock(host, flags);\n \n \tfd[0] = sockfd;\n \tfd[1] = dup(sockfd);\n@@ -646,7 +664,7 @@ static void git_proxy_connect(int fd[2], char *host)\n *\n * Does not return a negative value on error; it just dies.\n */\n-pid_t git_connect(int fd[2], char *url, const char *prog)\n+pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n {\n \tchar *host, *path = url;\n \tchar *end;\n@@ -719,7 +737,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog)\n \t\tif (git_use_proxy(host))\n \t\t\tgit_proxy_connect(fd, host);\n \t\telse\n-\t\t\tgit_tcp_connect(fd, host);\n+\t\t\tgit_tcp_connect(fd, host, flags);\n \t\t/*\n \t\t * Separate original protocol components prog and path\n \t\t * from extended components with a NUL byte.\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 06f4aec..050b01d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -733,7 +733,7 @@ int main(int argc, char **argv)\n \t}\n \tif (!dest)\n \t\tusage(fetch_pack_usage);\n-\tpid = git_connect(fd, dest, uploadpack);\n+\tpid = git_connect(fd, dest, uploadpack, quiet ? NET_QUIET : 0);\n \tif (pid < 0)\n \t\treturn 1;\n \tif (heads && nr_heads)\ndiff --git a/peek-remote.c b/peek-remote.c\nindex 96bfac4..a5e0fc1 100644\n--- a/peek-remote.c\n+++ b/peek-remote.c\n@@ -64,7 +64,7 @@ int main(int argc, char **argv)\n \tif (!dest || i != argc - 1)\n \t\tusage(peek_remote_usage);\n \n-\tpid = git_connect(fd, dest, uploadpack);\n+\tpid = git_connect(fd, dest, uploadpack, NET_QUIET);\n \tif (pid < 0)\n \t\treturn 1;\n \tret = peek_remote(fd, flags);\ndiff --git a/send-pack.c b/send-pack.c\nindex d5b5162..5d99b25 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -393,7 +393,7 @@ int main(int argc, char **argv)\n \t\tusage(send_pack_usage);\n \tverify_remote_names(nr_heads, heads);\n \n-\tpid = git_connect(fd, dest, receivepack);\n+\tpid = git_connect(fd, dest, receivepack, verbose ? 0 : NET_QUIET);\n \tif (pid < 0)\n \t\treturn 1;\n \tret = send_pack(fd[0], fd[1], nr_heads, heads);\n\n\n-- \nMST\n"},{"id":"41734","messageId":"81b0412b0705100439j4e6b072bk1ba19a4f971e5d0c@mail.gmail.com","threadId":"8005","inReplyTo":"20070510095156.GC13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T11:39:36Z","receivedAt":"2007-05-10T11:39:36Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n> -static int git_tcp_connect_sock(char *host)\n> +static int git_tcp_connect_sock(char *host, int flags)\n\nThere is only one bit of flags ever used. What are the others for?\nWhy use negative logic?\nWhat was wrong with plain \"int verbose\"?\nWhat addresses were tried by connect?\n"},{"id":"41737","messageId":"20070510120802.GG13655@mellanox.co.il","threadId":"8005","inReplyTo":"81b0412b0705100439j4e6b072bk1ba19a4f971e5d0c@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T12:08:02Z","receivedAt":"2007-05-10T12:08:02Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> Quoting Alex Riesen :\n> Subject: Re: [PATCHv2] connect: display connection progress\n> \n> On 5/10/07, Michael S. Tsirkin wrote:\n> >-static int git_tcp_connect_sock(char *host)\n> >+static int git_tcp_connect_sock(char *host, int flags)\n> \n> There is only one bit of flags ever used. What are the others for?\n\nHmm, I thought it's easier to read \ngit_tcp_connect_sock(host, NET_QUIET)\n\tthan\ngit_tcp_connect_sock(host, 1)\n\nbut maybe that's overdesign.\n\n> Why use negative logic?\n> What was wrong with plain \"int verbose\"?\n\nI want the default to report connections, and -q\nto silence them. Maybe \"int quiet\"?\n\n> What addresses were tried by connect?\n\nYou are speaking about your patch reporting the IP on failure?\nI think it makes sense, but it's a separate issue, isn't it?\n\n-- \nMST\n"},{"id":"41738","messageId":"81b0412b0705100519i3028fbc4y25e7c407c7c8216@mail.gmail.com","threadId":"8005","inReplyTo":"20070510120802.GG13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T12:19:45Z","receivedAt":"2007-05-10T12:19:45Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n> > Quoting Alex Riesen :\n> > Subject: Re: [PATCHv2] connect: display connection progress\n> >\n> > On 5/10/07, Michael S. Tsirkin wrote:\n> > >-static int git_tcp_connect_sock(char *host)\n> > >+static int git_tcp_connect_sock(char *host, int flags)\n> >\n> > There is only one bit of flags ever used. What are the others for?\n>\n> Hmm, I thought it's easier to read\n> git_tcp_connect_sock(host, NET_QUIET)\n\nIt is easier to read. \"int flags\" isn't easier to understand.\n\n> > Why use negative logic?\n> > What was wrong with plain \"int verbose\"?\n>\n> I want the default to report connections, and -q\n> to silence them. Maybe \"int quiet\"?\n\nIt depends. \"Quiet\" is negative, which automatically\nmakes the logic harder to follow (for humans, at least),\nand you had to put negations all over git_tcp_connect,\nexactly because the meaning is exactly the opposite to\nwhat you need.\n\n> > What addresses were tried by connect?\n>\n> You are speaking about your patch reporting the IP on failure?\n\nYes. Not on failure (not only). Every time an address is tried\nto connect.\n\n> I think it makes sense, but it's a separate issue, isn't it?\n\nYou are just about to make git_tcp_connect verbose,\nare you not?\n"},{"id":"41739","messageId":"20070510122550.GJ13655@mellanox.co.il","threadId":"8005","inReplyTo":"81b0412b0705100519i3028fbc4y25e7c407c7c8216@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T12:25:50Z","receivedAt":"2007-05-10T12:25:50Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> Quoting Alex Riesen :\n> Subject: Re: [PATCHv2] connect: display connection progress\n> \n> On 5/10/07, Michael S. Tsirkin wrote:\n> >> Quoting Alex Riesen :\n> >> Subject: Re: [PATCHv2] connect: display connection progress\n> >>\n> >> On 5/10/07, Michael S. Tsirkin wrote:\n> >> >-static int git_tcp_connect_sock(char *host)\n> >> >+static int git_tcp_connect_sock(char *host, int flags)\n> >>\n> >> There is only one bit of flags ever used. What are the others for?\n> >\n> >Hmm, I thought it's easier to read\n> >git_tcp_connect_sock(host, NET_QUIET)\n> \n> It is easier to read. \"int flags\" isn't easier to understand.\n> \n> >> Why use negative logic?\n> >> What was wrong with plain \"int verbose\"?\n> >\n> >I want the default to report connections, and -q\n> >to silence them. Maybe \"int quiet\"?\n> \n> It depends. \"Quiet\" is negative, which automatically\n> makes the logic harder to follow (for humans, at least),\n> and you had to put negations all over git_tcp_connect,\n> exactly because the meaning is exactly the opposite to\n> what you need.\n> \n> >> What addresses were tried by connect?\n> >\n> >You are speaking about your patch reporting the IP on failure?\n> \n> Yes. Not on failure (not only). Every time an address is tried\n> to connect.\n\nWhy not only on failure? IP addresses look ugly.\n\n> >I think it makes sense, but it's a separate issue, isn't it?\n> \n> You are just about to make git_tcp_connect verbose,\n> are you not?\n\nOnly if the flag is set. So git-fetch without -q qill be more verbose -\nbut it already spits out a fair amount of data on screen.\n\n-- \nMST\n"},{"id":"41740","messageId":"81b0412b0705100633t61ac0309jfc8536b30244adf6@mail.gmail.com","threadId":"8005","inReplyTo":"20070510122550.GJ13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T13:33:08Z","receivedAt":"2007-05-10T13:33:08Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n> > >> What addresses were tried by connect?\n> > >\n> > >You are speaking about your patch reporting the IP on failure?\n> >\n> > Yes. Not on failure (not only). Every time an address is tried\n> > to connect.\n>\n> Why not only on failure? IP addresses look ugly.\n\nSo you can see DNS problems you wanted to uncover.\nDNS is all about mapping names to that ugly IP.\nAnd DNS _problems_ often manifest themselves\nby mapping the name to an unexpected IP.\nNow that's really ugly\n\n> > >I think it makes sense, but it's a separate issue, isn't it?\n> >\n> > You are just about to make git_tcp_connect verbose,\n> > are you not?\n>\n> Only if the flag is set. So git-fetch without -q qill be more verbose -\n> but it already spits out a fair amount of data on screen.\n\nAnd so you added some more? Does not sound logical.\n\nHow about cleaning up this (reduce the amount of date\non screen) and adding another verbosity level (with your\nmessages and IP) instead?\n"},{"id":"41742","messageId":"20070510134622.GN13655@mellanox.co.il","threadId":"8005","inReplyTo":"81b0412b0705100633t61ac0309jfc8536b30244adf6@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T13:46:22Z","receivedAt":"2007-05-10T13:46:22Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> Quoting Alex Riesen :\n> Subject: Re: [PATCHv2] connect: display connection progress\n> \n> On 5/10/07, Michael S. Tsirkin wrote:\n> >> >> What addresses were tried by connect?\n> >> >\n> >> >You are speaking about your patch reporting the IP on failure?\n> >>\n> >> Yes. Not on failure (not only). Every time an address is tried\n> >> to connect.\n> >\n> >Why not only on failure? IP addresses look ugly.\n> \n> So you can see DNS problems you wanted to uncover.\n\nI really just wanted git to tell me what it's doing,\nso that I know it's not actually blocked on network,\nnot doing any work.\n\n> DNS is all about mapping names to that ugly IP.\n\nYes, but so far git port does not seem to be commonly open\non random IPs ;).\n\n> And DNS _problems_ often manifest themselves\n> by mapping the name to an unexpected IP.\n\n\n> Now that's really ugly\n\nSo, let's print the IP if -v is set?\nOh, look, now we'll have\nNET_QUIET\nNET_VERBOSE\n\n> >> >I think it makes sense, but it's a separate issue, isn't it?\n> >>\n> >> You are just about to make git_tcp_connect verbose,\n> >> are you not?\n> >\n> >Only if the flag is set. So git-fetch without -q qill be more verbose -\n> >but it already spits out a fair amount of data on screen.\n> \n> And so you added some more? Does not sound logical.\n> \n> How about cleaning up this (reduce the amount of date\n> on screen)\n\nIsn't this why we have -q?\n\n> and adding another verbosity level (with your\n> messages and IP) instead?\n\nWhat, yet another flag? Nooooooo\n\n-- \nMST\n"},{"id":"41743","messageId":"81b0412b0705100716t680290a3qd1d10cf588a65f5a@mail.gmail.com","threadId":"8005","inReplyTo":"20070510134622.GN13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T14:16:17Z","receivedAt":"2007-05-10T14:16:17Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n> > >Why not only on failure? IP addresses look ugly.\n> >\n> > So you can see DNS problems you wanted to uncover.\n>\n> I really just wanted git to tell me what it's doing,\n> so that I know it's not actually blocked on network,\n> not doing any work.\n\nAren't you interested in _what_ work is it doing?\n\n> > DNS is all about mapping names to that ugly IP.\n>\n> Yes, but so far git port does not seem to be commonly open\n> on random IPs ;).\n\nWell, it does. It happened.\n\n> > And DNS _problems_ often manifest themselves\n> > by mapping the name to an unexpected IP.\n> > Now that's really ugly\n>\n> So, let's print the IP if -v is set?\n> Oh, look, now we'll have\n> NET_QUIET\n> NET_VERBOSE\n\nNo. All you have is QUIET and !QUIET (or VERBOSE and\n!VERBOSE which is the same).\n\n> > How about cleaning up this (reduce the amount of date\n> > on screen)\n>\n> Isn't this why we have -q?\n\nOnly fetch-pack has -q (and -v, which is confusing to\nsay the least).\n"},{"id":"41744","messageId":"20070510143913.GF22029@mellanox.co.il","threadId":"8005","inReplyTo":"81b0412b0705100716t680290a3qd1d10cf588a65f5a@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T14:39:13Z","receivedAt":"2007-05-10T14:39:13Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"> >> How about cleaning up this (reduce the amount of date\n> >> on screen)\n> >\n> >Isn't this why we have -q?\n> \n> Only fetch-pack has -q (and -v, which is confusing to\n> say the least).\n\nI find it quite proper to have both.\nI would expect the following:\n\n- git fetch normally displays progress meter, possibly\n tells me what stage it's in (connecting/downloading ....)\n so I know it's not hung.\n- git fetch -q only tells me about errors/exceptional events\n good e.g. for scripts.\n- git fetch -v gives a lot of detail useful for debugging\n only used if I see problems and want to debug.\n\nIsn't this what's going on?\n\nSo, that's why the \"connecting\" message belongs in the default setup\n(it can hang there for minutes), IP and such technicalia\nbelong with -v, and -q would only print data on connection error.\n \n-- \nMST\n"},{"id":"41745","messageId":"81b0412b0705100752wa6dec37t787ccd61266f8944@mail.gmail.com","threadId":"8005","inReplyTo":"20070510143913.GF22029@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T14:52:00Z","receivedAt":"2007-05-10T14:52:00Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n> So, that's why the \"connecting\" message belongs in the default setup\n> (it can hang there for minutes), IP and such technicalia\n> belong with -v, and -q would only print data on connection error.\n\nHow about a config option? So that people working with\nrepos connected through fast links (say, in local network or\neven locally on the same system) are not bothered by the\n\"connecting\" messages (they're is useless then, local networks\nusually work).\n"},{"id":"41746","messageId":"20070510150225.GS13655@mellanox.co.il","threadId":"8005","inReplyTo":"81b0412b0705100752wa6dec37t787ccd61266f8944@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Michael S. Tsirkin","fromEmail":"mst@dev.mellanox.co.il","sentAt":"2007-05-10T15:02:25Z","receivedAt":"2007-05-10T15:02:25Z","isPatch":false,"sender":{"key":"mst@kernel.org","avatar":null},"body":"\n> Quoting Alex Riesen :\n> Subject: Re: [PATCHv2] connect: display connection progress\n> \n> On 5/10/07, Michael S. Tsirkin wrote:\n> >So, that's why the \"connecting\" message belongs in the default setup\n> >(it can hang there for minutes), IP and such technicalia\n> >belong with -v, and -q would only print data on connection error.\n> \n> How about a config option? So that people working with\n> repos connected through fast links (say, in local network or\n> even locally on the same system) are not bothered by the\n> \"connecting\" messages (they're is useless then, local networks\n> usually work).\n\nDo you really have git servers accessed over a local lan or on local system?\nI just use ssh in this case, and I think that's the common case ...\n\nI think making it possible to make -q a config option would be useful, though.\n\nWe *could* try doing something smart with non-blocking connect + select,\nand only print the message if it takes > 1 second. Are you\nsure it's worth the complication?\n\n-- \nMST\n"},{"id":"41749","messageId":"alpine.LFD.0.98.0705100901320.3986@woody.linux-foundation.org","threadId":"8005","inReplyTo":"81b0412b0705100439j4e6b072bk1ba19a4f971e5d0c@mail.gmail.com","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-10T16:05:00Z","receivedAt":"2007-05-10T16:05:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 10 May 2007, Alex Riesen wrote:\n> \n> There is only one bit of flags ever used. What are the others for?\n\nI actually think it tends to be better to have a \"flags\" field rather than \na boolean, even if it only ends up having one flag.\n\n> Why use negative logic?\n\nThis one I agree with. Ity would be nicer with CONNECT_VERBOSE than with \nNET_QUIET, and having the tests be\n\n\tif (flags & CONNECT_VERBOSE)\n\t\t..\n\ninstead.\n\n> What was wrong with plain \"int verbose\"?\n\nI could see wanting to add flags to do things like disable insecure \nconnections etc, so there's certainly nothing saying that \"verbose\" is the \nonly valid way to do things.\n\n> What addresses were tried by connect?\n\nThat would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?\n\n\t\tLinus\n"},{"id":"41756","messageId":"81b0412b0705101038p59aeef04qae1322adc788e336@mail.gmail.com","threadId":"8005","inReplyTo":"alpine.LFD.0.98.0705100901320.3986@woody.linux-foundation.org","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T17:38:23Z","receivedAt":"2007-05-10T17:38:23Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Linus Torvalds wrote:\n> > What addresses were tried by connect?\n>\n> That would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?\n>\n\nIt's nice to have. That's how I discovered which one of kernel.org\naddresses is more stable.\n"},{"id":"41757","messageId":"81b0412b0705101040g3a8426advc682d6560155d50c@mail.gmail.com","threadId":"8005","inReplyTo":"20070510150225.GS13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-10T17:40:18Z","receivedAt":"2007-05-10T17:40:18Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/10/07, Michael S. Tsirkin wrote:\n>\n> Do you really have git servers accessed over a local lan or on local system?\n\nYes and yes.\n\n> I just use ssh in this case, and I think that's the common case ...\n\nnot on windows\n\n> We *could* try doing something smart with non-blocking connect + select,\n> and only print the message if it takes > 1 second. Are you\n> sure it's worth the complication?\n\nI'm not sure this suggestion of yours is worth the complication.\nBesides, it's hard to get portably\n"},{"id":"41763","messageId":"7v7irgjx4f.fsf@assigned-by-dhcp.cox.net","threadId":"8005","inReplyTo":"20070510120802.GG13655@mellanox.co.il","subject":"Re: [PATCHv2] connect: display connection progress","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-10T19:29:20Z","receivedAt":"2007-05-10T19:29:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n>> Quoting Alex Riesen :\n>> Subject: Re: [PATCHv2] connect: display connection progress\n>> \n>> On 5/10/07, Michael S. Tsirkin wrote:\n>> >-static int git_tcp_connect_sock(char *host)\n>> >+static int git_tcp_connect_sock(char *host, int flags)\n>> \n>> There is only one bit of flags ever used. What are the others for?\n>\n> Hmm, I thought it's easier to read \n> git_tcp_connect_sock(host, NET_QUIET)\n> \tthan\n> git_tcp_connect_sock(host, 1)\n>\n> but maybe that's overdesign.\n>\n>> Why use negative logic?\n>> What was wrong with plain \"int verbose\"?\n>\n> I want the default to report connections, and -q\n> to silence them. Maybe \"int quiet\"?\n\nI would really feel this extra verbosity should not be the\ndefault. Thanks.\n"}]}