[PATCH] pull: optionally rebase submodules

[PATCH] pull: optionally rebase submodules

[Brandon Williams]

Teach pull to optionally update submodules when '--recurse-submodules'
is provided.  This will teach pull to run 'submodule update --rebase'
when the '--recurse-submodules' and '--rebase' flags are given.

Signed-off-by: Brandon Williams <bmwill@google.com>
---

Pull is already a shortcut for running fetch followed by a merge/rebase, so why
not have it be a shortcut for running 'submodule update --rebase' when the
recurse flag is given!

 builtin/pull.c            | 30 ++++++++++++++-
 t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index dd1a4a94e..d73d654e6 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -77,6 +77,7 @@ static const char * const pull_usage[] = {
 /* Shared options */
 static int opt_verbosity;
 static char *opt_progress;
+static int recurse_submodules;
 
 /* Options passed to git-merge or git-rebase */
 static enum rebase_type opt_rebase = -1;
@@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,
 	return 0;
 }
 
+static int  update_submodules(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	cp.git_cmd = 1;
+
+	argv_array_pushl(&cp.args, "submodule", "update", "--recursive", NULL);
+	argv_array_push(&cp.args, "--rebase");
+
+	return run_command(&cp);
+}
+
 /**
  * Runs git-merge, returning its exit status.
  */
@@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			oidclr(&rebase_fork_point);
 	}
 
+	if (opt_recurse_submodules &&
+	    !strcmp(opt_recurse_submodules, "--recurse-submodules")) {
+		recurse_submodules = 1;
+
+		if (!opt_rebase)
+			die(_("--recurse-submodules is only valid with --rebase"));
+	}
+
 	if (run_fetch(repo, refspecs))
 		return 1;
 
@@ -862,6 +882,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		die(_("Cannot rebase onto multiple branches."));
 
 	if (opt_rebase) {
+		int ret = 0;
 		struct commit_list *list = NULL;
 		struct commit *merge_head, *head;
 
@@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (is_descendant_of(merge_head, list)) {
 			/* we can fast-forward this without invoking rebase */
 			opt_ff = "--ff-only";
-			return run_merge();
+			ret = run_merge();
 		}
-		return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
+		ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
+
+		if (!ret && recurse_submodules)
+			ret = update_submodules();
+
+		return ret;
 	} else {
 		return run_merge();
 	}
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index accfa5cc0..234a22315 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -42,4 +42,101 @@ KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "git_pull_noff"
 
+test_expect_success 'pull --recurse-submodule setup' '
+	git init child &&
+	(
+		cd child &&
+		echo "bar" >file &&
+		git add file &&
+		git commit -m "initial commit"
+	) &&
+	git init parent &&
+	(
+		cd parent &&
+		echo "foo" >file &&
+		git add file &&
+		git commit -m "Initial commit" &&
+		git submodule add ../child sub &&
+		git commit -m "add submodule"
+	) &&
+	git clone --recurse-submodule parent super &&
+	git -C super/sub checkout master
+'
+
+test_expect_success 'pull recursive fails without --rebase' '
+	test_must_fail git -C super pull --recurse-submodules 2>actual &&
+	test_i18ngrep "recurse-submodules is only valid with --rebase" actual
+'
+
+test_expect_success 'pull basic recurse' '
+	(
+		cd child &&
+		echo "foobar" >>file &&
+		git add file &&
+		git commit -m "update file"
+	) &&
+	(
+		cd parent &&
+		git -C sub pull &&
+		git add sub &&
+		git commit -m "update submodule"
+	) &&
+
+	git -C parent rev-parse master >expect_super &&
+	git -C child rev-parse master >expect_sub &&
+
+	git -C super pull --rebase --recurse-submodules &&
+	git -C super rev-parse master >actual_super &&
+	git -C super/sub rev-parse master >actual_sub &&
+	test_cmp expect_super actual_super &&
+	test_cmp expect_sub actual_sub
+'
+
+test_expect_success 'pull basic rebase recurse' '
+	(
+		cd child &&
+		echo "a" >file &&
+		git add file &&
+		git commit -m "update file"
+	) &&
+	(
+		cd parent &&
+		git -C sub pull &&
+		git add sub &&
+		git commit -m "update submodule"
+	) &&
+	(
+		cd super/sub &&
+		echo "b" >file2 &&
+		git add file2 &&
+		git commit -m "add file2"
+	) &&
+
+	git -C parent rev-parse master >expect_super &&
+	git -C child rev-parse master >expect_sub &&
+
+	git -C super pull --rebase --recurse-submodules &&
+	git -C super rev-parse master >actual_super &&
+	git -C super/sub rev-parse master^ >actual_sub &&
+	test_cmp expect_super actual_super &&
+	test_cmp expect_sub actual_sub &&
+
+	echo "a" >expect &&
+	test_cmp expect super/sub/file &&
+	echo "b" >expect &&
+	test_cmp expect super/sub/file2
+'
+
+test_expect_success 'pull rebase recursing fails with conflicts' '
+	git -C super/sub reset --hard HEAD^^ &&
+	git -C super reset --hard HEAD^ &&
+	(
+		cd super/sub &&
+		echo "b" >file &&
+		git add file &&
+		git commit -m "update file"
+	) &&
+	test_must_fail git -C super pull --rebase --recurse-submodules
+'
+
 test_done
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH] pull: optionally rebase submodules

[Stefan Beller]

On Thu, May 11, 2017 at 10:24 AM, Brandon Williams <bmwill@google.com> wrote:
> Teach pull to optionally update submodules when '--recurse-submodules'
> is provided.  This will teach pull to run 'submodule update --rebase'
> when the '--recurse-submodules' and '--rebase' flags are given.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> Pull is already a shortcut for running fetch followed by a merge/rebase, so why
> not have it be a shortcut for running 'submodule update --rebase' when the
> recurse flag is given!

I have not thought about the implications of this shortcut, as opposed to
actually implementing it in C (which presumably would contain more checks).
Will do.

>
>  builtin/pull.c            | 30 ++++++++++++++-
>  t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index dd1a4a94e..d73d654e6 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -77,6 +77,7 @@ static const char * const pull_usage[] = {
>  /* Shared options */
>  static int opt_verbosity;
>  static char *opt_progress;
> +static int recurse_submodules;
>
>  /* Options passed to git-merge or git-rebase */
>  static enum rebase_type opt_rebase = -1;
> @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,
>         return 0;
>  }
>
> +static int  update_submodules(void)

Maybe s/update_submodules/rebase_submodules/ ?

> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       cp.git_cmd = 1;
> +
> +       argv_array_pushl(&cp.args, "submodule", "update", "--recursive", NULL);
> +       argv_array_push(&cp.args, "--rebase");

The --rebase could be part of the _pushl ?
Also we could set
    no_stdin = 1
we do need stdout/err though.


> +
> +       return run_command(&cp);
> +}
> +
>  /**
>   * Runs git-merge, returning its exit status.
>   */
> @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                         oidclr(&rebase_fork_point);
>         }
>
> +       if (opt_recurse_submodules &&
> +           !strcmp(opt_recurse_submodules, "--recurse-submodules")) {

So this checks if we pass --recurse-submodules to fetch and if it is not
a on-demand/no.
Maybe we'd want to use the same infrastructure as fetch does, such that
parse_fetch_recurse makes the decision. (Then "--recurse-submodules=TrUe"
would work as well, IIUC)


> +               recurse_submodules = 1;
> +
> +               if (!opt_rebase)
> +                       die(_("--recurse-submodules is only valid with --rebase"));

I wonder if there are existing users of "git pull --recurse --merge";
as of now this would fetch the submodules (on-demand) and merge
in the local commits of the superprojects. It sounds like a useful workflow
which we'd be blocking here? Maybe just do nothing in case of !opt_rebase,
i.e. make it part of the first condition added in this hunk?

> +               ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> +
> +               if (!ret && recurse_submodules)
> +                       ret = update_submodules();

Instead of doing the rebase of submodules here, we may just want to pass
'recurse_submodules' into run_rebase, which can do it, too. (It already has
a 'ret' value, so the main cmd is not as cluttered.

---
Before reviewing the tests, let's step a bit back and talk about the design
and what is useful to the user. From reading the code, we
  1) perform a fetch in the superproject
  2) rebase the superproject (not rewriting any submodule pointers,
    but that may be ok for now)
  3) sequentially:
  3a) fetch each submodule on demand
  3b) run rebase inside of it.


(A) On the sequence:
If in a normal pull --rebase we have a failure, we can fixup the failure
and then continue via "git rebase --continue"; now if we have a failure
in 3), we would need to fixup the submodule ourselves and then as
we lost all progress in the superproject, rerun "pull --rebase --recurse"?

(B)
As discussed offline this produces bad results if we have a non-ff
operation in the superproject, that also has submodule pointer updates.
So additionally we would need to walk the superprojects local commits
and check if any submodule is touched.


>
> +test_expect_success 'pull --recurse-submodule setup' '
> +       git init child &&

test_create_repo child

> +       (
> +               cd child &&
> +               echo "bar" >file &&
> +               git add file &&
> +               git commit -m "initial commit"

test_create_commit -C child

> +       ) &&
> +       git init parent &&
> +       (
> +               cd parent &&
> +               echo "foo" >file &&
> +               git add file &&
> +               git commit -m "Initial commit" &&
> +               git submodule add ../child sub &&
> +               git commit -m "add submodule"
> +       ) &&

Same setup comment as for the child


> +       git clone --recurse-submodule parent super &&
> +       git -C super/sub checkout master

I wonder if we want to keep these two commands in each test
as I noticed some test scripts are horribly messy others have
a pattern to cleanup after themselves:

test_expect_....
    test_when_finished "rm -rf super-clone" &&
    git clone ... into super-clone



> +'
> +
> +test_expect_success 'pull recursive fails without --rebase' '
> +       test_must_fail git -C super pull --recurse-submodules 2>actual &&
> +       test_i18ngrep "recurse-submodules is only valid with --rebase" actual
> +'

Side note: another place to add tests could be t5520 or t740*.

> +test_expect_success 'pull rebase recursing fails with conflicts' '
> +       git -C super/sub reset --hard HEAD^^ &&
> +       git -C super reset --hard HEAD^ &&
> +       (
> +               cd super/sub &&
> +               echo "b" >file &&
> +               git add file &&
> +               git commit -m "update file"
> +       ) &&
> +       test_must_fail git -C super pull --rebase --recurse-submodules

As discussed above: We'd also want to have a reasonable state here,
or some advice to the user telling them how to recover. Maybe in a
first approach we can tell them to re-run "submodule update --rebase"
after fixing the conflict?

Thanks,
Stefan

Re: [PATCH] pull: optionally rebase submodules

[Philip Oakley]

From: "Brandon Williams" <bmwill@google.com>
> Teach pull to optionally update submodules when '--recurse-submodules'
> is provided.  This will teach pull to run 'submodule update --rebase'
> when the '--recurse-submodules' and '--rebase' flags are given.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> Pull is already a shortcut for running fetch followed by a merge/rebase, 
> so why
> not have it be a shortcut for running 'submodule update --rebase' when the
> recurse flag is given!
>
> builtin/pull.c            | 30 ++++++++++++++-
> t/t5572-pull-submodule.sh | 97 
> +++++++++++++++++++++++++++++++++++++++++++++++

Shouldn't this also touch the documentation to say that this has been 
taught?
--
Philip


> 2 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index dd1a4a94e..d73d654e6 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -77,6 +77,7 @@ static const char * const pull_usage[] = {
> /* Shared options */
> static int opt_verbosity;
> static char *opt_progress;
> +static int recurse_submodules;
>
> /* Options passed to git-merge or git-rebase */
> static enum rebase_type opt_rebase = -1;
> @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id 
> *merge_head,
>  return 0;
> }
>
> +static int  update_submodules(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + cp.git_cmd = 1;
> +
> + argv_array_pushl(&cp.args, "submodule", "update", "--recursive", NULL);
> + argv_array_push(&cp.args, "--rebase");
> +
> + return run_command(&cp);
> +}
> +
> /**
>  * Runs git-merge, returning its exit status.
>  */
> @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>  oidclr(&rebase_fork_point);
>  }
>
> + if (opt_recurse_submodules &&
> +     !strcmp(opt_recurse_submodules, "--recurse-submodules")) {
> + recurse_submodules = 1;
> +
> + if (!opt_rebase)
> + die(_("--recurse-submodules is only valid with --rebase"));
> + }
> +
>  if (run_fetch(repo, refspecs))
>  return 1;
>
> @@ -862,6 +882,7 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>  die(_("Cannot rebase onto multiple branches."));
>
>  if (opt_rebase) {
> + int ret = 0;
>  struct commit_list *list = NULL;
>  struct commit *merge_head, *head;
>
> @@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>  if (is_descendant_of(merge_head, list)) {
>  /* we can fast-forward this without invoking rebase */
>  opt_ff = "--ff-only";
> - return run_merge();
> + ret = run_merge();
>  }
> - return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> + ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> +
> + if (!ret && recurse_submodules)
> + ret = update_submodules();
> +
> + return ret;
>  } else {
>  return run_merge();
>  }
> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index accfa5cc0..234a22315 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -42,4 +42,101 @@ 
> KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
> KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> test_submodule_switch "git_pull_noff"
>
> +test_expect_success 'pull --recurse-submodule setup' '
> + git init child &&
> + (
> + cd child &&
> + echo "bar" >file &&
> + git add file &&
> + git commit -m "initial commit"
> + ) &&
> + git init parent &&
> + (
> + cd parent &&
> + echo "foo" >file &&
> + git add file &&
> + git commit -m "Initial commit" &&
> + git submodule add ../child sub &&
> + git commit -m "add submodule"
> + ) &&
> + git clone --recurse-submodule parent super &&
> + git -C super/sub checkout master
> +'
> +
> +test_expect_success 'pull recursive fails without --rebase' '
> + test_must_fail git -C super pull --recurse-submodules 2>actual &&
> + test_i18ngrep "recurse-submodules is only valid with --rebase" actual
> +'
> +
> +test_expect_success 'pull basic recurse' '
> + (
> + cd child &&
> + echo "foobar" >>file &&
> + git add file &&
> + git commit -m "update file"
> + ) &&
> + (
> + cd parent &&
> + git -C sub pull &&
> + git add sub &&
> + git commit -m "update submodule"
> + ) &&
> +
> + git -C parent rev-parse master >expect_super &&
> + git -C child rev-parse master >expect_sub &&
> +
> + git -C super pull --rebase --recurse-submodules &&
> + git -C super rev-parse master >actual_super &&
> + git -C super/sub rev-parse master >actual_sub &&
> + test_cmp expect_super actual_super &&
> + test_cmp expect_sub actual_sub
> +'
> +
> +test_expect_success 'pull basic rebase recurse' '
> + (
> + cd child &&
> + echo "a" >file &&
> + git add file &&
> + git commit -m "update file"
> + ) &&
> + (
> + cd parent &&
> + git -C sub pull &&
> + git add sub &&
> + git commit -m "update submodule"
> + ) &&
> + (
> + cd super/sub &&
> + echo "b" >file2 &&
> + git add file2 &&
> + git commit -m "add file2"
> + ) &&
> +
> + git -C parent rev-parse master >expect_super &&
> + git -C child rev-parse master >expect_sub &&
> +
> + git -C super pull --rebase --recurse-submodules &&
> + git -C super rev-parse master >actual_super &&
> + git -C super/sub rev-parse master^ >actual_sub &&
> + test_cmp expect_super actual_super &&
> + test_cmp expect_sub actual_sub &&
> +
> + echo "a" >expect &&
> + test_cmp expect super/sub/file &&
> + echo "b" >expect &&
> + test_cmp expect super/sub/file2
> +'
> +
> +test_expect_success 'pull rebase recursing fails with conflicts' '
> + git -C super/sub reset --hard HEAD^^ &&
> + git -C super reset --hard HEAD^ &&
> + (
> + cd super/sub &&
> + echo "b" >file &&
> + git add file &&
> + git commit -m "update file"
> + ) &&
> + test_must_fail git -C super pull --rebase --recurse-submodules
> +'
> +
> test_done
> -- 
> 2.13.0.rc2.291.g57267f2277-goog
>
> 

Re: [PATCH] pull: optionally rebase submodules

[Brandon Williams]

On 05/11, Philip Oakley wrote:
> From: "Brandon Williams" <bmwill@google.com>
> >Teach pull to optionally update submodules when '--recurse-submodules'
> >is provided.  This will teach pull to run 'submodule update --rebase'
> >when the '--recurse-submodules' and '--rebase' flags are given.
> >
> >Signed-off-by: Brandon Williams <bmwill@google.com>
> >---
> >
> >Pull is already a shortcut for running fetch followed by a
> >merge/rebase, so why
> >not have it be a shortcut for running 'submodule update --rebase' when the
> >recurse flag is given!
> >
> >builtin/pull.c            | 30 ++++++++++++++-
> >t/t5572-pull-submodule.sh | 97
> >+++++++++++++++++++++++++++++++++++++++++++++++
> 
> Shouldn't this also touch the documentation to say that this has
> been taught?

Yes it should, thanks for reminding me!

> --
> Philip
> 
> 
> >2 files changed, 125 insertions(+), 2 deletions(-)
> >
> >diff --git a/builtin/pull.c b/builtin/pull.c
> >index dd1a4a94e..d73d654e6 100644
> >--- a/builtin/pull.c
> >+++ b/builtin/pull.c
> >@@ -77,6 +77,7 @@ static const char * const pull_usage[] = {
> >/* Shared options */
> >static int opt_verbosity;
> >static char *opt_progress;
> >+static int recurse_submodules;
> >
> >/* Options passed to git-merge or git-rebase */
> >static enum rebase_type opt_rebase = -1;
> >@@ -532,6 +533,17 @@ static int pull_into_void(const struct
> >object_id *merge_head,
> > return 0;
> >}
> >
> >+static int  update_submodules(void)
> >+{
> >+ struct child_process cp = CHILD_PROCESS_INIT;
> >+ cp.git_cmd = 1;
> >+
> >+ argv_array_pushl(&cp.args, "submodule", "update", "--recursive", NULL);
> >+ argv_array_push(&cp.args, "--rebase");
> >+
> >+ return run_command(&cp);
> >+}
> >+
> >/**
> > * Runs git-merge, returning its exit status.
> > */
> >@@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv,
> >const char *prefix)
> > oidclr(&rebase_fork_point);
> > }
> >
> >+ if (opt_recurse_submodules &&
> >+     !strcmp(opt_recurse_submodules, "--recurse-submodules")) {
> >+ recurse_submodules = 1;
> >+
> >+ if (!opt_rebase)
> >+ die(_("--recurse-submodules is only valid with --rebase"));
> >+ }
> >+
> > if (run_fetch(repo, refspecs))
> > return 1;
> >
> >@@ -862,6 +882,7 @@ int cmd_pull(int argc, const char **argv,
> >const char *prefix)
> > die(_("Cannot rebase onto multiple branches."));
> >
> > if (opt_rebase) {
> >+ int ret = 0;
> > struct commit_list *list = NULL;
> > struct commit *merge_head, *head;
> >
> >@@ -871,9 +892,14 @@ int cmd_pull(int argc, const char **argv,
> >const char *prefix)
> > if (is_descendant_of(merge_head, list)) {
> > /* we can fast-forward this without invoking rebase */
> > opt_ff = "--ff-only";
> >- return run_merge();
> >+ ret = run_merge();
> > }
> >- return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> >+ ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> >+
> >+ if (!ret && recurse_submodules)
> >+ ret = update_submodules();
> >+
> >+ return ret;
> > } else {
> > return run_merge();
> > }
> >diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> >index accfa5cc0..234a22315 100755
> >--- a/t/t5572-pull-submodule.sh
> >+++ b/t/t5572-pull-submodule.sh
> >@@ -42,4 +42,101 @@
> >KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
> >KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> >test_submodule_switch "git_pull_noff"
> >
> >+test_expect_success 'pull --recurse-submodule setup' '
> >+ git init child &&
> >+ (
> >+ cd child &&
> >+ echo "bar" >file &&
> >+ git add file &&
> >+ git commit -m "initial commit"
> >+ ) &&
> >+ git init parent &&
> >+ (
> >+ cd parent &&
> >+ echo "foo" >file &&
> >+ git add file &&
> >+ git commit -m "Initial commit" &&
> >+ git submodule add ../child sub &&
> >+ git commit -m "add submodule"
> >+ ) &&
> >+ git clone --recurse-submodule parent super &&
> >+ git -C super/sub checkout master
> >+'
> >+
> >+test_expect_success 'pull recursive fails without --rebase' '
> >+ test_must_fail git -C super pull --recurse-submodules 2>actual &&
> >+ test_i18ngrep "recurse-submodules is only valid with --rebase" actual
> >+'
> >+
> >+test_expect_success 'pull basic recurse' '
> >+ (
> >+ cd child &&
> >+ echo "foobar" >>file &&
> >+ git add file &&
> >+ git commit -m "update file"
> >+ ) &&
> >+ (
> >+ cd parent &&
> >+ git -C sub pull &&
> >+ git add sub &&
> >+ git commit -m "update submodule"
> >+ ) &&
> >+
> >+ git -C parent rev-parse master >expect_super &&
> >+ git -C child rev-parse master >expect_sub &&
> >+
> >+ git -C super pull --rebase --recurse-submodules &&
> >+ git -C super rev-parse master >actual_super &&
> >+ git -C super/sub rev-parse master >actual_sub &&
> >+ test_cmp expect_super actual_super &&
> >+ test_cmp expect_sub actual_sub
> >+'
> >+
> >+test_expect_success 'pull basic rebase recurse' '
> >+ (
> >+ cd child &&
> >+ echo "a" >file &&
> >+ git add file &&
> >+ git commit -m "update file"
> >+ ) &&
> >+ (
> >+ cd parent &&
> >+ git -C sub pull &&
> >+ git add sub &&
> >+ git commit -m "update submodule"
> >+ ) &&
> >+ (
> >+ cd super/sub &&
> >+ echo "b" >file2 &&
> >+ git add file2 &&
> >+ git commit -m "add file2"
> >+ ) &&
> >+
> >+ git -C parent rev-parse master >expect_super &&
> >+ git -C child rev-parse master >expect_sub &&
> >+
> >+ git -C super pull --rebase --recurse-submodules &&
> >+ git -C super rev-parse master >actual_super &&
> >+ git -C super/sub rev-parse master^ >actual_sub &&
> >+ test_cmp expect_super actual_super &&
> >+ test_cmp expect_sub actual_sub &&
> >+
> >+ echo "a" >expect &&
> >+ test_cmp expect super/sub/file &&
> >+ echo "b" >expect &&
> >+ test_cmp expect super/sub/file2
> >+'
> >+
> >+test_expect_success 'pull rebase recursing fails with conflicts' '
> >+ git -C super/sub reset --hard HEAD^^ &&
> >+ git -C super reset --hard HEAD^ &&
> >+ (
> >+ cd super/sub &&
> >+ echo "b" >file &&
> >+ git add file &&
> >+ git commit -m "update file"
> >+ ) &&
> >+ test_must_fail git -C super pull --rebase --recurse-submodules
> >+'
> >+
> >test_done
> >-- 
> >2.13.0.rc2.291.g57267f2277-goog
> >
> >
> 

-- 
Brandon Williams

Re: [PATCH] pull: optionally rebase submodules

[Brandon Williams]

On 05/11, Stefan Beller wrote:
> On Thu, May 11, 2017 at 10:24 AM, Brandon Williams <bmwill@google.com> wrote:
> > Teach pull to optionally update submodules when '--recurse-submodules'
> > is provided.  This will teach pull to run 'submodule update --rebase'
> > when the '--recurse-submodules' and '--rebase' flags are given.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >
> > Pull is already a shortcut for running fetch followed by a merge/rebase, so why
> > not have it be a shortcut for running 'submodule update --rebase' when the
> > recurse flag is given!
> 
> I have not thought about the implications of this shortcut, as opposed to
> actually implementing it in C (which presumably would contain more checks).
> Will do.

Well this would be a short up until we actually implement recursion in
merge and rebase.  For rebase we may want to wait until its completely
ported to C since that effort is still underway.  Alternatively we can avoid
this shortcut and wait until rebase is finished being ported.

> 
> >
> >  builtin/pull.c            | 30 ++++++++++++++-
> >  t/t5572-pull-submodule.sh | 97 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 125 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index dd1a4a94e..d73d654e6 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -77,6 +77,7 @@ static const char * const pull_usage[] = {
> >  /* Shared options */
> >  static int opt_verbosity;
> >  static char *opt_progress;
> > +static int recurse_submodules;
> >
> >  /* Options passed to git-merge or git-rebase */
> >  static enum rebase_type opt_rebase = -1;
> > @@ -532,6 +533,17 @@ static int pull_into_void(const struct object_id *merge_head,
> >         return 0;
> >  }
> >
> > +static int  update_submodules(void)
> 
> Maybe s/update_submodules/rebase_submodules/ ?
> 
> > +{
> > +       struct child_process cp = CHILD_PROCESS_INIT;
> > +       cp.git_cmd = 1;
> > +
> > +       argv_array_pushl(&cp.args, "submodule", "update", "--recursive", NULL);
> > +       argv_array_push(&cp.args, "--rebase");
> 
> The --rebase could be part of the _pushl ?
> Also we could set
>     no_stdin = 1
> we do need stdout/err though.

can do.

> 
> 
> > +
> > +       return run_command(&cp);
> > +}
> > +
> >  /**
> >   * Runs git-merge, returning its exit status.
> >   */
> > @@ -816,6 +828,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                         oidclr(&rebase_fork_point);
> >         }
> >
> > +       if (opt_recurse_submodules &&
> > +           !strcmp(opt_recurse_submodules, "--recurse-submodules")) {
> 
> So this checks if we pass --recurse-submodules to fetch and if it is not
> a on-demand/no.
> Maybe we'd want to use the same infrastructure as fetch does, such that
> parse_fetch_recurse makes the decision. (Then "--recurse-submodules=TrUe"
> would work as well, IIUC)

Agreed, it may be better to actually parse the switch properly.

> 
> 
> > +               recurse_submodules = 1;
> > +
> > +               if (!opt_rebase)
> > +                       die(_("--recurse-submodules is only valid with --rebase"));
> 
> I wonder if there are existing users of "git pull --recurse --merge";
> as of now this would fetch the submodules (on-demand) and merge
> in the local commits of the superprojects. It sounds like a useful workflow
> which we'd be blocking here? Maybe just do nothing in case of !opt_rebase,
> i.e. make it part of the first condition added in this hunk?
> 
> > +               ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> > +
> > +               if (!ret && recurse_submodules)
> > +                       ret = update_submodules();
> 
> Instead of doing the rebase of submodules here, we may just want to pass
> 'recurse_submodules' into run_rebase, which can do it, too. (It already has
> a 'ret' value, so the main cmd is not as cluttered.
> 
> ---
> Before reviewing the tests, let's step a bit back and talk about the design
> and what is useful to the user. From reading the code, we
>   1) perform a fetch in the superproject
>   2) rebase the superproject (not rewriting any submodule pointers,
>     but that may be ok for now)
>   3) sequentially:
>   3a) fetch each submodule on demand
>   3b) run rebase inside of it.
> 
> 
> (A) On the sequence:
> If in a normal pull --rebase we have a failure, we can fixup the failure
> and then continue via "git rebase --continue"; now if we have a failure
> in 3), we would need to fixup the submodule ourselves and then as
> we lost all progress in the superproject, rerun "pull --rebase --recurse"?

Yeah this may not have the best workflow...

> 
> (B)
> As discussed offline this produces bad results if we have a non-ff
> operation in the superproject, that also has submodule pointer updates.
> So additionally we would need to walk the superprojects local commits
> and check if any submodule is touched.
> 
> 
> >
> > +test_expect_success 'pull --recurse-submodule setup' '
> > +       git init child &&
> 
> test_create_repo child
> 
> > +       (
> > +               cd child &&
> > +               echo "bar" >file &&
> > +               git add file &&
> > +               git commit -m "initial commit"
> 
> test_create_commit -C child
> 
> > +       ) &&
> > +       git init parent &&
> > +       (
> > +               cd parent &&
> > +               echo "foo" >file &&
> > +               git add file &&
> > +               git commit -m "Initial commit" &&
> > +               git submodule add ../child sub &&
> > +               git commit -m "add submodule"
> > +       ) &&
> 
> Same setup comment as for the child
> 
> 
> > +       git clone --recurse-submodule parent super &&
> > +       git -C super/sub checkout master
> 
> I wonder if we want to keep these two commands in each test
> as I noticed some test scripts are horribly messy others have
> a pattern to cleanup after themselves:
> 
> test_expect_....
>     test_when_finished "rm -rf super-clone" &&
>     git clone ... into super-clone
> 
> 
> 
> > +'
> > +
> > +test_expect_success 'pull recursive fails without --rebase' '
> > +       test_must_fail git -C super pull --recurse-submodules 2>actual &&
> > +       test_i18ngrep "recurse-submodules is only valid with --rebase" actual
> > +'
> 
> Side note: another place to add tests could be t5520 or t740*.
> 
> > +test_expect_success 'pull rebase recursing fails with conflicts' '
> > +       git -C super/sub reset --hard HEAD^^ &&
> > +       git -C super reset --hard HEAD^ &&
> > +       (
> > +               cd super/sub &&
> > +               echo "b" >file &&
> > +               git add file &&
> > +               git commit -m "update file"
> > +       ) &&
> > +       test_must_fail git -C super pull --rebase --recurse-submodules
> 
> As discussed above: We'd also want to have a reasonable state here,
> or some advice to the user telling them how to recover. Maybe in a
> first approach we can tell them to re-run "submodule update --rebase"
> after fixing the conflict?
> 
> Thanks,
> Stefan

-- 
Brandon Williams