diff mbox series

sub-fetches discard --ipv4|6 option

Message ID 20200914121906.GD4705@pflmari
State Accepted
Commit 4e735c13267e65f8c37fc3e7dfaacb36e5d51ab8
Headers show
Series sub-fetches discard --ipv4|6 option | expand

Commit Message

Alex Riesen Sept. 14, 2020, 12:19 p.m. UTC
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:

Am I missing something obvious?

Regards,
Alex

Comments

Jeff King Sept. 14, 2020, 7:49 p.m. UTC | #1
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
Junio C Hamano Sept. 14, 2020, 8:06 p.m. UTC | #2
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.
Alex Riesen Sept. 15, 2020, 11:50 a.m. UTC | #3
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.
Jeff King Sept. 15, 2020, 1:05 p.m. UTC | #4
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
Alex Riesen Sept. 15, 2020, 2:06 p.m. UTC | #5
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
Jeff King Sept. 15, 2020, 3:27 p.m. UTC | #6
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
Alex Riesen Sept. 15, 2020, 4:03 p.m. UTC | #7
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?
Junio C Hamano Sept. 15, 2020, 8:09 p.m. UTC | #8
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?
Jeff King Sept. 15, 2020, 9:23 p.m. UTC | #9
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
Junio C Hamano Sept. 15, 2020, 9:32 p.m. UTC | #10
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.
Jeff King Sept. 16, 2020, 4:32 p.m. UTC | #11
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
Jeff King Sept. 16, 2020, 4:34 p.m. UTC | #12
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
Junio C Hamano Sept. 16, 2020, 9:20 p.m. UTC | #13
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.
Alex Riesen Sept. 17, 2020, 2:33 p.m. UTC | #14
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?
Jeff King Sept. 22, 2020, 5:08 a.m. UTC | #15
On Thu, Sep 17, 2020 at 04:33:39PM +0200, Alex Riesen wrote:

> > 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 don't know offhand. I'd suspect some of the command which take
--recurse-submodules do something similar, but the number of steps
between the main command the submodule argv may make it awkward to use
a parse-options solution. I don't think we have a "push to each of these
remotes" option the way we do for fetch.

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

Yeah, I agree it's complex, and I'm happy with simpler solutions (or
just fixing these ones as you did and punting on it for now).

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

I'm not super familiar with the internals of parse-options, but it
doesn't look like it. Each step consumes an argv and matches it to a
"struct option", but I don't think you get to know which struct it was
matched to. It would be reasonable for it to keep a pointer in the
parse_opt_ctx_t (and of course you'd need some bit in the option struct
itself to say "I am a recursive option").

-Peff
diff mbox series

Patch

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