{"thread":{"id":"45936","subject":"[PATCH] pull: optionally rebase submodules","startedAt":"2017-05-11T17:24:52Z","lastAt":"2017-05-15T23:09:26Z","messageCount":5,"participants":["Brandon Williams","Stefan Beller","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"319423","messageId":"20170511172437.96878-1-bmwill@google.com","threadId":"45936","inReplyTo":null,"subject":"[PATCH] pull: optionally rebase submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-11T17:24:37Z","receivedAt":"2017-05-11T17:24:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"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.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n\nPull is already a shortcut for running fetch followed by a merge/rebase, so why\nnot have it be a shortcut for running 'submodule update --rebase' when the\nrecurse flag is given!\n\n builtin/pull.c            | 30 ++++++++++++++-\n t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 125 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex dd1a4a94e..d73d654e6 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -77,6 +77,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;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n@@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,\n \treturn 0;\n }\n \n+static int  update_submodules(void)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tcp.git_cmd = 1;\n+\n+\targv_array_pushl(&cp.args, \"submodule\", \"update\", \"--recursive\", NULL);\n+\targv_array_push(&cp.args, \"--rebase\");\n+\n+\treturn run_command(&cp);\n+}\n+\n /**\n  * Runs git-merge, returning its exit status.\n  */\n@@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\toidclr(&rebase_fork_point);\n \t}\n \n+\tif (opt_recurse_submodules &&\n+\t    !strcmp(opt_recurse_submodules, \"--recurse-submodules\")) {\n+\t\trecurse_submodules = 1;\n+\n+\t\tif (!opt_rebase)\n+\t\t\tdie(_(\"--recurse-submodules is only valid with --rebase\"));\n+\t}\n+\n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\n \n@@ -862,6 +882,7 @@ 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\tstruct commit_list *list = NULL;\n \t\tstruct commit *merge_head, *head;\n \n@@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (is_descendant_of(merge_head, list)) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\topt_ff = \"--ff-only\";\n-\t\t\treturn run_merge();\n+\t\t\tret = run_merge();\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)\n+\t\t\tret = update_submodules();\n+\n+\t\treturn ret;\n \t} else {\n \t\treturn run_merge();\n \t}\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex accfa5cc0..234a22315 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -42,4 +42,101 @@ 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+\tgit init child &&\n+\t(\n+\t\tcd child &&\n+\t\techo \"bar\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"initial commit\"\n+\t) &&\n+\tgit init parent &&\n+\t(\n+\t\tcd parent &&\n+\t\techo \"foo\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"Initial commit\" &&\n+\t\tgit submodule add ../child sub &&\n+\t\tgit commit -m \"add submodule\"\n+\t) &&\n+\tgit clone --recurse-submodule parent super &&\n+\tgit -C super/sub checkout master\n+'\n+\n+test_expect_success 'pull recursive fails without --rebase' '\n+\ttest_must_fail git -C super pull --recurse-submodules 2>actual &&\n+\ttest_i18ngrep \"recurse-submodules is only valid with --rebase\" actual\n+'\n+\n+test_expect_success 'pull basic recurse' '\n+\t(\n+\t\tcd child &&\n+\t\techo \"foobar\" >>file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"update file\"\n+\t) &&\n+\t(\n+\t\tcd parent &&\n+\t\tgit -C sub pull &&\n+\t\tgit add sub &&\n+\t\tgit commit -m \"update submodule\"\n+\t) &&\n+\n+\tgit -C parent rev-parse master >expect_super &&\n+\tgit -C child rev-parse master >expect_sub &&\n+\n+\tgit -C super pull --rebase --recurse-submodules &&\n+\tgit -C super rev-parse master >actual_super &&\n+\tgit -C super/sub rev-parse master >actual_sub &&\n+\ttest_cmp expect_super actual_super &&\n+\ttest_cmp expect_sub actual_sub\n+'\n+\n+test_expect_success 'pull basic rebase recurse' '\n+\t(\n+\t\tcd child &&\n+\t\techo \"a\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"update file\"\n+\t) &&\n+\t(\n+\t\tcd parent &&\n+\t\tgit -C sub pull &&\n+\t\tgit add sub &&\n+\t\tgit commit -m \"update submodule\"\n+\t) &&\n+\t(\n+\t\tcd super/sub &&\n+\t\techo \"b\" >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"add file2\"\n+\t) &&\n+\n+\tgit -C parent rev-parse master >expect_super &&\n+\tgit -C child rev-parse master >expect_sub &&\n+\n+\tgit -C super pull --rebase --recurse-submodules &&\n+\tgit -C super rev-parse master >actual_super &&\n+\tgit -C super/sub rev-parse master^ >actual_sub &&\n+\ttest_cmp expect_super actual_super &&\n+\ttest_cmp expect_sub actual_sub &&\n+\n+\techo \"a\" >expect &&\n+\ttest_cmp expect super/sub/file &&\n+\techo \"b\" >expect &&\n+\ttest_cmp expect super/sub/file2\n+'\n+\n+test_expect_success 'pull rebase recursing fails with conflicts' '\n+\tgit -C super/sub reset --hard HEAD^^ &&\n+\tgit -C super reset --hard HEAD^ &&\n+\t(\n+\t\tcd super/sub &&\n+\t\techo \"b\" >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"update file\"\n+\t) &&\n+\ttest_must_fail git -C super pull --rebase --recurse-submodules\n+'\n+\n test_done\n-- \n2.13.0.rc2.291.g57267f2277-goog\n\n"},{"id":"319433","messageId":"CAGZ79kYYX_AMZm7Di8cUA_eiDS+SSAGnJyrDLcC5U7POk7WdSw@mail.gmail.com","threadId":"45936","inReplyTo":"20170511172437.96878-1-bmwill@google.com","subject":"Re: [PATCH] pull: optionally rebase submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-11T18:12:52Z","receivedAt":"2017-05-11T18:13:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 11, 2017 at 10:24 AM, Brandon Williams <bmwill@google.com> wrote:\n> Teach pull to optionally update submodules when '--recurse-submodules'\n> is provided.  This will teach pull to run 'submodule update --rebase'\n> when the '--recurse-submodules' and '--rebase' flags are given.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>\n> Pull is already a shortcut for running fetch followed by a merge/rebase, so why\n> not have it be a shortcut for running 'submodule update --rebase' when the\n> recurse flag is given!\n\nI have not thought about the implications of this shortcut, as opposed to\nactually implementing it in C (which presumably would contain more checks).\nWill do.\n\n>\n>  builtin/pull.c            | 30 ++++++++++++++-\n>  t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 125 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index dd1a4a94e..d73d654e6 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -77,6 +77,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;\n>\n>  /* Options passed to git-merge or git-rebase */\n>  static enum rebase_type opt_rebase = -1;\n> @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,\n>         return 0;\n>  }\n>\n> +static int  update_submodules(void)\n\nMaybe s/update_submodules/rebase_submodules/ ?\n\n> +{\n> +       struct child_process cp = CHILD_PROCESS_INIT;\n> +       cp.git_cmd = 1;\n> +\n> +       argv_array_pushl(&cp.args, \"submodule\", \"update\", \"--recursive\", NULL);\n> +       argv_array_push(&cp.args, \"--rebase\");\n\nThe --rebase could be part of the _pushl ?\nAlso we could set\n    no_stdin = 1\nwe do need stdout/err though.\n\n\n> +\n> +       return run_command(&cp);\n> +}\n> +\n>  /**\n>   * Runs git-merge, returning its exit status.\n>   */\n> @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>                         oidclr(&rebase_fork_point);\n>         }\n>\n> +       if (opt_recurse_submodules &&\n> +           !strcmp(opt_recurse_submodules, \"--recurse-submodules\")) {\n\nSo this checks if we pass --recurse-submodules to fetch and if it is not\na on-demand/no.\nMaybe we'd want to use the same infrastructure as fetch does, such that\nparse_fetch_recurse makes the decision. (Then \"--recurse-submodules=TrUe\"\nwould work as well, IIUC)\n\n\n> +               recurse_submodules = 1;\n> +\n> +               if (!opt_rebase)\n> +                       die(_(\"--recurse-submodules is only valid with --rebase\"));\n\nI wonder if there are existing users of \"git pull --recurse --merge\";\nas of now this would fetch the submodules (on-demand) and merge\nin the local commits of the superprojects. It sounds like a useful workflow\nwhich we'd be blocking here? Maybe just do nothing in case of !opt_rebase,\ni.e. make it part of the first condition added in this hunk?\n\n> +               ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> +\n> +               if (!ret && recurse_submodules)\n> +                       ret = update_submodules();\n\nInstead of doing the rebase of submodules here, we may just want to pass\n'recurse_submodules' into run_rebase, which can do it, too. (It already has\na 'ret' value, so the main cmd is not as cluttered.\n\n---\nBefore reviewing the tests, let's step a bit back and talk about the design\nand what is useful to the user. From reading the code, we\n  1) perform a fetch in the superproject\n  2) rebase the superproject (not rewriting any submodule pointers,\n    but that may be ok for now)\n  3) sequentially:\n  3a) fetch each submodule on demand\n  3b) run rebase inside of it.\n\n\n(A) On the sequence:\nIf in a normal pull --rebase we have a failure, we can fixup the failure\nand then continue via \"git rebase --continue\"; now if we have a failure\nin 3), we would need to fixup the submodule ourselves and then as\nwe lost all progress in the superproject, rerun \"pull --rebase --recurse\"?\n\n(B)\nAs discussed offline this produces bad results if we have a non-ff\noperation in the superproject, that also has submodule pointer updates.\nSo additionally we would need to walk the superprojects local commits\nand check if any submodule is touched.\n\n\n>\n> +test_expect_success 'pull --recurse-submodule setup' '\n> +       git init child &&\n\ntest_create_repo child\n\n> +       (\n> +               cd child &&\n> +               echo \"bar\" >file &&\n> +               git add file &&\n> +               git commit -m \"initial commit\"\n\ntest_create_commit -C child\n\n> +       ) &&\n> +       git init parent &&\n> +       (\n> +               cd parent &&\n> +               echo \"foo\" >file &&\n> +               git add file &&\n> +               git commit -m \"Initial commit\" &&\n> +               git submodule add ../child sub &&\n> +               git commit -m \"add submodule\"\n> +       ) &&\n\nSame setup comment as for the child\n\n\n> +       git clone --recurse-submodule parent super &&\n> +       git -C super/sub checkout master\n\nI wonder if we want to keep these two commands in each test\nas I noticed some test scripts are horribly messy others have\na pattern to cleanup after themselves:\n\ntest_expect_....\n    test_when_finished \"rm -rf super-clone\" &&\n    git clone ... into super-clone\n\n\n\n> +'\n> +\n> +test_expect_success 'pull recursive fails without --rebase' '\n> +       test_must_fail git -C super pull --recurse-submodules 2>actual &&\n> +       test_i18ngrep \"recurse-submodules is only valid with --rebase\" actual\n> +'\n\nSide note: another place to add tests could be t5520 or t740*.\n\n> +test_expect_success 'pull rebase recursing fails with conflicts' '\n> +       git -C super/sub reset --hard HEAD^^ &&\n> +       git -C super reset --hard HEAD^ &&\n> +       (\n> +               cd super/sub &&\n> +               echo \"b\" >file &&\n> +               git add file &&\n> +               git commit -m \"update file\"\n> +       ) &&\n> +       test_must_fail git -C super pull --rebase --recurse-submodules\n\nAs discussed above: We'd also want to have a reasonable state here,\nor some advice to the user telling them how to recover. Maybe in a\nfirst approach we can tell them to re-run \"submodule update --rebase\"\nafter fixing the conflict?\n\nThanks,\nStefan\n"},{"id":"319448","messageId":"702FD1AB212642BA80B1ADF27D75916B@PhilipOakley","threadId":"45936","inReplyTo":"20170511172437.96878-1-bmwill@google.com","subject":"Re: [PATCH] pull: optionally rebase submodules","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-05-11T20:00:09Z","receivedAt":"2017-05-11T20:00:19Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Brandon Williams\" <bmwill@google.com>\n> Teach pull to optionally update submodules when '--recurse-submodules'\n> is provided.  This will teach pull to run 'submodule update --rebase'\n> when the '--recurse-submodules' and '--rebase' flags are given.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>\n> Pull is already a shortcut for running fetch followed by a merge/rebase, \n> so why\n> not have it be a shortcut for running 'submodule update --rebase' when the\n> recurse flag is given!\n>\n> builtin/pull.c            | 30 ++++++++++++++-\n> t/t5572-pull-submodule.sh | 97 \n> +++++++++++++++++++++++++++++++++++++++++++++++\n\nShouldn't this also touch the documentation to say that this has been \ntaught?\n--\nPhilip\n\n\n> 2 files changed, 125 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index dd1a4a94e..d73d654e6 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -77,6 +77,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;\n>\n> /* Options passed to git-merge or git-rebase */\n> static enum rebase_type opt_rebase = -1;\n> @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id \n> *merge_head,\n>  return 0;\n> }\n>\n> +static int  update_submodules(void)\n> +{\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + cp.git_cmd = 1;\n> +\n> + argv_array_pushl(&cp.args, \"submodule\", \"update\", \"--recursive\", NULL);\n> + argv_array_push(&cp.args, \"--rebase\");\n> +\n> + return run_command(&cp);\n> +}\n> +\n> /**\n>  * Runs git-merge, returning its exit status.\n>  */\n> @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char \n> *prefix)\n>  oidclr(&rebase_fork_point);\n>  }\n>\n> + if (opt_recurse_submodules &&\n> +     !strcmp(opt_recurse_submodules, \"--recurse-submodules\")) {\n> + recurse_submodules = 1;\n> +\n> + if (!opt_rebase)\n> + die(_(\"--recurse-submodules is only valid with --rebase\"));\n> + }\n> +\n>  if (run_fetch(repo, refspecs))\n>  return 1;\n>\n> @@ -862,6 +882,7 @@ int cmd_pull(int argc, const char **argv, const char \n> *prefix)\n>  die(_(\"Cannot rebase onto multiple branches.\"));\n>\n>  if (opt_rebase) {\n> + int ret = 0;\n>  struct commit_list *list = NULL;\n>  struct commit *merge_head, *head;\n>\n> @@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv, const char \n> *prefix)\n>  if (is_descendant_of(merge_head, list)) {\n>  /* we can fast-forward this without invoking rebase */\n>  opt_ff = \"--ff-only\";\n> - return run_merge();\n> + ret = run_merge();\n>  }\n> - return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> + ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> +\n> + if (!ret && recurse_submodules)\n> + ret = update_submodules();\n> +\n> + return ret;\n>  } else {\n>  return run_merge();\n>  }\n> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> index accfa5cc0..234a22315 100755\n> --- a/t/t5572-pull-submodule.sh\n> +++ b/t/t5572-pull-submodule.sh\n> @@ -42,4 +42,101 @@ \n> 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> + git init child &&\n> + (\n> + cd child &&\n> + echo \"bar\" >file &&\n> + git add file &&\n> + git commit -m \"initial commit\"\n> + ) &&\n> + git init parent &&\n> + (\n> + cd parent &&\n> + echo \"foo\" >file &&\n> + git add file &&\n> + git commit -m \"Initial commit\" &&\n> + git submodule add ../child sub &&\n> + git commit -m \"add submodule\"\n> + ) &&\n> + git clone --recurse-submodule parent super &&\n> + git -C super/sub checkout master\n> +'\n> +\n> +test_expect_success 'pull recursive fails without --rebase' '\n> + test_must_fail git -C super pull --recurse-submodules 2>actual &&\n> + test_i18ngrep \"recurse-submodules is only valid with --rebase\" actual\n> +'\n> +\n> +test_expect_success 'pull basic recurse' '\n> + (\n> + cd child &&\n> + echo \"foobar\" >>file &&\n> + git add file &&\n> + git commit -m \"update file\"\n> + ) &&\n> + (\n> + cd parent &&\n> + git -C sub pull &&\n> + git add sub &&\n> + git commit -m \"update submodule\"\n> + ) &&\n> +\n> + git -C parent rev-parse master >expect_super &&\n> + git -C child rev-parse master >expect_sub &&\n> +\n> + git -C super pull --rebase --recurse-submodules &&\n> + git -C super rev-parse master >actual_super &&\n> + git -C super/sub rev-parse master >actual_sub &&\n> + test_cmp expect_super actual_super &&\n> + test_cmp expect_sub actual_sub\n> +'\n> +\n> +test_expect_success 'pull basic rebase recurse' '\n> + (\n> + cd child &&\n> + echo \"a\" >file &&\n> + git add file &&\n> + git commit -m \"update file\"\n> + ) &&\n> + (\n> + cd parent &&\n> + git -C sub pull &&\n> + git add sub &&\n> + git commit -m \"update submodule\"\n> + ) &&\n> + (\n> + cd super/sub &&\n> + echo \"b\" >file2 &&\n> + git add file2 &&\n> + git commit -m \"add file2\"\n> + ) &&\n> +\n> + git -C parent rev-parse master >expect_super &&\n> + git -C child rev-parse master >expect_sub &&\n> +\n> + git -C super pull --rebase --recurse-submodules &&\n> + git -C super rev-parse master >actual_super &&\n> + git -C super/sub rev-parse master^ >actual_sub &&\n> + test_cmp expect_super actual_super &&\n> + test_cmp expect_sub actual_sub &&\n> +\n> + echo \"a\" >expect &&\n> + test_cmp expect super/sub/file &&\n> + echo \"b\" >expect &&\n> + test_cmp expect super/sub/file2\n> +'\n> +\n> +test_expect_success 'pull rebase recursing fails with conflicts' '\n> + git -C super/sub reset --hard HEAD^^ &&\n> + git -C super reset --hard HEAD^ &&\n> + (\n> + cd super/sub &&\n> + echo \"b\" >file &&\n> + git add file &&\n> + git commit -m \"update file\"\n> + ) &&\n> + test_must_fail git -C super pull --rebase --recurse-submodules\n> +'\n> +\n> test_done\n> -- \n> 2.13.0.rc2.291.g57267f2277-goog\n>\n> \n\n"},{"id":"319537","messageId":"20170512173039.GC98586@google.com","threadId":"45936","inReplyTo":"702FD1AB212642BA80B1ADF27D75916B@PhilipOakley","subject":"Re: [PATCH] pull: optionally rebase submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-12T17:30:39Z","receivedAt":"2017-05-12T18:45:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/11, Philip Oakley wrote:\n> From: \"Brandon Williams\" <bmwill@google.com>\n> >Teach pull to optionally update submodules when '--recurse-submodules'\n> >is provided.  This will teach pull to run 'submodule update --rebase'\n> >when the '--recurse-submodules' and '--rebase' flags are given.\n> >\n> >Signed-off-by: Brandon Williams <bmwill@google.com>\n> >---\n> >\n> >Pull is already a shortcut for running fetch followed by a\n> >merge/rebase, so why\n> >not have it be a shortcut for running 'submodule update --rebase' when the\n> >recurse flag is given!\n> >\n> >builtin/pull.c            | 30 ++++++++++++++-\n> >t/t5572-pull-submodule.sh | 97\n> >+++++++++++++++++++++++++++++++++++++++++++++++\n> \n> Shouldn't this also touch the documentation to say that this has\n> been taught?\n\nYes it should, thanks for reminding me!\n\n> --\n> Philip\n> \n> \n> >2 files changed, 125 insertions(+), 2 deletions(-)\n> >\n> >diff --git a/builtin/pull.c b/builtin/pull.c\n> >index dd1a4a94e..d73d654e6 100644\n> >--- a/builtin/pull.c\n> >+++ b/builtin/pull.c\n> >@@ -77,6 +77,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;\n> >\n> >/* Options passed to git-merge or git-rebase */\n> >static enum rebase_type opt_rebase = -1;\n> >@@ -532,6 +533,17 @@ static int pull_into_void(const struct\n> >object_id *merge_head,\n> > return 0;\n> >}\n> >\n> >+static int  update_submodules(void)\n> >+{\n> >+ struct child_process cp = CHILD_PROCESS_INIT;\n> >+ cp.git_cmd = 1;\n> >+\n> >+ argv_array_pushl(&cp.args, \"submodule\", \"update\", \"--recursive\", NULL);\n> >+ argv_array_push(&cp.args, \"--rebase\");\n> >+\n> >+ return run_command(&cp);\n> >+}\n> >+\n> >/**\n> > * Runs git-merge, returning its exit status.\n> > */\n> >@@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv,\n> >const char *prefix)\n> > oidclr(&rebase_fork_point);\n> > }\n> >\n> >+ if (opt_recurse_submodules &&\n> >+     !strcmp(opt_recurse_submodules, \"--recurse-submodules\")) {\n> >+ recurse_submodules = 1;\n> >+\n> >+ if (!opt_rebase)\n> >+ die(_(\"--recurse-submodules is only valid with --rebase\"));\n> >+ }\n> >+\n> > if (run_fetch(repo, refspecs))\n> > return 1;\n> >\n> >@@ -862,6 +882,7 @@ int cmd_pull(int argc, const char **argv,\n> >const char *prefix)\n> > die(_(\"Cannot rebase onto multiple branches.\"));\n> >\n> > if (opt_rebase) {\n> >+ int ret = 0;\n> > struct commit_list *list = NULL;\n> > struct commit *merge_head, *head;\n> >\n> >@@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv,\n> >const char *prefix)\n> > if (is_descendant_of(merge_head, list)) {\n> > /* we can fast-forward this without invoking rebase */\n> > opt_ff = \"--ff-only\";\n> >- return run_merge();\n> >+ ret = run_merge();\n> > }\n> >- return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> >+ ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> >+\n> >+ if (!ret && recurse_submodules)\n> >+ ret = update_submodules();\n> >+\n> >+ return ret;\n> > } else {\n> > return run_merge();\n> > }\n> >diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> >index accfa5cc0..234a22315 100755\n> >--- a/t/t5572-pull-submodule.sh\n> >+++ b/t/t5572-pull-submodule.sh\n> >@@ -42,4 +42,101 @@\n> >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> >+ git init child &&\n> >+ (\n> >+ cd child &&\n> >+ echo \"bar\" >file &&\n> >+ git add file &&\n> >+ git commit -m \"initial commit\"\n> >+ ) &&\n> >+ git init parent &&\n> >+ (\n> >+ cd parent &&\n> >+ echo \"foo\" >file &&\n> >+ git add file &&\n> >+ git commit -m \"Initial commit\" &&\n> >+ git submodule add ../child sub &&\n> >+ git commit -m \"add submodule\"\n> >+ ) &&\n> >+ git clone --recurse-submodule parent super &&\n> >+ git -C super/sub checkout master\n> >+'\n> >+\n> >+test_expect_success 'pull recursive fails without --rebase' '\n> >+ test_must_fail git -C super pull --recurse-submodules 2>actual &&\n> >+ test_i18ngrep \"recurse-submodules is only valid with --rebase\" actual\n> >+'\n> >+\n> >+test_expect_success 'pull basic recurse' '\n> >+ (\n> >+ cd child &&\n> >+ echo \"foobar\" >>file &&\n> >+ git add file &&\n> >+ git commit -m \"update file\"\n> >+ ) &&\n> >+ (\n> >+ cd parent &&\n> >+ git -C sub pull &&\n> >+ git add sub &&\n> >+ git commit -m \"update submodule\"\n> >+ ) &&\n> >+\n> >+ git -C parent rev-parse master >expect_super &&\n> >+ git -C child rev-parse master >expect_sub &&\n> >+\n> >+ git -C super pull --rebase --recurse-submodules &&\n> >+ git -C super rev-parse master >actual_super &&\n> >+ git -C super/sub rev-parse master >actual_sub &&\n> >+ test_cmp expect_super actual_super &&\n> >+ test_cmp expect_sub actual_sub\n> >+'\n> >+\n> >+test_expect_success 'pull basic rebase recurse' '\n> >+ (\n> >+ cd child &&\n> >+ echo \"a\" >file &&\n> >+ git add file &&\n> >+ git commit -m \"update file\"\n> >+ ) &&\n> >+ (\n> >+ cd parent &&\n> >+ git -C sub pull &&\n> >+ git add sub &&\n> >+ git commit -m \"update submodule\"\n> >+ ) &&\n> >+ (\n> >+ cd super/sub &&\n> >+ echo \"b\" >file2 &&\n> >+ git add file2 &&\n> >+ git commit -m \"add file2\"\n> >+ ) &&\n> >+\n> >+ git -C parent rev-parse master >expect_super &&\n> >+ git -C child rev-parse master >expect_sub &&\n> >+\n> >+ git -C super pull --rebase --recurse-submodules &&\n> >+ git -C super rev-parse master >actual_super &&\n> >+ git -C super/sub rev-parse master^ >actual_sub &&\n> >+ test_cmp expect_super actual_super &&\n> >+ test_cmp expect_sub actual_sub &&\n> >+\n> >+ echo \"a\" >expect &&\n> >+ test_cmp expect super/sub/file &&\n> >+ echo \"b\" >expect &&\n> >+ test_cmp expect super/sub/file2\n> >+'\n> >+\n> >+test_expect_success 'pull rebase recursing fails with conflicts' '\n> >+ git -C super/sub reset --hard HEAD^^ &&\n> >+ git -C super reset --hard HEAD^ &&\n> >+ (\n> >+ cd super/sub &&\n> >+ echo \"b\" >file &&\n> >+ git add file &&\n> >+ git commit -m \"update file\"\n> >+ ) &&\n> >+ test_must_fail git -C super pull --rebase --recurse-submodules\n> >+'\n> >+\n> >test_done\n> >-- \n> >2.13.0.rc2.291.g57267f2277-goog\n> >\n> >\n> \n\n-- \nBrandon Williams\n"},{"id":"319849","messageId":"20170515230919.GF79147@google.com","threadId":"45936","inReplyTo":"CAGZ79kYYX_AMZm7Di8cUA_eiDS+SSAGnJyrDLcC5U7POk7WdSw@mail.gmail.com","subject":"Re: [PATCH] pull: optionally rebase submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-15T23:09:19Z","receivedAt":"2017-05-15T23:09:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/11, Stefan Beller wrote:\n> On Thu, May 11, 2017 at 10:24 AM, Brandon Williams <bmwill@google.com> wrote:\n> > Teach pull to optionally update submodules when '--recurse-submodules'\n> > is provided.  This will teach pull to run 'submodule update --rebase'\n> > when the '--recurse-submodules' and '--rebase' flags are given.\n> >\n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> > ---\n> >\n> > Pull is already a shortcut for running fetch followed by a merge/rebase, so why\n> > not have it be a shortcut for running 'submodule update --rebase' when the\n> > recurse flag is given!\n> \n> I have not thought about the implications of this shortcut, as opposed to\n> actually implementing it in C (which presumably would contain more checks).\n> Will do.\n\nWell this would be a short up until we actually implement recursion in\nmerge and rebase.  For rebase we may want to wait until its completely\nported to C since that effort is still underway.  Alternatively we can avoid\nthis shortcut and wait until rebase is finished being ported.\n\n> \n> >\n> >  builtin/pull.c            | 30 ++++++++++++++-\n> >  t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 125 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/pull.c b/builtin/pull.c\n> > index dd1a4a94e..d73d654e6 100644\n> > --- a/builtin/pull.c\n> > +++ b/builtin/pull.c\n> > @@ -77,6 +77,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;\n> >\n> >  /* Options passed to git-merge or git-rebase */\n> >  static enum rebase_type opt_rebase = -1;\n> > @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,\n> >         return 0;\n> >  }\n> >\n> > +static int  update_submodules(void)\n> \n> Maybe s/update_submodules/rebase_submodules/ ?\n> \n> > +{\n> > +       struct child_process cp = CHILD_PROCESS_INIT;\n> > +       cp.git_cmd = 1;\n> > +\n> > +       argv_array_pushl(&cp.args, \"submodule\", \"update\", \"--recursive\", NULL);\n> > +       argv_array_push(&cp.args, \"--rebase\");\n> \n> The --rebase could be part of the _pushl ?\n> Also we could set\n>     no_stdin = 1\n> we do need stdout/err though.\n\ncan do.\n\n> \n> \n> > +\n> > +       return run_command(&cp);\n> > +}\n> > +\n> >  /**\n> >   * Runs git-merge, returning its exit status.\n> >   */\n> > @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> >                         oidclr(&rebase_fork_point);\n> >         }\n> >\n> > +       if (opt_recurse_submodules &&\n> > +           !strcmp(opt_recurse_submodules, \"--recurse-submodules\")) {\n> \n> So this checks if we pass --recurse-submodules to fetch and if it is not\n> a on-demand/no.\n> Maybe we'd want to use the same infrastructure as fetch does, such that\n> parse_fetch_recurse makes the decision. (Then \"--recurse-submodules=TrUe\"\n> would work as well, IIUC)\n\nAgreed, it may be better to actually parse the switch properly.\n\n> \n> \n> > +               recurse_submodules = 1;\n> > +\n> > +               if (!opt_rebase)\n> > +                       die(_(\"--recurse-submodules is only valid with --rebase\"));\n> \n> I wonder if there are existing users of \"git pull --recurse --merge\";\n> as of now this would fetch the submodules (on-demand) and merge\n> in the local commits of the superprojects. It sounds like a useful workflow\n> which we'd be blocking here? Maybe just do nothing in case of !opt_rebase,\n> i.e. make it part of the first condition added in this hunk?\n> \n> > +               ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n> > +\n> > +               if (!ret && recurse_submodules)\n> > +                       ret = update_submodules();\n> \n> Instead of doing the rebase of submodules here, we may just want to pass\n> 'recurse_submodules' into run_rebase, which can do it, too. (It already has\n> a 'ret' value, so the main cmd is not as cluttered.\n> \n> ---\n> Before reviewing the tests, let's step a bit back and talk about the design\n> and what is useful to the user. From reading the code, we\n>   1) perform a fetch in the superproject\n>   2) rebase the superproject (not rewriting any submodule pointers,\n>     but that may be ok for now)\n>   3) sequentially:\n>   3a) fetch each submodule on demand\n>   3b) run rebase inside of it.\n> \n> \n> (A) On the sequence:\n> If in a normal pull --rebase we have a failure, we can fixup the failure\n> and then continue via \"git rebase --continue\"; now if we have a failure\n> in 3), we would need to fixup the submodule ourselves and then as\n> we lost all progress in the superproject, rerun \"pull --rebase --recurse\"?\n\nYeah this may not have the best workflow...\n\n> \n> (B)\n> As discussed offline this produces bad results if we have a non-ff\n> operation in the superproject, that also has submodule pointer updates.\n> So additionally we would need to walk the superprojects local commits\n> and check if any submodule is touched.\n> \n> \n> >\n> > +test_expect_success 'pull --recurse-submodule setup' '\n> > +       git init child &&\n> \n> test_create_repo child\n> \n> > +       (\n> > +               cd child &&\n> > +               echo \"bar\" >file &&\n> > +               git add file &&\n> > +               git commit -m \"initial commit\"\n> \n> test_create_commit -C child\n> \n> > +       ) &&\n> > +       git init parent &&\n> > +       (\n> > +               cd parent &&\n> > +               echo \"foo\" >file &&\n> > +               git add file &&\n> > +               git commit -m \"Initial commit\" &&\n> > +               git submodule add ../child sub &&\n> > +               git commit -m \"add submodule\"\n> > +       ) &&\n> \n> Same setup comment as for the child\n> \n> \n> > +       git clone --recurse-submodule parent super &&\n> > +       git -C super/sub checkout master\n> \n> I wonder if we want to keep these two commands in each test\n> as I noticed some test scripts are horribly messy others have\n> a pattern to cleanup after themselves:\n> \n> test_expect_....\n>     test_when_finished \"rm -rf super-clone\" &&\n>     git clone ... into super-clone\n> \n> \n> \n> > +'\n> > +\n> > +test_expect_success 'pull recursive fails without --rebase' '\n> > +       test_must_fail git -C super pull --recurse-submodules 2>actual &&\n> > +       test_i18ngrep \"recurse-submodules is only valid with --rebase\" actual\n> > +'\n> \n> Side note: another place to add tests could be t5520 or t740*.\n> \n> > +test_expect_success 'pull rebase recursing fails with conflicts' '\n> > +       git -C super/sub reset --hard HEAD^^ &&\n> > +       git -C super reset --hard HEAD^ &&\n> > +       (\n> > +               cd super/sub &&\n> > +               echo \"b\" >file &&\n> > +               git add file &&\n> > +               git commit -m \"update file\"\n> > +       ) &&\n> > +       test_must_fail git -C super pull --rebase --recurse-submodules\n> \n> As discussed above: We'd also want to have a reasonable state here,\n> or some advice to the user telling them how to recover. Maybe in a\n> first approach we can tell them to re-run \"submodule update --rebase\"\n> after fixing the conflict?\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"}]}