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
