diff mbox series

Config option to set the transport protocol version for network fetches

Message ID 20200917132047.GA14771@pflmari (mailing list archive)
State New, archived
Headers show
Series Config option to set the transport protocol version for network fetches | expand

Commit Message

Alex Riesen Sept. 17, 2020, 1:20 p.m. UTC 
Affecting the transfers initiated by fetch and push,
the option allows to control network operations similar
to --ipv4 and --ipv6 options.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
---

Jeff King, Wed, Sep 16, 2020 22:02:03 +0200:
> On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:
> 
> > Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:
> > > On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:
> > > > Sure! Thinking about it, I actually would have preferred to have both: a
> > > > config option and a command-line option. So that I can set --ipv4 in, say,
> > > > ~/.config/git/config file, but still have the option to try --ipv6 from time
> > > > to time to check if the network setup magically fixed itself.
> > > > 
> > > > What would the preferred name for that config option be? fetch.ipv?
> > > 
> > > It looks like we've got similar options for clone/pull (which are really
> > > fetch under the hood of course) and push. We have the "transfer.*"
> > > namespace which applies to both already. So maybe "transfer.ipversion"
> > > or something?
> > 
> > Something like this?
> 
> That's the right direction, but I think we'd want to make sure it
> impacted all of the spots that allow switching. "clone" on the fetching
> side, but probably also "push".
> 
> Each of those commands could learn the same config, but it might be
> easier to just enforce it in the transport layer. Something like this
> maybe (compiled but not tested):

I have merged the patches.

> A few notes:
> 
>   - it cheats a little by noting that command-line options can't get it
>     to "ALL". It might be a bit cleaner if we have an explicit
>     TRANSPORT_FAMILY_UNSET, and then resolve the default at look-up
>     time.
> 
>   - it probably needs more "if (family)" spots in each command, which is
>     unfortunate (which again might go away if "UNSET" is 0).

As this is still being discussed, and because I still imagine the
transport protocol family as a list of allowed protocols, this part
is not in the patch below.

>   - I waffled on the name. transfer.* is where we put things that apply
>     to both push/fetch, but it's usually more related to the git layer.
>     This is more of a core networking decision, and if we added other
>     network programs, I'd expect them to respect it. So maybe "core." is
>     a better namespace.

I agree.

 Documentation/config/core.txt |  7 +++++++
 builtin/fetch.c               |  3 ++-
 builtin/push.c                |  3 ++-
 transport.c                   | 19 +++++++++++++++++++
 4 files changed, 30 insertions(+), 2 deletions(-)

Comments

Alex Riesen Sept. 17, 2020, 1:26 p.m. UTC | #1 
Alex Riesen, Thu, Sep 17, 2020 15:20:47 +0200:
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 74619a9c03..dcb7db9799 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -626,3 +626,10 @@ core.abbrev::
>  	in your repository, which hopefully is enough for
>  	abbreviated object names to stay unique for some time.
>  	The minimum length is 4.
> +
> +core.ipversion::
> +	Limit the network operations to the specified version of the transport
> +	protocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or
> +	`all` to allow all protocols.

Eh. Option values are "ipv4" and "ipv6" indeed, not "4" and "6".

And I compiled and ran the code by now. Feels ok.
Jeff King Sept. 17, 2020, 1:31 p.m. UTC | #2 
On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:

> Affecting the transfers initiated by fetch and push,
> the option allows to control network operations similar
> to --ipv4 and --ipv6 options.
> 
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>

I think this misses some of the excellent suggestions from Junio
(naming, and the ability to override from the command line).

-Peff
Alex Riesen Sept. 17, 2020, 1:35 p.m. UTC | #3 
Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:
> On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:
> 
> > Affecting the transfers initiated by fetch and push,
> > the option allows to control network operations similar
> > to --ipv4 and --ipv6 options.
> > 
> > Suggested-by: Jeff King <peff@peff.net>
> > Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
> 
> I think this misses some of the excellent suggestions from Junio
> (naming, and the ability to override from the command line).

It does, sorry. Also the suggestions to the issue of consistently passing the
options to helper programs haven't been collected.
Haven't had the time yet.
Jeff King Sept. 17, 2020, 2:51 p.m. UTC | #4 
On Thu, Sep 17, 2020 at 03:35:25PM +0200, Alex Riesen wrote:

> Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:
> > On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:
> > 
> > > Affecting the transfers initiated by fetch and push,
> > > the option allows to control network operations similar
> > > to --ipv4 and --ipv6 options.
> > > 
> > > Suggested-by: Jeff King <peff@peff.net>
> > > Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
> > 
> > I think this misses some of the excellent suggestions from Junio
> > (naming, and the ability to override from the command line).
> 
> It does, sorry. Also the suggestions to the issue of consistently passing the
> options to helper programs haven't been collected.
> Haven't had the time yet.

No problem, and no rush. I just wanted to make sure those bits didn't
get overlooked.

-Peff
Alex Riesen Sept. 17, 2020, 3:17 p.m. UTC | #5 
Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:
> No problem, and no rush. I just wanted to make sure those bits didn't
> get overlooked.

I'll try and do my best :)
Alex Riesen Sept. 17, 2020, 4:05 p.m. UTC | #6 
Alex Riesen, Thu, Sep 17, 2020 15:20:47 +0200:
> diff --git a/transport.c b/transport.c
> index b41386eccb..e16c339f3e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -922,6 +922,23 @@ static struct transport_vtable builtin_smart_vtable = {
>  	disconnect_git
>  };
>  
> +static enum transport_family default_transport_family(void)
> +{
> +	static const char key[] = "core.ipversion";
> +	const char *v;
> +
> +	if (git_config_get_string_const(key, &v))

Sorry about that. git_config_get_string_tmp, indeed.
Junio C Hamano Dec. 22, 2020, 7:55 p.m. UTC | #7 
Alex Riesen <alexander.riesen@cetitec.com> writes:

> Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:
>> No problem, and no rush. I just wanted to make sure those bits didn't
>> get overlooked.
>
> I'll try and do my best :)

If I remember correctly, this discussion started as an introduction
of useful feature, but got stuck at the implementation phase of how
the feature is presented at the UI level after we had general
concensus on the design.

It has been about 3 months; has anything happened that I missed?  It
seems that I kept the thread-starter patch in the 'seen' branch, but
and haven't updated with a later attempt in the discussion.

Thanks.
Alex Riesen Jan. 7, 2021, 10:06 a.m. UTC | #8 
Junio C Hamano, Tue, Dec 22, 2020 20:55:25 +0100:
> Alex Riesen <alexander.riesen@cetitec.com> writes:
> > Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:
> >> No problem, and no rush. I just wanted to make sure those bits didn't
> >> get overlooked.
> >
> > I'll try and do my best :)
> 
> If I remember correctly, this discussion started as an introduction
> of useful feature, but got stuck at the implementation phase of how
> the feature is presented at the UI level after we had general
> concensus on the design.
> 
> It has been about 3 months; has anything happened that I missed?

No, nothing happened.

> It seems that I kept the thread-starter patch in the 'seen' branch,
> but and haven't updated with a later attempt in the discussion.

Sorry. I got very distracted. As my situation seems to persist,
I cannot promise to do something about it soon.

I keep the branch and the discussion close by, though. Just in case.
diff mbox series

Patch

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 74619a9c03..dcb7db9799 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -626,3 +626,10 @@  core.abbrev::
 	in your repository, which hopefully is enough for
 	abbreviated object names to stay unique for some time.
 	The minimum length is 4.
+
+core.ipversion::
+	Limit the network operations to the specified version of the transport
+	protocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or
+	`all` to allow all protocols.
+	See also linkgit:git-fetch[1] options `--ipv4` and `--ipv6`.
+	The default value is `all` to allow all protocols.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 447d28ac29..41f82d61d7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1248,7 +1248,8 @@  static struct transport *prepare_transport(struct remote *remote, int deepen)
 
 	transport = transport_get(remote, NULL);
 	transport_set_verbosity(transport, verbosity, progress);
-	transport->family = family;
+	if (family)
+		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 bc94078e72..f7a40b65cd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -343,7 +343,8 @@  static int push_with_options(struct transport *transport, struct refspec *rs,
 	char *anon_url = transport_anonymize_url(transport->url);
 
 	transport_set_verbosity(transport, verbosity, progress);
-	transport->family = family;
+	if (family)
+		transport->family = family;
 
 	if (receivepack)
 		transport_set_option(transport,
diff --git a/transport.c b/transport.c
index b41386eccb..e16c339f3e 100644
--- a/transport.c
+++ b/transport.c
@@ -922,6 +922,23 @@  static struct transport_vtable builtin_smart_vtable = {
 	disconnect_git
 };
 
+static enum transport_family default_transport_family(void)
+{
+	static const char key[] = "core.ipversion";
+	const char *v;
+
+	if (git_config_get_string_const(key, &v))
+		return TRANSPORT_FAMILY_ALL;
+	if (!strcmp(v, "all"))
+		return TRANSPORT_FAMILY_ALL;
+	if (!strcmp(v, "ipv4"))
+		return TRANSPORT_FAMILY_IPV4;
+	if (!strcmp(v, "ipv6"))
+		return TRANSPORT_FAMILY_IPV6;
+
+	die(_("%s: unknown value '%s'"), key, v);
+}
+
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	const char *helper;
@@ -951,6 +968,8 @@  struct transport *transport_get(struct remote *remote, const char *url)
 			helper = xstrndup(url, p - url);
 	}
 
+	ret->family = default_transport_family();
+
 	if (helper) {
 		transport_helper_init(ret, helper);
 	} else if (starts_with(url, "rsync:")) {