{"thread":{"id":"46700","subject":"[PATCHv2] pull: honor submodule.recurse config option","startedAt":"2017-09-04T06:31:51Z","lastAt":"2017-09-07T00:54:43Z","messageCount":4,"participants":["Nicolas Morey-Chaisemartin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327533","messageId":"40ecf559-0348-b838-72f7-0ad7746a7072@morey-chaisemartin.com","threadId":"46700","inReplyTo":null,"subject":"[PATCHv2] pull: honor submodule.recurse config option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-04T06:31:41Z","receivedAt":"2017-09-04T06:31:51Z","isPatch":false,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\"git pull\" supports a --recurse-submodules option but does not parse the\nsubmodule.recurse configuration item to set the default for that option.\nMeanwhile \"git fetch\" does support submodule.recurse, producing\nconfusing behavior: when submodule.recurse is enabled, \"git pull\"\nrecursively fetches submodules but does not update them after fetch.\n\nHandle submodule.recurse in \"git pull\" to fix this.\n\nReported-by: Magnus Homann \nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n\nChanges since v1:\n * Cleanup commit message\n * Add test\n * Remove extra var in code and fallthrough to git_default_config\n\n builtin/pull.c | 4 ++++\n t/t5572-pull-submodule.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 7fe281414..ce8ccb15b 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"rebase.autostash\")) {\n \t\tconfig_autostash = git_config_bool(var, value);\n \t\treturn 0;\n+\t} else if (!strcmp(var, \"submodule.recurse\")) {\n+\t\trecurse_submodules = git_config_bool(var, value) ?\n+\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n+\t\treturn 0;\n \t}\n \treturn git_default_config(var, value, cb);\n }\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 077eb07e1..1b3a3f445 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '\n \ttest_path_is_file super/sub/merge_strategy.t\n '\n \n+test_expect_success \"submodule.recurse option triggers recursive pull\" '\n+\ttest_commit -C child merge_strategy_2 &&\n+\tgit -C parent submodule update --remote &&\n+\tgit -C parent add sub &&\n+\tgit -C parent commit -m \"update submodule\" &&\n+\n+\tgit -C super -c submodule.recurse pull --no-rebase &&\n+\ttest_path_is_file super/sub/merge_strategy_2.t\n+'\n+\n test_expect_success 'recursive rebasing pull' '\n \t# change upstream\n \ttest_commit -C child rebase_strategy &&\n-- \n2.14.1.460.g695108176\n\n"},{"id":"327620","messageId":"xmqqvakwaan2.fsf@gitster.mtv.corp.google.com","threadId":"46700","inReplyTo":"40ecf559-0348-b838-72f7-0ad7746a7072@morey-chaisemartin.com","subject":"Re: [PATCHv2] pull: honor submodule.recurse config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T01:17:21Z","receivedAt":"2017-09-06T01:17:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> \"git pull\" supports a --recurse-submodules option but does not parse the\n> submodule.recurse configuration item to set the default for that option.\n> Meanwhile \"git fetch\" does support submodule.recurse, producing\n> confusing behavior: when submodule.recurse is enabled, \"git pull\"\n> recursively fetches submodules but does not update them after fetch.\n>\n> Handle submodule.recurse in \"git pull\" to fix this.\n>\n> Reported-by: Magnus Homann \n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n>\n> Changes since v1:\n> * Cleanup commit message\n> * Add test\n> * Remove extra var in code and fallthrough to git_default_config\n>\n> builtin/pull.c | 4 ++++\n> t/t5572-pull-submodule.sh | 10 ++++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 7fe281414..ce8ccb15b 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char *value, void *cb)\n> \tif (!strcmp(var, \"rebase.autostash\")) {\n> \t\tconfig_autostash = git_config_bool(var, value);\n> \t\treturn 0;\n> +\t} else if (!strcmp(var, \"submodule.recurse\")) {\n> +\t\trecurse_submodules = git_config_bool(var, value) ?\n> +\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n> +\t\treturn 0;\n> \t}\n> \treturn git_default_config(var, value, cb);\n> }\n\nIf I am reading the existing code correctly, things happen in\ncmd_pull() in this order:\n\n - recurse_submodules is a file-scope static that is initialized to\n RECURSE_SUBMODULES_DEFAULT\n\n - pull_options[] is given to parse_options() so that\n submodule-config.c::option_fetch_parse_recurse_submodules() can\n read \"--recurse-submodules=\" from the command line to\n update recurse_submodules.\n\n - git_pull_config() is given to git_config() so that settings in\n the configuration files are read.\n\nCare must be taken to make sure that values given from the command\nline is never overriden by the default value specified in the\nconfiguration system because the order of the second and third items\nin the above are backwards from the usual flow. This patch does not\nseem to have any such provision.\n\nExisting handling of \"--autostash\" vs \"rebase.autostash\" solves this\nissue by having opt_autostash and config_autostash as two separate\nvariables, so I suspect that something similar to it must be there,\nat least, for this new configuration.\n\nIf we want to keep the current code structure, that is. I do not\nrecall if we did not notice the fact that the order of options and\nconfig parsing is backwards and unknowingly worked it around with\ntwo variables when we added the rebase.autostash thing, or we knew\nthe order was unusual but there was a good reason to keep that\nunusual order (iow, if we simply swapped the order of\nparse_options() and git_config() calls, there are things that will\nbreak). \n\nIf it is not the latter, perhaps we may want to flip the order of\nconfig parsing and option parsing around? That will allow us to fix\nthe handling of autostash thing to use only one variable, and also\nfix your patch to do the right thing.\n\n> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> index 077eb07e1..1b3a3f445 100755\n> --- a/t/t5572-pull-submodule.sh\n> +++ b/t/t5572-pull-submodule.sh\n> @@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '\n> \ttest_path_is_file super/sub/merge_strategy.t\n> '\n> \n> +test_expect_success \"submodule.recurse option triggers recursive pull\" '\n> +\ttest_commit -C child merge_strategy_2 &&\n> +\tgit -C parent submodule update --remote &&\n> +\tgit -C parent add sub &&\n> +\tgit -C parent commit -m \"update submodule\" &&\n> +\n> +\tgit -C super -c submodule.recurse pull --no-rebase &&\n> +\ttest_path_is_file super/sub/merge_strategy_2.t\n> +'\n\nThis new test does not test interactions with submodule.recurse\nconfiguration and --recurse-submodules= from the command\nline. It would be necessary to add tests to cover the permutations\nin addition to the basic test we see above.\n\nThanks.\n"},{"id":"327634","messageId":"c3842980-22ef-5a8c-2895-90c48a97ed71@suse.de","threadId":"46700","inReplyTo":"xmqqvakwaan2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] pull: honor submodule.recurse config option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.de","sentAt":"2017-09-06T06:25:06Z","receivedAt":"2017-09-06T06:25:14Z","isPatch":false,"sender":{"key":"nmoreychaisemartin@suse.de","avatar":"https://gravatar.com/avatar/5546322ccb9067f56b6939d9d5c758a40cab5b978b1379fd6ec8ab9b8a6a12b1?d=mp&s=160"},"body":"\n\nLe 06/09/2017 à 03:17, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> \"git pull\" supports a --recurse-submodules option but does not parse the\n>> submodule.recurse configuration item to set the default for that option.\n>> Meanwhile \"git fetch\" does support submodule.recurse, producing\n>> confusing behavior: when submodule.recurse is enabled, \"git pull\"\n>> recursively fetches submodules but does not update them after fetch.\n>>\n>> Handle submodule.recurse in \"git pull\" to fix this.\n>>\n>> Reported-by: Magnus Homann \n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>>\n>> Changes since v1:\n>> * Cleanup commit message\n>> * Add test\n>> * Remove extra var in code and fallthrough to git_default_config\n>>\n>> builtin/pull.c | 4 ++++\n>> t/t5572-pull-submodule.sh | 10 ++++++++++\n>> 2 files changed, 14 insertions(+)\n>>\n>> diff --git a/builtin/pull.c b/builtin/pull.c\n>> index 7fe281414..ce8ccb15b 100644\n>> --- a/builtin/pull.c\n>> +++ b/builtin/pull.c\n>> @@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char *value, void *cb)\n>> \tif (!strcmp(var, \"rebase.autostash\")) {\n>> \t\tconfig_autostash = git_config_bool(var, value);\n>> \t\treturn 0;\n>> +\t} else if (!strcmp(var, \"submodule.recurse\")) {\n>> +\t\trecurse_submodules = git_config_bool(var, value) ?\n>> +\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n>> +\t\treturn 0;\n>> \t}\n>> \treturn git_default_config(var, value, cb);\n>> }\n> If I am reading the existing code correctly, things happen in\n> cmd_pull() in this order:\n>\n> - recurse_submodules is a file-scope static that is initialized to\n> RECURSE_SUBMODULES_DEFAULT\n>\n> - pull_options[] is given to parse_options() so that\n> submodule-config.c::option_fetch_parse_recurse_submodules() can\n> read \"--recurse-submodules=\" from the command line to\n> update recurse_submodules.\n>\n> - git_pull_config() is given to git_config() so that settings in\n> the configuration files are read.\n>\n> Care must be taken to make sure that values given from the command\n> line is never overriden by the default value specified in the\n> configuration system because the order of the second and third items\n> in the above are backwards from the usual flow. This patch does not\n> seem to have any such provision.\n>\n> Existing handling of \"--autostash\" vs \"rebase.autostash\" solves this\n> issue by having opt_autostash and config_autostash as two separate\n> variables, so I suspect that something similar to it must be there,\n> at least, for this new configuration.\n>\n> If we want to keep the current code structure, that is. I do not\n> recall if we did not notice the fact that the order of options and\n> config parsing is backwards and unknowingly worked it around with\n> two variables when we added the rebase.autostash thing, or we knew\n> the order was unusual but there was a good reason to keep that\n> unusual order (iow, if we simply swapped the order of\n> parse_options() and git_config() calls, there are things that will\n> break). \n>\n> If it is not the latter, perhaps we may want to flip the order of\n> config parsing and option parsing around? That will allow us to fix\n> the handling of autostash thing to use only one variable, and also\n> fix your patch to do the right thing.\n\nI see what you mean.\nIt looks like switching the code around works but I think there still needs to be 2variables for autstash for this piece of code:\n\n    if (!opt_rebase && opt_autostash != -1)\n        die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n\nThe config option should not cause git pull to die when not using --rebase, the CLI option should.\n\n>> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n>> index 077eb07e1..1b3a3f445 100755\n>> --- a/t/t5572-pull-submodule.sh\n>> +++ b/t/t5572-pull-submodule.sh\n>> @@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '\n>> \ttest_path_is_file super/sub/merge_strategy.t\n>> '\n>> \n>> +test_expect_success \"submodule.recurse option triggers recursive pull\" '\n>> +\ttest_commit -C child merge_strategy_2 &&\n>> +\tgit -C parent submodule update --remote &&\n>> +\tgit -C parent add sub &&\n>> +\tgit -C parent commit -m \"update submodule\" &&\n>> +\n>> +\tgit -C super -c submodule.recurse pull --no-rebase &&\n>> +\ttest_path_is_file super/sub/merge_strategy_2.t\n>> +'\n> This new test does not test interactions with submodule.recurse\n> configuration and --recurse-submodules= from the command\n> line. It would be necessary to add tests to cover the permutations\n> in addition to the basic test we see above.\n\nWill fix\n\nThanks\n\nNicolas\n"},{"id":"327684","messageId":"xmqqbmmn2ur7.fsf@gitster.mtv.corp.google.com","threadId":"46700","inReplyTo":"c3842980-22ef-5a8c-2895-90c48a97ed71@suse.de","subject":"Re: [PATCHv2] pull: honor submodule.recurse config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-07T00:54:36Z","receivedAt":"2017-09-07T00:54:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n>> If it is not the latter, perhaps we may want to flip the order of\n>> config parsing and option parsing around? That will allow us to fix\n>> the handling of autostash thing to use only one variable, and also\n>> fix your patch to do the right thing.\n>\n> I see what you mean.\n\n> It looks like switching the code around works but I think there\n> still needs to be 2 variables for autstash for this piece of code:\n>\n> if (!opt_rebase && opt_autostash != -1)\n> die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n>\n> The config option should not cause git pull to die when not using\n> --rebase, the CLI option should.\n\nAh, OK. That is a worthwhile observation that needs to be recorded\nin the log message of a commit that flips the order of option/config\nparsing.\n\nThanks.\n"}]}