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

From: Alex Riesen <alexander.riesen@cetitec.com>
Date: Sep 15, 2020, 11:50 UTC
Message-ID: <20200915115025.GA18984@pflmari>
In-Reply-To: <20200914194951.GA2819729@coredump.intra.peff.net>

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

Show 21 quoted lines

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

Show 5 quoted lines

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