Re: sub-fetches discard --ipv4|6 option

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

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

quoted

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

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

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

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

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

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

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

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

Sure! Thinking about it, I actually would have preferred to have both: a
config option and a command-line option. So that I can set --ipv4 in, say,
~/.config/git/config file, but still have the option to try --ipv6 from time
to time to check if the network setup magically fixed itself.

What would the preferred name for that config option be? fetch.ipv?

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