* [PATCH] pull: optionally rebase submodules
@ 2017-05-11 17:24 Brandon Williams
2017-05-11 18:12 ` Stefan Beller
2017-05-11 20:00 ` Philip Oakley
0 siblings, 2 replies; 5+ messages in thread
From: Brandon Williams @ 2017-05-11 17:24 UTC (permalink / raw)
To: git; +Cc: sbeller, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pull: optionally rebase submodules
2017-05-11 17:24 [PATCH] pull: optionally rebase submodules Brandon Williams
@ 2017-05-11 18:12 ` Stefan Beller
2017-05-15 23:09 ` Brandon Williams
2017-05-11 20:00 ` Philip Oakley
1 sibling, 1 reply; 5+ messages in thread
From: Stefan Beller @ 2017-05-11 18:12 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pull: optionally rebase submodules
2017-05-11 17:24 [PATCH] pull: optionally rebase submodules Brandon Williams
2017-05-11 18:12 ` Stefan Beller
@ 2017-05-11 20:00 ` Philip Oakley
2017-05-12 17:30 ` Brandon Williams
1 sibling, 1 reply; 5+ messages in thread
From: Philip Oakley @ 2017-05-11 20:00 UTC (permalink / raw)
To: Brandon Williams, git; +Cc: sbeller, Brandon Williams
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pull: optionally rebase submodules
2017-05-11 20:00 ` Philip Oakley
@ 2017-05-12 17:30 ` Brandon Williams
0 siblings, 0 replies; 5+ messages in thread
From: Brandon Williams @ 2017-05-12 17:30 UTC (permalink / raw)
To: Philip Oakley; +Cc: git, sbeller
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pull: optionally rebase submodules
2017-05-11 18:12 ` Stefan Beller
@ 2017-05-15 23:09 ` Brandon Williams
0 siblings, 0 replies; 5+ messages in thread
From: Brandon Williams @ 2017-05-15 23:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-15 23:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-11 17:24 [PATCH] pull: optionally rebase submodules Brandon Williams
2017-05-11 18:12 ` Stefan Beller
2017-05-15 23:09 ` Brandon Williams
2017-05-11 20:00 ` Philip Oakley
2017-05-12 17:30 ` Brandon Williams
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).