pull: optionally rebase submodules

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 <[email protected]>

---

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(-)

builtin/pull.c +1 -0

index dd1a4a94e..d73d654e6 100644
@@ -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;

Quoted 7 lines · 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 <[email protected]>

---

Quoted 3 lines

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.

Quoted 19 lines

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

builtin/pull.c +1 -0

index dd1a4a94e..d73d654e6 100644
@@ -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;

builtin/pull.c static int pull_into_void(const struct object_id *merge_head, · line 536

Quoted 4 lines

builtin/pull.c +1 -0

@@ -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/ ?

builtin/pull.c static int pull_into_void(const struct object_id *merge_head, · line 542

Quoted 6 lines

builtin/pull.c +6 -0

+{
+       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.

Quoted 13 lines

builtin/pull.c +4 -0

+
+       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);
        }
 

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 832

Quoted 2 lines

builtin/pull.c +2 -0

+       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)

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 836

Quoted 6 lines

builtin/pull.c +4 -0

+               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?

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 900

Quoted 4 lines

builtin/pull.c +4 -0

+               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.

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix)

Quoted 3 lines

builtin/pull.c +2 -0

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

test_create_repo child

builtin/pull.c

Quoted 5 lines

builtin/pull.c +5 -0

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

test_create_commit -C child

builtin/pull.c

Quoted 10 lines

builtin/pull.c +10 -0

+       ) &&
+       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

builtin/pull.c

Quoted 2 lines

builtin/pull.c +2 -0

+       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

builtin/pull.c

Quoted 6 lines

+'

builtin/pull.c +5 -0

+
+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*.

builtin/pull.c

Quoted 10 lines

builtin/pull.c +10 -0

+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

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 <[email protected]>

---

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(-)

builtin/pull.c +12 -0

index dd1a4a94e..d73d654e6 100644
@@ -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.

*/

builtin/pull.c +16 -2

@@ -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();

}

t/t5572-pull-submodule.sh +0 -0

index accfa5cc0..234a22315 100755
@@ -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"

t/t5572-pull-submodule.sh +97 -0

+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

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

From: "Brandon Williams" <[email protected]>

Quoted 12 lines

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 <[email protected]>

---

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!

Quoted 3 lines

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

Quoted 187 lines

2 files changed, 125 insertions(+), 2 deletions(-)

builtin/pull.c +12 -0

index dd1a4a94e..d73d654e6 100644
@@ -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.

*/

builtin/pull.c +16 -2

@@ -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();

}

t/t5572-pull-submodule.sh +0 -0

index accfa5cc0..234a22315 100755
@@ -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"

t/t5572-pull-submodule.sh +97 -0

+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

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog