{"thread":{"id":"46243","subject":"[PATCH 0/3] pull: optionally rebase submodules","startedAt":"2017-06-23T19:13:17Z","lastAt":"2017-06-27T21:32:17Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"323127","messageId":"20170623191302.16053-1-sbeller@google.com","threadId":"46243","inReplyTo":null,"subject":"[PATCH 0/3] pull: optionally rebase submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T19:12:59Z","receivedAt":"2017-06-23T19:13:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This supersedes the RFC in May by Brandon\nhttps://public-inbox.org/git/20170511172437.96878-1-bmwill@google.com/\n\nIt adds more (enough?) error checking, as explained in the last\ncommit message.\n\nDocumentation and issues raised in the review\nof that RFC have been addressed.\n\nThe first two patches are a little refactoring needed for the last patch\nthat adds the functionality.\n\nThanks,\nStefan\n\nStefan Beller (3):\n  builtin/fetch: factor submodule recurse parsing out to submodule\n    config\n  builtin/fetch: parse recurse-submodules-default at default options\n    parsing\n  pull: optionally rebase submodules (remote submodule changes only)\n\n Documentation/git-pull.txt | 12 ++++----\n builtin/fetch.c            | 37 ++++++++---------------\n builtin/pull.c             | 73 +++++++++++++++++++++++++++++++++++++++-------\n submodule-config.c         | 22 ++++++++++++++\n submodule-config.h         |  3 ++\n submodule.c                | 26 +++++++++++++++++\n submodule.h                |  4 +++\n t/t5572-pull-submodule.sh  | 58 ++++++++++++++++++++++++++++++++++++\n 8 files changed, 194 insertions(+), 41 deletions(-)\n\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"323128","messageId":"20170623191302.16053-2-sbeller@google.com","threadId":"46243","inReplyTo":"20170623191302.16053-1-sbeller@google.com","subject":"[PATCH 1/3] builtin/fetch: factor submodule recurse parsing out to submodule config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T19:13:00Z","receivedAt":"2017-06-23T19:13:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Later we want to access this parsing in builtin/pull as well.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/fetch.c    | 18 ++----------------\n submodule-config.c | 22 ++++++++++++++++++++++\n submodule-config.h |  3 +++\n 3 files changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 100248c5af..9d58dc0a8a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -53,20 +53,6 @@ static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n \n-static int option_parse_recurse_submodules(const struct option *opt,\n-\t\t\t\t   const char *arg, int unset)\n-{\n-\tif (unset) {\n-\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n-\t} else {\n-\t\tif (arg)\n-\t\t\trecurse_submodules = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n-\t\telse\n-\t\t\trecurse_submodules = RECURSE_SUBMODULES_ON;\n-\t}\n-\treturn 0;\n-}\n-\n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n \tif (!strcmp(k, \"fetch.prune\")) {\n@@ -115,9 +101,9 @@ static struct option builtin_fetch_options[] = {\n \t\t    N_(\"number of submodules fetched in parallel\")),\n \tOPT_BOOL('p', \"prune\", &prune,\n \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n-\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n+\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n \t\t    N_(\"control recursive fetching of submodules\"),\n-\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules },\n+\t\t    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },\n \tOPT_BOOL(0, \"dry-run\", &dry_run,\n \t\t N_(\"dry run\")),\n \tOPT_BOOL('k', \"keep\", &keep, N_(\"keep downloaded pack\")),\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4f58491ddb..265d036095 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -2,6 +2,7 @@\n #include \"submodule-config.h\"\n #include \"submodule.h\"\n #include \"strbuf.h\"\n+#include \"parse-options.h\"\n \n /*\n  * submodule cache lookup structure\n@@ -234,6 +235,27 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)\n \treturn parse_fetch_recurse(opt, arg, 1);\n }\n \n+int option_fetch_parse_recurse_submodules(const struct option *opt,\n+\t\t\t\t\t  const char *arg, int unset)\n+{\n+\tint *v;\n+\n+\tif (!opt->value)\n+\t\treturn -1;\n+\n+\tv = opt->value;\n+\n+\tif (unset) {\n+\t\t*v = RECURSE_SUBMODULES_OFF;\n+\t} else {\n+\t\tif (arg)\n+\t\t\t*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n+\t\telse\n+\t\t\t*v = RECURSE_SUBMODULES_ON;\n+\t}\n+\treturn 0;\n+}\n+\n static int parse_update_recurse(const char *opt, const char *arg,\n \t\t\t\tint die_on_error)\n {\ndiff --git a/submodule-config.h b/submodule-config.h\nindex d434ecdb45..1076a68653 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -23,6 +23,9 @@ struct submodule {\n };\n \n extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n+struct option;\n+extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n+\t\t\t\t\t\t const char *arg, int unset);\n extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n extern int parse_submodule_config_option(const char *var, const char *value);\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"323129","messageId":"20170623191302.16053-3-sbeller@google.com","threadId":"46243","inReplyTo":"20170623191302.16053-1-sbeller@google.com","subject":"[PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T19:13:01Z","receivedAt":"2017-06-23T19:13:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of just storing the string and then later calling our own\nparsing function 'parse_fetch_recurse_submodules_arg', make use of the\nfunction callback 'option_fetch_parse_recurse_submodules' that was\nintroduced in the last patch. Also move all submodule recursing variables\nin one spot at the top of the file.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/fetch.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9d58dc0a8a..3cca568173 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -36,7 +36,7 @@ static int prune = -1; /* unspecified */\n #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;\n-static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int progress = -1;\n static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;\n static int max_children = -1;\n static enum transport_family family;\n@@ -48,7 +48,8 @@ static struct strbuf default_rla = STRBUF_INIT;\n static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n-static const char *recurse_submodules_default;\n+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_default = RECURSE_SUBMODULES_DEFAULT;\n static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n@@ -123,9 +124,11 @@ static struct option builtin_fetch_options[] = {\n \t\t   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },\n \t{ OPTION_STRING, 0, \"submodule-prefix\", &submodule_prefix, N_(\"dir\"),\n \t\t   N_(\"prepend this to submodule path output\"), PARSE_OPT_HIDDEN },\n-\t{ OPTION_STRING, 0, \"recurse-submodules-default\",\n-\t\t   &recurse_submodules_default, NULL,\n-\t\t   N_(\"default mode for recursion\"), PARSE_OPT_HIDDEN },\n+\t{ OPTION_CALLBACK, 0, \"recurse-submodules-default\",\n+\t\t   &recurse_submodules_default, N_(\"on-demand\"),\n+\t\t   N_(\"default for recursive fetching of submodules \"\n+\t\t      \"(lower priority than config files)\"),\n+\t\t   PARSE_OPT_HIDDEN, option_fetch_parse_recurse_submodules },\n \tOPT_BOOL(0, \"update-shallow\", &update_shallow,\n \t\t N_(\"accept refs that update .git/shallow\")),\n \t{ OPTION_CALLBACK, 0, \"refmap\", NULL, N_(\"refmap\"),\n@@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tdeepen = 1;\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tif (recurse_submodules_default) {\n-\t\t\tint arg = parse_fetch_recurse_submodules_arg(\"--recurse-submodules-default\", recurse_submodules_default);\n-\t\t\tset_config_fetch_recurse_submodules(arg);\n-\t\t}\n+\t\tif (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n+\t\t\tset_config_fetch_recurse_submodules(recurse_submodules_default);\n \t\tgitmodules_config();\n \t\tgit_config(submodule_config, NULL);\n \t}\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"323130","messageId":"20170623191302.16053-4-sbeller@google.com","threadId":"46243","inReplyTo":"20170623191302.16053-1-sbeller@google.com","subject":"[PATCH 3/3] pull: optionally rebase submodules (remote submodule changes only)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T19:13:02Z","receivedAt":"2017-06-23T19:13:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Teach pull to optionally update submodules when '--recurse-submodules'\nis provided.  This will teach pull to run 'submodule update --rebase'\nwhen the '--recurse-submodules' and '--rebase' flags are given under\nspecific circumstances.\n\nOn a rebase workflow:\n=====================\n\n1. Both sides change the submodule\n ------------------------------\nLet's assume the following history in a submodule:\n\n  H---I---J---K---L local branch\n       \\\n        M---N---O---P remote branch\n\nand the following in the superproject (recorded submodule in parens):\n\n  A(H)---B(I)---F(K)---G(L)  local branch\n          \\\n           C(N)---D(N)---E(P) remote branch\n\nIn an ideal world this would rebase the submodule and rewrite\nthe submodule pointers that the superproject points at such that\nthe superproject looks like\n\n  A(H)---B(I)              F(K')---G(L')  rebased branch\n           \\                /\n           C(N)---D(N)---E(P) remote branch\n\nand the submodule as:\n\n        J---K---L (old dangeling tip)\n       /\n  H---I               J'---K'---L' rebased branch\n       \\             /\n        M---N---O---P remote branch\n\nAnd if a conflict arises in the submodule the superproject rebase\nwould stop at that commit at which the submodule conflict occurs.\n\nCurrently a \"pull --rebase\" in the superproject produces\na merge conflict as the submodule pointer changes are\nconflicting and cannot be resolved.\n\n2. Local submodule changes only\n -----------------------\nAssuming histories as above, except that the remote branch\nwould not contain submodule changes, then a result as\n\n  A(H)---B(I)               F(K)---G(L)  rebased branch\n           \\                /\n           C(I)---D(I)---E(I) remote branch\n\nis desire-able. This is what currently happens in rebase.\n\nIf the recursive flag is given, the ideal git would\nproduce a superproject as:\n\n  A(H)---B(I)              F(K')---G(L')  rebased branch (incl. sub rebase!)\n           \\                /\n           C(I)---D(I)---E(I) remote branch\n\nand the submodule as:\n\n        J---K---L (old dangeling tip)\n       /\n  H---I               J'---K'---L' locally rebased branch\n       \\             /\n        M---N---O---P advanced branch\n\nThis patch doesn't address this issue, however\na test is added that this fails up front.\n\n3. Remote submodule changes only\n ----------------------\nAssuming histories as in (1) except that the local superproject branch\nwould not have touched the submodule the rebase already works out in the\nsuperproject with no conflicts:\n\n  A(H)---B(I)               F(P)---G(P)  rebased branch (no sub changes)\n           \\                 /\n           C(N)---D(N)---E(P) remote branch\n\nThe recurse flag as presented in this patch would additionally\nupdate the submodule as:\n\n  H---I              J'---K'---L' rebased branch\n       \\            /\n        M---N---O---P remote branch\n\nAs neither J, K, L nor J', K', L' are referred to from the superproject,\nno rewriting of the superproject commits is required.\n\nConclusion for 'pull --rebase --recursive'\n -----------------------------------------\nIf there are no local superproject changes it is sufficient to call\n\"submodule update --rebase\" as this produces the desired results. In case\nof conflicts, the behavior is the same as in 'submodule update --recursive'\nwhich is assumed to be sane.\n\nThis patch implements (3) only.\n\nOn a merge workflow:\n====================\n\nWe'll start off with the same underlying DAG as in (1) in the rebase\nworkflow. So in an ideal world a 'pull --merge --recursive' would\nproduce this:\n\n  H---I---J---K---L----X\n       \\              /\n        M---N---O---P\n\nwith X as the new merge-commit in the submodule and the superproject\nas:\n\n  A(H)---B(I)---F(K)---G(L)---Y(X)\n          \\                  /\n           C(N)---D(N)---E(P)\n\nHowever modifying the submodules on the fly is not supported in git-merge\nsuch that Y(X) is not easy to produce in a single patch. In fact git-merge\ndoesn't know about submodules at all.\n\nHowever when at least one side does not contain commits touching the\nsubmodule at all, then we do not need to perform the merge for the\nsubmodule but a fast-forward can be done via checking out either L or P\nin the submodule.  This strategy is implemented in 68d03e4a6e (Implement\nautomatic fast-forward merge for submodules, 2010-07-07) already, so\nto align with the rebase behavior we need to also update the worktree\nof the submodule.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-pull.txt | 12 ++++----\n builtin/pull.c             | 73 +++++++++++++++++++++++++++++++++++++++-------\n submodule.c                | 26 +++++++++++++++++\n submodule.h                |  4 +++\n t/t5572-pull-submodule.sh  | 58 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 157 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex e414185f5a..b201af6f19 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -86,12 +86,12 @@ OPTIONS\n \n --[no-]recurse-submodules[=yes|on-demand|no]::\n \tThis option controls if new commits of all populated submodules should\n-\tbe fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).\n-\tThat might be necessary to get the data needed for merging submodule\n-\tcommits, a feature Git learned in 1.7.3. Notice that the result of a\n-\tmerge will not be checked out in the submodule, \"git submodule update\"\n-\thas to be called afterwards to bring the work tree up to date with the\n-\tmerge result.\n+\tbe fetched and updated, too (see linkgit:git-config[1] and\n+\tlinkgit:gitmodules[5]).\n++\n+If the checkout is done via rebase, local submodule commits are rebased as well.\n++\n+If the update is done via merge, the submodule conflicts are resolved and checked out.\n \n Options related to merging\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 69417e4f36..7048fdf005 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -15,6 +15,8 @@\n #include \"dir.h\"\n #include \"refs.h\"\n #include \"revision.h\"\n+#include \"submodule.h\"\n+#include \"submodule-config.h\"\n #include \"tempfile.h\"\n #include \"lockfile.h\"\n #include \"wt-status.h\"\n@@ -77,6 +79,7 @@ static const char * const pull_usage[] = {\n /* Shared options */\n static int opt_verbosity;\n static char *opt_progress;\n+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n@@ -101,7 +104,6 @@ static char *opt_upload_pack;\n static int opt_force;\n static char *opt_tags;\n static char *opt_prune;\n-static char *opt_recurse_submodules;\n static char *max_children;\n static int opt_dry_run;\n static char *opt_keep;\n@@ -116,6 +118,10 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"progress\", &opt_progress, NULL,\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n+\t{ OPTION_CALLBACK, 0, \"recurse-submodules\",\n+\t\t   &recurse_submodules, N_(\"on-demand\"),\n+\t\t   N_(\"control for recursive fetching of submodules\"),\n+\t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },\n \n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n@@ -187,10 +193,6 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU('p', \"prune\", &opt_prune, NULL,\n \t\tN_(\"prune remote-tracking branches no longer on remote\"),\n \t\tPARSE_OPT_NOARG),\n-\tOPT_PASSTHRU(0, \"recurse-submodules\", &opt_recurse_submodules,\n-\t\tN_(\"on-demand\"),\n-\t\tN_(\"control recursive fetching of submodules\"),\n-\t\tPARSE_OPT_OPTARG),\n \tOPT_PASSTHRU('j', \"jobs\", &max_children, N_(\"n\"),\n \t\tN_(\"number of submodules pulled in parallel\"),\n \t\tPARSE_OPT_OPTARG),\n@@ -483,8 +485,20 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\targv_array_push(&args, opt_tags);\n \tif (opt_prune)\n \t\targv_array_push(&args, opt_prune);\n-\tif (opt_recurse_submodules)\n-\t\targv_array_push(&args, opt_recurse_submodules);\n+\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n+\t\tswitch (recurse_submodules) {\n+\t\tcase RECURSE_SUBMODULES_ON:\n+\t\t\targv_array_push(&args, \"--recurse-submodules=on\");\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_OFF:\n+\t\t\targv_array_push(&args, \"--recurse-submodules=no\");\n+\t\t\tbreak;\n+\t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n+\t\t\targv_array_push(&args, \"--recurse-submodules=on-demand\");\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tBUG(\"submodule recursion option not understood\");\n+\t\t}\n \tif (max_children)\n \t\targv_array_push(&args, max_children);\n \tif (opt_dry_run)\n@@ -531,6 +545,30 @@ static int pull_into_void(const struct object_id *merge_head,\n \treturn 0;\n }\n \n+static int rebase_submodules(void)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\targv_array_pushl(&cp.args, \"submodule\", \"update\",\n+\t\t\t\t   \"--recursive\", \"--rebase\", NULL);\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int update_submodules(void)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\targv_array_pushl(&cp.args, \"submodule\", \"update\",\n+\t\t\t\t   \"--recursive\", \"--checkout\", NULL);\n+\n+\treturn run_command(&cp);\n+}\n+\n /**\n  * Runs git-merge, returning its exit status.\n  */\n@@ -862,6 +900,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n \n \tif (opt_rebase) {\n+\t\tint ret = 0;\n+\t\tif ((recurse_submodules == RECURSE_SUBMODULES_ON ||\n+\t\t     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n+\t\t    submodule_touches_in_range(&rebase_fork_point, &curr_head))\n+\t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \t\tif (!autostash) {\n \t\t\tstruct commit_list *list = NULL;\n \t\t\tstruct commit *merge_head, *head;\n@@ -872,11 +915,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tif (is_descendant_of(merge_head, list)) {\n \t\t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\t\topt_ff = \"--ff-only\";\n-\t\t\t\treturn run_merge();\n+\t\t\t\tret = run_merge();\n \t\t\t}\n \t\t}\n-\t\treturn run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n+\t\tret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n+\n+\t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n+\t\t\t     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n+\t\t\tret = rebase_submodules();\n+\n+\t\treturn ret;\n \t} else {\n-\t\treturn run_merge();\n+\t\tint ret = run_merge();\n+\t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n+\t\t\t     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n+\t\t\tret = update_submodules();\n+\t\treturn ret;\n \t}\n }\ndiff --git a/submodule.c b/submodule.c\nindex 1b8a3b575d..6e2e35a7fb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1126,6 +1126,32 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+int submodule_touches_in_range(struct object_id *excl_oid,\n+\t\t\t       struct object_id *incl_oid)\n+{\n+\tstruct string_list subs = STRING_LIST_INIT_DUP;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tint ret;\n+\n+\tgitmodules_config();\n+\t/* No need to check if there are no submodules configured */\n+\tif (!submodule_from_path(NULL, NULL))\n+\t\treturn 0;\n+\n+\targv_array_push(&args, \"--\"); /* args[0] program name */\n+\targv_array_push(&args, oid_to_hex(incl_oid));\n+\targv_array_push(&args, \"--not\");\n+\targv_array_push(&args, oid_to_hex(excl_oid));\n+\n+\tcollect_changed_submodules(&subs, &args);\n+\tret = subs.nr;\n+\n+\targv_array_clear(&args);\n+\n+\tfree_submodules_oids(&subs);\n+\treturn ret;\n+}\n+\n struct submodule_parallel_fetch {\n \tint count;\n \tstruct argv_array args;\ndiff --git a/submodule.h b/submodule.h\nindex cbe5c1726f..ab1f01b3ce 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -97,6 +97,10 @@ extern int merge_submodule(struct object_id *result, const char *path,\n \t\t\t   const struct object_id *base,\n \t\t\t   const struct object_id *a,\n \t\t\t   const struct object_id *b, int search);\n+\n+/* Checks if there are submodule changes in a..b. */\n+extern int submodule_touches_in_range(struct object_id *a,\n+\t\t\t\t      struct object_id *b);\n extern int find_unpushed_submodules(struct oid_array *commits,\n \t\t\t\t    const char *remotes_name,\n \t\t\t\t    struct string_list *needs_pushing);\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex accfa5cc0c..077eb07e11 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -42,4 +42,62 @@ KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n test_submodule_switch \"git_pull_noff\"\n \n+test_expect_success 'pull --recurse-submodule setup' '\n+\ttest_create_repo child &&\n+\ttest_commit -C child bar &&\n+\n+\ttest_create_repo parent &&\n+\ttest_commit -C child foo &&\n+\n+\tgit -C parent submodule add ../child sub &&\n+\tgit -C parent commit -m \"add submodule\" &&\n+\n+\tgit clone --recurse-submodules parent super\n+'\n+\n+test_expect_success 'recursive pull updates working tree' '\n+\ttest_commit -C child merge_strategy &&\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 pull --no-rebase --recurse-submodules &&\n+\ttest_path_is_file super/sub/merge_strategy.t\n+'\n+\n+test_expect_success 'recursive rebasing pull' '\n+\t# change upstream\n+\ttest_commit -C child rebase_strategy &&\n+\tgit -C parent submodule update --remote &&\n+\tgit -C parent add sub &&\n+\tgit -C parent commit -m \"update submodule\" &&\n+\n+\t# also have local commits\n+\ttest_commit -C super/sub local_stuff &&\n+\n+\tgit -C super pull --rebase --recurse-submodules &&\n+\ttest_path_is_file super/sub/rebase_strategy.t &&\n+\ttest_path_is_file super/sub/local_stuff.t\n+'\n+\n+test_expect_success 'pull rebase recursing fails with conflicts' '\n+\n+\t# local changes in submodule recorded in superproject:\n+\ttest_commit -C super/sub local_stuff_2 &&\n+\tgit -C super add sub &&\n+\tgit -C super commit -m \"local update submodule\" &&\n+\n+\t# and in the remote as well:\n+\ttest_commit -C child important_upstream_work &&\n+\tgit -C parent submodule update --remote &&\n+\tgit -C parent add sub &&\n+\tgit -C parent commit -m \"remote update submodule\" &&\n+\n+\t# Unfortunately we fail here, despite no conflict in the\n+\t# submodule itself, but the merge strategy in submodules\n+\t# does not support rebase:\n+\ttest_must_fail git -C super pull --rebase --recurse-submodules 2>err &&\n+\ttest_i18ngrep \"locally recorded submodule modifications\" err\n+'\n+\n test_done\n-- \n2.12.2.575.gb14f27f917\n\n"},{"id":"323164","messageId":"xmqqshiqbash.fsf@gitster.mtv.corp.google.com","threadId":"46243","inReplyTo":"20170623191302.16053-2-sbeller@google.com","subject":"Re: [PATCH 1/3] builtin/fetch: factor submodule recurse parsing out to submodule config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T22:26:38Z","receivedAt":"2017-06-23T22:26:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Later we want to access this parsing in builtin/pull as well.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/fetch.c    | 18 ++----------------\n>  submodule-config.c | 22 ++++++++++++++++++++++\n>  submodule-config.h |  3 +++\n>  3 files changed, 27 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 100248c5af..9d58dc0a8a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -53,20 +53,6 @@ static int shown_url = 0;\n>  static int refmap_alloc, refmap_nr;\n>  static const char **refmap_array;\n>  \n> -static int option_parse_recurse_submodules(const struct option *opt,\n> -\t\t\t\t   const char *arg, int unset)\n> -{\n> -\tif (unset) {\n> -\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> -\t} else {\n> -\t\tif (arg)\n> -\t\t\trecurse_submodules = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n> -\t\telse\n> -\t\t\trecurse_submodules = RECURSE_SUBMODULES_ON;\n> -\t}\n> -\treturn 0;\n> -}\n> -\n>  static int git_fetch_config(const char *k, const char *v, void *cb)\n>  {\n>  \tif (!strcmp(k, \"fetch.prune\")) {\n> @@ -115,9 +101,9 @@ static struct option builtin_fetch_options[] = {\n>  \t\t    N_(\"number of submodules fetched in parallel\")),\n>  \tOPT_BOOL('p', \"prune\", &prune,\n>  \t\t N_(\"prune remote-tracking branches no longer on remote\")),\n> -\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", NULL, N_(\"on-demand\"),\n> +\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules, N_(\"on-demand\"),\n>  \t\t    N_(\"control recursive fetching of submodules\"),\n> -\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules },\n> +\t\t    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules },\n>  \tOPT_BOOL(0, \"dry-run\", &dry_run,\n>  \t\t N_(\"dry run\")),\n>  \tOPT_BOOL('k', \"keep\", &keep, N_(\"keep downloaded pack\")),\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 4f58491ddb..265d036095 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -2,6 +2,7 @@\n>  #include \"submodule-config.h\"\n>  #include \"submodule.h\"\n>  #include \"strbuf.h\"\n> +#include \"parse-options.h\"\n>  \n>  /*\n>   * submodule cache lookup structure\n> @@ -234,6 +235,27 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)\n>  \treturn parse_fetch_recurse(opt, arg, 1);\n>  }\n>  \n> +int option_fetch_parse_recurse_submodules(const struct option *opt,\n> +\t\t\t\t\t  const char *arg, int unset)\n> +{\n> +\tint *v;\n> +\n> +\tif (!opt->value)\n> +\t\treturn -1;\n\nIt would have been easier to view this change if the original\nalready had used opt->value to specify where to place the parsed\nvalue, but that is water under the bridge ;-)\n\nLooks like a faithful converison to me.\n\n> +\tv = opt->value;\n> +\n> +\tif (unset) {\n> +\t\t*v = RECURSE_SUBMODULES_OFF;\n> +\t} else {\n> +\t\tif (arg)\n> +\t\t\t*v = parse_fetch_recurse_submodules_arg(opt->long_name, arg);\n> +\t\telse\n> +\t\t\t*v = RECURSE_SUBMODULES_ON;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n>  static int parse_update_recurse(const char *opt, const char *arg,\n>  \t\t\t\tint die_on_error)\n>  {\n> diff --git a/submodule-config.h b/submodule-config.h\n> index d434ecdb45..1076a68653 100644\n> --- a/submodule-config.h\n> +++ b/submodule-config.h\n> @@ -23,6 +23,9 @@ struct submodule {\n>  };\n>  \n>  extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n> +struct option;\n> +extern int option_fetch_parse_recurse_submodules(const struct option *opt,\n> +\t\t\t\t\t\t const char *arg, int unset);\n>  extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n>  extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n>  extern int parse_submodule_config_option(const char *var, const char *value);\n"},{"id":"323165","messageId":"xmqqo9tebaco.fsf@gitster.mtv.corp.google.com","threadId":"46243","inReplyTo":"20170623191302.16053-3-sbeller@google.com","subject":"Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T22:36:07Z","receivedAt":"2017-06-23T22:36:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> @@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \t\tdeepen = 1;\n>  \n>  \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n> -\t\tif (recurse_submodules_default) {\n> -\t\t\tint arg = parse_fetch_recurse_submodules_arg(\"--recurse-submodules-default\", recurse_submodules_default);\n> -\t\t\tset_config_fetch_recurse_submodules(arg);\n> -\t\t}\n> +\t\tif (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n> +\t\t\tset_config_fetch_recurse_submodules(recurse_submodules_default);\n\nThis is not a new thing, and it may not even be a problem, but I\nhave to wonder why this needs to be done conditionally.  \"The\ncommand line did not explicitly say --no-recurse-submodules, so we\nwould set what came from --recurse-submodule-default if and only if\nthat is given---otherwise we leave it as a compiled-in default\" is\nwhat the code before or after this patch tells me.  But if we drop\nthe \"if (default is not DEFAULT)\", the resulting code becomes \"The\ncommand line did not explicitly say --no-recurse-submodules, so we\nwould set whatever is the default (which may not have been given\nfrom the command line, but came built-in to the binary)\".  Aren't\nthey saying the same thing?\n\nIt's not like there is a configuration knob that further interferes\nthe interaction between these two.  I am puzzled.\n\n\n>  \t\tgitmodules_config();\n>  \t\tgit_config(submodule_config, NULL);\n>  \t}\n"},{"id":"323166","messageId":"CAGZ79ka+va5bRNQL3x3fvn+738iAfJXJfD2PKpt2hmGEg09mTQ@mail.gmail.com","threadId":"46243","inReplyTo":"xmqqo9tebaco.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T22:49:54Z","receivedAt":"2017-06-23T22:50:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 23, 2017 at 3:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> @@ -1333,10 +1336,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>>               deepen = 1;\n>>\n>>       if (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n>> -             if (recurse_submodules_default) {\n>> -                     int arg = parse_fetch_recurse_submodules_arg(\"--recurse-submodules-default\", recurse_submodules_default);\n>> -                     set_config_fetch_recurse_submodules(arg);\n>> -             }\n>> +             if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n>> +                     set_config_fetch_recurse_submodules(recurse_submodules_default);\n>\n> This is not a new thing, and it may not even be a problem, but I\n> have to wonder why this needs to be done conditionally.  \"The\n> command line did not explicitly say --no-recurse-submodules, so we\n> would set what came from --recurse-submodule-default if and only if\n> that is given---otherwise we leave it as a compiled-in default\" is\n> what the code before or after this patch tells me.  But if we drop\n> the \"if (default is not DEFAULT)\", the resulting code becomes \"The\n> command line did not explicitly say --no-recurse-submodules, so we\n> would set whatever is the default (which may not have been given\n> from the command line, but came built-in to the binary)\".  Aren't\n> they saying the same thing?\n\nI assumed so as well, yes. But the test suite pointed out there\nis another subtlety going on, which I have not dug into, yet.\n\n>\n> It's not like there is a configuration knob that further interferes\n> the interaction between these two.  I am puzzled.\n\nAs far as I suspect, the original author considered\nevaluating the additional config too expensive.\n\n    gitmodules_config(); // <- this specifically?\n    git_config(submodule_config, NULL);\n\nAnd that is why we only react if any switch is\ngiven to recurse. Before be254a0ea9 (Add the\n'fetch.recurseSubmodules' config setting, 2010-11-11)\nwe would not load the config unless the user\nasked for submodule action.\n\nThen with the config switch we *had* to load the config\n(both .gitmodules as well as .git/config), so at that commit\nit would have made sense to inline this into the regular config\nand command line argument parsing.\n"},{"id":"323174","messageId":"xmqq1sqab436.fsf@gitster.mtv.corp.google.com","threadId":"46243","inReplyTo":"CAGZ79ka+va5bRNQL3x3fvn+738iAfJXJfD2PKpt2hmGEg09mTQ@mail.gmail.com","subject":"Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-24T00:51:25Z","receivedAt":"2017-06-24T00:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>>       if (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n>>> -             if (recurse_submodules_default) {\n>>> -                     int arg = parse_fetch_recurse_submodules_arg(\"--recurse-submodules-default\", recurse_submodules_default);\n>>> -                     set_config_fetch_recurse_submodules(arg);\n>>> -             }\n>>> +             if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n>>> +                     set_config_fetch_recurse_submodules(recurse_submodules_default);\n>>\n>> This is not a new thing, and it may not even be a problem, but I\n>> have to wonder why this needs to be done conditionally.  \"The\n>> ...\n>\n> As far as I suspect, the original author considered\n> evaluating the additional config too expensive.\n>\n>     gitmodules_config(); // <- this specifically?\n>     git_config(submodule_config, NULL);\n>\n> And that is why we only react if any switch is\n> given to recurse. \n\nI am not talking about the outer \"if\" condition.  \n\nThe inner \n\n    \"if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\"\n\nblock, is what I am questioning, i.e. I am wondering why\nset_config_fetch_recurse_submodules() need to be conditionally\ncalled.\n\nThe two statement you quoted above will be executed either way,\nregardless of the value of recurse_submodule_default.\n\nUnless --recurse-submodules=off is given, in which case the outer\n\"if\" condition rejects.  And I think you are explaining that part,\nbut that was not what I am questioning.\n"},{"id":"323309","messageId":"CAGZ79kZUkQCEEcLUcg8tSr33Wng1Fq4_i4n80+Xr45y0mcvB6A@mail.gmail.com","threadId":"46243","inReplyTo":"xmqq1sqab436.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] builtin/fetch: parse recurse-submodules-default at default options parsing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-27T03:00:24Z","receivedAt":"2017-06-27T03:00:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 23, 2017 at 5:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n>>>>       if (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n>>>> -             if (recurse_submodules_default) {\n>>>> -                     int arg = parse_fetch_recurse_submodules_arg(\"--recurse-submodules-default\", recurse_submodules_default);\n>>>> -                     set_config_fetch_recurse_submodules(arg);\n>>>> -             }\n>>>> +             if (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n>>>> +                     set_config_fetch_recurse_submodules(recurse_submodules_default);\n>>>\n\n\n>\n> I am not talking about the outer \"if\" condition.\n\nI agree with your analysis, my answer was evasive.\nI'll dig into the details why we do not set the default by default.\n"},{"id":"323394","messageId":"20170627213159.3717-1-sbeller@google.com","threadId":"46243","inReplyTo":"CAGZ79kZUkQCEEcLUcg8tSr33Wng1Fq4_i4n80+Xr45y0mcvB6A@mail.gmail.com","subject":"[PATCH] builtin/fetch cleanup: always set default value for submodule recursing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-27T21:31:59Z","receivedAt":"2017-06-27T21:32:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The check for the default was introduced with 88a21979c5 (fetch/pull:\nrecurse into submodules when necessary, 2011-03-06), which replaced an\nolder construct (builtin/fetchs own implementation of the super-prefix)\nintroduced in be254a0ea9 (Add the 'fetch.recurseSubmodules' config setting,\n2010-11-11) which made sense at the time as there was no default fetch\noption for submodules at the time.\n\nSet builtin/fetch.c#recurse_submodules_default to the same value as\nsubmodule.c#config_fetch_recurse_submodules which is set via\nset_config_fetch_recurse_submodules, such that the condition for checking\nwhether we have to set the default value becomes unnecessary.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis queues on top of origin/sb/pull-rebase-submodule,\nand answers questions raised in the review of origin/sb/pull-rebase-submodule^.\n\nI thought about just squashing these two patches, but they do different cleanups\nso I am personally fine with having them both.\n\nI may squash them if further review of that series\nrequires a resend of the whole series.\n\nThanks,\nStefan\n\n builtin/fetch.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3cca568173..7ea52b8b07 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -49,7 +49,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n-static int recurse_submodules_default = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;\n static int shown_url = 0;\n static int refmap_alloc, refmap_nr;\n static const char **refmap_array;\n@@ -1336,8 +1336,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tdeepen = 1;\n \n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tif (recurse_submodules_default != RECURSE_SUBMODULES_DEFAULT)\n-\t\t\tset_config_fetch_recurse_submodules(recurse_submodules_default);\n+\t\tset_config_fetch_recurse_submodules(recurse_submodules_default);\n \t\tgitmodules_config();\n \t\tgit_config(submodule_config, NULL);\n \t}\n-- \n2.13.1.519.gc4803e3bc3\n\n"}]}