sub-fetches discard --ipv4|6 option

Hi everyone,

I have a slight problem with IPv6 configuration in my local network (connect
works but transfers do not) and would like to temporarily disable use of the
transport for a series of fetches. The fetches are all done from within a
script to which I can pass options for "git fetch" commands in its
command-line. The options will be appended to the fetch commands, i.e.:

    git fetch <hard-coded-script-options> --ipv4

Unfortunately, it only worked for the fetches which didn't use --all or
--multiple. After a light searching, I failed to find an explanation as to
why --all|--multiple are handled so inconsistently with single remote fetches
and added the options (similar to --force or --keep) to the argument list for
sub-fetches:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 82ac4be8a5..5e06c07106 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)
 		argv_array_push(argv, "-v");
 	else if (verbosity < 0)
 		argv_array_push(argv, "-q");
+	if (family == TRANSPORT_FAMILY_IPV4)
+		argv_array_push(argv, "--ipv4");
+	else if (family == TRANSPORT_FAMILY_IPV6)
+		argv_array_push(argv, "--ipv6");
 
 }
 

Am I missing something obvious?

Regards,
Alex

On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:

I don't think so. When we're starting fetch sub-processes, some options
will make sense to pass along and some won't. The parent has to either
pass all options and omit some, or explicitly pass ones it knows are
useful. It looks like the code chooses the latter, but these particular
options never got added (and it seems like they should be, as they are
only useful to the child fetch processes that actually touch the
network).

So your patch above looks quite sensible (modulo useful bits like a
signoff and maybe a test, though I guess the impact of those options
is probably hard to cover in our tests).

It is rather unfortunate that anybody adding new fetch options needs to
remember to (maybe) add them to add_options_to_argv() themselves.

Also, regarding these two specific options, it sounds like you'd want
them set for all fetches during the time your IPv6 setup is broken. In
which case I think a config option might have served you better. So that
might be something worth implementing (though either way I think the fix
above is worth doing independently).

-Peff

Jeff King, Mon, Sep 14, 2020 21:49:51 +0200:

On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:

quoted

Unfortunately, it only worked for the fetches which didn't use --all or
--multiple. After a light searching, I failed to find an explanation as to
why --all|--multiple are handled so inconsistently with single remote fetches
and added the options (similar to --force or --keep) to the argument list for
sub-fetches: ...
 
Am I missing something obvious?

I don't think so. When we're starting fetch sub-processes, some options
will make sense to pass along and some won't. The parent has to either
pass all options and omit some, or explicitly pass ones it knows are
useful. It looks like the code chooses the latter, but these particular
options never got added (and it seems like they should be, as they are
only useful to the child fetch processes that actually touch the
network).

So your patch above looks quite sensible (modulo useful bits like a
signoff and maybe a test, though I guess the impact of those options
is probably hard to cover in our tests).

I tried to come up with one, but (aside from rather pointless checking of
option presence in the trace output) failed to.

Or may be precisely this could be the point of the test: just do a fetch with
all options we intend to pass down to sub-fetches and check that they are
indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?

It is rather unfortunate that anybody adding new fetch options needs to
remember to (maybe) add them to add_options_to_argv() themselves.

Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy
the options with a special marker if they were provided?
And use the word "recursive" in help text as the marker :)

Also, regarding these two specific options, it sounds like you'd want
them set for all fetches during the time your IPv6 setup is broken. In
which case I think a config option might have served you better. So that
might be something worth implementing (though either way I think the fix
above is worth doing independently).

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?

I'll be sending the first change reformatted as patch shortly. Just in case.

The options indicate user intent for the whole fetch operation, and
ignoring them in sub-fetches is quite unexpected when, for instance,
it is intended to limit all of the communication to a specific transport
protocol for some reason.

Signed-off-by: Alex Riesen <redacted>
---

 builtin/fetch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 82ac4be8a5..447d28ac29 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)
 		argv_array_push(argv, "-v");
 	else if (verbosity < 0)
 		argv_array_push(argv, "-q");
+	if (family == TRANSPORT_FAMILY_IPV4)
+		argv_array_push(argv, "--ipv4");
+	else if (family == TRANSPORT_FAMILY_IPV6)
+		argv_array_push(argv, "--ipv6");
 
 }
 

-- 
2.28.0.21.g09e033b31f

On Tue, Sep 15, 2020 at 01:54:07PM +0200, Alex Riesen wrote:

The options indicate user intent for the whole fetch operation, and
ignoring them in sub-fetches is quite unexpected when, for instance,
it is intended to limit all of the communication to a specific transport
protocol for some reason.

Signed-off-by: Alex Riesen <redacted>
---

Regardless of whether we move forward with the parse-options flag or
config discussed in the other thread, I think this is an obvious
improvement that we should take in the meantime.

-Peff

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

So your patch above looks quite sensible (modulo useful bits like a
signoff and maybe a test, though I guess the impact of those options
is probably hard to cover in our tests).

I tried to come up with one, but (aside from rather pointless checking of
option presence in the trace output) failed to.

Or may be precisely this could be the point of the test: just do a fetch with
all options we intend to pass down to sub-fetches and check that they are
indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?

Unfortunately I don't think that accomplishes much, since the main bug
we're worried about is missing options. And it would require somebody
adding the new options to the test, at which point you could just assume
they would add it to add_options_to_argv().

Though I guess we can automatically get the list of options these days.
So perhaps something like:

  subopts=
  for opt in $(git fetch --git-completion-helper)
  do
        case "$opt" in
        # options that we know do not go to sub-fetches
        --all|--jobs|etc...)
                ;;
	# try/match only the positive versions
	--no-*)
	        ;;
	# give a fake value for options with values
	*=)
                subopts="$subopts ${opt}1"
		;;
	# and pass through any boolean options
	*)
                subopts="$subopts $opt"
		;;
        esac
  done
  GIT_TRACE=$PWD/trace.out git fetch --all $subopts
  perl -lne '
    BEGIN { @want = @ARGV; @ARGV = () }
    /run_command: git fetch (.*)/ and $seen{$_}++ for split(/ /, $1);
    END { print for grep { !$seen{$_} } @want }
  ' <trace.out -- $subopts

Except that doesn't quite work, because the parent fetch will complain
about nonsense values (e.g., --filter=1). So it would probably need a
bit more manual intelligence to cover those options. It looks like some
options are mutually exclusive, too (--deepen/--depth), so maybe we'd
need to run an individual "fetch --all" for each option.

I dunno. It's getting pretty complicated. :)

quoted

It is rather unfortunate that anybody adding new fetch options needs to
remember to (maybe) add them to add_options_to_argv() themselves.

Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy
the options with a special marker if they were provided?
And use the word "recursive" in help text as the marker :)

Yeah, that would solve the duplication problem. We could probably add a
"recursive" bit to the parse-options flag variable. Even if
parse-options itself doesn't use it, it could be a convenience for
callers like this one. It is a little inconvenient to set flags there,
just because it usually means ditching our wrapper macros in favor of a
raw struct declaration.

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?

-Peff

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

quoted

So your patch above looks quite sensible (modulo useful bits like a
signoff and maybe a test, though I guess the impact of those options
is probably hard to cover in our tests).

I tried to come up with one, but (aside from rather pointless checking of
option presence in the trace output) failed to.

Or may be precisely this could be the point of the test: just do a fetch with
all options we intend to pass down to sub-fetches and check that they are
indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?

Unfortunately I don't think that accomplishes much, since the main bug
we're worried about is missing options. And it would require somebody
adding the new options to the test, at which point you could just assume
they would add it to add_options_to_argv().

Though I guess we can automatically get the list of options these days.
So perhaps something like:

  subopts=
  for opt in $(git fetch --git-completion-helper)

...

Except that doesn't quite work, because the parent fetch will complain
about nonsense values (e.g., --filter=1). So it would probably need a
bit more manual intelligence to cover those options. It looks like some
options are mutually exclusive, too (--deepen/--depth), so maybe we'd
need to run an individual "fetch --all" for each option.

I dunno. It's getting pretty complicated. :)

It does :-( And the manual parts will require perpetual maintenance.
Not doing that yet than.

quoted

quoted

It is rather unfortunate that anybody adding new fetch options needs to
remember to (maybe) add them to add_options_to_argv() themselves.

Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy
the options with a special marker if they were provided?
And use the word "recursive" in help text as the marker :)

Yeah, that would solve the duplication problem. We could probably add a
"recursive" bit to the parse-options flag variable. Even if
parse-options itself doesn't use it, it could be a convenience for
callers like this one. It is a little inconvenient to set flags there,
just because it usually means ditching our wrapper macros in favor of a
raw struct declaration.

Or extend the list of wrappers with _REC(URSIVE) macros

Regards,
Alex

On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:

quoted

Yeah, that would solve the duplication problem. We could probably add a
"recursive" bit to the parse-options flag variable. Even if
parse-options itself doesn't use it, it could be a convenience for
callers like this one. It is a little inconvenient to set flags there,
just because it usually means ditching our wrapper macros in favor of a
raw struct declaration.

Or extend the list of wrappers with _REC(URSIVE) macros

If you go that route, we have some "_F" macros that take flags. Probably
would make sense to add it more consistently, which lets you convert:

  OPT_BOOL('f', "foo", &foo, "the foo option");

into:

  OPT_BOOL_F('f', "foo", &foo, "the foo option", PARSE_OPT_RECURSIVE);

but could also be used for other flags.

-Peff

Jeff King, Tue, Sep 15, 2020 17:27:30 +0200:

On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:

quoted

quoted

Yeah, that would solve the duplication problem. We could probably add a
"recursive" bit to the parse-options flag variable. Even if
parse-options itself doesn't use it, it could be a convenience for
callers like this one. It is a little inconvenient to set flags there,
just because it usually means ditching our wrapper macros in favor of a
raw struct declaration.

Or extend the list of wrappers with _REC(URSIVE) macros

If you go that route, we have some "_F" macros that take flags. Probably
would make sense to add it more consistently, which lets you convert:

  OPT_BOOL('f', "foo", &foo, "the foo option");

into:

  OPT_BOOL_F('f', "foo", &foo, "the foo option", PARSE_OPT_RECURSIVE);

but could also be used for other flags.

This part (marking of the options) was easy. What's left is finding out if an
option was actually specified in the command-line. The ...options[] arrays are
not update by parse_options() with what was given, are they?

Maybe extend struct option with a field to store given command-line argument
(as it was specified) and parse_options() will update the field if
PARSE_OPT_RECURSIVE is present in .flags?
Is it allowed for parse_options() to modify the options array?
Or is it possible to use something in parse-options.h API to note the
arguments somewhere while they are parse? I mean, there are
parse_options_start/step/end, can cmd_fetch argument parsing use those
so that the options marked recursive can be saved for sub-fetches?

On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:

quoted

If you go that route, we have some "_F" macros that take flags. Probably
would make sense to add it more consistently, which lets you convert:

  OPT_BOOL('f', "foo", &foo, "the foo option");

into:

  OPT_BOOL_F('f', "foo", &foo, "the foo option", PARSE_OPT_RECURSIVE);

but could also be used for other flags.

This part (marking of the options) was easy. What's left is finding out if an
option was actually specified in the command-line. The ...options[] arrays are
not update by parse_options() with what was given, are they?

Oh right. Having the list of options is not that helpful because
add_options_argv() is actually working off the parsed data in individual
variables. Sorry for leading you in a (maybe) wrong direction.

I think this approach would have to be coupled with some mechanism for
looking over the original list of options (either saving the original
argv before parsing, or teaching parse-options the kind of two-pass
"don't parse these the first time" mechanism discussed elsewhere in the
thread).

Maybe extend struct option with a field to store given command-line argument
(as it was specified) and parse_options() will update the field if
PARSE_OPT_RECURSIVE is present in .flags?
Is it allowed for parse_options() to modify the options array?

No, we take the options array as a const pointer, so many callers would
likely need to be updated to handle that. Plus it's possible some may
actually re-use the array multiple times in some cases.

Or is it possible to use something in parse-options.h API to note the
arguments somewhere while they are parse? I mean, there are
parse_options_start/step/end, can cmd_fetch argument parsing use those
so that the options marked recursive can be saved for sub-fetches?

Possibly the step-wise parsing could help. But I think it might be
easier to just let parse_options() save a copy of parsed options. And
then our PARSE_OPT_RECURSIVE really becomes PARSE_OPT_SAVE or similar,
which would cause parse-options to save the original option (and any
value argument) in its original form.

There's one slight complication, which is how the array of saved options
gets communicated back to the caller. Leaving them in the original argv
probably isn't a good idea (because the caller relies on it having
options removed in order to find the non-option arguments).

Adding a new strvec pointer to parse_options() works, but means updating
all of the callers, most of which will pass NULL. Possibly the existing
"flags" parameter to parse_options() could grow into a struct. That
requires modifying each caller, but at least solves the problem once and
for all.

Another option is to stick it into parse_opt_ctx_t. That's used only be
step-wise callers, of which there are very few.

-Peff

Jeff King, Wed, Sep 16, 2020 18:32:18 +0200:

On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:

quoted

quoted

If you go that route, we have some "_F" macros that take flags. Probably
would make sense to add it more consistently, which lets you convert:

  OPT_BOOL('f', "foo", &foo, "the foo option");

into:

  OPT_BOOL_F('f', "foo", &foo, "the foo option", PARSE_OPT_RECURSIVE);

but could also be used for other flags.

This part (marking of the options) was easy. What's left is finding out if an
option was actually specified in the command-line. The ...options[] arrays are
not update by parse_options() with what was given, are they?

Oh right. Having the list of options is not that helpful because
add_options_argv() is actually working off the parsed data in individual
variables. Sorry for leading you in a (maybe) wrong direction.

...

quoted

Or is it possible to use something in parse-options.h API to note the
arguments somewhere while they are parsed? I mean, there are
parse_options_start/step/end, can cmd_fetch argument parsing use those
so that the options marked recursive can be saved for sub-fetches?

Possibly the step-wise parsing could help. But I think it might be
easier to just let parse_options() save a copy of parsed options. And
then our PARSE_OPT_RECURSIVE really becomes PARSE_OPT_SAVE or similar,
which would cause parse-options to save the original option (and any
value argument) in its original form.

There's one slight complication, which is how the array of saved options
gets communicated back to the caller. Leaving them in the original argv
probably isn't a good idea (because the caller relies on it having
options removed in order to find the non-option arguments).

Adding a new strvec pointer to parse_options() works, but means updating
all of the callers, most of which will pass NULL. Possibly the existing
"flags" parameter to parse_options() could grow into a struct. That
requires modifying each caller, but at least solves the problem once and
for all.

With such complication a step-wise parsing sounds easier, given that at the
moment there is only one user for the feature. Are there *existing* callers
of parse_options with similar requirements?

I feel that doing this kind of selection work in parse_options is an overkill:
if it is specific for just this use case, the implementation might be more
complex than necessary, while profiting just one caller.

Another option is to stick it into parse_opt_ctx_t. That's used only be
step-wise callers, of which there are very few.

Does that mean that currently there is no way to find out which option
corresponds to the last parsed command-line argument after a call to
parse_options_step? Which in turn makes the marking of recursive options
inaccessible to step-wise command line parsing code, right?

On Thu, Sep 17, 2020 at 04:33:39PM +0200, Alex Riesen wrote:

quoted

Adding a new strvec pointer to parse_options() works, but means updating
all of the callers, most of which will pass NULL. Possibly the existing
"flags" parameter to parse_options() could grow into a struct. That
requires modifying each caller, but at least solves the problem once and
for all.

With such complication a step-wise parsing sounds easier, given that at the
moment there is only one user for the feature. Are there *existing* callers
of parse_options with similar requirements?

I don't know offhand. I'd suspect some of the command which take
--recurse-submodules do something similar, but the number of steps
between the main command the submodule argv may make it awkward to use
a parse-options solution. I don't think we have a "push to each of these
remotes" option the way we do for fetch.

I feel that doing this kind of selection work in parse_options is an overkill:
if it is specific for just this use case, the implementation might be more
complex than necessary, while profiting just one caller.

Yeah, I agree it's complex, and I'm happy with simpler solutions (or
just fixing these ones as you did and punting on it for now).

quoted

Another option is to stick it into parse_opt_ctx_t. That's used only be
step-wise callers, of which there are very few.

Does that mean that currently there is no way to find out which option
corresponds to the last parsed command-line argument after a call to
parse_options_step? Which in turn makes the marking of recursive options
inaccessible to step-wise command line parsing code, right?

I'm not super familiar with the internals of parse-options, but it
doesn't look like it. Each step consumes an argv and matches it to a
"struct option", but I don't think you get to know which struct it was
matched to. It would be reasonable for it to keep a pointer in the
parse_opt_ctx_t (and of course you'd need some bit in the option struct
itself to say "I am a recursive option").

-Peff

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

Suggested-by: Jeff King <redacted>
Signed-off-by: Alex Riesen <redacted>
---

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

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(+)

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);
 }
 

-- 
2.28.0.21.g178b32a6fd.dirty

On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:

quoted

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

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

Jeff King, Wed, Sep 16, 2020 22:02:03 +0200:

On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:

quoted

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:

quoted

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

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.

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 <redacted>
Signed-off-by: Alex Riesen <redacted>
---

Jeff King, Wed, Sep 16, 2020 22:02:03 +0200:

On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:

quoted

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:

quoted

On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:

quoted

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

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

-- 
2.28.0.22.gfab0d4627e

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 <redacted>
Signed-off-by: Alex Riesen <redacted>

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

-Peff

Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:

On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:

quoted

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 <redacted>
Signed-off-by: Alex Riesen <redacted>

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.

On Thu, Sep 17, 2020 at 03:35:25PM +0200, Alex Riesen wrote:

Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:

quoted

On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:

quoted

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 <redacted>
Signed-off-by: Alex Riesen <redacted>

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

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, Thu, Sep 17, 2020 15:20:47 +0200:

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

And I compiled and ran the code by now. Feels ok.

Alex Riesen, Thu, Sep 17, 2020 15:20:47 +0200:

Sorry about that. git_config_get_string_tmp, indeed.