* [PATCH] pass transport verbosity down to git_connect
@ 2016-01-28 22:51 Eric Wong
2016-01-28 23:45 ` Junio C Hamano
2016-01-28 23:53 ` [PATCH] pass transport verbosity down to git_connect Jeff King
0 siblings, 2 replies; 18+ messages in thread
From: Eric Wong @ 2016-01-28 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
While working in connect.c to perform non-blocking connections,
I noticed calling "git fetch -v" was not causing the progress
messages inside git_tcp_connect_sock to be emitted as I
expected.
Looking at history, it seems connect_setup has never been called
with the verbose parameter. Since transport already has a
"verbose" field, use that field instead of another parameter
in connect_setup.
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
Related, but independent of my other change to connect.c:
http://mid.gmane.org/20160128115720.GA1827@dcvr.yhbt.net
("attempt connects in parallel for IPv6-capable builds")
transport.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/transport.c b/transport.c
index 67f3666..9ae7184 100644
--- a/transport.c
+++ b/transport.c
@@ -481,9 +481,10 @@ static int set_git_option(struct git_transport_options *opts,
return 1;
}
-static int connect_setup(struct transport *transport, int for_push, int verbose)
+static int connect_setup(struct transport *transport, int for_push)
{
struct git_transport_data *data = transport->data;
+ int flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0;
if (data->conn)
return 0;
@@ -491,7 +492,7 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
data->conn = git_connect(data->fd, transport->url,
for_push ? data->options.receivepack :
data->options.uploadpack,
- verbose ? CONNECT_VERBOSE : 0);
+ flags);
return 0;
}
@@ -501,7 +502,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
struct git_transport_data *data = transport->data;
struct ref *refs;
- connect_setup(transport, for_push, 0);
+ connect_setup(transport, for_push);
get_remote_heads(data->fd[0], NULL, 0, &refs,
for_push ? REF_NORMAL : 0,
&data->extra_have,
@@ -536,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.update_shallow = data->options.update_shallow;
if (!data->got_remote_heads) {
- connect_setup(transport, 0, 0);
+ connect_setup(transport, 0);
get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
NULL, &data->shallow);
data->got_remote_heads = 1;
@@ -812,7 +813,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
if (!data->got_remote_heads) {
struct ref *tmp_refs;
- connect_setup(transport, 1, 0);
+ connect_setup(transport, 1);
get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
NULL, &data->shallow);
--
EW
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-28 22:51 [PATCH] pass transport verbosity down to git_connect Eric Wong @ 2016-01-28 23:45 ` Junio C Hamano 2016-01-30 8:50 ` [PATCH v2] " Eric Wong 2016-01-28 23:53 ` [PATCH] pass transport verbosity down to git_connect Jeff King 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-01-28 23:45 UTC (permalink / raw) To: Eric Wong; +Cc: git Eric Wong <normalperson@yhbt.net> writes: > While working in connect.c to perform non-blocking connections, > I noticed calling "git fetch -v" was not causing the progress > messages inside git_tcp_connect_sock to be emitted as I > expected. Nice. Can we demonstrate and protect this fix with simple tests? Thanks. Will queue. > transport.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/transport.c b/transport.c > index 67f3666..9ae7184 100644 > --- a/transport.c > +++ b/transport.c > @@ -481,9 +481,10 @@ static int set_git_option(struct git_transport_options *opts, > return 1; > } > > -static int connect_setup(struct transport *transport, int for_push, int verbose) > +static int connect_setup(struct transport *transport, int for_push) > { > struct git_transport_data *data = transport->data; > + int flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0; > > if (data->conn) > return 0; > @@ -491,7 +492,7 @@ static int connect_setup(struct transport *transport, int for_push, int verbose) > data->conn = git_connect(data->fd, transport->url, > for_push ? data->options.receivepack : > data->options.uploadpack, > - verbose ? CONNECT_VERBOSE : 0); > + flags); > > return 0; > } > @@ -501,7 +502,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus > struct git_transport_data *data = transport->data; > struct ref *refs; > > - connect_setup(transport, for_push, 0); > + connect_setup(transport, for_push); > get_remote_heads(data->fd[0], NULL, 0, &refs, > for_push ? REF_NORMAL : 0, > &data->extra_have, > @@ -536,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport, > args.update_shallow = data->options.update_shallow; > > if (!data->got_remote_heads) { > - connect_setup(transport, 0, 0); > + connect_setup(transport, 0); > get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, > NULL, &data->shallow); > data->got_remote_heads = 1; > @@ -812,7 +813,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re > > if (!data->got_remote_heads) { > struct ref *tmp_refs; > - connect_setup(transport, 1, 0); > + connect_setup(transport, 1); > > get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, > NULL, &data->shallow); ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] pass transport verbosity down to git_connect 2016-01-28 23:45 ` Junio C Hamano @ 2016-01-30 8:50 ` Eric Wong 2016-01-30 13:13 ` [PATCH 2/1] support -4 and -6 switches for remote operations Eric Wong 0 siblings, 1 reply; 18+ messages in thread From: Eric Wong @ 2016-01-30 8:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> wrote: > Nice. Can we demonstrate and protect this fix with simple tests? I just added the tests to t5570 since we don't use git:// much in the tests and I didn't want to introduce potential port conflicts. ----------------8<---------------- Subject: [PATCH] pass transport verbosity down to git_connect While working in connect.c to perform non-blocking connections, I noticed calling "git fetch -v" was not causing the progress messages inside git_tcp_connect_sock to be emitted as I expected. Looking at history, it seems connect_setup has never been called with the verbose parameter. Since transport already has a "verbose" field, use that field instead of another parameter in connect_setup. v2: add "-v" tests for clone/fetch/pull to t5570-git-daemon.sh Signed-off-by: Eric Wong <normalperson@yhbt.net> --- t/t5570-git-daemon.sh | 25 +++++++++++++++++++++++-- transport.c | 11 ++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh index b7e2832..678c8ba 100755 --- a/t/t5570-git-daemon.sh +++ b/t/t5570-git-daemon.sh @@ -6,6 +6,13 @@ test_description='test fetching over git protocol' . "$TEST_DIRECTORY"/lib-git-daemon.sh start_git_daemon +check_verbose_connect () { + grep -qF "Looking up 127.0.0.1 ..." stderr && + grep -qF "Connecting to 127.0.0.1 (port " stderr && + grep -qF "done." stderr && + rm stderr +} + test_expect_success 'setup repository' ' git config push.default matching && echo content >file && @@ -24,18 +31,32 @@ test_expect_success 'create git-accessible bare repository' ' ' test_expect_success 'clone git repository' ' - git clone "$GIT_DAEMON_URL/repo.git" clone && + git clone -v "$GIT_DAEMON_URL/repo.git" clone 2>stderr && test_cmp file clone/file ' +test_expect_success 'clone -v stderr is as expected' check_verbose_connect + test_expect_success 'fetch changes via git protocol' ' echo content >>file && git commit -a -m two && git push public && - (cd clone && git pull) && + (cd clone && git pull -v) 2>stderr && test_cmp file clone/file ' +test_expect_success 'pull -v stderr is as expected' check_verbose_connect + +test_expect_success 'no-op fetch -v stderr is as expected' ' + (cd clone && git fetch -v) 2>stderr && + check_verbose_connect +' + +test_expect_success 'no-op fetch without "-v" is quiet' ' + (cd clone && git fetch) 2>stderr && + ! test -s stderr +' + test_expect_success 'remote detects correct HEAD' ' git push public master:other && (cd clone && diff --git a/transport.c b/transport.c index 67f3666..9ae7184 100644 --- a/transport.c +++ b/transport.c @@ -481,9 +481,10 @@ static int set_git_option(struct git_transport_options *opts, return 1; } -static int connect_setup(struct transport *transport, int for_push, int verbose) +static int connect_setup(struct transport *transport, int for_push) { struct git_transport_data *data = transport->data; + int flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0; if (data->conn) return 0; @@ -491,7 +492,7 @@ static int connect_setup(struct transport *transport, int for_push, int verbose) data->conn = git_connect(data->fd, transport->url, for_push ? data->options.receivepack : data->options.uploadpack, - verbose ? CONNECT_VERBOSE : 0); + flags); return 0; } @@ -501,7 +502,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus struct git_transport_data *data = transport->data; struct ref *refs; - connect_setup(transport, for_push, 0); + connect_setup(transport, for_push); get_remote_heads(data->fd[0], NULL, 0, &refs, for_push ? REF_NORMAL : 0, &data->extra_have, @@ -536,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport, args.update_shallow = data->options.update_shallow; if (!data->got_remote_heads) { - connect_setup(transport, 0, 0); + connect_setup(transport, 0); get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL, &data->shallow); data->got_remote_heads = 1; @@ -812,7 +813,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re if (!data->got_remote_heads) { struct ref *tmp_refs; - connect_setup(transport, 1, 0); + connect_setup(transport, 1); get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, NULL, &data->shallow); -- EW ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-30 8:50 ` [PATCH v2] " Eric Wong @ 2016-01-30 13:13 ` Eric Wong 2016-01-30 13:28 ` Eric Wong 2016-01-30 23:34 ` Torsten Bögershausen 0 siblings, 2 replies; 18+ messages in thread From: Eric Wong @ 2016-01-30 13:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Torsten Bögershausen, Jeff King Sometimes, it is necessary to force IPv4-only or IPv6-only operation on networks where name lookups may return a non-routable address and stall remote operations. The ssh(1) command has an equivalent switches which we may pass when we run them. rsync support is untouched for now since it is deprecated and scheduled to be removed. Signed-off-by: Eric Wong <normalperson@yhbt.net> --- Documentation/fetch-options.txt | 8 ++++++++ Documentation/git-push.txt | 7 +++++++ builtin/clone.c | 4 ++++ builtin/fetch.c | 4 ++++ builtin/push.c | 4 ++++ connect.c | 8 ++++++++ connect.h | 2 ++ http.c | 9 +++++++++ http.h | 1 + remote-curl.c | 19 +++++++++++++++++++ transport-helper.c | 7 +++++++ transport.c | 18 ++++++++++++++++++ transport.h | 11 +++++++++++ 13 files changed, 102 insertions(+) diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 952dfdf..6ec7dde 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -158,3 +158,11 @@ endif::git-pull[] by default when it is attached to a terminal, unless -q is specified. This flag forces progress status even if the standard error stream is not directed to a terminal. + +-4:: +--ipv4:: + Resolve IPv4 addresses only, ignoring IPv6 addresses. + +-6:: +--ipv6:: + Resolve IPv6 addresses only, ignoring IPv4 addresses. diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 32482ce..559c166 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -277,6 +277,13 @@ origin +master` to force a push to the `master` branch). See the default is --verify, giving the hook a chance to prevent the push. With --no-verify, the hook is bypassed completely. +-4:: +--ipv4:: + Resolve IPv4 addresses only, ignoring IPv6 addresses. + +-6:: +--ipv6:: + Resolve IPv6 addresses only, ignoring IPv4 addresses. include::urls-remotes.txt[] diff --git a/builtin/clone.c b/builtin/clone.c index 81e238f..3feae64 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -47,6 +47,7 @@ static const char *real_git_dir; static char *option_upload_pack = "git-upload-pack"; static int option_verbosity; static int option_progress = -1; +static int ipv4, ipv6; static struct string_list option_config; static struct string_list option_reference; static int option_dissociate; @@ -92,6 +93,8 @@ static struct option builtin_clone_options[] = { N_("separate git dir from working tree")), OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), N_("set config inside the new repository")), + OPT_BOOL('4', "ipv4", &ipv4, N_("resolve IPv4 addresses only")), + OPT_BOOL('6', "ipv6", &ipv6, N_("resolve IPv6 addresses only")), OPT_END() }; @@ -970,6 +973,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote = remote_get(option_origin); transport = transport_get(remote, remote->url[0]); transport_set_verbosity(transport, option_verbosity, option_progress); + transport_set_family(transport, ipv4, ipv6); path = get_repo_path(remote->url[0], &is_bundle); is_local = option_local != 0 && path && !is_bundle; diff --git a/builtin/fetch.c b/builtin/fetch.c index 8e74213..c77af86 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity; static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT; static int tags = TAGS_DEFAULT, unshallow, update_shallow; +static int ipv4, ipv6; static int max_children = 1; static const char *depth; static const char *upload_pack; @@ -127,6 +128,8 @@ static struct option builtin_fetch_options[] = { N_("accept refs that update .git/shallow")), { OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"), N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg }, + OPT_BOOL('4', "ipv4", &ipv4, N_("resolve IPv4 addresses only")), + OPT_BOOL('6', "ipv6", &ipv6, N_("resolve IPv6 addresses only")), OPT_END() }; @@ -864,6 +867,7 @@ static struct transport *prepare_transport(struct remote *remote) struct transport *transport; transport = transport_get(remote, NULL); transport_set_verbosity(transport, verbosity, progress); + transport_set_family(transport, ipv4, ipv6); if (upload_pack) set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack); if (keep) diff --git a/builtin/push.c b/builtin/push.c index 960ffc3..e0e8b40 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -23,6 +23,7 @@ static const char *receivepack; static int verbosity; static int progress = -1; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static int ipv4, ipv6; static struct push_cas_option cas; @@ -346,6 +347,7 @@ static int push_with_options(struct transport *transport, int flags) unsigned int reject_reasons; transport_set_verbosity(transport, verbosity, progress); + transport_set_family(transport, ipv4, ipv6); if (receivepack) transport_set_option(transport, @@ -565,6 +567,8 @@ int cmd_push(int argc, const char **argv, const char *prefix) 0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"), PARSE_OPT_OPTARG, option_parse_push_signed }, OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC), + OPT_BOOL('4', "ipv4", &ipv4, N_("resolve IPv4 addresses only")), + OPT_BOOL('6', "ipv6", &ipv6, N_("resolve IPv6 addresses only")), OPT_END() }; diff --git a/connect.c b/connect.c index fd7ffe1..99178d0 100644 --- a/connect.c +++ b/connect.c @@ -357,6 +357,10 @@ static int git_tcp_connect_sock(char *host, int flags) port = "<none>"; memset(&hints, 0, sizeof(hints)); + if (flags & CONNECT_IPV4ONLY) + hints.ai_family = AF_INET; + else if (flags & CONNECT_IPV6ONLY) + hints.ai_family = AF_INET6; hints.ai_socktype = SOCK_STREAM; hints.ai_protocol = IPPROTO_TCP; @@ -783,6 +787,10 @@ struct child_process *git_connect(int fd[2], const char *url, } argv_array_push(&conn->args, ssh); + if (flags & CONNECT_IPV4ONLY) + argv_array_push(&conn->args, "-4"); + else if (flags & CONNECT_IPV6ONLY) + argv_array_push(&conn->args, "-6"); if (tortoiseplink) argv_array_push(&conn->args, "-batch"); if (port) { diff --git a/connect.h b/connect.h index c41a685..1cf2412 100644 --- a/connect.h +++ b/connect.h @@ -3,6 +3,8 @@ #define CONNECT_VERBOSE (1u << 0) #define CONNECT_DIAG_URL (1u << 1) +#define CONNECT_IPV4ONLY (1u << 2) +#define CONNECT_IPV6ONLY (1u << 3) extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); extern int finish_connect(struct child_process *conn); extern int git_connection_is_socket(struct child_process *conn); diff --git a/http.c b/http.c index 0da9e66..67e7bc2 100644 --- a/http.c +++ b/http.c @@ -11,6 +11,11 @@ #include "gettext.h" #include "transport.h" +#if LIBCURL_VERSION_NUM >= 0x070a08 +long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; +#else +long int git_curl_ipresolve; +#endif int active_requests; int http_is_verbose; size_t http_post_buffer = 16 * LARGE_PACKET_MAX; @@ -692,6 +697,10 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); + +#if LIBCURL_VERSION_NUM >= 0x070a08 + curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve); +#endif #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods); #endif diff --git a/http.h b/http.h index 4f97b60..fa45c2b 100644 --- a/http.h +++ b/http.h @@ -106,6 +106,7 @@ extern void http_init(struct remote *remote, const char *url, int proactive_auth); extern void http_cleanup(void); +extern long int git_curl_ipresolve; extern int active_requests; extern int http_is_verbose; extern size_t http_post_buffer; diff --git a/remote-curl.c b/remote-curl.c index c704857..2297fa1 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -119,6 +119,25 @@ static int set_option(const char *name, const char *value) else return -1; return 0; + +#if LIBCURL_VERSION_NUM >= 0x070a08 + } else if (!strcmp(name, "ipv4")) { + if (!strcmp(value, "true")) + git_curl_ipresolve = CURL_IPRESOLVE_V4; + else if (!strcmp(value, "false")) + git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; + else + return -1; + return 0; + } else if (!strcmp(name, "ipv6")) { + if (!strcmp(value, "true")) + git_curl_ipresolve = CURL_IPRESOLVE_V6; + else if (!strcmp(value, "false")) + git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; + else + return -1; + return 0; +#endif /* LIBCURL_VERSION_NUM >= 0x070a08 */ } else { return 1 /* unsupported */; } diff --git a/transport-helper.c b/transport-helper.c index e45d88f..e6ffb9a 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -258,6 +258,8 @@ static const char *boolean_options[] = { TRANS_OPT_THIN, TRANS_OPT_KEEP, TRANS_OPT_FOLLOWTAGS, + TRANS_OPT_IPV4, + TRANS_OPT_IPV6, }; static int set_helper_option(struct transport *transport, @@ -321,6 +323,11 @@ static void standard_options(struct transport *t) if (n >= sizeof(buf)) die("impossibly large verbosity value"); set_helper_option(t, "verbosity", buf); + + if (t->ipv4) + set_helper_option(t, "ipv4", "true"); + if (t->ipv6) + set_helper_option(t, "ipv6", "true"); } static int release_helper(struct transport *transport) diff --git a/transport.c b/transport.c index 9ae7184..8fd31d3 100644 --- a/transport.c +++ b/transport.c @@ -489,6 +489,10 @@ static int connect_setup(struct transport *transport, int for_push) if (data->conn) return 0; + if (transport->ipv4) + flags |= CONNECT_IPV4ONLY; + if (transport->ipv6) + flags |= CONNECT_IPV6ONLY; data->conn = git_connect(data->fd, transport->url, for_push ? data->options.receivepack : data->options.uploadpack, @@ -1086,6 +1090,20 @@ void transport_set_verbosity(struct transport *transport, int verbosity, transport->progress = verbosity >= 0 && isatty(2); } +void transport_set_family(struct transport *transport, int ipv4, int ipv6) +{ + if (ipv4 && ipv6) + die("-4/--ipv4 and -6/--ipv6 are incompatible"); + if (ipv4) { + transport->ipv4 = 1; + transport_set_option(transport, TRANS_OPT_IPV4, "true"); + } + if (ipv6) { + transport->ipv6 = 1; + transport_set_option(transport, TRANS_OPT_IPV6, "true"); + } +} + static void die_with_unpushed_submodules(struct string_list *needs_pushing) { int i; diff --git a/transport.h b/transport.h index 8ebaaf2..f9f3c00 100644 --- a/transport.h +++ b/transport.h @@ -104,6 +104,10 @@ struct transport { * in transport_set_verbosity(). **/ unsigned progress : 1; + + /* mutually exclusive, set by transport_set_family */ + unsigned ipv4 : 1; + unsigned ipv6 : 1; /* * If transport is at least potentially smart, this points to * git_transport_options structure to use in case transport @@ -180,6 +184,12 @@ int transport_restrict_protocols(void); /* Send push certificates */ #define TRANS_OPT_PUSH_CERT "pushcert" +/* Limit to IPv4 only */ +#define TRANS_OPT_IPV4 "ipv4" + +/* Limit to IPv6 only */ +#define TRANS_OPT_IPV6 "ipv6" + /** * Returns 0 if the option was used, non-zero otherwise. Prints a * message to stderr if the option is not used. @@ -188,6 +198,7 @@ int transport_set_option(struct transport *transport, const char *name, const char *value); void transport_set_verbosity(struct transport *transport, int verbosity, int force_progress); +void transport_set_family(struct transport *transport, int ipv4, int ipv6); #define REJECT_NON_FF_HEAD 0x01 #define REJECT_NON_FF_OTHER 0x02 -- EW ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-30 13:13 ` [PATCH 2/1] support -4 and -6 switches for remote operations Eric Wong @ 2016-01-30 13:28 ` Eric Wong 2016-01-30 23:34 ` Torsten Bögershausen 1 sibling, 0 replies; 18+ messages in thread From: Eric Wong @ 2016-01-30 13:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Torsten Bögershausen, Jeff King Eric Wong <normalperson@yhbt.net> wrote: > rsync support is untouched for now since it is deprecated > and scheduled to be removed. I forgot add I'm not sure how to actually go about testing these changes automatically since they involve DNS setups. And the test suite seems really slow nowadays, I guess/hope our test coverage has improved much in the past few years? I did test the ssh push case manually and all the fetch/clone cases, including rsync (where I encountered the breakage from 2007). So I reverted my rsync change for now to avoid conflicting with its removal. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-30 13:13 ` [PATCH 2/1] support -4 and -6 switches for remote operations Eric Wong 2016-01-30 13:28 ` Eric Wong @ 2016-01-30 23:34 ` Torsten Bögershausen 2016-01-31 0:01 ` Eric Wong 1 sibling, 1 reply; 18+ messages in thread From: Torsten Bögershausen @ 2016-01-30 23:34 UTC (permalink / raw) To: Eric Wong, Junio C Hamano; +Cc: git, Jeff King On 2016-01-30 14.13, Eric Wong wrote: Nicely done, some minor questions inline. > Sometimes, it is necessary to force IPv4-only or IPv6-only > operation on networks where name lookups may return a > non-routable address and stall remote operations. > > The ssh(1) command has an equivalent switches which we may > pass when we run them. Should we mention that putty and tortoiseplink don't have these options ? At least in the commit message ? > rsync support is untouched for now since it is deprecated > and scheduled to be removed. > > Signed-off-by: Eric Wong <normalperson@yhbt.net> > --- > Documentation/fetch-options.txt | 8 ++++++++ > Documentation/git-push.txt | 7 +++++++ > builtin/clone.c | 4 ++++ > builtin/fetch.c | 4 ++++ > builtin/push.c | 4 ++++ > connect.c | 8 ++++++++ > connect.h | 2 ++ > http.c | 9 +++++++++ > http.h | 1 + > remote-curl.c | 19 +++++++++++++++++++ > transport-helper.c | 7 +++++++ > transport.c | 18 ++++++++++++++++++ > transport.h | 11 +++++++++++ > 13 files changed, 102 insertions(+) > > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt > index 952dfdf..6ec7dde 100644 > --- a/Documentation/fetch-options.txt > +++ b/Documentation/fetch-options.txt > @@ -158,3 +158,11 @@ endif::git-pull[] > by default when it is attached to a terminal, unless -q > is specified. This flag forces progress status even if the > standard error stream is not directed to a terminal. > + > +-4:: > +--ipv4:: > + Resolve IPv4 addresses only, ignoring IPv6 addresses. > + > +-6:: > +--ipv6:: > + Resolve IPv6 addresses only, ignoring IPv4 addresses. > diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt > index 32482ce..559c166 100644 > --- a/Documentation/git-push.txt > +++ b/Documentation/git-push.txt > @@ -277,6 +277,13 @@ origin +master` to force a push to the `master` branch). See the > default is --verify, giving the hook a chance to prevent the > push. With --no-verify, the hook is bypassed completely. > > +-4:: > +--ipv4:: > + Resolve IPv4 addresses only, ignoring IPv6 addresses. > + > +-6:: > +--ipv6:: > + Resolve IPv6 addresses only, ignoring IPv4 addresses. > > include::urls-remotes.txt[] > > diff --git a/builtin/clone.c b/builtin/clone.c > index 81e238f..3feae64 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -47,6 +47,7 @@ static const char *real_git_dir; > static char *option_upload_pack = "git-upload-pack"; > static int option_verbosity; > static int option_progress = -1; > +static int ipv4, ipv6; Do we need 2 variables here ? Or would int preferred_ip_version be better ? > static struct string_list option_config; > static struct string_list option_reference; > static int option_dissociate; > @@ -92,6 +93,8 @@ static struct option builtin_clone_options[] = { > N_("separate git dir from working tree")), > OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), > N_("set config inside the new repository")), > + OPT_BOOL('4', "ipv4", &ipv4, N_("resolve IPv4 addresses only")), > + OPT_BOOL('6', "ipv6", &ipv6, N_("resolve IPv6 addresses only")), Technically OK to mention resolve, but does it give any information to the user ? s/resolve IPv4 addresses only/use IPv4 addresses only/ > OPT_END() > }; > > @@ -970,6 +973,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > remote = remote_get(option_origin); > transport = transport_get(remote, remote->url[0]); > transport_set_verbosity(transport, option_verbosity, option_progress); > + transport_set_family(transport, ipv4, ipv6); > Does it make sense to name the variable into ipv4only (to make clear that it does not mean ipv4_allowed ?) (and similar in the rest of the code) [snip] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-30 23:34 ` Torsten Bögershausen @ 2016-01-31 0:01 ` Eric Wong 2016-01-31 1:13 ` Jeff King 2016-01-31 16:03 ` [PATCH " Torsten Bögershausen 0 siblings, 2 replies; 18+ messages in thread From: Eric Wong @ 2016-01-31 0:01 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Junio C Hamano, git, Jeff King Torsten Bögershausen <tboegi@web.de> wrote: > On 2016-01-30 14.13, Eric Wong wrote: > > The ssh(1) command has an equivalent switches which we may > > pass when we run them. > Should we mention that putty and tortoiseplink don't have these options ? > At least in the commit message ? Sure, will remember for v2 and document in the manpages. Curious, do these ssh implementations throw out a meaningful error message when given these options? > > --- a/builtin/clone.c > > +++ b/builtin/clone.c > > @@ -47,6 +47,7 @@ static const char *real_git_dir; > > static char *option_upload_pack = "git-upload-pack"; > > static int option_verbosity; > > static int option_progress = -1; > > +static int ipv4, ipv6; > Do we need 2 variables here ? Yes, I'm not sure how else to use OPT_BOOL below... > Or would > int preferred_ip_version > be better ? > > static struct string_list option_config; > > static struct string_list option_reference; > > static int option_dissociate; > > @@ -92,6 +93,8 @@ static struct option builtin_clone_options[] = { > > N_("separate git dir from working tree")), > > OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), > > N_("set config inside the new repository")), > > + OPT_BOOL('4', "ipv4", &ipv4, N_("resolve IPv4 addresses only")), > > + OPT_BOOL('6', "ipv6", &ipv6, N_("resolve IPv6 addresses only")), > Technically OK to mention resolve, but does it give any information to the user ? > s/resolve IPv4 addresses only/use IPv4 addresses only/ I suppose "use" is shorter and just as informational. Will prepare that for v2. > > @@ -970,6 +973,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > > remote = remote_get(option_origin); > > transport = transport_get(remote, remote->url[0]); > > transport_set_verbosity(transport, option_verbosity, option_progress); > > + transport_set_family(transport, ipv4, ipv6); > > > Does it make sense to name the variable into > ipv4only (to make clear that it does not mean ipv4_allowed ?) > (and similar in the rest of the code) I actually had "only" in the variables originally, but didn't want to line-wrap in builtin/push.c Furthermore, the non-"only" name is used by the long switch (just like in both curl(1) and rsync(1)), so I figured we should remain consistent with what the user will see in documentation. I think I will drop "ONLY" from the CONNECT_* macros instead... Will await further comments and prepare v2 in a day or two. Thanks for the comments. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-31 0:01 ` Eric Wong @ 2016-01-31 1:13 ` Jeff King 2016-02-03 4:09 ` [PATCH v2 " Eric Wong 2016-01-31 16:03 ` [PATCH " Torsten Bögershausen 1 sibling, 1 reply; 18+ messages in thread From: Jeff King @ 2016-01-31 1:13 UTC (permalink / raw) To: Eric Wong; +Cc: Torsten Bögershausen, Junio C Hamano, git On Sun, Jan 31, 2016 at 12:01:44AM +0000, Eric Wong wrote: > > > --- a/builtin/clone.c > > > +++ b/builtin/clone.c > > > @@ -47,6 +47,7 @@ static const char *real_git_dir; > > > static char *option_upload_pack = "git-upload-pack"; > > > static int option_verbosity; > > > static int option_progress = -1; > > > +static int ipv4, ipv6; > > Do we need 2 variables here ? > > Yes, I'm not sure how else to use OPT_BOOL below... If they're mutually exclusive, and saying "-6" cancels "-4", you probably want something like: enum { USE_NET_ALL = 0, USE_NET_IPV4, USE_NET_IPV6, } use_net; ... OPT_SET_INT('4', "ipv4", &use_net, N_("resolve IPv4 addresses only"), USE_NET_IPV4), OPT_SET_INT('6', "ipv6", &use_net, N_("resolve IPv6 addresses only"), USE_NET_IPV6), Using --no-ipv4 will set it back to USE_NET_ALL, which is probably OK. It will cancel a previous "--ipv4", which is logically consistent, though I guess some people might assume that "--no-ipv4" means "do not use ipv4 addresses". Supporting that would be more complicated. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/1] support -4 and -6 switches for remote operations 2016-01-31 1:13 ` Jeff King @ 2016-02-03 4:09 ` Eric Wong 2016-02-12 11:31 ` Eric Wong 0 siblings, 1 reply; 18+ messages in thread From: Eric Wong @ 2016-02-03 4:09 UTC (permalink / raw) To: Jeff King; +Cc: Torsten Bögershausen, Junio C Hamano, git Jeff King <peff@peff.net> wrote: > OPT_SET_INT('4', "ipv4", &use_net, > N_("resolve IPv4 addresses only"), USE_NET_IPV4), > OPT_SET_INT('6', "ipv6", &use_net, > N_("resolve IPv6 addresses only"), USE_NET_IPV6), Thanks, I missed OPT_SET_INT before. > Using --no-ipv4 will set it back to USE_NET_ALL, which is probably OK. > It will cancel a previous "--ipv4", which is logically consistent, > though I guess some people might assume that "--no-ipv4" means "do not > use ipv4 addresses". Supporting that would be more complicated. Right, not worth the effort to support that and I prefer leaving the "--no-" variants undocumented as an implementation artifact. (no rush to review this, unlikely to be around the next few days) -----------------8<------------------ Subject: [PATCH] support -4 and -6 switches for remote operations Sometimes it is necessary to force IPv4-only or IPv6-only operation on networks where name lookups may return a non-routable address and stall remote operations. The ssh(1) command has an equivalent switches which we may pass when we run them. There may be old ssh(1) implementations out there which do not support these switches; they should report the appropriate error in that case. rsync support is untouched for now since it is deprecated and scheduled to be removed. v2: - switch from boolean ints to enum - shorted CONNECT_* macro names to be consistent with switches - remove unneeded TRANS_OPT_* macros and usage - remove transport_set_family wrapper, enum is enough validation - s/resolve/use/ in documentation - document potential (but unconfirmed) problems with some ssh(1) Signed-off-by: Eric Wong <normalperson@yhbt.net> --- Documentation/fetch-options.txt | 8 ++++++++ Documentation/git-push.txt | 7 +++++++ builtin/clone.c | 6 ++++++ builtin/fetch.c | 6 ++++++ builtin/push.c | 6 ++++++ connect.c | 8 ++++++++ connect.h | 2 ++ http.c | 9 +++++++++ http.h | 1 + remote-curl.c | 13 +++++++++++++ transport-helper.c | 15 +++++++++++++++ transport.c | 6 ++++++ transport.h | 8 ++++++++ 13 files changed, 95 insertions(+) diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 952dfdf..036edfb 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -158,3 +158,11 @@ endif::git-pull[] by default when it is attached to a terminal, unless -q is specified. This flag forces progress status even if the standard error stream is not directed to a terminal. + +-4:: +--ipv4:: + Use IPv4 addresses only, ignoring IPv6 addresses. + +-6:: +--ipv6:: + Use IPv6 addresses only, ignoring IPv4 addresses. diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 32482ce..669a357 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -277,6 +277,13 @@ origin +master` to force a push to the `master` branch). See the default is --verify, giving the hook a chance to prevent the push. With --no-verify, the hook is bypassed completely. +-4:: +--ipv4:: + Use IPv4 addresses only, ignoring IPv6 addresses. + +-6:: +--ipv6:: + Use IPv6 addresses only, ignoring IPv4 addresses. include::urls-remotes.txt[] diff --git a/builtin/clone.c b/builtin/clone.c index 81e238f..592e6db 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -47,6 +47,7 @@ static const char *real_git_dir; static char *option_upload_pack = "git-upload-pack"; static int option_verbosity; static int option_progress = -1; +static enum transport_family family; static struct string_list option_config; static struct string_list option_reference; static int option_dissociate; @@ -92,6 +93,10 @@ static struct option builtin_clone_options[] = { N_("separate git dir from working tree")), OPT_STRING_LIST('c', "config", &option_config, N_("key=value"), N_("set config inside the new repository")), + OPT_SET_INT('4', "ipv4", &family, N_("use IPv4 addresses only"), + TRANSPORT_FAMILY_IPV4), + OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"), + TRANSPORT_FAMILY_IPV6), OPT_END() }; @@ -970,6 +975,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote = remote_get(option_origin); transport = transport_get(remote, remote->url[0]); transport_set_verbosity(transport, option_verbosity, option_progress); + transport->family = family; path = get_repo_path(remote->url[0], &is_bundle); is_local = option_local != 0 && path && !is_bundle; diff --git a/builtin/fetch.c b/builtin/fetch.c index 8e74213..03ba709 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity; static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT; static int tags = TAGS_DEFAULT, unshallow, update_shallow; +static enum transport_family family; static int max_children = 1; static const char *depth; static const char *upload_pack; @@ -127,6 +128,10 @@ static struct option builtin_fetch_options[] = { N_("accept refs that update .git/shallow")), { OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"), N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg }, + OPT_SET_INT('4', "ipv4", &family, N_("use IPv4 addresses only"), + TRANSPORT_FAMILY_IPV4), + OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"), + TRANSPORT_FAMILY_IPV6), OPT_END() }; @@ -864,6 +869,7 @@ static struct transport *prepare_transport(struct remote *remote) struct transport *transport; transport = transport_get(remote, NULL); transport_set_verbosity(transport, verbosity, progress); + transport->family = family; if (upload_pack) set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack); if (keep) diff --git a/builtin/push.c b/builtin/push.c index 960ffc3..6e13b3c 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -23,6 +23,7 @@ static const char *receivepack; static int verbosity; static int progress = -1; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static enum transport_family family; static struct push_cas_option cas; @@ -346,6 +347,7 @@ static int push_with_options(struct transport *transport, int flags) unsigned int reject_reasons; transport_set_verbosity(transport, verbosity, progress); + transport->family = family; if (receivepack) transport_set_option(transport, @@ -565,6 +567,10 @@ int cmd_push(int argc, const char **argv, const char *prefix) 0, "signed", &push_cert, "yes|no|if-asked", N_("GPG sign the push"), PARSE_OPT_OPTARG, option_parse_push_signed }, OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC), + OPT_SET_INT('4', "ipv4", &family, N_("use IPv4 addresses only"), + TRANSPORT_FAMILY_IPV4), + OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"), + TRANSPORT_FAMILY_IPV6), OPT_END() }; diff --git a/connect.c b/connect.c index fd7ffe1..0478631 100644 --- a/connect.c +++ b/connect.c @@ -357,6 +357,10 @@ static int git_tcp_connect_sock(char *host, int flags) port = "<none>"; memset(&hints, 0, sizeof(hints)); + if (flags & CONNECT_IPV4) + hints.ai_family = AF_INET; + else if (flags & CONNECT_IPV6) + hints.ai_family = AF_INET6; hints.ai_socktype = SOCK_STREAM; hints.ai_protocol = IPPROTO_TCP; @@ -783,6 +787,10 @@ struct child_process *git_connect(int fd[2], const char *url, } argv_array_push(&conn->args, ssh); + if (flags & CONNECT_IPV4) + argv_array_push(&conn->args, "-4"); + else if (flags & CONNECT_IPV6) + argv_array_push(&conn->args, "-6"); if (tortoiseplink) argv_array_push(&conn->args, "-batch"); if (port) { diff --git a/connect.h b/connect.h index c41a685..01f14cd 100644 --- a/connect.h +++ b/connect.h @@ -3,6 +3,8 @@ #define CONNECT_VERBOSE (1u << 0) #define CONNECT_DIAG_URL (1u << 1) +#define CONNECT_IPV4 (1u << 2) +#define CONNECT_IPV6 (1u << 3) extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); extern int finish_connect(struct child_process *conn); extern int git_connection_is_socket(struct child_process *conn); diff --git a/http.c b/http.c index 0da9e66..67e7bc2 100644 --- a/http.c +++ b/http.c @@ -11,6 +11,11 @@ #include "gettext.h" #include "transport.h" +#if LIBCURL_VERSION_NUM >= 0x070a08 +long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; +#else +long int git_curl_ipresolve; +#endif int active_requests; int http_is_verbose; size_t http_post_buffer = 16 * LARGE_PACKET_MAX; @@ -692,6 +697,10 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); + +#if LIBCURL_VERSION_NUM >= 0x070a08 + curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve); +#endif #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods); #endif diff --git a/http.h b/http.h index 4f97b60..fa45c2b 100644 --- a/http.h +++ b/http.h @@ -106,6 +106,7 @@ extern void http_init(struct remote *remote, const char *url, int proactive_auth); extern void http_cleanup(void); +extern long int git_curl_ipresolve; extern int active_requests; extern int http_is_verbose; extern size_t http_post_buffer; diff --git a/remote-curl.c b/remote-curl.c index c704857..b33a1e4 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -119,6 +119,19 @@ static int set_option(const char *name, const char *value) else return -1; return 0; + +#if LIBCURL_VERSION_NUM >= 0x070a08 + } else if (!strcmp(name, "family")) { + if (!strcmp(value, "ipv4")) + git_curl_ipresolve = CURL_IPRESOLVE_V4; + else if (!strcmp(value, "ipv6")) + git_curl_ipresolve = CURL_IPRESOLVE_V6; + else if (!strcmp(value, "all")) + git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER; + else + return -1; + return 0; +#endif /* LIBCURL_VERSION_NUM >= 0x070a08 */ } else { return 1 /* unsupported */; } diff --git a/transport-helper.c b/transport-helper.c index e45d88f..f6b8e7b 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -321,6 +321,21 @@ static void standard_options(struct transport *t) if (n >= sizeof(buf)) die("impossibly large verbosity value"); set_helper_option(t, "verbosity", buf); + + switch (t->family) { + case TRANSPORT_FAMILY_ALL: + /* + * this is already the default, + * do not break old remote helpers by setting "all" here + */ + break; + case TRANSPORT_FAMILY_IPV4: + set_helper_option(t, "family", "ipv4"); + break; + case TRANSPORT_FAMILY_IPV6: + set_helper_option(t, "family", "ipv6"); + break; + } } static int release_helper(struct transport *transport) diff --git a/transport.c b/transport.c index 9ae7184..afcec43 100644 --- a/transport.c +++ b/transport.c @@ -489,6 +489,12 @@ static int connect_setup(struct transport *transport, int for_push) if (data->conn) return 0; + switch (transport->family) { + case TRANSPORT_FAMILY_ALL: break; + case TRANSPORT_FAMILY_IPV4: flags |= CONNECT_IPV4; break; + case TRANSPORT_FAMILY_IPV6: flags |= CONNECT_IPV6; break; + } + data->conn = git_connect(data->fd, transport->url, for_push ? data->options.receivepack : data->options.uploadpack, diff --git a/transport.h b/transport.h index 8ebaaf2..c681408 100644 --- a/transport.h +++ b/transport.h @@ -18,6 +18,12 @@ struct git_transport_options { struct push_cas_option *cas; }; +enum transport_family { + TRANSPORT_FAMILY_ALL = 0, + TRANSPORT_FAMILY_IPV4, + TRANSPORT_FAMILY_IPV6 +}; + struct transport { struct remote *remote; const char *url; @@ -110,6 +116,8 @@ struct transport { * actually turns out to be smart. */ struct git_transport_options *smart_options; + + enum transport_family family; }; #define TRANSPORT_PUSH_ALL 1 -- EW ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/1] support -4 and -6 switches for remote operations 2016-02-03 4:09 ` [PATCH v2 " Eric Wong @ 2016-02-12 11:31 ` Eric Wong 2016-02-12 15:43 ` Torsten Bögershausen 0 siblings, 1 reply; 18+ messages in thread From: Eric Wong @ 2016-02-12 11:31 UTC (permalink / raw) To: git; +Cc: Jeff King, Torsten Bögershausen, Junio C Hamano Eric Wong <normalperson@yhbt.net> wrote: > (no rush to review this, unlikely to be around the next few days) Ping on v2. I will be online the next few days and likely working on some other git stuff anyways :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/1] support -4 and -6 switches for remote operations 2016-02-12 11:31 ` Eric Wong @ 2016-02-12 15:43 ` Torsten Bögershausen 0 siblings, 0 replies; 18+ messages in thread From: Torsten Bögershausen @ 2016-02-12 15:43 UTC (permalink / raw) To: Eric Wong, git; +Cc: Jeff King, Torsten Bögershausen, Junio C Hamano On 12.02.16 12:31, Eric Wong wrote: > Eric Wong <normalperson@yhbt.net> wrote: >> (no rush to review this, unlikely to be around the next few days) > Ping on v2. I will be online the next few days and likely > working on some other git stuff anyways :) Patch V2 looks OK for me. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/1] support -4 and -6 switches for remote operations 2016-01-31 0:01 ` Eric Wong 2016-01-31 1:13 ` Jeff King @ 2016-01-31 16:03 ` Torsten Bögershausen 1 sibling, 0 replies; 18+ messages in thread From: Torsten Bögershausen @ 2016-01-31 16:03 UTC (permalink / raw) To: Eric Wong, Torsten Bögershausen; +Cc: Junio C Hamano, git, Jeff King On 2016-01-31 01.01, Eric Wong wrote: > Torsten Bögershausen <tboegi@web.de> wrote: >> On 2016-01-30 14.13, Eric Wong wrote: >>> The ssh(1) command has an equivalent switches which we may >>> pass when we run them. > >> Should we mention that putty and tortoiseplink don't have these options ? >> At least in the commit message ? I may need to take that back; Just did a test with putty (under Debian) And both -4 and -6 are supported, nice. I couldn't find it first, but here seems to be the latest documentation, which mentions -4 and -6. http://the.earth.li/~sgtatham/putty/0.66/puttydoc.txt And even plink acceptes -4 and -6 (again under Debian) Sorry for the noise. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-28 22:51 [PATCH] pass transport verbosity down to git_connect Eric Wong 2016-01-28 23:45 ` Junio C Hamano @ 2016-01-28 23:53 ` Jeff King 2016-01-29 0:38 ` Eric Wong 1 sibling, 1 reply; 18+ messages in thread From: Jeff King @ 2016-01-28 23:53 UTC (permalink / raw) To: Eric Wong; +Cc: Junio C Hamano, git On Thu, Jan 28, 2016 at 10:51:23PM +0000, Eric Wong wrote: > While working in connect.c to perform non-blocking connections, > I noticed calling "git fetch -v" was not causing the progress > messages inside git_tcp_connect_sock to be emitted as I > expected. > > Looking at history, it seems connect_setup has never been called > with the verbose parameter. Since transport already has a > "verbose" field, use that field instead of another parameter > in connect_setup. That makes sense, but... > -static int connect_setup(struct transport *transport, int for_push, int verbose) > +static int connect_setup(struct transport *transport, int for_push) > { > struct git_transport_data *data = transport->data; > + int flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0; Do we want to trigger this only for "transport->verbose > 1"? Right now, "git fetch -v" gives us a verbose status table (i.e., includes up-to-date refs), but no more debugging than that. Should we reserve more debug-ish information like for "git fetch -vv"? -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-28 23:53 ` [PATCH] pass transport verbosity down to git_connect Jeff King @ 2016-01-29 0:38 ` Eric Wong 2016-01-29 3:19 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Eric Wong @ 2016-01-29 0:38 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git Jeff King <peff@peff.net> wrote: > On Thu, Jan 28, 2016 at 10:51:23PM +0000, Eric Wong wrote: > > -static int connect_setup(struct transport *transport, int for_push, int verbose) > > +static int connect_setup(struct transport *transport, int for_push) > > { > > struct git_transport_data *data = transport->data; > > + int flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0; > > Do we want to trigger this only for "transport->verbose > 1"? > > Right now, "git fetch -v" gives us a verbose status table (i.e., > includes up-to-date refs), but no more debugging than that. Should we > reserve more debug-ish information like for "git fetch -vv"? I'm not sure, I've never used "-v" at all in the past with fetch. On one hand, I suspect someone who looks up "-v" and uses it is likely wondering: "why is it so slow?" At least, that's what I did before resorting to strace :) On the other hand, I'm not sure if there's anything parsing the stderr out of "git fetch -v" right now. In that case, perhaps only changing "-vv" (and documenting it) is a better idea. I've always been of the opinion stderr is for humans and test suites, only; and not considered an interface somebody should be parsing. For reference, "curl -v" includes connection info which I rely on all the time. Junio: I'm leaning towards putting the test into t/t5570-git-daemon.sh to avoid potential port conflicts if that's OK with you. It doesn't seem like we have a lot of fetch tests on the git:// at all. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-29 0:38 ` Eric Wong @ 2016-01-29 3:19 ` Junio C Hamano 2016-01-29 3:47 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-01-29 3:19 UTC (permalink / raw) To: Eric Wong; +Cc: Jeff King, git Eric Wong <normalperson@yhbt.net> writes: > On the other hand, I'm not sure if there's anything parsing the stderr > out of "git fetch -v" right now. It would also affect end-user experience, depending on what new pieces of lines are emitted to the terminal. From "git fetch -v", I expect to see the transfer progress and the final listing of fetched and updated refs, and as long as the line "remote: Compressing objects" and the lines that follow do not get garbled, I would imagine it would be fine. ... remote: Compressing objects: 100% (1195/1195), done. remote: Total 1869 (delta 1551), reused 841 (delta 672) Receiving objects: 100% (1869/1869), 462.80 KiB | 0 bytes/s, done. Resolving deltas: 100% (1551/1551), completed with 335 local objects. From $over_there * branch pu -> FETCH_HEAD > In that case, perhaps only changing > "-vv" (and documenting it) is a better idea. I just reviewed the output that are protected by CONNECT_VERBOSE; they look somewhere between pure debugging aid (like the protocol dump that are shown by "fetch -vv") and progress display, and at least to me they are much closer to the latter than the former, in the sense that they are not _so_ annoying as the protocol dump that are clearly not meant for the end users, and that they say "I am looking up this host's address", "Now connecting to this host:port", etc. So, I personally find this addtional output not _too_ bad if we give it with "fetch -v" (not limiting to "fetch -vv"). I dunno. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-29 3:19 ` Junio C Hamano @ 2016-01-29 3:47 ` Jeff King 2016-01-29 17:34 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Jeff King @ 2016-01-29 3:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, git On Thu, Jan 28, 2016 at 07:19:30PM -0800, Junio C Hamano wrote: > I just reviewed the output that are protected by CONNECT_VERBOSE; > they look somewhere between pure debugging aid (like the protocol > dump that are shown by "fetch -vv") and progress display, and at > least to me they are much closer to the latter than the former, in > the sense that they are not _so_ annoying as the protocol dump that > are clearly not meant for the end users, and that they say "I am > looking up this host's address", "Now connecting to this host:port", > etc. > > So, I personally find this addtional output not _too_ bad if we give > it with "fetch -v" (not limiting to "fetch -vv"). Yeah, I do not feel that strongly, and am OK if it is attached to a single "-v". I don't think we make any promises about scraping stderr, so it is really just about end-user experience. It is mostly just my gut feeling on what I'd expect based on other parts of git (especially "fetch -vv" in other circumstances). -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-29 3:47 ` Jeff King @ 2016-01-29 17:34 ` Junio C Hamano 2016-01-29 17:41 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-01-29 17:34 UTC (permalink / raw) To: Jeff King; +Cc: Eric Wong, git Jeff King <peff@peff.net> writes: > On Thu, Jan 28, 2016 at 07:19:30PM -0800, Junio C Hamano wrote: > >> I just reviewed the output that are protected by CONNECT_VERBOSE; >> they look somewhere between pure debugging aid (like the protocol >> dump that are shown by "fetch -vv") and progress display, and at >> least to me they are much closer to the latter than the former, in >> the sense that they are not _so_ annoying as the protocol dump that >> are clearly not meant for the end users, and that they say "I am >> looking up this host's address", "Now connecting to this host:port", >> etc. >> >> So, I personally find this addtional output not _too_ bad if we give >> it with "fetch -v" (not limiting to "fetch -vv"). > > Yeah, I do not feel that strongly, and am OK if it is attached to a > single "-v". I don't think we make any promises about scraping stderr, > so it is really just about end-user experience. It is mostly just my > gut feeling on what I'd expect based on other parts of git (especially > "fetch -vv" in other circumstances). Yeah, after re-reading the messages in this thread, I realize that I missed the fact that you do consider these CONNECT_VERBOSE messages as debugging aid and from that point of view "fetch -v" that shows these messagse in addition to what you get from "fetch" may be a bad idea. But after inspecting what CONNECT_VERBOSE would add to the output, I am inclined to say that, especially if some of these steps can exhibit multi-second stalls, they fall more into the "progress indicator" (aka "do not worry, we are not stuck, be patient") category than "debugging aid" category. The former includes "remote: Counting objects..." that is shown by default (you need to give --quiet to squelch). So I think it is OK to show the CONNECT_VERBOSE messages with "fetch -v"; if somebody feels strongly that these messages should be shown unless --quiet is given, I might even be persuaded to agree, but I am not that somebody, so... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] pass transport verbosity down to git_connect 2016-01-29 17:34 ` Junio C Hamano @ 2016-01-29 17:41 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2016-01-29 17:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, git On Fri, Jan 29, 2016 at 09:34:27AM -0800, Junio C Hamano wrote: > Yeah, after re-reading the messages in this thread, I realize that I > missed the fact that you do consider these CONNECT_VERBOSE messages > as debugging aid and from that point of view "fetch -v" that shows > these messagse in addition to what you get from "fetch" may be a bad > idea. > > But after inspecting what CONNECT_VERBOSE would add to the output, I > am inclined to say that, especially if some of these steps can > exhibit multi-second stalls, they fall more into the "progress > indicator" (aka "do not worry, we are not stuck, be patient") > category than "debugging aid" category. OK, I can buy that line of reasoning (and I agree it implies showing with a single "-v"). -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-02-12 15:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-28 22:51 [PATCH] pass transport verbosity down to git_connect Eric Wong 2016-01-28 23:45 ` Junio C Hamano 2016-01-30 8:50 ` [PATCH v2] " Eric Wong 2016-01-30 13:13 ` [PATCH 2/1] support -4 and -6 switches for remote operations Eric Wong 2016-01-30 13:28 ` Eric Wong 2016-01-30 23:34 ` Torsten Bögershausen 2016-01-31 0:01 ` Eric Wong 2016-01-31 1:13 ` Jeff King 2016-02-03 4:09 ` [PATCH v2 " Eric Wong 2016-02-12 11:31 ` Eric Wong 2016-02-12 15:43 ` Torsten Bögershausen 2016-01-31 16:03 ` [PATCH " Torsten Bögershausen 2016-01-28 23:53 ` [PATCH] pass transport verbosity down to git_connect Jeff King 2016-01-29 0:38 ` Eric Wong 2016-01-29 3:19 ` Junio C Hamano 2016-01-29 3:47 ` Jeff King 2016-01-29 17:34 ` Junio C Hamano 2016-01-29 17:41 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.