Message ID | 20200914121906.GD4705@pflmari (mailing list archive) |
---|---|
State | Accepted |
Commit | 4e735c13267e65f8c37fc3e7dfaacb36e5d51ab8 |
Headers | show |
Series | sub-fetches discard --ipv4|6 option | expand |
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
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.
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.
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
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
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
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?
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?
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
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.
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
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
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.
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?
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 --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"); }