[2/4] t5572: add notes on a peculiar test

Commit Message

Philippe Blain Nov. 14, 2020, 12:34 a.m. UTC

From: Philippe Blain <[e-mail]>

Test 5572.63 ("branch has no merge base with remote-tracking
counterpart") was introduced in 4d36f88be7 (submodule: do not pass null
OID to setup_revisions, 2018-05-24), as a regression test for the bug
this commit was fixing (preventing a 'fatal: bad object' error when the
current branch and the remote-tracking branch we are pulling have no
merge-base).

However, the commit message for 4d36f88be7 does not describe in which
real-life situation this bug was encountered. The brief discussion on the
mailing list [1] does not either.

The regression test is not really representative of a real-life
scenario: both the local repository and its upstream have only a single
commit, and the "no merge-base" scenario is simulated by recreating this
root commit in the local repository using 'git commit-tree' before
calling 'git pull --rebase --recurse-submodules'. The rebase succeeds
and results in the local branch being reset to the same root commit as
the upstream branch.

The fix in 4d36f88be7 modifies 'submodule.c::submodule_touches_in_range'
so that if 'excl_oid' is null, which is the case when the 'git merge-base
--fork-point' invocation in 'builtin/pull.c::get_rebase_fork_point'
errors (no fork-point), then instead of 'incl_oid --not excl_oid' being
passed to setup_revisions, only 'incl_oid' is passed, and
'submodule_touches_in_range' examines 'incl_oid' and all its ancestors
to verify that they do not touch the submodule.

In test 5572.63, the recreated lone root commit in the local repository is
thus the only commit being examined by 'submodule_touches_in_range', and
this commit *adds* the submodule. However, 'submodule_touches_in_range'
*succeeds* because 'combine-diff.c::diff_tree_combined' (see the
backtrace below) returns early since this commit is the root commit
and has no parents.

  #0  diff_tree_combined at combine-diff.c:1494
  #1  0x0000000100150cbe in diff_tree_combined_merge at combine-diff.c:1649
  #2  0x00000001002c7147 in collect_changed_submodules at submodule.c:869
  #3  0x00000001002c7d6f in submodule_touches_in_range at submodule.c:1268
  #4  0x00000001000ad58b in cmd_pull at builtin/pull.c:1040

In light of all this, add a note in t5572 documenting this peculiar
test.

[1] https://lore.kernel.org/git/[e-mail]/t/#u

Signed-off-by: Philippe Blain <[e-mail]>
---
 t/t5572-pull-submodule.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Patch

diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 1d75e3b12b..7f658dba6d 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -136,6 +136,21 @@  test_expect_success 'pull rebase recursing fails with conflicts' '
 	test_i18ngrep "locally recorded submodule modifications" err
 '
 
+# NOTE:
+#
+# This test is particular because there is only a single commit in the upstream superproject
+# 'parent' (which adds the submodule 'a-submodule'). The clone of the superproject
+# ('child') hard-resets its branch to a new root commit with the same tree as the one
+# from the upstream superproject, so that its branch has no merge-base with its
+# remote-tracking counterpart, and then calls 'git pull --recurse-submodules --rebase'.
+# The result is that the local branch is reset to the remote-tracking branch (as it was
+# originally before the hard-reset).
+
+# The only commit in the range generated by 'submodule.c::submodule_touches_in_range' and
+# passed to 'submodule.c::collect_changed_submodules' is the new (regenerated) initial commit,
+# which adds the submodule.
+# However, 'submodule_touches_in_range' does not error (even though this commit adds the submodule)
+# because 'combine-diff.c::diff_tree_combined' returns early, as the initial commit has no parents.
 test_expect_success 'branch has no merge base with remote-tracking counterpart' '
 	rm -rf parent child &&