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

From: Jeff King <peff@peff.net>
Date: Sep 22, 2020, 05:08 UTC
Message-ID: <20200922050855.GC528837@coredump.intra.peff.net>
In-Reply-To: <20200917143339.GF8079@pflmari>

On Thu, Sep 17, 2020 at 04:33:39PM +0200, Alex Riesen wrote:

Show 9 quoted lines

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

Show 7 quoted lines

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