sub-fetches discard --ipv4|6 option

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> Jeff King, 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 <alexander.riesen@cetitec.com> writes:
> > Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:
> >> No problem, and no rush. I just wanted to make sure those bits didn't
> >> get overlooked.
> >
> > I'll try and do my best :)
> 
> If I remember correctly, this discussion started as an introduction
> of useful feature, but got stuck at the implementation phase of how
> the feature is presented at the UI level after we had general
> concensus on the design.
> 
> It has been about 3 months; has anything happened that I missed?

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

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

> Affecting the transfers caused by git-fetch, the
> option allows to control network operations similar
> to --ipv4 and --ipv6 options.
> ...
> 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);
>  }

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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