git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* sub-fetches discard --ipv4|6 option
@ 2020-09-14 12:19 Alex Riesen
  2020-09-14 19:49 ` Jeff King
  2020-09-14 20:06 ` Junio C Hamano
  0 siblings, 2 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-14 12:19 UTC (permalink / raw)
  To: git; +Cc: Eric Wong, Junio C Hamano, Jeff King

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-14 12:19 sub-fetches discard --ipv4|6 option Alex Riesen
@ 2020-09-14 19:49 ` Jeff King
  2020-09-15 11:50   ` Alex Riesen
  2020-09-14 20:06 ` Junio C Hamano
  1 sibling, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-14 19:49 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Eric Wong, Junio C Hamano

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

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

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-14 12:19 sub-fetches discard --ipv4|6 option Alex Riesen
  2020-09-14 19:49 ` Jeff King
@ 2020-09-14 20:06 ` Junio C Hamano
  1 sibling, 0 replies; 44+ messages in thread
From: Junio C Hamano @ 2020-09-14 20:06 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Eric Wong, Jeff King

Alex Riesen <alexander.riesen@cetitec.com> writes:

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

I think something obvious was missed back wne -4/-6 was added at
c915f11e (connect & http: support -4 and -6 switches for remote
operations, 2016-02-03) ;-).

The other candidate was 9c4a036b (Teach the --all option to 'git
fetch', 2009-11-09) that introduced this helper to relay various
options, but back then there weren't -4/-6 invented yet, so...

It is somewhat sad that we need to manually relay these down, but I
do not offhand think of a way to automate this sensibly.

Thanks for noticing.




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-14 19:49 ` Jeff King
@ 2020-09-15 11:50   ` Alex Riesen
  2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
  2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
  0 siblings, 2 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-15 11:50 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Eric Wong, Junio C Hamano

Jeff King, Mon, Sep 14, 2020 21:49:51 +0200:
> On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:
> 
> > 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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-15 11:50   ` Alex Riesen
@ 2020-09-15 11:54     ` Alex Riesen
  2020-09-15 13:06       ` Jeff King
  2020-09-15 21:19       ` Junio C Hamano
  2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
  1 sibling, 2 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-15 11:54 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Eric Wong

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 <alexander.riesen@cetitec.com>
---

 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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 11:50   ` Alex Riesen
  2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
@ 2020-09-15 13:05     ` Jeff King
  2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
  2020-09-15 14:06       ` sub-fetches discard --ipv4|6 option Alex Riesen
  1 sibling, 2 replies; 44+ messages in thread
From: Jeff King @ 2020-09-15 13:05 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Eric Wong, Junio C Hamano

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

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

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
@ 2020-09-15 13:06       ` Jeff King
  2020-09-16  4:17         ` Junio C Hamano
  2020-09-15 21:19       ` Junio C Hamano
  1 sibling, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-15 13:06 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Junio C Hamano, Eric Wong

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 <alexander.riesen@cetitec.com>
> ---

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
@ 2020-09-15 13:54       ` Alex Riesen
  2020-09-16 20:02         ` Jeff King
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
  2020-09-15 14:06       ` sub-fetches discard --ipv4|6 option Alex Riesen
  1 sibling, 2 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-15 13:54 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Eric Wong

Affecting the transfers caused by git-fetch, 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, 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(+)

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
  2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
@ 2020-09-15 14:06       ` Alex Riesen
  2020-09-15 15:27         ` Jeff King
  1 sibling, 1 reply; 44+ messages in thread
From: Alex Riesen @ 2020-09-15 14:06 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Eric Wong, Junio C Hamano

Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:
> On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:
> 
> > > 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.

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 14:06       ` sub-fetches discard --ipv4|6 option Alex Riesen
@ 2020-09-15 15:27         ` Jeff King
  2020-09-15 16:03           ` Alex Riesen
  2020-09-15 20:09           ` Junio C Hamano
  0 siblings, 2 replies; 44+ messages in thread
From: Jeff King @ 2020-09-15 15:27 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Eric Wong, Junio C Hamano

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

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 15:27         ` Jeff King
@ 2020-09-15 16:03           ` Alex Riesen
  2020-09-16 16:32             ` Jeff King
  2020-09-15 20:09           ` Junio C Hamano
  1 sibling, 1 reply; 44+ messages in thread
From: Alex Riesen @ 2020-09-15 16:03 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Eric Wong, Junio C Hamano

Jeff King, Tue, Sep 15, 2020 17:27:30 +0200:
> On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:
> 
> > > 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?


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 15:27         ` Jeff King
  2020-09-15 16:03           ` Alex Riesen
@ 2020-09-15 20:09           ` Junio C Hamano
  2020-09-15 21:23             ` Jeff King
  1 sibling, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-15 20:09 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> writes:

> On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:
>
>> > 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.

What is this "recursive" about?  Does it have much in common with
"passthru", or are they orthogonal?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
  2020-09-15 13:06       ` Jeff King
@ 2020-09-15 21:19       ` Junio C Hamano
  2020-09-16  7:25         ` Alex Riesen
  1 sibling, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-15 21:19 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Jeff King, Eric Wong

Alex Riesen <alexander.riesen@cetitec.com> writes:

> 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 <alexander.riesen@cetitec.com>
> ---

To avoid an overlong title and conform to project convention (aka
"easier to read 'git shortlog --no-merges' output), I shortened the
title and tweaked the text a bit to compensate for the change.

Thanks.


-- >8 --
From: Alex Riesen <alexander.riesen@cetitec.com>
Date: Tue, 15 Sep 2020 13:54:07 +0200
Subject: [PATCH] fetch: pass --ipv4 and --ipv6 options to sub-fetches

The options indicate user intent for the whole fetch operation, and
ignoring them in sub-fetches (i.e. "--all" and recursive fetching of
submodules) 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 <alexander.riesen@cetitec.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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-618-g53f972bf7d


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 20:09           ` Junio C Hamano
@ 2020-09-15 21:23             ` Jeff King
  2020-09-15 21:32               ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-15 21:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alex Riesen, git, Eric Wong

On Tue, Sep 15, 2020 at 01:09:10PM -0700, Junio C Hamano wrote:

> > 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.
> 
> What is this "recursive" about?  Does it have much in common with
> "passthru", or are they orthogonal?

I agree the name is not super-descriptive. And it's a bit odd that the
parse-options code itself would not care about it. It's simply a
convenient bit for the calling code to use (rather than try to manage a
separate array whose values correspond). It could be PARSE_OPT_USER1,
but that is probably too inscrutable. :)

It's sort-of similar to passthru, but not quite. The problem with
passthru is that it _always_ applies to the option. We never parse it as
an option itself, but rather always stick its canonicalized form into an
array of strings. But for the case we're talking about here, we don't
know ahead of time whether we want the passthru behavior or not. It
depends on whether we see an option like "--all" (which might even come
after us).

So I think the best you could do is:

  1. Keep two separate option lists, "parent" and "child". The parent
     list has "--all" in it. The child list has stuff like "--ipv6".

  2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets
     you decide whether we're in a mode that is spawning child fetch
     processes.

  3. If we are spawning, then everything in the "child" option list
     becomes passthru. We could either mark them as such, or really, I
     guess we could just pass the remainder of argv on as-is (though it
     might be nice to diagnose a bogus config option once in the parent
     rather than in each child).

That would work OK. One downside is that PARSE_OPT_KEEP_UNKNOWN is
inherently flaky. It doesn't know if "--foo --bar" is two options, or
the option "--foo" with the value "--bar". You could solve that by leaving
dummy passthru options in the "parent" list, but now we're back to
having two copies.

I guess parse-options could provide a MAYBE_PASSTHRU flag. On the first
parse_options() call, it would skip over any such options, leaving them
in argv. On the second, the caller would tell it to actually parse them.

-Peff

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 21:23             ` Jeff King
@ 2020-09-15 21:32               ` Junio C Hamano
  2020-09-16 16:34                 ` Jeff King
  0 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-15 21:32 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> writes:

> So I think the best you could do is:
>
>   1. Keep two separate option lists, "parent" and "child". The parent
>      list has "--all" in it. The child list has stuff like "--ipv6".
>
>   2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets
>      you decide whether we're in a mode that is spawning child fetch
>      processes.

Hmph, I vaguely recall discussion about cascading options[] list but
do not find anything that may be involved in an implementation like
that in <parse-options.h>.  I agree that neither of the above is so
attractive.

> I guess parse-options could provide a MAYBE_PASSTHRU flag. On the first
> parse_options() call, it would skip over any such options, leaving them
> in argv. On the second, the caller would tell it to actually parse them.

Or calling it USR1, which is a good way to make it crystal clear
that parse_options() API does not do anything to it.  The code like
"builtin/fetch.c" can locally give it a more meaningful name with
"#define PARSE_OPT_RECURSIVE PARSE_OPT_USR1". if recursive is the
appropriate name for the bit in the context of the options[] array.

I agree that _F() convention that can be used across different types
would be a good thing to have in the longer term, by the way.

Thanks.




^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-15 13:06       ` Jeff King
@ 2020-09-16  4:17         ` Junio C Hamano
  2020-09-16  7:27           ` Alex Riesen
  0 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16  4:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> writes:

> 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 <alexander.riesen@cetitec.com>
>> ---
>
> 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.

Yes.  Others can wait.  ipversion configuration variable is probably
easier to sell; parse_options thing deserves a longer and deeper
thought as it will affect the API future codebase would rely on.

Thanks.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-15 21:19       ` Junio C Hamano
@ 2020-09-16  7:25         ` Alex Riesen
  0 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-16  7:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Eric Wong

Junio C Hamano, Tue, Sep 15, 2020 23:19:11 +0200:
> Alex Riesen <alexander.riesen@cetitec.com> writes:
> 
> > 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.
> >
> 
> To avoid an overlong title and conform to project convention (aka
> "easier to read 'git shortlog --no-merges' output), I shortened the
> title and tweaked the text a bit to compensate for the change.

Thanks :)


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules
  2020-09-16  4:17         ` Junio C Hamano
@ 2020-09-16  7:27           ` Alex Riesen
  0 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-16  7:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Eric Wong

Junio C Hamano, Wed, Sep 16, 2020 06:17:41 +0200:
> Jeff King <peff@peff.net> writes:
> 
> > 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 <alexander.riesen@cetitec.com>
> >> ---
> >
> > 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.
> 
> Yes.  Others can wait.  ipversion configuration variable is probably
> easier to sell; ...

Is its choice of namespace (transfer.) alright, too?

> ... parse_options thing deserves a longer and deeper
> thought as it will affect the API future codebase would rely on.

Of course.

Regards,
Alex


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 16:03           ` Alex Riesen
@ 2020-09-16 16:32             ` Jeff King
  2020-09-17 14:33               ` Alex Riesen
  0 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-16 16:32 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Eric Wong, Junio C Hamano

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

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-15 21:32               ` Junio C Hamano
@ 2020-09-16 16:34                 ` Jeff King
  2020-09-16 21:20                   ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-16 16:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alex Riesen, git, Eric Wong

On Tue, Sep 15, 2020 at 02:32:50PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So I think the best you could do is:
> >
> >   1. Keep two separate option lists, "parent" and "child". The parent
> >      list has "--all" in it. The child list has stuff like "--ipv6".
> >
> >   2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets
> >      you decide whether we're in a mode that is spawning child fetch
> >      processes.
> 
> Hmph, I vaguely recall discussion about cascading options[] list but
> do not find anything that may be involved in an implementation like
> that in <parse-options.h>.  I agree that neither of the above is so
> attractive.

I think we just use KEEP_UNKNOWN in those cases and ignore any downsides
to it.

> > I guess parse-options could provide a MAYBE_PASSTHRU flag. On the first
> > parse_options() call, it would skip over any such options, leaving them
> > in argv. On the second, the caller would tell it to actually parse them.
> 
> Or calling it USR1, which is a good way to make it crystal clear
> that parse_options() API does not do anything to it.  The code like
> "builtin/fetch.c" can locally give it a more meaningful name with
> "#define PARSE_OPT_RECURSIVE PARSE_OPT_USR1". if recursive is the
> appropriate name for the bit in the context of the options[] array.

Ah, that's a good suggestion. My earlier "USER" suggestion was
tongue-in-cheek, because I think it makes the resulting options list
quite confusing.  But a local #define fixes that nicely.

That said, it sounds from the other part of the thread like we'll need
better parse-options support anyway, so this "noop flag bit" idea
probably isn't a good direction anyway.

-Peff

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
@ 2020-09-16 20:02         ` Jeff King
  2020-09-17  8:07           ` Alex Riesen
  2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
  1 sibling, 2 replies; 44+ messages in thread
From: Jeff King @ 2020-09-16 20:02 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Junio C Hamano, Eric Wong

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
  2020-09-16 20:02         ` Jeff King
@ 2020-09-16 20:14         ` Junio C Hamano
  2020-09-16 20:18           ` Jeff King
                             ` (3 more replies)
  1 sibling, 4 replies; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16 20:14 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Jeff King, Eric Wong

Alex Riesen <alexander.riesen@cetitec.com> 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);
>  }

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
@ 2020-09-16 20:18           ` Jeff King
  2020-09-16 22:50             ` Junio C Hamano
  2020-09-16 21:35           ` Junio C Hamano
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-16 20:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alex Riesen, git, Eric Wong

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

> Alex Riesen <alexander.riesen@cetitec.com> 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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-16 16:34                 ` Jeff King
@ 2020-09-16 21:20                   ` Junio C Hamano
  0 siblings, 0 replies; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16 21:20 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> writes:

> That said, it sounds from the other part of the thread like we'll need
> better parse-options support anyway, so this "noop flag bit" idea
> probably isn't a good direction anyway.

OK.  Thanks.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
  2020-09-16 20:18           ` Jeff King
@ 2020-09-16 21:35           ` Junio C Hamano
  2020-09-17 14:02             ` Alex Riesen
  2020-09-17  8:04           ` Alex Riesen
  2020-09-17  8:18           ` Alex Riesen
  3 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16 21:35 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Jeff King, Eric Wong

Junio C Hamano <gitster@pobox.com> 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.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:18           ` Jeff King
@ 2020-09-16 22:50             ` Junio C Hamano
  2020-09-16 22:52               ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16 22:50 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> 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
    };

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 22:50             ` Junio C Hamano
@ 2020-09-16 22:52               ` Junio C Hamano
  2020-09-17  0:48                 ` Jeff King
  0 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-16 22:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> 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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 22:52               ` Junio C Hamano
@ 2020-09-17  0:48                 ` Jeff King
  2020-09-17  0:57                   ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-17  0:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alex Riesen, git, Eric Wong

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-17  0:48                 ` Jeff King
@ 2020-09-17  0:57                   ` Junio C Hamano
  0 siblings, 0 replies; 44+ messages in thread
From: Junio C Hamano @ 2020-09-17  0:57 UTC (permalink / raw)
  To: Jeff King; +Cc: Alex Riesen, git, Eric Wong

Jeff King <peff@peff.net> 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 ;-).


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
  2020-09-16 20:18           ` Jeff King
  2020-09-16 21:35           ` Junio C Hamano
@ 2020-09-17  8:04           ` Alex Riesen
  2020-09-17  8:18           ` Alex Riesen
  3 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17  8:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Eric Wong

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?



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:02         ` Jeff King
@ 2020-09-17  8:07           ` Alex Riesen
  2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
  1 sibling, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17  8:07 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
                             ` (2 preceding siblings ...)
  2020-09-17  8:04           ` Alex Riesen
@ 2020-09-17  8:18           ` Alex Riesen
  3 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17  8:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Eric Wong

Junio C Hamano, Wed, Sep 16, 2020 22:14:08 +0200:
> Alex Riesen <alexander.riesen@cetitec.com> 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.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-16 20:02         ` Jeff King
  2020-09-17  8:07           ` Alex Riesen
@ 2020-09-17 13:20           ` Alex Riesen
  2020-09-17 13:26             ` Alex Riesen
                               ` (2 more replies)
  1 sibling, 3 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 13:20 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
@ 2020-09-17 13:26             ` Alex Riesen
  2020-09-17 13:31             ` Jeff King
  2020-09-17 16:05             ` Alex Riesen
  2 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 13:26 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
  2020-09-17 13:26             ` Alex Riesen
@ 2020-09-17 13:31             ` Jeff King
  2020-09-17 13:35               ` Alex Riesen
  2020-09-17 16:05             ` Alex Riesen
  2 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-17 13:31 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Junio C Hamano, Eric Wong

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 13:31             ` Jeff King
@ 2020-09-17 13:35               ` Alex Riesen
  2020-09-17 14:51                 ` Jeff King
  0 siblings, 1 reply; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 13:35 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-16 21:35           ` Junio C Hamano
@ 2020-09-17 14:02             ` Alex Riesen
  2020-09-17 22:31               ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 14:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Eric Wong

Junio C Hamano, Wed, Sep 16, 2020 23:35:23 +0200:
> Junio C Hamano <gitster@pobox.com> 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 ...



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: sub-fetches discard --ipv4|6 option
  2020-09-16 16:32             ` Jeff King
@ 2020-09-17 14:33               ` Alex Riesen
  0 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 14:33 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Eric Wong, Junio C Hamano

Jeff King, Wed, Sep 16, 2020 18:32:18 +0200:
> On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:
> 
> > > 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.
...
> > 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?


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 13:35               ` Alex Riesen
@ 2020-09-17 14:51                 ` Jeff King
  2020-09-17 15:17                   ` Alex Riesen
  0 siblings, 1 reply; 44+ messages in thread
From: Jeff King @ 2020-09-17 14:51 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Junio C Hamano, Eric Wong

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

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 14:51                 ` Jeff King
@ 2020-09-17 15:17                   ` Alex Riesen
  0 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 15:17 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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



^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] Config option to set the transport protocol version for network fetches
  2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
  2020-09-17 13:26             ` Alex Riesen
  2020-09-17 13:31             ` Jeff King
@ 2020-09-17 16:05             ` Alex Riesen
  2 siblings, 0 replies; 44+ messages in thread
From: Alex Riesen @ 2020-09-17 16:05 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Eric Wong

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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-17 14:02             ` Alex Riesen
@ 2020-09-17 22:31               ` Junio C Hamano
  2020-09-18  7:16                 ` Alex Riesen
  0 siblings, 1 reply; 44+ messages in thread
From: Junio C Hamano @ 2020-09-17 22:31 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Jeff King, Eric Wong

Alex Riesen <alexander.riesen@cetitec.com> 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.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-17 22:31               ` Junio C Hamano
@ 2020-09-18  7:16                 ` Alex Riesen
  2020-09-18 16:37                   ` Junio C Hamano
  0 siblings, 1 reply; 44+ messages in thread
From: Alex Riesen @ 2020-09-18  7:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Eric Wong

Junio C Hamano, Fri, Sep 18, 2020 00:31:58 +0200:
> Alex Riesen <alexander.riesen@cetitec.com> 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.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches
  2020-09-18  7:16                 ` Alex Riesen
@ 2020-09-18 16:37                   ` Junio C Hamano
  0 siblings, 0 replies; 44+ messages in thread
From: Junio C Hamano @ 2020-09-18 16:37 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Jeff King, Eric Wong

Alex Riesen <alexander.riesen@cetitec.com> 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.

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, other threads:[~2020-09-18 16:37 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 12:19 sub-fetches discard --ipv4|6 option Alex Riesen
2020-09-14 19:49 ` Jeff King
2020-09-15 11:50   ` Alex Riesen
2020-09-15 11:54     ` [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules Alex Riesen
2020-09-15 13:06       ` Jeff King
2020-09-16  4:17         ` Junio C Hamano
2020-09-16  7:27           ` Alex Riesen
2020-09-15 21:19       ` Junio C Hamano
2020-09-16  7:25         ` Alex Riesen
2020-09-15 13:05     ` sub-fetches discard --ipv4|6 option Jeff King
2020-09-15 13:54       ` [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches Alex Riesen
2020-09-16 20:02         ` Jeff King
2020-09-17  8:07           ` Alex Riesen
2020-09-17 13:20           ` [PATCH] Config option to set the " Alex Riesen
2020-09-17 13:26             ` Alex Riesen
2020-09-17 13:31             ` Jeff King
2020-09-17 13:35               ` Alex Riesen
2020-09-17 14:51                 ` Jeff King
2020-09-17 15:17                   ` Alex Riesen
2020-09-17 16:05             ` Alex Riesen
2020-09-16 20:14         ` [PATCH] config: option transfer.ipversion to set " Junio C Hamano
2020-09-16 20:18           ` Jeff King
2020-09-16 22:50             ` Junio C Hamano
2020-09-16 22:52               ` Junio C Hamano
2020-09-17  0:48                 ` Jeff King
2020-09-17  0:57                   ` Junio C Hamano
2020-09-16 21:35           ` Junio C Hamano
2020-09-17 14:02             ` Alex Riesen
2020-09-17 22:31               ` Junio C Hamano
2020-09-18  7:16                 ` Alex Riesen
2020-09-18 16:37                   ` Junio C Hamano
2020-09-17  8:04           ` Alex Riesen
2020-09-17  8:18           ` Alex Riesen
2020-09-15 14:06       ` sub-fetches discard --ipv4|6 option Alex Riesen
2020-09-15 15:27         ` Jeff King
2020-09-15 16:03           ` Alex Riesen
2020-09-16 16:32             ` Jeff King
2020-09-17 14:33               ` Alex Riesen
2020-09-15 20:09           ` Junio C Hamano
2020-09-15 21:23             ` Jeff King
2020-09-15 21:32               ` Junio C Hamano
2020-09-16 16:34                 ` Jeff King
2020-09-16 21:20                   ` Junio C Hamano
2020-09-14 20:06 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git