Skip to content

Commit c692e1b

Browse files
gitstergitster
committed
Merge branch 'pb/pull-rebase-recurse-submodules'
"git pull --rebase --recurse-submodules" checked for local changes in a wrong range and failed to run correctly when it should. * pb/pull-rebase-recurse-submodules: pull: check for local submodule modifications with the right range t5572: describe '--rebase' tests a little more t5572: add notes on a peculiar test pull --rebase: compute rebase arguments in separate function
2 parents e89ecfb + 5176f20 commit c692e1b

File tree

2 files changed

+90
-14
lines changed

2 files changed

+90
-14
lines changed

‎builtin/pull.c‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -852,21 +852,42 @@ static int get_octopus_merge_base(struct object_id *merge_base,
852852

853853
/**
854854
* Given the current HEAD oid, the merge head returned from git-fetch and the
855-
* fork point calculated by get_rebase_fork_point(), runs git-rebase with the
856-
* appropriate arguments and returns its exit status.
855+
* fork point calculated by get_rebase_fork_point(), compute the <newbase> and
856+
* <upstream> arguments to use for the upcoming git-rebase invocation.
857857
*/
858-
static int run_rebase(const struct object_id *curr_head,
858+
static int get_rebase_newbase_and_upstream(struct object_id *newbase,
859+
struct object_id *upstream,
860+
const struct object_id *curr_head,
859861
const struct object_id *merge_head,
860862
const struct object_id *fork_point)
861863
{
862-
int ret;
863864
struct object_id oct_merge_base;
864-
struct strvec args = STRVEC_INIT;
865865

866866
if (!get_octopus_merge_base(&oct_merge_base, curr_head, merge_head, fork_point))
867867
if (!is_null_oid(fork_point) && oideq(&oct_merge_base, fork_point))
868868
fork_point = NULL;
869869

870+
if (fork_point && !is_null_oid(fork_point))
871+
oidcpy(upstream, fork_point);
872+
else
873+
oidcpy(upstream, merge_head);
874+
875+
oidcpy(newbase, merge_head);
876+
877+
return 0;
878+
}
879+
880+
/**
881+
* Given the <newbase> and <upstream> calculated by
882+
* get_rebase_newbase_and_upstream(), runs git-rebase with the
883+
* appropriate arguments and returns its exit status.
884+
*/
885+
static int run_rebase(const struct object_id *newbase,
886+
const struct object_id *upstream)
887+
{
888+
int ret;
889+
struct strvec args = STRVEC_INIT;
890+
870891
strvec_push(&args, "rebase");
871892

872893
/* Shared options */
@@ -894,12 +915,9 @@ static int run_rebase(const struct object_id *curr_head,
894915
warning(_("ignoring --verify-signatures for rebase"));
895916

896917
strvec_push(&args, "--onto");
897-
strvec_push(&args, oid_to_hex(merge_head));
918+
strvec_push(&args, oid_to_hex(newbase));
898919

899-
if (fork_point && !is_null_oid(fork_point))
900-
strvec_push(&args, oid_to_hex(fork_point));
901-
else
902-
strvec_push(&args, oid_to_hex(merge_head));
920+
strvec_push(&args, oid_to_hex(upstream));
903921

904922
ret = run_command_v_opt(args.v, RUN_GIT_CMD);
905923
strvec_clear(&args);
@@ -1011,9 +1029,15 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
10111029
if (opt_rebase) {
10121030
int ret = 0;
10131031
int ran_ff = 0;
1032+
1033+
struct object_id newbase;
1034+
struct object_id upstream;
1035+
get_rebase_newbase_and_upstream(&newbase, &upstream, &curr_head,
1036+
merge_heads.oid, &rebase_fork_point);
1037+
10141038
if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
10151039
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
1016-
submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
1040+
submodule_touches_in_range(the_repository, &upstream, &curr_head))
10171041
die(_("cannot rebase with locally recorded submodule modifications"));
10181042
if (!autostash) {
10191043
struct commit_list *list = NULL;
@@ -1034,7 +1058,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
10341058
free_commit_list(list);
10351059
}
10361060
if (!ran_ff)
1037-
ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
1061+
ret = run_rebase(&newbase, &upstream);
10381062

10391063
if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
10401064
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))

‎t/t5572-pull-submodule.sh‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,12 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
101101
test_path_is_file super/sub/merge_strategy_4.t
102102
'
103103

104-
test_expect_success 'recursive rebasing pull' '
104+
test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
105+
# This tests the following scenario :
106+
# - local submodule has new commits
107+
# - local superproject does not have new commits
108+
# - upstream superproject has new commits that change the submodule pointer
109+
105110
# change upstream
106111
test_commit -C child rebase_strategy &&
107112
git -C parent submodule update --remote &&
@@ -116,7 +121,10 @@ test_expect_success 'recursive rebasing pull' '
116121
test_path_is_file super/sub/local_stuff.t
117122
'
118123

119-
test_expect_success 'pull rebase recursing fails with conflicts' '
124+
test_expect_success 'pull --rebase --recurse-submodules fails if both sides record submodule changes' '
125+
# This tests the following scenario :
126+
# - local superproject has new commits that change the submodule pointer
127+
# - upstream superproject has new commits that change the submodule pointer
120128
121129
# local changes in submodule recorded in superproject:
122130
test_commit -C super/sub local_stuff_2 &&
@@ -136,6 +144,50 @@ test_expect_success 'pull rebase recursing fails with conflicts' '
136144
test_i18ngrep "locally recorded submodule modifications" err
137145
'
138146

147+
test_expect_success 'pull --rebase --recurse-submodules (no submodule changes, no fork-point)' '
148+
# This tests the following scenario :
149+
# - local submodule does not have new commits
150+
# - local superproject has new commits that *do not* change the submodule pointer
151+
# - upstream superproject has new commits that *do not* change the submodule pointer
152+
# - local superproject branch has no fork-point with its remote-tracking counter-part
153+
154+
# create upstream superproject
155+
test_create_repo submodule &&
156+
test_commit -C submodule first_in_sub &&
157+
158+
test_create_repo superprojet &&
159+
test_commit -C superprojet first_in_super &&
160+
git -C superprojet submodule add ../submodule &&
161+
git -C superprojet commit -m "add submodule" &&
162+
test_commit -C superprojet third_in_super &&
163+
164+
# clone superproject
165+
git clone --recurse-submodules superprojet superclone &&
166+
167+
# add commits upstream
168+
test_commit -C superprojet fourth_in_super &&
169+
170+
# create topic branch in clone, not based on any remote-tracking branch
171+
git -C superclone checkout -b feat HEAD~1 &&
172+
test_commit -C superclone first_on_feat &&
173+
git -C superclone pull --rebase --recurse-submodules origin master
174+
'
175+
176+
# NOTE:
177+
#
178+
# This test is particular because there is only a single commit in the upstream superproject
179+
# 'parent' (which adds the submodule 'a-submodule'). The clone of the superproject
180+
# ('child') hard-resets its branch to a new root commit with the same tree as the one
181+
# from the upstream superproject, so that its branch has no merge-base with its
182+
# remote-tracking counterpart, and then calls 'git pull --recurse-submodules --rebase'.
183+
# The result is that the local branch is reset to the remote-tracking branch (as it was
184+
# originally before the hard-reset).
185+
186+
# The only commit in the range generated by 'submodule.c::submodule_touches_in_range' and
187+
# passed to 'submodule.c::collect_changed_submodules' is the new (regenerated) initial commit,
188+
# which adds the submodule.
189+
# However, 'submodule_touches_in_range' does not error (even though this commit adds the submodule)
190+
# because 'combine-diff.c::diff_tree_combined' returns early, as the initial commit has no parents.
139191
test_expect_success 'branch has no merge base with remote-tracking counterpart' '
140192
rm -rf parent child &&
141193

0 commit comments

Comments
 (0)