config: option transfer.ipversion to set transport protocol version for network fetches

Commit Message

Alex Riesen Sept. 15, 2020, 1:54 p.m. UTC

Affecting the transfers caused by git-fetch, the
option allows to control network operations similar
to --ipv4 and --ipv6 options.

Suggested-by: Jeff King <[e-mail]>
Signed-off-by: Alex Riesen <[e-mail]>
---

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?

 Documentation/config/transfer.txt |  7 +++++++
 builtin/fetch.c                   | 11 +++++++++++
 2 files changed, 18 insertions(+)

Comments

Jeff King Sept. 16, 2020, 8:02 p.m. UTC | #1

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a6d3268661..2f7a734eb2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1267,7 +1267,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 43e24bf1e5..92f81b414d 100644
--- a/transport.c
+++ b/transport.c
@@ -919,6 +919,20 @@ static struct transport_vtable builtin_smart_vtable = {
 	disconnect_git
 };
 
+enum transport_family default_transport_family(void)
+{
+	const char *v;
+
+	if (git_config_get_string_tmp("core.ipversion", &v))
+		return TRANSPORT_FAMILY_ALL;
+	else if (!strcmp(v, "ipv4"))
+		return TRANSPORT_FAMILY_IPV4;
+	else if (!strcmp(v, "ipv6"))
+		return TRANSPORT_FAMILY_IPV6;
+
+	die(_("invalid core.ipversion: %s"), v);
+}
+
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	const char *helper;
@@ -948,6 +962,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:")) {

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

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

-Peff

Junio C Hamano Sept. 16, 2020, 8:14 p.m. UTC | #2

Alex Riesen <[e-mail]> writes:

> Affecting the transfers caused by git-fetch, the
> option allows to control network operations similar
> to --ipv4 and --ipv6 options.
> ...
> Something like this?

Good start.

If I configure it to "4", I do not see a way to override it and say
"I don't care which one is used".  With the introduction of this
configuration variable, we'd need a bit more support on the command
line side.

How about introducing a new command line option

	--transfer-protocol-family=("any"|<protocol>)

where <protocol> is either "ipv4" or "ipv6" [*1*], and make existing
"--ipv4" a synonym for "--transfer-protocol-family=ipv4" and "--ipv6"
for "--transfer-protocol-family=ipv6"

With such an extended command line override, we can override
configured 

	[transfer]
		ipversion = 6

with "--transfer-protocol-family=any" from the command line.

Also, we should follow the usual "the last one wins" for a
configuration variable like this, which is *not* a multi-valued
variable.  So the config parsing would look more like this:

	if (!strcmp(k, "transfer.ipversion")) {
		if (!v)
			return config_error_nonbool("transfer.ipversion");
		if (!strcmp(v, "any"))
			family = 0;
		else if (!strcmp(v, "4") || !strcmp(v, "ipv4"))
			family = TRANSPORT_FAMILY_IPV4;
		else if (!strcmp(v, "6") || !strcmp(v, "ipv6"))
			family = TRANSPORT_FAMILY_IPV6;
		else
			return error("transfer.ipversion: unknown value '%s'", v);
	}

Would we regret to choose 'ipversion' as the variable name, by the
way?  On the command line side, --transfer-protocol-family=ipv4
makes it clear that we leave room to support protocols outside the
Internet protocol family, and existing --ipv4 is grandfathered in
by making it a synonym to --transfer-protocol-family=ipv4.  Calling
the variable "transfer.ipversion" and still allowing future protocols
outside the Internet protocol family is rather awkward.

Calling "transfer.protocolFamily" would not have such a problem,
though.



[Footnote]

*1* But leave a room to extend it in the future to a comma-separated
    list of them to allow something like "ipv6,ipv7,ipv8" (i.e. "not
    just 'any'---we want to say that 'ipv4' is not welcomed").


>  Documentation/config/transfer.txt |  7 +++++++
>  builtin/fetch.c                   | 11 +++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
> index f5b6245270..cc0e97fbb1 100644
> --- a/Documentation/config/transfer.txt
> +++ b/Documentation/config/transfer.txt
> @@ -69,3 +69,10 @@ transfer.unpackLimit::
>  	When `fetch.unpackLimit` or `receive.unpackLimit` are
>  	not set, the value of this variable is used instead.
>  	The default value is 100.
> +
> +transfer.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..da01c8f7b3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -118,6 +118,17 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "transfer.ipversion")) {
> +		if (!strcmp(v, "all"))
> +			;
> +		else if (!strcmp(v, "4"))
> +			family = TRANSPORT_FAMILY_IPV4;
> +		else if (!strcmp(v, "6"))
> +			family = TRANSPORT_FAMILY_IPV6;
> +		else
> +			die(_("transfer.ipversion can be only 4, 6, or any"));
> +		return 0;
> +	}
>  	return git_default_config(k, v, cb);
>  }

Jeff King Sept. 16, 2020, 8:18 p.m. UTC | #3

On Wed, Sep 16, 2020 at 01:14:08PM -0700, Junio C Hamano wrote:

> Alex Riesen <[e-mail]> writes:
> 
> > Affecting the transfers caused by git-fetch, the
> > option allows to control network operations similar
> > to --ipv4 and --ipv6 options.
> > ...
> > Something like this?
> 
> Good start.
> [...]

A lot of your comments apply to the "something like this" suggestion I
just posted, so I wanted to save a round-trip and say: yes, I agree with
all of your suggestions here.

Adding a command-line option for "all" is a good idea, but will probably
mean needing to add the "unset" sentinel value I mentioned in the other
email.

> Would we regret to choose 'ipversion' as the variable name, by the
> way?  On the command line side, --transfer-protocol-family=ipv4
> makes it clear that we leave room to support protocols outside the
> Internet protocol family, and existing --ipv4 is grandfathered in
> by making it a synonym to --transfer-protocol-family=ipv4.  Calling
> the variable "transfer.ipversion" and still allowing future protocols
> outside the Internet protocol family is rather awkward.
> 
> Calling "transfer.protocolFamily" would not have such a problem,
> though.

I agree that's a better name. I'm still on the fence about "transfer"
versus "core".

-Peff

Junio C Hamano Sept. 16, 2020, 9:35 p.m. UTC | #4

Junio C Hamano <[e-mail]> writes:

> Also, we should follow the usual "the last one wins" for a
> configuration variable like this, which is *not* a multi-valued
> variable.  So the config parsing would look more like this:
>
> 	if (!strcmp(k, "transfer.ipversion")) {
> 		if (!v)
> 			return config_error_nonbool("transfer.ipversion");
> 		if (!strcmp(v, "any"))
> 			family = 0;
> 		else if (!strcmp(v, "4") || !strcmp(v, "ipv4"))
> 			family = TRANSPORT_FAMILY_IPV4;
> 		else if (!strcmp(v, "6") || !strcmp(v, "ipv6"))
> 			family = TRANSPORT_FAMILY_IPV6;
> 		else
> 			return error("transfer.ipversion: unknown value '%s'", v);
> 	}
>
> Would we regret to choose 'ipversion' as the variable name, by the
> way?  On the command line side, --transfer-protocol-family=ipv4
> makes it clear that we leave room to support protocols outside the
> Internet protocol family, and existing --ipv4 is grandfathered in
> by making it a synonym to --transfer-protocol-family=ipv4.  Calling
> the variable "transfer.ipversion" and still allowing future protocols
> outside the Internet protocol family is rather awkward.
>
> Calling "transfer.protocolFamily" would not have such a problem,
> though.

In case it wasn't clear, I consider the current TRANSPORT_FAMILY_ALL
a misnomer.  It's not like specifying "all" will make us use both
ipv4 and ipv6 at the same time0---it just indicates our lack of
preference, i.e. "any transport protocol family would do".

I mention this because this topic starts to expose that 'lack of
preference' to the end user; I do not think we want to use "all"
as the potential value for the command line option or the
configuration variable.

Thanks.

Junio C Hamano Sept. 16, 2020, 10:50 p.m. UTC | #5

Jeff King <[e-mail]> writes:

> Adding a command-line option for "all" is a good idea, but will probably
> mean needing to add the "unset" sentinel value I mentioned in the other
> email.

Sorry, I do not quite follow.  I thought that assigning the
(misnamed --- see other mail) ALL to the "family" variable would be
sufficient?

    enum transport_family {
            TRANSPORT_FAMILY_ALL = 0,
            TRANSPORT_FAMILY_IPV4,
            TRANSPORT_FAMILY_IPV6
    };

Junio C Hamano Sept. 16, 2020, 10:52 p.m. UTC | #6

Junio C Hamano <[e-mail]> writes:

> Jeff King <[e-mail]> writes:
>
>> Adding a command-line option for "all" is a good idea, but will probably
>> mean needing to add the "unset" sentinel value I mentioned in the other
>> email.
>
> Sorry, I do not quite follow.  I thought that assigning the
> (misnamed --- see other mail) ALL to the "family" variable would be
> sufficient?
>
>     enum transport_family {
>             TRANSPORT_FAMILY_ALL = 0,
>             TRANSPORT_FAMILY_IPV4,
>             TRANSPORT_FAMILY_IPV6
>     };

Ah, I see.  We want a way to tell "nobody has set it from the command
line or the config" and "we were explicitly told to accept any"
apart.

But wouldn't the usual "read config first and then override from the
command line" handle that without "not yet set" value?  I thought we
by default accept any.

Jeff King Sept. 17, 2020, 12:48 a.m. UTC | #7

On Wed, Sep 16, 2020 at 03:52:16PM -0700, Junio C Hamano wrote:

> >> Adding a command-line option for "all" is a good idea, but will probably
> >> mean needing to add the "unset" sentinel value I mentioned in the other
> >> email.
> >
> > Sorry, I do not quite follow.  I thought that assigning the
> > (misnamed --- see other mail) ALL to the "family" variable would be
> > sufficient?
> >
> >     enum transport_family {
> >             TRANSPORT_FAMILY_ALL = 0,
> >             TRANSPORT_FAMILY_IPV4,
> >             TRANSPORT_FAMILY_IPV6
> >     };
> 
> Ah, I see.  We want a way to tell "nobody has set it from the command
> line or the config" and "we were explicitly told to accept any"
> apart.
> 
> But wouldn't the usual "read config first and then override from the
> command line" handle that without "not yet set" value?  I thought we
> by default accept any.

It would, if each individual program does it in that order. But that
means every caller of the transport code needs to be updated to handle
the config. That might not be that bad (after all, they have to take
--ipv6 etc, options, and I guess they'd need a new "any" option).

My suggestion elsewhere was to have an "unset" value, and then resolve
it at the time-of-use, something like:

diff --git a/transport.c b/transport.c
index 43e24bf1e5..6414a847ae 100644
--- a/transport.c
+++ b/transport.c
@@ -248,6 +248,9 @@ static int connect_setup(struct transport *transport, int for_push)
 	if (data->conn)
 		return 0;
 
+	if (transport->family == TRANSPORT_FAMILY_UNSET)
+		transport->family = transport_family_config;
+
 	switch (transport->family) {
 	case TRANSPORT_FAMILY_ALL: break;
 	case TRANSPORT_FAMILY_IPV4: flags |= CONNECT_IPV4; break;

but I am happy either way as long as the code does the right thing.

-Peff

Junio C Hamano Sept. 17, 2020, 12:57 a.m. UTC | #8

Jeff King <[e-mail]> writes:

> My suggestion elsewhere was to have an "unset" value, and then resolve
> it at the time-of-use, something like:
>
> diff --git a/transport.c b/transport.c
> index 43e24bf1e5..6414a847ae 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -248,6 +248,9 @@ static int connect_setup(struct transport *transport, int for_push)
>  	if (data->conn)
>  		return 0;
>  
> +	if (transport->family == TRANSPORT_FAMILY_UNSET)
> +		transport->family = transport_family_config;
> +

Ah, OK, if we want to configure it the other way around, yes, we
need "the command line didn't say any" value.  The context of the
"elsewhere" discussion was wnat I was missing (I'd happily blame
vger for not delivering mails in order ;-).

Alex Riesen Sept. 17, 2020, 8:04 a.m. UTC | #9

Junio C Hamano, Wed, Sep 16, 2020 22:14:08 +0200:
> How about introducing a new command line option
> 
> 	--transfer-protocol-family=("any"|<protocol>)
> 
> where <protocol> is either "ipv4" or "ipv6" [*1*], and make existing
> "--ipv4" a synonym for "--transfer-protocol-family=ipv4" and "--ipv6"
> for "--transfer-protocol-family=ipv6"
> 
> With such an extended command line override, we can override
> configured 
> 
> 	[transfer]
> 		ipversion = 6
> 

So the config option starts looking like "transfer.protocols" and multi-value?
With the command-line option named "--transfer-protocol=", allowing multiple
specification, and with "any" value taking precedence if specified anywhere?

Alex Riesen Sept. 17, 2020, 8:07 a.m. UTC | #10

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

Ah sorry. Missed that push case.

Alex Riesen Sept. 17, 2020, 8:18 a.m. UTC | #11

Junio C Hamano, Wed, Sep 16, 2020 22:14:08 +0200:
> Alex Riesen <[e-mail]> writes:
> 
> > Affecting the transfers caused by git-fetch, the
> > option allows to control network operations similar
> > to --ipv4 and --ipv6 options.
> > ...
> > Something like this?
> 
> Also, we should follow the usual "the last one wins" for a
> configuration variable like this, which is *not* a multi-valued
> variable. ...
...
> [Footnote]
> 
> *1* But leave a room to extend it in the future to a comma-separated
>     list of them to allow something like "ipv6,ipv7,ipv8" (i.e. "not
>     just 'any'---we want to say that 'ipv4' is not welcomed").

I think this footnote is the best description of this option. From what
I gathered, it looks like really a list of protocols the networking code
is allowed to try to reach the remote.

The above is orthogonal to how it given on the command-line: there it can be
"unset". It is just not "unset" for the networking code, rather reset to the
default list of protocols.

Alex Riesen Sept. 17, 2020, 2:02 p.m. UTC | #12

Junio C Hamano, Wed, Sep 16, 2020 23:35:23 +0200:
> Junio C Hamano <[e-mail]> writes:
> > Would we regret to choose 'ipversion' as the variable name, by the
> > way?  On the command line side, --transfer-protocol-family=ipv4
> > makes it clear that we leave room to support protocols outside the
> > Internet protocol family, and existing --ipv4 is grandfathered in
> > by making it a synonym to --transfer-protocol-family=ipv4.  Calling
> > the variable "transfer.ipversion" and still allowing future protocols
> > outside the Internet protocol family is rather awkward.
> >
> > Calling "transfer.protocolFamily" would not have such a problem,
> > though.
> 
> In case it wasn't clear, I consider the current TRANSPORT_FAMILY_ALL
> a misnomer.  It's not like specifying "all" will make us use both
> ipv4 and ipv6 at the same time0---it just indicates our lack of
> preference, i.e. "any transport protocol family would do".
> 
> I mention this because this topic starts to expose that 'lack of
> preference' to the end user; I do not think we want to use "all"
> as the potential value for the command line option or the
> configuration variable.

If the configuration variable is allowed to be set to that "lack of
preference" value, we kind of have a command line option for it:

    git -c transfer.protocolFamily=any fetch ...

Junio C Hamano Sept. 17, 2020, 10:31 p.m. UTC | #13

Alex Riesen <[e-mail]> writes:

> If the configuration variable is allowed to be set to that "lack of
> preference" value, we kind of have a command line option for it:
>
>     git -c transfer.protocolFamily=any fetch ...

Yes.

But we typically do not count that as being able to override from
the command line.  If "git fetch --ipv4" will defeat the configured
"transfer.protoclFamily=ipv6", the users will expect there is some
way to do the same and say they accept any protocol family to be
used.

Thanks.

Alex Riesen Sept. 18, 2020, 7:16 a.m. UTC | #14

Junio C Hamano, Fri, Sep 18, 2020 00:31:58 +0200:
> Alex Riesen <[e-mail]> writes:
> 
> > If the configuration variable is allowed to be set to that "lack of
> > preference" value, we kind of have a command line option for it:
> >
> >     git -c transfer.protocolFamily=any fetch ...
> 
> Yes.
> 
> But we typically do not count that as being able to override from
> the command line.  If "git fetch --ipv4" will defeat the configured
> "transfer.protoclFamily=ipv6", the users will expect there is some
> way to do the same and say they accept any protocol family to be
> used.

Makes sense. I even wondered myself why there is no way to override
an --ipv4/--ipv6 in command-line back to "any" with an option added
after it:

    $ cat my-fetch
    #!/bin/sh
    git fetch --ipv4 "$@"

    $ ./my-fetch --ipv46

But how about making the command-line and config option a list?
(renaming it to ipversion, as elsewhere in the discussion)

    git fetch --ipversions=ipv6,ipv8

Given multiple times, the last option wins, as usual:

    $ cat my-fetch
    #!/bin/sh
    git fetch --ipversions=ipv4 "$@"

    $ ./my-fetch --ipversions=all

BTW, transport.c already converts transport->family to bit-flags in
connect_setup.

Junio C Hamano Sept. 18, 2020, 4:37 p.m. UTC | #15

Alex Riesen <[e-mail]> writes:

>     git fetch --ipversions=ipv6,ipv8
>
> Given multiple times, the last option wins, as usual:

Just a clarification on "the last option wins".

You do not mean "I said v6 earlier but no, I want v8", with the
above.  What you mean is that

>
>     $ cat my-fetch
>     #!/bin/sh
>     git fetch --ipversions=ipv4 "$@"
>
>     $ ./my-fetch --ipversions=all

the argument given to 'my-fetch' overrides what is hardcoded in
'my-fetch', i.e. "I said v4, but I take it back; I want to accept
any".

I find the above sensible.

> BTW, transport.c already converts transport->family to bit-flags in
> connect_setup.

Yes, that is why I suggested the "list of acceptable choices"
approach as a direction to go in the future, primarily to limit the
scope of this current work.  I do not mind it if you want to bite
the whole piece right now, though.

By the way, I have a mild preference to call the option after the
phrase "protocol-family", without "IP", so that we won't be limited
to Internet protocols.  IOW, --ipversions is a bad name for the new
commnad line option in my mind.

As I said elsewhere, I also think TRANSPORT_FAMILY_ALL is a
misnomer.  When it is specified, we don't use all the available ones
at the same time.  What it says is that we accept use of any
protocol families that are supported.  It is OK to use ALL in the
CPP macro as it is merely an internal implementation detail, but if
we are going to expose it to end users as one of the choices, I'd
prefer to use 'any', and not 'all', as the value for the new command
line option.

Thanks.

Alex Riesen Sept. 21, 2020, 4:39 p.m. UTC | #16

Junio C Hamano, Fri, Sep 18, 2020 18:37:30 +0200:
> Alex Riesen <[e-mail]> writes:
> 
> >     git fetch --ipversions=ipv6,ipv8
> >
> > Given multiple times, the last option wins, as usual:
> 
> Just a clarification on "the last option wins".
> 
> You do not mean "I said v6 earlier but no, I want v8", with the
> above.  What you mean is that
> 
> >
> >     $ cat my-fetch
> >     #!/bin/sh
> >     git fetch --ipversions=ipv4 "$@"
> >
> >     $ ./my-fetch --ipversions=all
> 
> the argument given to 'my-fetch' overrides what is hardcoded in
> 'my-fetch', i.e. "I said v4, but I take it back; I want to accept
> any".

Absolutely correct.

> I find the above sensible.

... And is precisely my intention.

> > BTW, transport.c already converts transport->family to bit-flags in
> > connect_setup.
> 
> Yes, that is why I suggested the "list of acceptable choices"
> approach as a direction to go in the future, primarily to limit the
> scope of this current work.  I do not mind it if you want to bite
> the whole piece right now, though.

I shall try, I think.

> By the way, I have a mild preference to call the option after the
> phrase "protocol-family", without "IP", so that we won't be limited
> to Internet protocols.  IOW, --ipversions is a bad name for the new
> commnad line option in my mind.

I have nothing against protocol-family, with or without "transport-".
I just didn't want to ... over-generalize it prematurely: currently,
the transport family is very fixed on IP on many levels.

> As I said elsewhere, I also think TRANSPORT_FAMILY_ALL is a
> misnomer.  When it is specified, we don't use all the available ones
> at the same time.  What it says is that we accept use of any
> protocol families that are supported.  It is OK to use ALL in the
> CPP macro as it is merely an internal implementation detail, but if
> we are going to expose it to end users as one of the choices, I'd
> prefer to use 'any', and not 'all', as the value for the new command
> line option.

Noted.

Regards,
Alex

Jeff King Sept. 22, 2020, 5:03 a.m. UTC | #17

On Mon, Sep 21, 2020 at 06:39:01PM +0200, Alex Riesen wrote:

> > By the way, I have a mild preference to call the option after the
> > phrase "protocol-family", without "IP", so that we won't be limited
> > to Internet protocols.  IOW, --ipversions is a bad name for the new
> > commnad line option in my mind.
> 
> I have nothing against protocol-family, with or without "transport-".
> I just didn't want to ... over-generalize it prematurely: currently,
> the transport family is very fixed on IP on many levels.

I'll echo Junio's mild preference. I like it also because
"--ipversion=6" feels too terse in the argument, but "--ipversion=ipv6"
feels redundant.  Saying "--protocol-family=ipv6" is more characters,
but strikes me as more clear. (Or "transport-family" or whatever).

-Peff

Patch

diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
index f5b6245270..cc0e97fbb1 100644
--- a/Documentation/config/transfer.txt
+++ b/Documentation/config/transfer.txt
@@ -69,3 +69,10 @@  transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
 	The default value is 100.
+
+transfer.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..da01c8f7b3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -118,6 +118,17 @@  static int git_fetch_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "transfer.ipversion")) {
+		if (!strcmp(v, "all"))
+			;
+		else if (!strcmp(v, "4"))
+			family = TRANSPORT_FAMILY_IPV4;
+		else if (!strcmp(v, "6"))
+			family = TRANSPORT_FAMILY_IPV6;
+		else
+			die(_("transfer.ipversion can be only 4, 6, or any"));
+		return 0;
+	}
 	return git_default_config(k, v, cb);
 }