{"thread":{"id":"54236","subject":"sub-fetches discard --ipv4|6 option","startedAt":"2020-09-14T17:43:27Z","lastAt":"2021-01-07T10:09:11Z","messageCount":49,"participants":["Alex Riesen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"405519","messageId":"20200914121906.GD4705@pflmari","threadId":"54236","inReplyTo":null,"subject":"sub-fetches discard --ipv4|6 option","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-14T12:19:06Z","receivedAt":"2020-09-14T17:43:27Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Hi everyone,\n\nI have a slight problem with IPv6 configuration in my local network (connect\nworks but transfers do not) and would like to temporarily disable use of the\ntransport for a series of fetches. The fetches are all done from within a\nscript to which I can pass options for \"git fetch\" commands in its\ncommand-line. The options will be appended to the fetch commands, i.e.:\n\n git fetch --ipv4\n\nUnfortunately, it only worked for the fetches which didn't use --all or\n--multiple. After a light searching, I failed to find an explanation as to\nwhy --all|--multiple are handled so inconsistently with single remote fetches\nand added the options (similar to --force or --keep) to the argument list for\nsub-fetches:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 82ac4be8a5..5e06c07106 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)\n \t\targv_array_push(argv, \"-v\");\n \telse if (verbosity < 0)\n \t\targv_array_push(argv, \"-q\");\n+\tif (family == TRANSPORT_FAMILY_IPV4)\n+\t\targv_array_push(argv, \"--ipv4\");\n+\telse if (family == TRANSPORT_FAMILY_IPV6)\n+\t\targv_array_push(argv, \"--ipv6\");\n \n }\n \nAm I missing something obvious?\n\nRegards,\nAlex\n"},{"id":"405521","messageId":"20200914194951.GA2819729@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200914121906.GD4705@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-14T19:49:51Z","receivedAt":"2020-09-14T19:49:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:\n\n> Unfortunately, it only worked for the fetches which didn't use --all or\n> --multiple. After a light searching, I failed to find an explanation as to\n> why --all|--multiple are handled so inconsistently with single remote fetches\n> and added the options (similar to --force or --keep) to the argument list for\n> sub-fetches:\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 82ac4be8a5..5e06c07106 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)\n> \t\targv_array_push(argv, \"-v\");\n> \telse if (verbosity < 0)\n> \t\targv_array_push(argv, \"-q\");\n> +\tif (family == TRANSPORT_FAMILY_IPV4)\n> +\t\targv_array_push(argv, \"--ipv4\");\n> +\telse if (family == TRANSPORT_FAMILY_IPV6)\n> +\t\targv_array_push(argv, \"--ipv6\");\n> \n> }\n> \n> Am I missing something obvious?\n\nI don't think so. When we're starting fetch sub-processes, some options\nwill make sense to pass along and some won't. The parent has to either\npass all options and omit some, or explicitly pass ones it knows are\nuseful. It looks like the code chooses the latter, but these particular\noptions never got added (and it seems like they should be, as they are\nonly useful to the child fetch processes that actually touch the\nnetwork).\n\nSo your patch above looks quite sensible (modulo useful bits like a\nsignoff and maybe a test, though I guess the impact of those options\nis probably hard to cover in our tests).\n\nIt is rather unfortunate that anybody adding new fetch options needs to\nremember to (maybe) add them to add_options_to_argv() themselves.\n\nAlso, regarding these two specific options, it sounds like you'd want\nthem set for all fetches during the time your IPv6 setup is broken. In\nwhich case I think a config option might have served you better. So that\nmight be something worth implementing (though either way I think the fix\nabove is worth doing independently).\n\n-Peff\n"},{"id":"405522","messageId":"xmqqk0wwktrr.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200914121906.GD4705@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T20:06:32Z","receivedAt":"2020-09-14T20:06:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 82ac4be8a5..5e06c07106 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)\n> \t\targv_array_push(argv, \"-v\");\n> \telse if (verbosity < 0)\n> \t\targv_array_push(argv, \"-q\");\n> +\tif (family == TRANSPORT_FAMILY_IPV4)\n> +\t\targv_array_push(argv, \"--ipv4\");\n> +\telse if (family == TRANSPORT_FAMILY_IPV6)\n> +\t\targv_array_push(argv, \"--ipv6\");\n> \n> }\n> \n> Am I missing something obvious?\n\nI think something obvious was missed back wne -4/-6 was added at\nc915f11e (connect & http: support -4 and -6 switches for remote\noperations, 2016-02-03) ;-).\n\nThe other candidate was 9c4a036b (Teach the --all option to 'git\nfetch', 2009-11-09) that introduced this helper to relay various\noptions, but back then there weren't -4/-6 invented yet, so...\n\nIt is somewhat sad that we need to manually relay these down, but I\ndo not offhand think of a way to automate this sensibly.\n\nThanks for noticing.\n\n\n\n"},{"id":"405553","messageId":"20200915115025.GA18984@pflmari","threadId":"54236","inReplyTo":"20200914194951.GA2819729@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-15T11:50:25Z","receivedAt":"2020-09-15T11:53:55Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Mon, Sep 14, 2020 21:49:51 +0200:\n> On Mon, Sep 14, 2020 at 02:19:06PM +0200, Alex Riesen wrote:\n> \n> > Unfortunately, it only worked for the fetches which didn't use --all or\n> > --multiple. After a light searching, I failed to find an explanation as to\n> > why --all|--multiple are handled so inconsistently with single remote fetches\n> > and added the options (similar to --force or --keep) to the argument list for\n> > sub-fetches: ...\n> > \n> > Am I missing something obvious?\n> \n> I don't think so. When we're starting fetch sub-processes, some options\n> will make sense to pass along and some won't. The parent has to either\n> pass all options and omit some, or explicitly pass ones it knows are\n> useful. It looks like the code chooses the latter, but these particular\n> options never got added (and it seems like they should be, as they are\n> only useful to the child fetch processes that actually touch the\n> network).\n> \n> So your patch above looks quite sensible (modulo useful bits like a\n> signoff and maybe a test, though I guess the impact of those options\n> is probably hard to cover in our tests).\n\nI tried to come up with one, but (aside from rather pointless checking of\noption presence in the trace output) failed to.\n\nOr may be precisely this could be the point of the test: just do a fetch with\nall options we intend to pass down to sub-fetches and check that they are\nindeed present in the invocation of fetch --all/--multiple/--recurse-submodules?\n\n> It is rather unfortunate that anybody adding new fetch options needs to\n> remember to (maybe) add them to add_options_to_argv() themselves.\n\nMaybe make add_options_to_argv to go through builtin_fetch_options[] and copy\nthe options with a special marker if they were provided?\nAnd use the word \"recursive\" in help text as the marker :)\n\n> Also, regarding these two specific options, it sounds like you'd want\n> them set for all fetches during the time your IPv6 setup is broken. In\n> which case I think a config option might have served you better. So that\n> might be something worth implementing (though either way I think the fix\n> above is worth doing independently).\n\nSure! Thinking about it, I actually would have preferred to have both: a\nconfig option and a command-line option. So that I can set --ipv4 in, say,\n~/.config/git/config file, but still have the option to try --ipv6 from time\nto time to check if the network setup magically fixed itself.\n\nWhat would the preferred name for that config option be? fetch.ipv?\n\nI'll be sending the first change reformatted as patch shortly. Just in case.\n\n"},{"id":"405555","messageId":"20200915115407.GA31786@pflmari","threadId":"54236","inReplyTo":"20200915115025.GA18984@pflmari","subject":"[PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-15T11:54:07Z","receivedAt":"2020-09-15T12:30:46Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"The options indicate user intent for the whole fetch operation, and\nignoring them in sub-fetches is quite unexpected when, for instance,\nit is intended to limit all of the communication to a specific transport\nprotocol for some reason.\n\nSigned-off-by: Alex Riesen \n---\n\n builtin/fetch.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 82ac4be8a5..447d28ac29 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)\n \t\targv_array_push(argv, \"-v\");\n \telse if (verbosity < 0)\n \t\targv_array_push(argv, \"-q\");\n+\tif (family == TRANSPORT_FAMILY_IPV4)\n+\t\targv_array_push(argv, \"--ipv4\");\n+\telse if (family == TRANSPORT_FAMILY_IPV6)\n+\t\targv_array_push(argv, \"--ipv6\");\n \n }\n \n-- \n2.28.0.21.g09e033b31f\n"},{"id":"405556","messageId":"20200915140613.GB18984@pflmari","threadId":"54236","inReplyTo":"20200915130506.GA2839276@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-15T14:06:13Z","receivedAt":"2020-09-15T14:07:20Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:\n> On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n> \n> > > So your patch above looks quite sensible (modulo useful bits like a\n> > > signoff and maybe a test, though I guess the impact of those options\n> > > is probably hard to cover in our tests).\n> > \n> > I tried to come up with one, but (aside from rather pointless checking of\n> > option presence in the trace output) failed to.\n> > \n> > Or may be precisely this could be the point of the test: just do a fetch with\n> > all options we intend to pass down to sub-fetches and check that they are\n> > indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?\n> \n> Unfortunately I don't think that accomplishes much, since the main bug\n> we're worried about is missing options. And it would require somebody\n> adding the new options to the test, at which point you could just assume\n> they would add it to add_options_to_argv().\n> \n> Though I guess we can automatically get the list of options these days.\n> So perhaps something like:\n> \n> subopts=\n> for opt in $(git fetch --git-completion-helper)\n...\n> Except that doesn't quite work, because the parent fetch will complain\n> about nonsense values (e.g., --filter=1). So it would probably need a\n> bit more manual intelligence to cover those options. It looks like some\n> options are mutually exclusive, too (--deepen/--depth), so maybe we'd\n> need to run an individual \"fetch --all\" for each option.\n> \n> I dunno. It's getting pretty complicated. :)\n\nIt does :-( And the manual parts will require perpetual maintenance.\nNot doing that yet than.\n\n> > > It is rather unfortunate that anybody adding new fetch options needs to\n> > > remember to (maybe) add them to add_options_to_argv() themselves.\n> > \n> > Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy\n> > the options with a special marker if they were provided?\n> > And use the word \"recursive\" in help text as the marker :)\n> \n> Yeah, that would solve the duplication problem. We could probably add a\n> \"recursive\" bit to the parse-options flag variable. Even if\n> parse-options itself doesn't use it, it could be a convenience for\n> callers like this one. It is a little inconvenient to set flags there,\n> just because it usually means ditching our wrapper macros in favor of a\n> raw struct declaration.\n\nOr extend the list of wrappers with _REC(URSIVE) macros\n\nRegards,\nAlex\n"},{"id":"405557","messageId":"20200915152730.GA2853972@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200915140613.GB18984@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-15T15:27:30Z","receivedAt":"2020-09-15T15:40:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:\n\n> > Yeah, that would solve the duplication problem. We could probably add a\n> > \"recursive\" bit to the parse-options flag variable. Even if\n> > parse-options itself doesn't use it, it could be a convenience for\n> > callers like this one. It is a little inconvenient to set flags there,\n> > just because it usually means ditching our wrapper macros in favor of a\n> > raw struct declaration.\n> \n> Or extend the list of wrappers with _REC(URSIVE) macros\n\nIf you go that route, we have some \"_F\" macros that take flags. Probably\nwould make sense to add it more consistently, which lets you convert:\n\n OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n\ninto:\n\n OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n\nbut could also be used for other flags.\n\n-Peff\n"},{"id":"405563","messageId":"xmqq4kny2461.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200915152730.GA2853972@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-15T20:09:10Z","receivedAt":"2020-09-15T20:10:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:\n>\n>> > Yeah, that would solve the duplication problem. We could probably add a\n>> > \"recursive\" bit to the parse-options flag variable. Even if\n>> > parse-options itself doesn't use it, it could be a convenience for\n>> > callers like this one. It is a little inconvenient to set flags there,\n>> > just because it usually means ditching our wrapper macros in favor of a\n>> > raw struct declaration.\n>> \n>> Or extend the list of wrappers with _REC(URSIVE) macros\n>\n> If you go that route, we have some \"_F\" macros that take flags. Probably\n> would make sense to add it more consistently, which lets you convert:\n>\n> OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n>\n> into:\n>\n> OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n>\n> but could also be used for other flags.\n\nWhat is this \"recursive\" about? Does it have much in common with\n\"passthru\", or are they orthogonal?\n"},{"id":"405569","messageId":"20200915212338.GA2868700@coredump.intra.peff.net","threadId":"54236","inReplyTo":"xmqq4kny2461.fsf@gitster.c.googlers.com","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-15T21:23:38Z","receivedAt":"2020-09-15T21:24:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 01:09:10PM -0700, Junio C Hamano wrote:\n\n> > If you go that route, we have some \"_F\" macros that take flags. Probably\n> > would make sense to add it more consistently, which lets you convert:\n> >\n> > OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n> >\n> > into:\n> >\n> > OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n> >\n> > but could also be used for other flags.\n> \n> What is this \"recursive\" about? Does it have much in common with\n> \"passthru\", or are they orthogonal?\n\nI agree the name is not super-descriptive. And it's a bit odd that the\nparse-options code itself would not care about it. It's simply a\nconvenient bit for the calling code to use (rather than try to manage a\nseparate array whose values correspond). It could be PARSE_OPT_USER1,\nbut that is probably too inscrutable. :)\n\nIt's sort-of similar to passthru, but not quite. The problem with\npassthru is that it _always_ applies to the option. We never parse it as\nan option itself, but rather always stick its canonicalized form into an\narray of strings. But for the case we're talking about here, we don't\nknow ahead of time whether we want the passthru behavior or not. It\ndepends on whether we see an option like \"--all\" (which might even come\nafter us).\n\nSo I think the best you could do is:\n\n 1. Keep two separate option lists, \"parent\" and \"child\". The parent\n list has \"--all\" in it. The child list has stuff like \"--ipv6\".\n\n 2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets\n you decide whether we're in a mode that is spawning child fetch\n processes.\n\n 3. If we are spawning, then everything in the \"child\" option list\n becomes passthru. We could either mark them as such, or really, I\n guess we could just pass the remainder of argv on as-is (though it\n might be nice to diagnose a bogus config option once in the parent\n rather than in each child).\n\nThat would work OK. One downside is that PARSE_OPT_KEEP_UNKNOWN is\ninherently flaky. It doesn't know if \"--foo --bar\" is two options, or\nthe option \"--foo\" with the value \"--bar\". You could solve that by leaving\ndummy passthru options in the \"parent\" list, but now we're back to\nhaving two copies.\n\nI guess parse-options could provide a MAYBE_PASSTHRU flag. On the first\nparse_options() call, it would skip over any such options, leaving them\nin argv. On the second, the caller would tell it to actually parse them.\n\n-Peff\n"},{"id":"405570","messageId":"xmqqeen2zqk0.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200915115407.GA31786@pflmari","subject":"Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-15T21:19:11Z","receivedAt":"2020-09-15T21:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> The options indicate user intent for the whole fetch operation, and\n> ignoring them in sub-fetches is quite unexpected when, for instance,\n> it is intended to limit all of the communication to a specific transport\n> protocol for some reason.\n>\n> Signed-off-by: Alex Riesen \n> ---\n\nTo avoid an overlong title and conform to project convention (aka\n\"easier to read 'git shortlog --no-merges' output), I shortened the\ntitle and tweaked the text a bit to compensate for the change.\n\nThanks.\n\n\n-- >8 --\nFrom: Alex Riesen \nDate: Tue, 15 Sep 2020 13:54:07 +0200\nSubject: [PATCH] fetch: pass --ipv4 and --ipv6 options to sub-fetches\n\nThe options indicate user intent for the whole fetch operation, and\nignoring them in sub-fetches (i.e. \"--all\" and recursive fetching of\nsubmodules) is quite unexpected when, for instance, it is intended\nto limit all of the communication to a specific transport protocol\nfor some reason.\n\nSigned-off-by: Alex Riesen \nSigned-off-by: Junio C Hamano \n---\n builtin/fetch.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 82ac4be8a5..447d28ac29 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1531,6 +1531,10 @@ static void add_options_to_argv(struct argv_array *argv)\n \t\targv_array_push(argv, \"-v\");\n \telse if (verbosity < 0)\n \t\targv_array_push(argv, \"-q\");\n+\tif (family == TRANSPORT_FAMILY_IPV4)\n+\t\targv_array_push(argv, \"--ipv4\");\n+\telse if (family == TRANSPORT_FAMILY_IPV6)\n+\t\targv_array_push(argv, \"--ipv6\");\n \n }\n \n-- \n2.28.0-618-g53f972bf7d\n\n"},{"id":"405572","messageId":"xmqqa6xqzpx9.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200915212338.GA2868700@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-15T21:32:50Z","receivedAt":"2020-09-15T21:34:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think the best you could do is:\n>\n> 1. Keep two separate option lists, \"parent\" and \"child\". The parent\n> list has \"--all\" in it. The child list has stuff like \"--ipv6\".\n>\n> 2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets\n> you decide whether we're in a mode that is spawning child fetch\n> processes.\n\nHmph, I vaguely recall discussion about cascading options[] list but\ndo not find anything that may be involved in an implementation like\nthat in . I agree that neither of the above is so\nattractive.\n\n> I guess parse-options could provide a MAYBE_PASSTHRU flag. On the first\n> parse_options() call, it would skip over any such options, leaving them\n> in argv. On the second, the caller would tell it to actually parse them.\n\nOr calling it USR1, which is a good way to make it crystal clear\nthat parse_options() API does not do anything to it. The code like\n\"builtin/fetch.c\" can locally give it a more meaningful name with\n\"#define PARSE_OPT_RECURSIVE PARSE_OPT_USR1\". if recursive is the\nappropriate name for the bit in the context of the options[] array.\n\nI agree that _F() convention that can be used across different types\nwould be a good thing to have in the longer term, by the way.\n\nThanks.\n\n\n\n"},{"id":"405582","messageId":"20200915160357.GC18984@pflmari","threadId":"54236","inReplyTo":"20200915152730.GA2853972@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-15T16:03:57Z","receivedAt":"2020-09-15T22:35:02Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Tue, Sep 15, 2020 17:27:30 +0200:\n> On Tue, Sep 15, 2020 at 04:06:13PM +0200, Alex Riesen wrote:\n> \n> > > Yeah, that would solve the duplication problem. We could probably add a\n> > > \"recursive\" bit to the parse-options flag variable. Even if\n> > > parse-options itself doesn't use it, it could be a convenience for\n> > > callers like this one. It is a little inconvenient to set flags there,\n> > > just because it usually means ditching our wrapper macros in favor of a\n> > > raw struct declaration.\n> > \n> > Or extend the list of wrappers with _REC(URSIVE) macros\n> \n> If you go that route, we have some \"_F\" macros that take flags. Probably\n> would make sense to add it more consistently, which lets you convert:\n> \n> OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n> \n> into:\n> \n> OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n> \n> but could also be used for other flags.\n\nThis part (marking of the options) was easy. What's left is finding out if an\noption was actually specified in the command-line. The ...options[] arrays are\nnot update by parse_options() with what was given, are they?\n\nMaybe extend struct option with a field to store given command-line argument\n(as it was specified) and parse_options() will update the field if\nPARSE_OPT_RECURSIVE is present in .flags?\nIs it allowed for parse_options() to modify the options array?\nOr is it possible to use something in parse-options.h API to note the\narguments somewhere while they are parse? I mean, there are\nparse_options_start/step/end, can cmd_fetch argument parsing use those\nso that the options marked recursive can be saved for sub-fetches?\n\n"},{"id":"405588","messageId":"20200915135428.GA28038@pflmari","threadId":"54236","inReplyTo":"20200915130506.GA2839276@coredump.intra.peff.net","subject":"[PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-15T13:54:28Z","receivedAt":"2020-09-16T00:26:10Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Affecting the transfers caused by git-fetch, the\noption allows to control network operations similar\nto --ipv4 and --ipv6 options.\n\nSuggested-by: Jeff King \nSigned-off-by: Alex Riesen \n---\n\nJeff King, Tue, Sep 15, 2020 15:05:06 +0200:\n> On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n> > Sure! Thinking about it, I actually would have preferred to have both: a\n> > config option and a command-line option. So that I can set --ipv4 in, say,\n> > ~/.config/git/config file, but still have the option to try --ipv6 from time\n> > to time to check if the network setup magically fixed itself.\n> > \n> > What would the preferred name for that config option be? fetch.ipv?\n> \n> It looks like we've got similar options for clone/pull (which are really\n> fetch under the hood of course) and push. We have the \"transfer.*\"\n> namespace which applies to both already. So maybe \"transfer.ipversion\"\n> or something?\n\nSomething like this?\n\n Documentation/config/transfer.txt | 7 +++++++\n builtin/fetch.c | 11 +++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt\nindex f5b6245270..cc0e97fbb1 100644\n--- a/Documentation/config/transfer.txt\n+++ b/Documentation/config/transfer.txt\n@@ -69,3 +69,10 @@ transfer.unpackLimit::\n \tWhen `fetch.unpackLimit` or `receive.unpackLimit` are\n \tnot set, the value of this variable is used instead.\n \tThe default value is 100.\n+\n+transfer.ipversion::\n+\tLimit the network operations to the specified version of the transport\n+\tprotocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or\n+\t`all` to allow all protocols.\n+\tSee also linkgit:git-fetch[1] options `--ipv4` and `--ipv6`.\n+\tThe default value is `all` to allow all protocols.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 447d28ac29..da01c8f7b3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -118,6 +118,17 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"transfer.ipversion\")) {\n+\t\tif (!strcmp(v, \"all\"))\n+\t\t\t;\n+\t\telse if (!strcmp(v, \"4\"))\n+\t\t\tfamily = TRANSPORT_FAMILY_IPV4;\n+\t\telse if (!strcmp(v, \"6\"))\n+\t\t\tfamily = TRANSPORT_FAMILY_IPV6;\n+\t\telse\n+\t\t\tdie(_(\"transfer.ipversion can be only 4, 6, or any\"));\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n-- \n2.28.0.21.g178b32a6fd.dirty\n"},{"id":"405590","messageId":"20200915130506.GA2839276@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200915115025.GA18984@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-15T13:05:06Z","receivedAt":"2020-09-16T00:39:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n\n> > So your patch above looks quite sensible (modulo useful bits like a\n> > signoff and maybe a test, though I guess the impact of those options\n> > is probably hard to cover in our tests).\n> \n> I tried to come up with one, but (aside from rather pointless checking of\n> option presence in the trace output) failed to.\n> \n> Or may be precisely this could be the point of the test: just do a fetch with\n> all options we intend to pass down to sub-fetches and check that they are\n> indeed present in the invocation of fetch --all/--multiple/--recurse-submodules?\n\nUnfortunately I don't think that accomplishes much, since the main bug\nwe're worried about is missing options. And it would require somebody\nadding the new options to the test, at which point you could just assume\nthey would add it to add_options_to_argv().\n\nThough I guess we can automatically get the list of options these days.\nSo perhaps something like:\n\n subopts=\n for opt in $(git fetch --git-completion-helper)\n do\n case \"$opt\" in\n # options that we know do not go to sub-fetches\n --all|--jobs|etc...)\n ;;\n\t# try/match only the positive versions\n\t--no-*)\n\t ;;\n\t# give a fake value for options with values\n\t*=)\n subopts=\"$subopts ${opt}1\"\n\t\t;;\n\t# and pass through any boolean options\n\t*)\n subopts=\"$subopts $opt\"\n\t\t;;\n esac\n done\n GIT_TRACE=$PWD/trace.out git fetch --all $subopts\n perl -lne '\n BEGIN { @want = @ARGV; @ARGV = () }\n /run_command: git fetch (.*)/ and $seen{$_}++ for split(/ /, $1);\n END { print for grep { !$seen{$_} } @want }\n ' > It is rather unfortunate that anybody adding new fetch options needs to\n> > remember to (maybe) add them to add_options_to_argv() themselves.\n> \n> Maybe make add_options_to_argv to go through builtin_fetch_options[] and copy\n> the options with a special marker if they were provided?\n> And use the word \"recursive\" in help text as the marker :)\n\nYeah, that would solve the duplication problem. We could probably add a\n\"recursive\" bit to the parse-options flag variable. Even if\nparse-options itself doesn't use it, it could be a convenience for\ncallers like this one. It is a little inconvenient to set flags there,\njust because it usually means ditching our wrapper macros in favor of a\nraw struct declaration.\n\n> Sure! Thinking about it, I actually would have preferred to have both: a\n> config option and a command-line option. So that I can set --ipv4 in, say,\n> ~/.config/git/config file, but still have the option to try --ipv6 from time\n> to time to check if the network setup magically fixed itself.\n> \n> What would the preferred name for that config option be? fetch.ipv?\n\nIt looks like we've got similar options for clone/pull (which are really\nfetch under the hood of course) and push. We have the \"transfer.*\"\nnamespace which applies to both already. So maybe \"transfer.ipversion\"\nor something?\n\n-Peff\n"},{"id":"405591","messageId":"20200915130606.GB2839276@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200915115407.GA31786@pflmari","subject":"Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-15T13:06:06Z","receivedAt":"2020-09-16T00:40:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 01:54:07PM +0200, Alex Riesen wrote:\n\n> The options indicate user intent for the whole fetch operation, and\n> ignoring them in sub-fetches is quite unexpected when, for instance,\n> it is intended to limit all of the communication to a specific transport\n> protocol for some reason.\n> \n> Signed-off-by: Alex Riesen \n> ---\n\nRegardless of whether we move forward with the parse-options flag or\nconfig discussed in the other thread, I think this is an obvious\nimprovement that we should take in the meantime.\n\n-Peff\n"},{"id":"405598","messageId":"xmqqimcexsm2.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200915130606.GB2839276@coredump.intra.peff.net","subject":"Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T04:17:41Z","receivedAt":"2020-09-16T04:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 15, 2020 at 01:54:07PM +0200, Alex Riesen wrote:\n>\n>> The options indicate user intent for the whole fetch operation, and\n>> ignoring them in sub-fetches is quite unexpected when, for instance,\n>> it is intended to limit all of the communication to a specific transport\n>> protocol for some reason.\n>> \n>> Signed-off-by: Alex Riesen \n>> ---\n>\n> Regardless of whether we move forward with the parse-options flag or\n> config discussed in the other thread, I think this is an obvious\n> improvement that we should take in the meantime.\n\nYes. Others can wait. ipversion configuration variable is probably\neasier to sell; parse_options thing deserves a longer and deeper\nthought as it will affect the API future codebase would rely on.\n\nThanks.\n"},{"id":"405605","messageId":"20200916072523.GA15595@pflmari","threadId":"54236","inReplyTo":"xmqqeen2zqk0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-16T07:25:23Z","receivedAt":"2020-09-16T07:25:41Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Tue, Sep 15, 2020 23:19:11 +0200:\n> Alex Riesen writes:\n> \n> > The options indicate user intent for the whole fetch operation, and\n> > ignoring them in sub-fetches is quite unexpected when, for instance,\n> > it is intended to limit all of the communication to a specific transport\n> > protocol for some reason.\n> >\n> \n> To avoid an overlong title and conform to project convention (aka\n> \"easier to read 'git shortlog --no-merges' output), I shortened the\n> title and tweaked the text a bit to compensate for the change.\n\nThanks :)\n\n"},{"id":"405606","messageId":"20200916072738.GB15595@pflmari","threadId":"54236","inReplyTo":"xmqqimcexsm2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] Pass --ipv4 and --ipv6 options to sub-fetches when fetching multiple remotes and submodules","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-16T07:27:38Z","receivedAt":"2020-09-16T07:27:57Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Wed, Sep 16, 2020 06:17:41 +0200:\n> Jeff King writes:\n> \n> > On Tue, Sep 15, 2020 at 01:54:07PM +0200, Alex Riesen wrote:\n> >\n> >> The options indicate user intent for the whole fetch operation, and\n> >> ignoring them in sub-fetches is quite unexpected when, for instance,\n> >> it is intended to limit all of the communication to a specific transport\n> >> protocol for some reason.\n> >> \n> >> Signed-off-by: Alex Riesen \n> >> ---\n> >\n> > Regardless of whether we move forward with the parse-options flag or\n> > config discussed in the other thread, I think this is an obvious\n> > improvement that we should take in the meantime.\n> \n> Yes. Others can wait. ipversion configuration variable is probably\n> easier to sell; ...\n\nIs its choice of namespace (transfer.) alright, too?\n\n> ... parse_options thing deserves a longer and deeper\n> thought as it will affect the API future codebase would rely on.\n\nOf course.\n\nRegards,\nAlex\n\n"},{"id":"405674","messageId":"20200916200203.GA37225@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200915135428.GA28038@pflmari","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-16T20:02:03Z","receivedAt":"2020-09-16T20:02:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:\n\n> Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:\n> > On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n> > > Sure! Thinking about it, I actually would have preferred to have both: a\n> > > config option and a command-line option. So that I can set --ipv4 in, say,\n> > > ~/.config/git/config file, but still have the option to try --ipv6 from time\n> > > to time to check if the network setup magically fixed itself.\n> > > \n> > > What would the preferred name for that config option be? fetch.ipv?\n> > \n> > It looks like we've got similar options for clone/pull (which are really\n> > fetch under the hood of course) and push. We have the \"transfer.*\"\n> > namespace which applies to both already. So maybe \"transfer.ipversion\"\n> > or something?\n> \n> Something like this?\n\nThat's the right direction, but I think we'd want to make sure it\nimpacted all of the spots that allow switching. \"clone\" on the fetching\nside, but probably also \"push\".\n\nEach of those commands could learn the same config, but it might be\neasier to just enforce it in the transport layer. Something like this\nmaybe (compiled but not tested):\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a6d3268661..2f7a734eb2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1267,7 +1267,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \n \ttransport = transport_get(remote, NULL);\n \ttransport_set_verbosity(transport, verbosity, progress);\n-\ttransport->family = family;\n+\tif (family)\n+\t\ttransport->family = family;\n \tif (upload_pack)\n \t\tset_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);\n \tif (keep)\ndiff --git a/builtin/push.c b/builtin/push.c\nindex bc94078e72..f7a40b65cd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -343,7 +343,8 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \tchar *anon_url = transport_anonymize_url(transport->url);\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n-\ttransport->family = family;\n+\tif (family)\n+\t\ttransport->family = family;\n \n \tif (receivepack)\n \t\ttransport_set_option(transport,\ndiff --git a/transport.c b/transport.c\nindex 43e24bf1e5..92f81b414d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -919,6 +919,20 @@ static struct transport_vtable builtin_smart_vtable = {\n \tdisconnect_git\n };\n \n+enum transport_family default_transport_family(void)\n+{\n+\tconst char *v;\n+\n+\tif (git_config_get_string_tmp(\"core.ipversion\", &v))\n+\t\treturn TRANSPORT_FAMILY_ALL;\n+\telse if (!strcmp(v, \"ipv4\"))\n+\t\treturn TRANSPORT_FAMILY_IPV4;\n+\telse if (!strcmp(v, \"ipv6\"))\n+\t\treturn TRANSPORT_FAMILY_IPV6;\n+\n+\tdie(_(\"invalid core.ipversion: %s\"), v);\n+}\n+\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tconst char *helper;\n@@ -948,6 +962,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\t\thelper = xstrndup(url, p - url);\n \t}\n \n+\tret->family = default_transport_family();\n+\n \tif (helper) {\n \t\ttransport_helper_init(ret, helper);\n \t} else if (starts_with(url, \"rsync:\")) {\n\nA few notes:\n\n - it cheats a little by noting that command-line options can't get it\n to \"ALL\". It might be a bit cleaner if we have an explicit\n TRANSPORT_FAMILY_UNSET, and then resolve the default at look-up\n time.\n\n - it probably needs more \"if (family)\" spots in each command, which is\n unfortunate (which again might go away if \"UNSET\" is 0).\n\n - I waffled on the name. transfer.* is where we put things that apply\n to both push/fetch, but it's usually more related to the git layer.\n This is more of a core networking decision, and if we added other\n network programs, I'd expect them to respect it. So maybe \"core.\" is\n a better namespace.\n\n-Peff\n"},{"id":"405675","messageId":"xmqqtuvxwkbz.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200915135428.GA28038@pflmari","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T20:14:08Z","receivedAt":"2020-09-16T20:14:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Affecting the transfers caused by git-fetch, the\n> option allows to control network operations similar\n> to --ipv4 and --ipv6 options.\n> ...\n> Something like this?\n\nGood start.\n\nIf I configure it to \"4\", I do not see a way to override it and say\n\"I don't care which one is used\". With the introduction of this\nconfiguration variable, we'd need a bit more support on the command\nline side.\n\nHow about introducing a new command line option\n\n\t--transfer-protocol-family=(\"any\"|)\n\nwhere is either \"ipv4\" or \"ipv6\" [*1*], and make existing\n\"--ipv4\" a synonym for \"--transfer-protocol-family=ipv4\" and \"--ipv6\"\nfor \"--transfer-protocol-family=ipv6\"\n\nWith such an extended command line override, we can override\nconfigured \n\n\t[transfer]\n\t\tipversion = 6\n\nwith \"--transfer-protocol-family=any\" from the command line.\n\nAlso, we should follow the usual \"the last one wins\" for a\nconfiguration variable like this, which is *not* a multi-valued\nvariable. So the config parsing would look more like this:\n\n\tif (!strcmp(k, \"transfer.ipversion\")) {\n\t\tif (!v)\n\t\t\treturn config_error_nonbool(\"transfer.ipversion\");\n\t\tif (!strcmp(v, \"any\"))\n\t\t\tfamily = 0;\n\t\telse if (!strcmp(v, \"4\") || !strcmp(v, \"ipv4\"))\n\t\t\tfamily = TRANSPORT_FAMILY_IPV4;\n\t\telse if (!strcmp(v, \"6\") || !strcmp(v, \"ipv6\"))\n\t\t\tfamily = TRANSPORT_FAMILY_IPV6;\n\t\telse\n\t\t\treturn error(\"transfer.ipversion: unknown value '%s'\", v);\n\t}\n\nWould we regret to choose 'ipversion' as the variable name, by the\nway? On the command line side, --transfer-protocol-family=ipv4\nmakes it clear that we leave room to support protocols outside the\nInternet protocol family, and existing --ipv4 is grandfathered in\nby making it a synonym to --transfer-protocol-family=ipv4. Calling\nthe variable \"transfer.ipversion\" and still allowing future protocols\noutside the Internet protocol family is rather awkward.\n\nCalling \"transfer.protocolFamily\" would not have such a problem,\nthough.\n\n\n\n[Footnote]\n\n*1* But leave a room to extend it in the future to a comma-separated\n list of them to allow something like \"ipv6,ipv7,ipv8\" (i.e. \"not\n just 'any'---we want to say that 'ipv4' is not welcomed\").\n\n\n> Documentation/config/transfer.txt | 7 +++++++\n> builtin/fetch.c | 11 +++++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt\n> index f5b6245270..cc0e97fbb1 100644\n> --- a/Documentation/config/transfer.txt\n> +++ b/Documentation/config/transfer.txt\n> @@ -69,3 +69,10 @@ transfer.unpackLimit::\n> \tWhen `fetch.unpackLimit` or `receive.unpackLimit` are\n> \tnot set, the value of this variable is used instead.\n> \tThe default value is 100.\n> +\n> +transfer.ipversion::\n> +\tLimit the network operations to the specified version of the transport\n> +\tprotocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or\n> +\t`all` to allow all protocols.\n> +\tSee also linkgit:git-fetch[1] options `--ipv4` and `--ipv6`.\n> +\tThe default value is `all` to allow all protocols.\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 447d28ac29..da01c8f7b3 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -118,6 +118,17 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(k, \"transfer.ipversion\")) {\n> +\t\tif (!strcmp(v, \"all\"))\n> +\t\t\t;\n> +\t\telse if (!strcmp(v, \"4\"))\n> +\t\t\tfamily = TRANSPORT_FAMILY_IPV4;\n> +\t\telse if (!strcmp(v, \"6\"))\n> +\t\t\tfamily = TRANSPORT_FAMILY_IPV6;\n> +\t\telse\n> +\t\t\tdie(_(\"transfer.ipversion can be only 4, 6, or any\"));\n> +\t\treturn 0;\n> +\t}\n> \treturn git_default_config(k, v, cb);\n> }\n"},{"id":"405676","messageId":"20200916163218.GA17726@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200915160357.GC18984@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-16T16:32:18Z","receivedAt":"2020-09-16T20:15:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:\n\n> > If you go that route, we have some \"_F\" macros that take flags. Probably\n> > would make sense to add it more consistently, which lets you convert:\n> > \n> > OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n> > \n> > into:\n> > \n> > OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n> > \n> > but could also be used for other flags.\n> \n> This part (marking of the options) was easy. What's left is finding out if an\n> option was actually specified in the command-line. The ...options[] arrays are\n> not update by parse_options() with what was given, are they?\n\nOh right. Having the list of options is not that helpful because\nadd_options_argv() is actually working off the parsed data in individual\nvariables. Sorry for leading you in a (maybe) wrong direction.\n\nI think this approach would have to be coupled with some mechanism for\nlooking over the original list of options (either saving the original\nargv before parsing, or teaching parse-options the kind of two-pass\n\"don't parse these the first time\" mechanism discussed elsewhere in the\nthread).\n\n> Maybe extend struct option with a field to store given command-line argument\n> (as it was specified) and parse_options() will update the field if\n> PARSE_OPT_RECURSIVE is present in .flags?\n> Is it allowed for parse_options() to modify the options array?\n\nNo, we take the options array as a const pointer, so many callers would\nlikely need to be updated to handle that. Plus it's possible some may\nactually re-use the array multiple times in some cases.\n\n> Or is it possible to use something in parse-options.h API to note the\n> arguments somewhere while they are parse? I mean, there are\n> parse_options_start/step/end, can cmd_fetch argument parsing use those\n> so that the options marked recursive can be saved for sub-fetches?\n\nPossibly the step-wise parsing could help. But I think it might be\neasier to just let parse_options() save a copy of parsed options. And\nthen our PARSE_OPT_RECURSIVE really becomes PARSE_OPT_SAVE or similar,\nwhich would cause parse-options to save the original option (and any\nvalue argument) in its original form.\n\nThere's one slight complication, which is how the array of saved options\ngets communicated back to the caller. Leaving them in the original argv\nprobably isn't a good idea (because the caller relies on it having\noptions removed in order to find the non-option arguments).\n\nAdding a new strvec pointer to parse_options() works, but means updating\nall of the callers, most of which will pass NULL. Possibly the existing\n\"flags\" parameter to parse_options() could grow into a struct. That\nrequires modifying each caller, but at least solves the problem once and\nfor all.\n\nAnother option is to stick it into parse_opt_ctx_t. That's used only be\nstep-wise callers, of which there are very few.\n\n-Peff\n"},{"id":"405677","messageId":"20200916201830.GA44969@coredump.intra.peff.net","threadId":"54236","inReplyTo":"xmqqtuvxwkbz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-16T20:18:30Z","receivedAt":"2020-09-16T20:19:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 16, 2020 at 01:14:08PM -0700, Junio C Hamano wrote:\n\n> Alex Riesen writes:\n> \n> > Affecting the transfers caused by git-fetch, the\n> > option allows to control network operations similar\n> > to --ipv4 and --ipv6 options.\n> > ...\n> > Something like this?\n> \n> Good start.\n> [...]\n\nA lot of your comments apply to the \"something like this\" suggestion I\njust posted, so I wanted to save a round-trip and say: yes, I agree with\nall of your suggestions here.\n\nAdding a command-line option for \"all\" is a good idea, but will probably\nmean needing to add the \"unset\" sentinel value I mentioned in the other\nemail.\n\n> Would we regret to choose 'ipversion' as the variable name, by the\n> way? On the command line side, --transfer-protocol-family=ipv4\n> makes it clear that we leave room to support protocols outside the\n> Internet protocol family, and existing --ipv4 is grandfathered in\n> by making it a synonym to --transfer-protocol-family=ipv4. Calling\n> the variable \"transfer.ipversion\" and still allowing future protocols\n> outside the Internet protocol family is rather awkward.\n> \n> Calling \"transfer.protocolFamily\" would not have such a problem,\n> though.\n\nI agree that's a better name. I'm still on the fence about \"transfer\"\nversus \"core\".\n\n-Peff\n"},{"id":"405686","messageId":"20200916163427.GB17726@coredump.intra.peff.net","threadId":"54236","inReplyTo":"xmqqa6xqzpx9.fsf@gitster.c.googlers.com","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-16T16:34:27Z","receivedAt":"2020-09-16T20:47:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 02:32:50PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So I think the best you could do is:\n> >\n> > 1. Keep two separate option lists, \"parent\" and \"child\". The parent\n> > list has \"--all\" in it. The child list has stuff like \"--ipv6\".\n> >\n> > 2. Parse using the parent list with PARSE_OPT_KEEP_UNKNOWN. That lets\n> > you decide whether we're in a mode that is spawning child fetch\n> > processes.\n> \n> Hmph, I vaguely recall discussion about cascading options[] list but\n> do not find anything that may be involved in an implementation like\n> that in . I agree that neither of the above is so\n> attractive.\n\nI think we just use KEEP_UNKNOWN in those cases and ignore any downsides\nto it.\n\n> > I guess parse-options could provide a MAYBE_PASSTHRU flag. On the first\n> > parse_options() call, it would skip over any such options, leaving them\n> > in argv. On the second, the caller would tell it to actually parse them.\n> \n> Or calling it USR1, which is a good way to make it crystal clear\n> that parse_options() API does not do anything to it. The code like\n> \"builtin/fetch.c\" can locally give it a more meaningful name with\n> \"#define PARSE_OPT_RECURSIVE PARSE_OPT_USR1\". if recursive is the\n> appropriate name for the bit in the context of the options[] array.\n\nAh, that's a good suggestion. My earlier \"USER\" suggestion was\ntongue-in-cheek, because I think it makes the resulting options list\nquite confusing. But a local #define fixes that nicely.\n\nThat said, it sounds from the other part of the thread like we'll need\nbetter parse-options support anyway, so this \"noop flag bit\" idea\nprobably isn't a good direction anyway.\n\n-Peff\n"},{"id":"405701","messageId":"xmqqk0wtv204.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"xmqqtuvxwkbz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T21:35:23Z","receivedAt":"2020-09-16T22:14:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Also, we should follow the usual \"the last one wins\" for a\n> configuration variable like this, which is *not* a multi-valued\n> variable. So the config parsing would look more like this:\n>\n> \tif (!strcmp(k, \"transfer.ipversion\")) {\n> \t\tif (!v)\n> \t\t\treturn config_error_nonbool(\"transfer.ipversion\");\n> \t\tif (!strcmp(v, \"any\"))\n> \t\t\tfamily = 0;\n> \t\telse if (!strcmp(v, \"4\") || !strcmp(v, \"ipv4\"))\n> \t\t\tfamily = TRANSPORT_FAMILY_IPV4;\n> \t\telse if (!strcmp(v, \"6\") || !strcmp(v, \"ipv6\"))\n> \t\t\tfamily = TRANSPORT_FAMILY_IPV6;\n> \t\telse\n> \t\t\treturn error(\"transfer.ipversion: unknown value '%s'\", v);\n> \t}\n>\n> Would we regret to choose 'ipversion' as the variable name, by the\n> way? On the command line side, --transfer-protocol-family=ipv4\n> makes it clear that we leave room to support protocols outside the\n> Internet protocol family, and existing --ipv4 is grandfathered in\n> by making it a synonym to --transfer-protocol-family=ipv4. Calling\n> the variable \"transfer.ipversion\" and still allowing future protocols\n> outside the Internet protocol family is rather awkward.\n>\n> Calling \"transfer.protocolFamily\" would not have such a problem,\n> though.\n\nIn case it wasn't clear, I consider the current TRANSPORT_FAMILY_ALL\na misnomer. It's not like specifying \"all\" will make us use both\nipv4 and ipv6 at the same time0---it just indicates our lack of\npreference, i.e. \"any transport protocol family would do\".\n\nI mention this because this topic starts to expose that 'lack of\npreference' to the end user; I do not think we want to use \"all\"\nas the potential value for the command line option or the\nconfiguration variable.\n\nThanks.\n"},{"id":"405702","messageId":"xmqqsgbhv2ot.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200916163427.GB17726@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T21:20:34Z","receivedAt":"2020-09-16T22:17:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That said, it sounds from the other part of the thread like we'll need\n> better parse-options support anyway, so this \"noop flag bit\" idea\n> probably isn't a good direction anyway.\n\nOK. Thanks.\n"},{"id":"405705","messageId":"xmqqbli5uyj4.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200916201830.GA44969@coredump.intra.peff.net","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T22:50:23Z","receivedAt":"2020-09-16T22:50:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Adding a command-line option for \"all\" is a good idea, but will probably\n> mean needing to add the \"unset\" sentinel value I mentioned in the other\n> email.\n\nSorry, I do not quite follow. I thought that assigning the\n(misnamed --- see other mail) ALL to the \"family\" variable would be\nsufficient?\n\n enum transport_family {\n TRANSPORT_FAMILY_ALL = 0,\n TRANSPORT_FAMILY_IPV4,\n TRANSPORT_FAMILY_IPV6\n };\n"},{"id":"405707","messageId":"xmqq4knxuyfz.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"xmqqbli5uyj4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T22:52:16Z","receivedAt":"2020-09-16T22:52:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Adding a command-line option for \"all\" is a good idea, but will probably\n>> mean needing to add the \"unset\" sentinel value I mentioned in the other\n>> email.\n>\n> Sorry, I do not quite follow. I thought that assigning the\n> (misnamed --- see other mail) ALL to the \"family\" variable would be\n> sufficient?\n>\n> enum transport_family {\n> TRANSPORT_FAMILY_ALL = 0,\n> TRANSPORT_FAMILY_IPV4,\n> TRANSPORT_FAMILY_IPV6\n> };\n\nAh, I see. We want a way to tell \"nobody has set it from the command\nline or the config\" and \"we were explicitly told to accept any\"\napart.\n\nBut wouldn't the usual \"read config first and then override from the\ncommand line\" handle that without \"not yet set\" value? I thought we\nby default accept any.\n\n"},{"id":"405713","messageId":"20200917004828.GA2442845@coredump.intra.peff.net","threadId":"54236","inReplyTo":"xmqq4knxuyfz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T00:48:28Z","receivedAt":"2020-09-17T00:55:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 16, 2020 at 03:52:16PM -0700, Junio C Hamano wrote:\n\n> >> Adding a command-line option for \"all\" is a good idea, but will probably\n> >> mean needing to add the \"unset\" sentinel value I mentioned in the other\n> >> email.\n> >\n> > Sorry, I do not quite follow. I thought that assigning the\n> > (misnamed --- see other mail) ALL to the \"family\" variable would be\n> > sufficient?\n> >\n> > enum transport_family {\n> > TRANSPORT_FAMILY_ALL = 0,\n> > TRANSPORT_FAMILY_IPV4,\n> > TRANSPORT_FAMILY_IPV6\n> > };\n> \n> Ah, I see. We want a way to tell \"nobody has set it from the command\n> line or the config\" and \"we were explicitly told to accept any\"\n> apart.\n> \n> But wouldn't the usual \"read config first and then override from the\n> command line\" handle that without \"not yet set\" value? I thought we\n> by default accept any.\n\nIt would, if each individual program does it in that order. But that\nmeans every caller of the transport code needs to be updated to handle\nthe config. That might not be that bad (after all, they have to take\n--ipv6 etc, options, and I guess they'd need a new \"any\" option).\n\nMy suggestion elsewhere was to have an \"unset\" value, and then resolve\nit at the time-of-use, something like:\n\ndiff --git a/transport.c b/transport.c\nindex 43e24bf1e5..6414a847ae 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -248,6 +248,9 @@ static int connect_setup(struct transport *transport, int for_push)\n \tif (data->conn)\n \t\treturn 0;\n \n+\tif (transport->family == TRANSPORT_FAMILY_UNSET)\n+\t\ttransport->family = transport_family_config;\n+\n \tswitch (transport->family) {\n \tcase TRANSPORT_FAMILY_ALL: break;\n \tcase TRANSPORT_FAMILY_IPV4: flags |= CONNECT_IPV4; break;\n\nbut I am happy either way as long as the code does the right thing.\n\n-Peff\n"},{"id":"405714","messageId":"xmqqimcdte3g.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200917004828.GA2442845@coredump.intra.peff.net","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T00:57:07Z","receivedAt":"2020-09-17T00:57:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My suggestion elsewhere was to have an \"unset\" value, and then resolve\n> it at the time-of-use, something like:\n>\n> diff --git a/transport.c b/transport.c\n> index 43e24bf1e5..6414a847ae 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -248,6 +248,9 @@ static int connect_setup(struct transport *transport, int for_push)\n> \tif (data->conn)\n> \t\treturn 0;\n> \n> +\tif (transport->family == TRANSPORT_FAMILY_UNSET)\n> +\t\ttransport->family = transport_family_config;\n> +\n\nAh, OK, if we want to configure it the other way around, yes, we\nneed \"the command line didn't say any\" value. The context of the\n\"elsewhere\" discussion was wnat I was missing (I'd happily blame\nvger for not delivering mails in order ;-).\n\n"},{"id":"405730","messageId":"20200917080418.GA8079@pflmari","threadId":"54236","inReplyTo":"xmqqtuvxwkbz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T08:04:18Z","receivedAt":"2020-09-17T08:11:30Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Wed, Sep 16, 2020 22:14:08 +0200:\n> How about introducing a new command line option\n> \n> \t--transfer-protocol-family=(\"any\"|)\n> \n> where is either \"ipv4\" or \"ipv6\" [*1*], and make existing\n> \"--ipv4\" a synonym for \"--transfer-protocol-family=ipv4\" and \"--ipv6\"\n> for \"--transfer-protocol-family=ipv6\"\n> \n> With such an extended command line override, we can override\n> configured \n> \n> \t[transfer]\n> \t\tipversion = 6\n> \n\nSo the config option starts looking like \"transfer.protocols\" and multi-value?\nWith the command-line option named \"--transfer-protocol=\", allowing multiple\nspecification, and with \"any\" value taking precedence if specified anywhere?\n\n\n"},{"id":"405731","messageId":"20200917080732.GB8079@pflmari","threadId":"54236","inReplyTo":"20200916200203.GA37225@coredump.intra.peff.net","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T08:07:32Z","receivedAt":"2020-09-17T08:13:37Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Wed, Sep 16, 2020 22:02:03 +0200:\n> On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:\n> \n> > Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:\n> > > On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n> > > > Sure! Thinking about it, I actually would have preferred to have both: a\n> > > > config option and a command-line option. So that I can set --ipv4 in, say,\n> > > > ~/.config/git/config file, but still have the option to try --ipv6 from time\n> > > > to time to check if the network setup magically fixed itself.\n> > > > \n> > > > What would the preferred name for that config option be? fetch.ipv?\n> > > \n> > > It looks like we've got similar options for clone/pull (which are really\n> > > fetch under the hood of course) and push. We have the \"transfer.*\"\n> > > namespace which applies to both already. So maybe \"transfer.ipversion\"\n> > > or something?\n> > \n> > Something like this?\n> \n> That's the right direction, but I think we'd want to make sure it\n> impacted all of the spots that allow switching. \"clone\" on the fetching\n> side, but probably also \"push\".\n\nAh sorry. Missed that push case.\n\n"},{"id":"405732","messageId":"20200917081854.GC8079@pflmari","threadId":"54236","inReplyTo":"xmqqtuvxwkbz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T08:18:54Z","receivedAt":"2020-09-17T08:19:12Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Wed, Sep 16, 2020 22:14:08 +0200:\n> Alex Riesen writes:\n> \n> > Affecting the transfers caused by git-fetch, the\n> > option allows to control network operations similar\n> > to --ipv4 and --ipv6 options.\n> > ...\n> > Something like this?\n> \n> Also, we should follow the usual \"the last one wins\" for a\n> configuration variable like this, which is *not* a multi-valued\n> variable. ...\n...\n> [Footnote]\n> \n> *1* But leave a room to extend it in the future to a comma-separated\n> list of them to allow something like \"ipv6,ipv7,ipv8\" (i.e. \"not\n> just 'any'---we want to say that 'ipv4' is not welcomed\").\n\nI think this footnote is the best description of this option. From what\nI gathered, it looks like really a list of protocols the networking code\nis allowed to try to reach the remote.\n\nThe above is orthogonal to how it given on the command-line: there it can be\n\"unset\". It is just not \"unset\" for the networking code, rather reset to the\ndefault list of protocols.\n"},{"id":"405746","messageId":"20200917132047.GA14771@pflmari","threadId":"54236","inReplyTo":"20200916200203.GA37225@coredump.intra.peff.net","subject":"[PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T13:20:47Z","receivedAt":"2020-09-17T13:25:46Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Affecting the transfers initiated by fetch and push,\nthe option allows to control network operations similar\nto --ipv4 and --ipv6 options.\n\nSuggested-by: Jeff King \nSigned-off-by: Alex Riesen \n---\n\nJeff King, Wed, Sep 16, 2020 22:02:03 +0200:\n> On Tue, Sep 15, 2020 at 03:54:28PM +0200, Alex Riesen wrote:\n> \n> > Jeff King, Tue, Sep 15, 2020 15:05:06 +0200:\n> > > On Tue, Sep 15, 2020 at 01:50:25PM +0200, Alex Riesen wrote:\n> > > > Sure! Thinking about it, I actually would have preferred to have both: a\n> > > > config option and a command-line option. So that I can set --ipv4 in, say,\n> > > > ~/.config/git/config file, but still have the option to try --ipv6 from time\n> > > > to time to check if the network setup magically fixed itself.\n> > > > \n> > > > What would the preferred name for that config option be? fetch.ipv?\n> > > \n> > > It looks like we've got similar options for clone/pull (which are really\n> > > fetch under the hood of course) and push. We have the \"transfer.*\"\n> > > namespace which applies to both already. So maybe \"transfer.ipversion\"\n> > > or something?\n> > \n> > Something like this?\n> \n> That's the right direction, but I think we'd want to make sure it\n> impacted all of the spots that allow switching. \"clone\" on the fetching\n> side, but probably also \"push\".\n> \n> Each of those commands could learn the same config, but it might be\n> easier to just enforce it in the transport layer. Something like this\n> maybe (compiled but not tested):\n\nI have merged the patches.\n\n> A few notes:\n> \n> - it cheats a little by noting that command-line options can't get it\n> to \"ALL\". It might be a bit cleaner if we have an explicit\n> TRANSPORT_FAMILY_UNSET, and then resolve the default at look-up\n> time.\n> \n> - it probably needs more \"if (family)\" spots in each command, which is\n> unfortunate (which again might go away if \"UNSET\" is 0).\n\nAs this is still being discussed, and because I still imagine the\ntransport protocol family as a list of allowed protocols, this part\nis not in the patch below.\n\n> - I waffled on the name. transfer.* is where we put things that apply\n> to both push/fetch, but it's usually more related to the git layer.\n> This is more of a core networking decision, and if we added other\n> network programs, I'd expect them to respect it. So maybe \"core.\" is\n> a better namespace.\n\nI agree.\n\n Documentation/config/core.txt | 7 +++++++\n builtin/fetch.c | 3 ++-\n builtin/push.c | 3 ++-\n transport.c | 19 +++++++++++++++++++\n 4 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 74619a9c03..dcb7db9799 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -626,3 +626,10 @@ core.abbrev::\n \tin your repository, which hopefully is enough for\n \tabbreviated object names to stay unique for some time.\n \tThe minimum length is 4.\n+\n+core.ipversion::\n+\tLimit the network operations to the specified version of the transport\n+\tprotocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or\n+\t`all` to allow all protocols.\n+\tSee also linkgit:git-fetch[1] options `--ipv4` and `--ipv6`.\n+\tThe default value is `all` to allow all protocols.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 447d28ac29..41f82d61d7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1248,7 +1248,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)\n \n \ttransport = transport_get(remote, NULL);\n \ttransport_set_verbosity(transport, verbosity, progress);\n-\ttransport->family = family;\n+\tif (family)\n+\t\ttransport->family = family;\n \tif (upload_pack)\n \t\tset_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);\n \tif (keep)\ndiff --git a/builtin/push.c b/builtin/push.c\nindex bc94078e72..f7a40b65cd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -343,7 +343,8 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \tchar *anon_url = transport_anonymize_url(transport->url);\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n-\ttransport->family = family;\n+\tif (family)\n+\t\ttransport->family = family;\n \n \tif (receivepack)\n \t\ttransport_set_option(transport,\ndiff --git a/transport.c b/transport.c\nindex b41386eccb..e16c339f3e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -922,6 +922,23 @@ static struct transport_vtable builtin_smart_vtable = {\n \tdisconnect_git\n };\n \n+static enum transport_family default_transport_family(void)\n+{\n+\tstatic const char key[] = \"core.ipversion\";\n+\tconst char *v;\n+\n+\tif (git_config_get_string_const(key, &v))\n+\t\treturn TRANSPORT_FAMILY_ALL;\n+\tif (!strcmp(v, \"all\"))\n+\t\treturn TRANSPORT_FAMILY_ALL;\n+\tif (!strcmp(v, \"ipv4\"))\n+\t\treturn TRANSPORT_FAMILY_IPV4;\n+\tif (!strcmp(v, \"ipv6\"))\n+\t\treturn TRANSPORT_FAMILY_IPV6;\n+\n+\tdie(_(\"%s: unknown value '%s'\"), key, v);\n+}\n+\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tconst char *helper;\n@@ -951,6 +968,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\t\thelper = xstrndup(url, p - url);\n \t}\n \n+\tret->family = default_transport_family();\n+\n \tif (helper) {\n \t\ttransport_helper_init(ret, helper);\n \t} else if (starts_with(url, \"rsync:\")) {\n-- \n2.28.0.22.gfab0d4627e\n"},{"id":"405747","messageId":"20200917133153.GA3038002@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200917132047.GA14771@pflmari","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T13:31:53Z","receivedAt":"2020-09-17T13:33:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:\n\n> Affecting the transfers initiated by fetch and push,\n> the option allows to control network operations similar\n> to --ipv4 and --ipv6 options.\n> \n> Suggested-by: Jeff King \n> Signed-off-by: Alex Riesen \n\nI think this misses some of the excellent suggestions from Junio\n(naming, and the ability to override from the command line).\n\n-Peff\n"},{"id":"405749","messageId":"20200917132612.GD8079@pflmari","threadId":"54236","inReplyTo":"20200917132047.GA14771@pflmari","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T13:26:12Z","receivedAt":"2020-09-17T13:45:52Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Alex Riesen, Thu, Sep 17, 2020 15:20:47 +0200:\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> index 74619a9c03..dcb7db9799 100644\n> --- a/Documentation/config/core.txt\n> +++ b/Documentation/config/core.txt\n> @@ -626,3 +626,10 @@ core.abbrev::\n> \tin your repository, which hopefully is enough for\n> \tabbreviated object names to stay unique for some time.\n> \tThe minimum length is 4.\n> +\n> +core.ipversion::\n> +\tLimit the network operations to the specified version of the transport\n> +\tprotocol. Can be specified as `4` to allow IPv4 only, `6` for IPv6, or\n> +\t`all` to allow all protocols.\n\nEh. Option values are \"ipv4\" and \"ipv6\" indeed, not \"4\" and \"6\".\n\nAnd I compiled and ran the code by now. Feels ok.\n\n"},{"id":"405753","messageId":"20200917133525.GE8079@pflmari","threadId":"54236","inReplyTo":"20200917133153.GA3038002@coredump.intra.peff.net","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T13:35:25Z","receivedAt":"2020-09-17T14:31:05Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:\n> On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:\n> \n> > Affecting the transfers initiated by fetch and push,\n> > the option allows to control network operations similar\n> > to --ipv4 and --ipv6 options.\n> > \n> > Suggested-by: Jeff King \n> > Signed-off-by: Alex Riesen \n> \n> I think this misses some of the excellent suggestions from Junio\n> (naming, and the ability to override from the command line).\n\nIt does, sorry. Also the suggestions to the issue of consistently passing the\noptions to helper programs haven't been collected.\nHaven't had the time yet.\n\n"},{"id":"405754","messageId":"20200917143339.GF8079@pflmari","threadId":"54236","inReplyTo":"20200916163218.GA17726@coredump.intra.peff.net","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T14:33:39Z","receivedAt":"2020-09-17T14:39:00Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Wed, Sep 16, 2020 18:32:18 +0200:\n> On Tue, Sep 15, 2020 at 06:03:57PM +0200, Alex Riesen wrote:\n> \n> > > If you go that route, we have some \"_F\" macros that take flags. Probably\n> > > would make sense to add it more consistently, which lets you convert:\n> > > \n> > > OPT_BOOL('f', \"foo\", &foo, \"the foo option\");\n> > > \n> > > into:\n> > > \n> > > OPT_BOOL_F('f', \"foo\", &foo, \"the foo option\", PARSE_OPT_RECURSIVE);\n> > > \n> > > but could also be used for other flags.\n> > \n> > This part (marking of the options) was easy. What's left is finding out if an\n> > option was actually specified in the command-line. The ...options[] arrays are\n> > not update by parse_options() with what was given, are they?\n> \n> Oh right. Having the list of options is not that helpful because\n> add_options_argv() is actually working off the parsed data in individual\n> variables. Sorry for leading you in a (maybe) wrong direction.\n...\n> > Or is it possible to use something in parse-options.h API to note the\n> > arguments somewhere while they are parsed? I mean, there are\n> > parse_options_start/step/end, can cmd_fetch argument parsing use those\n> > so that the options marked recursive can be saved for sub-fetches?\n> \n> Possibly the step-wise parsing could help. But I think it might be\n> easier to just let parse_options() save a copy of parsed options. And\n> then our PARSE_OPT_RECURSIVE really becomes PARSE_OPT_SAVE or similar,\n> which would cause parse-options to save the original option (and any\n> value argument) in its original form.\n> \n> There's one slight complication, which is how the array of saved options\n> gets communicated back to the caller. Leaving them in the original argv\n> probably isn't a good idea (because the caller relies on it having\n> options removed in order to find the non-option arguments).\n> \n> Adding a new strvec pointer to parse_options() works, but means updating\n> all of the callers, most of which will pass NULL. Possibly the existing\n> \"flags\" parameter to parse_options() could grow into a struct. That\n> requires modifying each caller, but at least solves the problem once and\n> for all.\n\nWith such complication a step-wise parsing sounds easier, given that at the\nmoment there is only one user for the feature. Are there *existing* callers\nof parse_options with similar requirements?\n\nI feel that doing this kind of selection work in parse_options is an overkill:\nif it is specific for just this use case, the implementation might be more\ncomplex than necessary, while profiting just one caller.\n\n> Another option is to stick it into parse_opt_ctx_t. That's used only be\n> step-wise callers, of which there are very few.\n\nDoes that mean that currently there is no way to find out which option\ncorresponds to the last parsed command-line argument after a call to\nparse_options_step? Which in turn makes the marking of recursive options\ninaccessible to step-wise command line parsing code, right?\n\n"},{"id":"405756","messageId":"20200917145142.GA3076467@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200917133525.GE8079@pflmari","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T14:51:42Z","receivedAt":"2020-09-17T14:52:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 03:35:25PM +0200, Alex Riesen wrote:\n\n> Jeff King, Thu, Sep 17, 2020 15:31:53 +0200:\n> > On Thu, Sep 17, 2020 at 03:20:47PM +0200, Alex Riesen wrote:\n> > \n> > > Affecting the transfers initiated by fetch and push,\n> > > the option allows to control network operations similar\n> > > to --ipv4 and --ipv6 options.\n> > > \n> > > Suggested-by: Jeff King \n> > > Signed-off-by: Alex Riesen \n> > \n> > I think this misses some of the excellent suggestions from Junio\n> > (naming, and the ability to override from the command line).\n> \n> It does, sorry. Also the suggestions to the issue of consistently passing the\n> options to helper programs haven't been collected.\n> Haven't had the time yet.\n\nNo problem, and no rush. I just wanted to make sure those bits didn't\nget overlooked.\n\n-Peff\n"},{"id":"405763","messageId":"20200917151730.GG8079@pflmari","threadId":"54236","inReplyTo":"20200917145142.GA3076467@coredump.intra.peff.net","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T15:17:30Z","receivedAt":"2020-09-17T15:42:08Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:\n> No problem, and no rush. I just wanted to make sure those bits didn't\n> get overlooked.\n\nI'll try and do my best :)\n\n\n"},{"id":"405767","messageId":"20200917160556.GH8079@pflmari","threadId":"54236","inReplyTo":"20200917132047.GA14771@pflmari","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T16:05:56Z","receivedAt":"2020-09-17T16:23:22Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Alex Riesen, Thu, Sep 17, 2020 15:20:47 +0200:\n> diff --git a/transport.c b/transport.c\n> index b41386eccb..e16c339f3e 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -922,6 +922,23 @@ static struct transport_vtable builtin_smart_vtable = {\n> \tdisconnect_git\n> };\n> \n> +static enum transport_family default_transport_family(void)\n> +{\n> +\tstatic const char key[] = \"core.ipversion\";\n> +\tconst char *v;\n> +\n> +\tif (git_config_get_string_const(key, &v))\n\nSorry about that. git_config_get_string_tmp, indeed.\n\n"},{"id":"405794","messageId":"20200917140254.GA28281@pflmari","threadId":"54236","inReplyTo":"xmqqk0wtv204.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-17T14:02:54Z","receivedAt":"2020-09-17T19:55:59Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Wed, Sep 16, 2020 23:35:23 +0200:\n> Junio C Hamano writes:\n> > Would we regret to choose 'ipversion' as the variable name, by the\n> > way? On the command line side, --transfer-protocol-family=ipv4\n> > makes it clear that we leave room to support protocols outside the\n> > Internet protocol family, and existing --ipv4 is grandfathered in\n> > by making it a synonym to --transfer-protocol-family=ipv4. Calling\n> > the variable \"transfer.ipversion\" and still allowing future protocols\n> > outside the Internet protocol family is rather awkward.\n> >\n> > Calling \"transfer.protocolFamily\" would not have such a problem,\n> > though.\n> \n> In case it wasn't clear, I consider the current TRANSPORT_FAMILY_ALL\n> a misnomer. It's not like specifying \"all\" will make us use both\n> ipv4 and ipv6 at the same time0---it just indicates our lack of\n> preference, i.e. \"any transport protocol family would do\".\n> \n> I mention this because this topic starts to expose that 'lack of\n> preference' to the end user; I do not think we want to use \"all\"\n> as the potential value for the command line option or the\n> configuration variable.\n\nIf the configuration variable is allowed to be set to that \"lack of\npreference\" value, we kind of have a command line option for it:\n\n git -c transfer.protocolFamily=any fetch ...\n\n\n"},{"id":"405808","messageId":"xmqqbli4ox0h.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200917140254.GA28281@pflmari","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T22:31:58Z","receivedAt":"2020-09-17T22:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> If the configuration variable is allowed to be set to that \"lack of\n> preference\" value, we kind of have a command line option for it:\n>\n> git -c transfer.protocolFamily=any fetch ...\n\nYes.\n\nBut we typically do not count that as being able to override from\nthe command line. If \"git fetch --ipv4\" will defeat the configured\n\"transfer.protoclFamily=ipv6\", the users will expect there is some\nway to do the same and say they accept any protocol family to be\nused.\n\nThanks.\n"},{"id":"405840","messageId":"20200918071647.GA17896@pflmari","threadId":"54236","inReplyTo":"xmqqbli4ox0h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-18T07:16:47Z","receivedAt":"2020-09-18T07:17:05Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Fri, Sep 18, 2020 00:31:58 +0200:\n> Alex Riesen writes:\n> \n> > If the configuration variable is allowed to be set to that \"lack of\n> > preference\" value, we kind of have a command line option for it:\n> >\n> > git -c transfer.protocolFamily=any fetch ...\n> \n> Yes.\n> \n> But we typically do not count that as being able to override from\n> the command line. If \"git fetch --ipv4\" will defeat the configured\n> \"transfer.protoclFamily=ipv6\", the users will expect there is some\n> way to do the same and say they accept any protocol family to be\n> used.\n\nMakes sense. I even wondered myself why there is no way to override\nan --ipv4/--ipv6 in command-line back to \"any\" with an option added\nafter it:\n\n $ cat my-fetch\n #!/bin/sh\n git fetch --ipv4 \"$@\"\n\n $ ./my-fetch --ipv46\n\nBut how about making the command-line and config option a list?\n(renaming it to ipversion, as elsewhere in the discussion)\n\n git fetch --ipversions=ipv6,ipv8\n\nGiven multiple times, the last option wins, as usual:\n\n $ cat my-fetch\n #!/bin/sh\n git fetch --ipversions=ipv4 \"$@\"\n\n $ ./my-fetch --ipversions=all\n\nBTW, transport.c already converts transport->family to bit-flags in\nconnect_setup.\n\n"},{"id":"405880","messageId":"xmqq363fnir9.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200918071647.GA17896@pflmari","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T16:37:30Z","receivedAt":"2020-09-18T16:37:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> git fetch --ipversions=ipv6,ipv8\n>\n> Given multiple times, the last option wins, as usual:\n\nJust a clarification on \"the last option wins\".\n\nYou do not mean \"I said v6 earlier but no, I want v8\", with the\nabove. What you mean is that\n\n>\n> $ cat my-fetch\n> #!/bin/sh\n> git fetch --ipversions=ipv4 \"$@\"\n>\n> $ ./my-fetch --ipversions=all\n\nthe argument given to 'my-fetch' overrides what is hardcoded in\n'my-fetch', i.e. \"I said v4, but I take it back; I want to accept\nany\".\n\nI find the above sensible.\n\n> BTW, transport.c already converts transport->family to bit-flags in\n> connect_setup.\n\nYes, that is why I suggested the \"list of acceptable choices\"\napproach as a direction to go in the future, primarily to limit the\nscope of this current work. I do not mind it if you want to bite\nthe whole piece right now, though.\n\nBy the way, I have a mild preference to call the option after the\nphrase \"protocol-family\", without \"IP\", so that we won't be limited\nto Internet protocols. IOW, --ipversions is a bad name for the new\ncommnad line option in my mind.\n\nAs I said elsewhere, I also think TRANSPORT_FAMILY_ALL is a\nmisnomer. When it is specified, we don't use all the available ones\nat the same time. What it says is that we accept use of any\nprotocol families that are supported. It is OK to use ALL in the\nCPP macro as it is merely an internal implementation detail, but if\nwe are going to expose it to end users as one of the choices, I'd\nprefer to use 'any', and not 'all', as the value for the new command\nline option.\n\nThanks.\n"},{"id":"406055","messageId":"20200921163901.GB4541@pflmari","threadId":"54236","inReplyTo":"xmqq363fnir9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2020-09-21T16:39:01Z","receivedAt":"2020-09-21T17:01:00Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Fri, Sep 18, 2020 18:37:30 +0200:\n> Alex Riesen writes:\n> \n> > git fetch --ipversions=ipv6,ipv8\n> >\n> > Given multiple times, the last option wins, as usual:\n> \n> Just a clarification on \"the last option wins\".\n> \n> You do not mean \"I said v6 earlier but no, I want v8\", with the\n> above. What you mean is that\n> \n> >\n> > $ cat my-fetch\n> > #!/bin/sh\n> > git fetch --ipversions=ipv4 \"$@\"\n> >\n> > $ ./my-fetch --ipversions=all\n> \n> the argument given to 'my-fetch' overrides what is hardcoded in\n> 'my-fetch', i.e. \"I said v4, but I take it back; I want to accept\n> any\".\n\nAbsolutely correct.\n\n> I find the above sensible.\n\n... And is precisely my intention.\n\n> > BTW, transport.c already converts transport->family to bit-flags in\n> > connect_setup.\n> \n> Yes, that is why I suggested the \"list of acceptable choices\"\n> approach as a direction to go in the future, primarily to limit the\n> scope of this current work. I do not mind it if you want to bite\n> the whole piece right now, though.\n\nI shall try, I think.\n\n> By the way, I have a mild preference to call the option after the\n> phrase \"protocol-family\", without \"IP\", so that we won't be limited\n> to Internet protocols. IOW, --ipversions is a bad name for the new\n> commnad line option in my mind.\n\nI have nothing against protocol-family, with or without \"transport-\".\nI just didn't want to ... over-generalize it prematurely: currently,\nthe transport family is very fixed on IP on many levels.\n\n> As I said elsewhere, I also think TRANSPORT_FAMILY_ALL is a\n> misnomer. When it is specified, we don't use all the available ones\n> at the same time. What it says is that we accept use of any\n> protocol families that are supported. It is OK to use ALL in the\n> CPP macro as it is merely an internal implementation detail, but if\n> we are going to expose it to end users as one of the choices, I'd\n> prefer to use 'any', and not 'all', as the value for the new command\n> line option.\n\nNoted.\n\nRegards,\nAlex\n\n"},{"id":"406109","messageId":"20200922050331.GB528837@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200921163901.GB4541@pflmari","subject":"Re: [PATCH] config: option transfer.ipversion to set transport protocol version for network fetches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-22T05:03:31Z","receivedAt":"2020-09-22T05:03:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 21, 2020 at 06:39:01PM +0200, Alex Riesen wrote:\n\n> > By the way, I have a mild preference to call the option after the\n> > phrase \"protocol-family\", without \"IP\", so that we won't be limited\n> > to Internet protocols. IOW, --ipversions is a bad name for the new\n> > commnad line option in my mind.\n> \n> I have nothing against protocol-family, with or without \"transport-\".\n> I just didn't want to ... over-generalize it prematurely: currently,\n> the transport family is very fixed on IP on many levels.\n\nI'll echo Junio's mild preference. I like it also because\n\"--ipversion=6\" feels too terse in the argument, but \"--ipversion=ipv6\"\nfeels redundant. Saying \"--protocol-family=ipv6\" is more characters,\nbut strikes me as more clear. (Or \"transport-family\" or whatever).\n\n-Peff\n"},{"id":"406110","messageId":"20200922050855.GC528837@coredump.intra.peff.net","threadId":"54236","inReplyTo":"20200917143339.GF8079@pflmari","subject":"Re: sub-fetches discard --ipv4|6 option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-22T05:08:55Z","receivedAt":"2020-09-22T05:08:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 04:33:39PM +0200, Alex Riesen wrote:\n\n> > Adding a new strvec pointer to parse_options() works, but means updating\n> > all of the callers, most of which will pass NULL. Possibly the existing\n> > \"flags\" parameter to parse_options() could grow into a struct. That\n> > requires modifying each caller, but at least solves the problem once and\n> > for all.\n> \n> With such complication a step-wise parsing sounds easier, given that at the\n> moment there is only one user for the feature. Are there *existing* callers\n> of parse_options with similar requirements?\n\nI don't know offhand. I'd suspect some of the command which take\n--recurse-submodules do something similar, but the number of steps\nbetween the main command the submodule argv may make it awkward to use\na parse-options solution. I don't think we have a \"push to each of these\nremotes\" option the way we do for fetch.\n\n> I feel that doing this kind of selection work in parse_options is an overkill:\n> if it is specific for just this use case, the implementation might be more\n> complex than necessary, while profiting just one caller.\n\nYeah, I agree it's complex, and I'm happy with simpler solutions (or\njust fixing these ones as you did and punting on it for now).\n\n> > Another option is to stick it into parse_opt_ctx_t. That's used only be\n> > step-wise callers, of which there are very few.\n> \n> Does that mean that currently there is no way to find out which option\n> corresponds to the last parsed command-line argument after a call to\n> parse_options_step? Which in turn makes the marking of recursive options\n> inaccessible to step-wise command line parsing code, right?\n\nI'm not super familiar with the internals of parse-options, but it\ndoesn't look like it. Each step consumes an argv and matches it to a\n\"struct option\", but I don't think you get to know which struct it was\nmatched to. It would be reasonable for it to keep a pointer in the\nparse_opt_ctx_t (and of course you'd need some bit in the option struct\nitself to say \"I am a recursive option\").\n\n-Peff\n"},{"id":"412853","messageId":"xmqq1rfhipjm.fsf@gitster.c.googlers.com","threadId":"54236","inReplyTo":"20200917151730.GG8079@pflmari","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T19:55:25Z","receivedAt":"2020-12-22T19:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:\n>> No problem, and no rush. I just wanted to make sure those bits didn't\n>> get overlooked.\n>\n> I'll try and do my best :)\n\nIf I remember correctly, this discussion started as an introduction\nof useful feature, but got stuck at the implementation phase of how\nthe feature is presented at the UI level after we had general\nconcensus on the design.\n\nIt has been about 3 months; has anything happened that I missed? It\nseems that I kept the thread-starter patch in the 'seen' branch, but\nand haven't updated with a later attempt in the discussion.\n\nThanks.\n\n"},{"id":"413675","messageId":"X/bdGIF9A/wKAw5H@pflmari","threadId":"54236","inReplyTo":"xmqq1rfhipjm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] Config option to set the transport protocol version for network fetches","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2021-01-07T10:06:16Z","receivedAt":"2021-01-07T10:09:11Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Tue, Dec 22, 2020 20:55:25 +0100:\n> Alex Riesen writes:\n> > Jeff King, Thu, Sep 17, 2020 16:51:42 +0200:\n> >> No problem, and no rush. I just wanted to make sure those bits didn't\n> >> get overlooked.\n> >\n> > I'll try and do my best :)\n> \n> If I remember correctly, this discussion started as an introduction\n> of useful feature, but got stuck at the implementation phase of how\n> the feature is presented at the UI level after we had general\n> concensus on the design.\n> \n> It has been about 3 months; has anything happened that I missed?\n\nNo, nothing happened.\n\n> It seems that I kept the thread-starter patch in the 'seen' branch,\n> but and haven't updated with a later attempt in the discussion.\n\nSorry. I got very distracted. As my situation seems to persist,\nI cannot promise to do something about it soon.\n\nI keep the branch and the discussion close by, though. Just in case.\n\n"}]}