[PATCH v5 00/10] fetch --recurse-submodules: fetch unpopulated submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Mar 8, 2022, 00:14 UTC
- Message-ID
- <20220308001433.94995-1-chooglen@google.com>
- In-Reply-To
- <20220304005757.70107-1-chooglen@google.com>
Original cover letter: https://lore.kernel.org/git/20220210044152.78352-1-chooglen@google.com
Based off 'master'.
Thanks as always for the kind feedback :) I believe I incorporated all of feedback from the previous round except for the following:
- <xmqqk0d9w91r.fsf@gitster.g> In a recursive fetch, we thought that we could use the submodule name instead of the superproject commit oid and path.
It's true that we don't need <.super_oid, .path> in order to init the subrepo, but it turns out that recursive fetch reads some configuration values from .gitmodules (via submodule_from_path()), so we still need to store super_oid in order to read the correct .gitmodules file.
- <20220304235328.649768-1-jonathantanmy@google.com> I've described the differences between the no-submodule-in-index test and the other-submodule-in-index test (their comments now refer to one another, so the contrast is more obvious), but didn't reorder them because I thought that made the test setup less intuitive to read.
- <20220304234622.647776-1-jonathantanmy@google.com> I added expect.err.sub2 to verify_test_result() but didn't change write_expected_super() to account for sub2. It turned out to be tricky to predict the output when 'super' fetches >1 branch because each fetched branch can affect the formatting. e.g.
OLD_HEAD..super super -> origin/super
can become
OLD_HEAD..super super -> origin/super
OLD_HEAD..super some-other-branch -> origin/some-other-branch(I could work around this by replacing the whitespace with sed, but it seemed like too much overhead for a one-off test).
= Patch organization
- Patches 1-3 are quality-of-life improvements to the test suite that make it easier to write the tests in patch 9. - Patches 4-6 are preparation for "git fetch" to read .gitmodules from the superproject commit in patch 7. - Patches 7-8 refactor out the logic of "finding which submodules to fetch" and "fetching the submodules", making it easier to tell "git fetch" to fetch unpopulated submodules. - Patch 9 teaches "git fetch" to fetch changed, unpopulated submodules in addition to populated submodules. - Patch 10 is an optional bugfix + cleanup of the "git fetch" code that removes the last caller of the deprecated "add_submodule_odb()".
= Changes
== Since v4 - Rename test helpers (s/check_/write_expected_) - Test style fixes - Update test comments - Remove the manual test_cmp in the test that checks sub2 (but we still construct expect.err.super manually).
== Since v3 - Numerous style fixes + improved comments. - Fix sed portability issues. - Fix failing test due to default branch name assumptions. - Patch 3: change a test so that it no longer depends on state from the previous test. - Patch 9: fix memory leak when recording super_oid and path + add explanatory comment.
== Since v2
- Numerous small fixes to the code and commit message (thanks to all who
helped spot these :))
- In patch 2, use test_cmp + sed to assert on test output, effectively
reverting the "use grep" approach of v1-2 (see patch 2's description).
- New patch 3: introduce a test helper that creates the expected
superproject commit (instead of copy-pasting the code over and over).
- I did not get rid of "git fetch" inside the test helper (as Jonathan
suggested) though, because that requires a bigger change in the test
setup, and I think the test helper makes the test straightforward
enough.
- New patch 8: refactor some shared logic out into fetch_task_create().
This reduces code duplication between the get_fetch_task_from_*
functions.
- In patch 9, add additional tests for 'submodules with the same name'.
- In patch 9, handle a bug where a submodule that is unpopulated by "git
rm" still has "core.worktree" set and cannot be fetched (see patch 9's
description).
- Remove the "git fetch --update-shallow" patch (I'll try to send it
separately).== Since v1
- Numerous style fixes suggested by Jonathan (thanks!)
- In patch 3, don't prematurely read submodules from the superproject
commit (see:
<kl6l5yplyat6.fsf@chooglen-macbookpro.roam.corp.google.com>).
- In patch 7, stop using "git checkout" and "! grep" in tests.
- In patch 7, stop doing the "find changed submodules" rev walk
unconditionally. Instead, continue to check for .gitmodules, but also
check for submodules in $GIT_DIR/modules.
- I'm not entirely happy with the helper function name, see "---" for
details.
- Move "git fetch --update-shallow" bugfix to patch 8.
- Because the "find changed submodules" rev walk is no longer
unconditional, this fix is no longer needed for tests to pass.
- Rename fetch_populated_submodules() to fetch_submodules().Glen Choo (10): t5526: introduce test helper to assert on fetches t5526: stop asserting on stderr literally t5526: create superproject commits with test helper submodule: make static functions read submodules from commits submodule: inline submodule_commits() into caller submodule: store new submodule commits oid_array in a struct submodule: extract get_fetch_task() submodule: move logic into fetch_task_create() fetch: fetch unpopulated, changed submodules submodule: fix latent check_has_commit() bug
Documentation/fetch-options.txt | 26 +- Documentation/git-fetch.txt | 10 +- builtin/fetch.c | 14 +- submodule.c | 442 +++++++++++++++++--------- submodule.h | 21 +- t/t5526-fetch-submodules.sh | 545 ++++++++++++++++++++++++-------- 6 files changed, 746 insertions(+), 312 deletions(-)
Range-diff against v4:
1: 57cd31afc2 ! 1: f22f992e2b t5526: introduce test helper to assert on fetches
@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
+#
+# If a repo should not be fetched in the test, its corresponding
+# expect.err file should be rm-ed.
-+verify_fetch_result() {
++verify_fetch_result () {
+ ACTUAL_ERR=$1 &&
+ rm -f expect.err.combined &&
+ if test -f expect.err.super
2: b70c894cff ! 2: f6ee125e16 t5526: stop asserting on stderr literally
@@ Commit message
Stop asserting on $head1 by replacing it with a dummy value in the
actual and expected output. Do this by introducing test
- helpers (check_*()) that make it easier to construct the expected
- output, and use sed to munge the actual output.
+ helpers (write_expected_*()) that make it easier to construct the
+ expected output, and use sed to munge the actual output.
Signed-off-by: Glen Choo <chooglen@google.com>
@@ t/t5526-fetch-submodules.sh: export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
pwd=$(pwd)
-+check_sub () {
++write_expected_sub () {
+ NEW_HEAD=$1 &&
-+ cat >$pwd/expect.err.sub <<-EOF
++ cat >"$pwd/expect.err.sub" <<-EOF
+ Fetching submodule submodule
+ From $pwd/submodule
+ OLD_HEAD..$NEW_HEAD sub -> origin/sub
+ EOF
+}
+
-+check_deep () {
++write_expected_deep () {
+ NEW_HEAD=$1 &&
-+ cat >$pwd/expect.err.deep <<-EOF
++ cat >"$pwd/expect.err.deep" <<-EOF
+ Fetching submodule submodule/subdir/deepsubmodule
+ From $pwd/deepsubmodule
+ OLD_HEAD..$NEW_HEAD deep -> origin/deep
+ EOF
+}
+
-+check_super () {
++write_expected_super () {
+ NEW_HEAD=$1 &&
-+ cat >$pwd/expect.err.super <<-EOF
++ cat >"$pwd/expect.err.super" <<-EOF
+ From $pwd/.
+ OLD_HEAD..$NEW_HEAD super -> origin/super
+ EOF
@@ t/t5526-fetch-submodules.sh: pwd=$(pwd)
- echo "From $pwd/submodule" >> ../expect.err.sub &&
- echo " $head1..$head2 sub -> origin/sub" >> ../expect.err.sub
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_sub $new_head
++ write_expected_sub $new_head
) &&
(
cd deepsubmodule &&
@@ t/t5526-fetch-submodules.sh: pwd=$(pwd)
- echo "From $pwd/deepsubmodule" >> ../expect.err.deep &&
- echo " $head1..$head2 deep -> origin/deep" >> ../expect.err.deep
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_deep $new_head
++ write_expected_deep $new_head
)
}
-@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
- #
- # If a repo should not be fetched in the test, its corresponding
- # expect.err file should be rm-ed.
--verify_fetch_result() {
-+verify_fetch_result () {
- ACTUAL_ERR=$1 &&
- rm -f expect.err.combined &&
- if test -f expect.err.super
-@@ t/t5526-fetch-submodules.sh: verify_fetch_result() {
+@@ t/t5526-fetch-submodules.sh: verify_fetch_result () {
then
cat expect.err.deep >>expect.err.combined
fi &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion doesn't happen when
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >>expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.deep &&
(
cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion stops when no new su
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >> expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.sub &&
rm expect.err.deep &&
(
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion picks up config in s
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >> expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
(
cd downstream &&
git fetch >../actual.out 2>../actual.err &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion picks up all submodu
- echo "From $pwd/submodule" >> ../expect.err.sub &&
- echo " $head1..$head2 sub -> origin/sub" >> ../expect.err.sub
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_sub $new_head
++ write_expected_sub $new_head
) &&
- head1=$(git rev-parse --short HEAD) &&
git add submodule &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion picks up all submodu
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >> expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
(
cd downstream &&
git fetch >../actual.out 2>../actual.err
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
- echo "From $pwd/submodule" >> ../expect.err.sub &&
- echo " $head1..$head2 sub -> origin/sub" >> ../expect.err.sub
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_sub $new_head
++ write_expected_sub $new_head
) &&
(
cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >>expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
(
cd downstream &&
git config fetch.recurseSubmodules false &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >> expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.sub &&
rm expect.err.deep &&
(
@@ t/t5526-fetch-submodules.sh: test_expect_success "'fetch.recurseSubmodules=on-de
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >>expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.deep &&
(
cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'submodule.<sub>.fetchRecurseS
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >>expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.deep &&
(
cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "don't fetch submodule when new
- echo "From $pwd/." > expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >> expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.sub &&
# This file does not exist, but rm -f for readability
rm -f expect.err.deep &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'fetch.recurseSubmodules=on-de
- echo "From $pwd/." >expect.err.super &&
- echo " $head1..$head2 super -> origin/super" >>expect.err.super &&
+ new_head=$(git rev-parse --short HEAD) &&
-+ check_super $new_head &&
++ write_expected_super $new_head &&
rm expect.err.deep &&
(
cd downstream &&
3: 7e2a01164e ! 3: 17ccae2933 t5526: create superproject commits with test helper
@@ Commit message
Signed-off-by: Glen Choo <chooglen@google.com>
## t/t5526-fetch-submodules.sh ##
-@@ t/t5526-fetch-submodules.sh: check_super () {
+@@ t/t5526-fetch-submodules.sh: write_expected_super () {
# a file that contains the expected err if that new commit were fetched.
# These output files get concatenated in the right order by
# verify_fetch_result().
@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
+ git commit -m "new submodule" &&
+ super_head=$(git rev-parse --short HEAD) &&
+ sub_head=$(git -C submodule rev-parse --short HEAD) &&
-+ check_super $super_head &&
-+ check_sub $sub_head
++ write_expected_super $super_head &&
++ write_expected_sub $sub_head
+}
+
# Verifies that the expected repositories were fetched. This is done by
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion picks up config in s
- git add subdir/deepsubmodule &&
- git commit -m "new deepsubmodule" &&
- new_head=$(git rev-parse --short HEAD) &&
-- check_sub $new_head
+- write_expected_sub $new_head
- ) &&
- git add submodule &&
- git commit -m "new submodule" &&
- new_head=$(git rev-parse --short HEAD) &&
-- check_super $new_head &&
+- write_expected_super $new_head &&
+ add_submodule_commits &&
+ add_superproject_commits &&
(
@@ t/t5526-fetch-submodules.sh: test_expect_success "Recursion picks up all submodu
- git add subdir/deepsubmodule &&
- git commit -m "new deepsubmodule" &&
- new_head=$(git rev-parse --short HEAD) &&
-- check_sub $new_head
+- write_expected_sub $new_head
- ) &&
+ add_submodule_commits &&
(
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
- git add submodule &&
- git commit -m "new submodule" &&
- new_head=$(git rev-parse --short HEAD) &&
-- check_super $new_head &&
+- write_expected_super $new_head &&
++ add_submodule_commits &&
+ add_superproject_commits &&
(
cd downstream &&
4: 88112ee225 = 4: b220dd32c1 submodule: make static functions read submodules from commits
5: 007cd97aba = 5: da346aa12a submodule: inline submodule_commits() into caller
6: f34ea88fe9 = 6: c1fd9c3abf submodule: store new submodule commits oid_array in a struct
7: f66ab663c5 = 7: dbb931fe30 submodule: extract get_fetch_task()
8: 4e3db1bc9d = 8: 7242236df9 submodule: move logic into fetch_task_create()
9: 9e7b1c1bbe ! 9: 0dada865d4 fetch: fetch unpopulated, changed submodules
@@ submodule.h: int should_update_submodules(void);
## t/t5526-fetch-submodules.sh ##
@@ t/t5526-fetch-submodules.sh: pwd=$(pwd)
- check_sub () {
+ write_expected_sub () {
NEW_HEAD=$1 &&
+ SUPER_HEAD=$2 &&
- cat >$pwd/expect.err.sub <<-EOF
+ cat >"$pwd/expect.err.sub" <<-EOF
- Fetching submodule submodule
+ Fetching submodule submodule${SUPER_HEAD:+ at commit $SUPER_HEAD}
From $pwd/submodule
OLD_HEAD..$NEW_HEAD sub -> origin/sub
EOF
-@@ t/t5526-fetch-submodules.sh: check_sub () {
+ }
- check_deep () {
++write_expected_sub2 () {
++ NEW_HEAD=$1 &&
++ SUPER_HEAD=$2 &&
++ cat >"$pwd/expect.err.sub2" <<-EOF
++ Fetching submodule submodule2${SUPER_HEAD:+ at commit $SUPER_HEAD}
++ From $pwd/submodule2
++ OLD_HEAD..$NEW_HEAD sub2 -> origin/sub2
++ EOF
++}
++
+ write_expected_deep () {
NEW_HEAD=$1 &&
+ SUB_HEAD=$2 &&
- cat >$pwd/expect.err.deep <<-EOF
+ cat >"$pwd/expect.err.deep" <<-EOF
- Fetching submodule submodule/subdir/deepsubmodule
+ Fetching submodule submodule/subdir/deepsubmodule${SUB_HEAD:+ at commit $SUB_HEAD}
From $pwd/deepsubmodule
OLD_HEAD..$NEW_HEAD deep -> origin/deep
EOF
-@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no ne
- '
-
- test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necessary (and ignores config)" '
-+ add_submodule_commits &&
- add_superproject_commits &&
- (
- cd downstream &&
+@@ t/t5526-fetch-submodules.sh: verify_fetch_result () {
+ then
+ cat expect.err.deep >>expect.err.combined
+ fi &&
++ if test -f expect.err.sub2
++ then
++ cat expect.err.sub2 >>expect.err.combined
++ fi &&
+ sed -e 's/[0-9a-f][0-9a-f]*\.\./OLD_HEAD\.\./' "$ACTUAL_ERR" >actual.err.cmp &&
+ test_cmp expect.err.combined actual.err.cmp
+ }
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
verify_fetch_result actual.err
'
-+# Test that we can fetch submodules in other branches by running fetch
-+# in a commit that has no submodules.
++# These tests verify that we can fetch submodules that aren't in the
++# index.
++#
++# First, test the simple case where the index is empty and we only fetch
++# submodules that are not in the index.
+test_expect_success 'setup downstream branch without submodules' '
+ (
+ cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ deep_head=$(git -C submodule/subdir/deepsubmodule rev-parse --short HEAD) &&
+
+ # assert that these are fetched from commits, not the index
-+ check_sub $sub_head $super_head &&
-+ check_deep $deep_head $sub_head &&
++ write_expected_sub $sub_head $super_head &&
++ write_expected_deep $deep_head $sub_head &&
+
+ test_must_be_empty actual.out &&
+ verify_fetch_result actual.err
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ deep_head=$(git -C submodule/subdir/deepsubmodule rev-parse --short HEAD) &&
+
+ # assert that these are fetched from commits, not the index
-+ check_sub $sub_head $super_head &&
-+ check_deep $deep_head $sub_head &&
++ write_expected_sub $sub_head $super_head &&
++ write_expected_deep $deep_head $sub_head &&
+
+ test_must_be_empty actual.out &&
+ verify_fetch_result actual.err
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ ) &&
+ test_must_be_empty actual.out &&
+ super_head=$(git rev-parse --short HEAD) &&
-+ check_super $super_head &&
++ write_expected_super $super_head &&
+ # Neither should be fetched because the submodule is inactive
+ rm expect.err.sub &&
+ rm expect.err.deep &&
+ verify_fetch_result actual.err
+'
+
-+# In downstream, init "submodule2", but do not check it out while
-+# fetching. This lets us assert that unpopulated submodules can be
-+# fetched.
++# Now that we know we can fetch submodules that are not in the index,
++# test that we can fetch index and non-index submodules in the same
++# operation.
+test_expect_success 'setup downstream branch with other submodule' '
+ mkdir submodule2 &&
+ (
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+'
+
+test_expect_success "'--recurse-submodules' should fetch submodule commits in changed submodules and the index" '
++ test_when_finished "rm expect.err.sub2" &&
+ # Create new commit in origin/super
+ add_submodule_commits &&
+ add_superproject_commits &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ git fetch --recurse-submodules >../actual.out 2>../actual.err
+ ) &&
+ test_must_be_empty actual.out &&
-+ deep_head=$(git -C submodule/subdir/deepsubmodule rev-parse --short HEAD) &&
-+ sub_head=$(git -C submodule rev-parse --short HEAD) &&
+ sub2_head=$(git -C submodule2 rev-parse --short HEAD) &&
-+ super_head=$(git rev-parse --short HEAD) &&
++ super_head=$(git rev-parse --short super) &&
+ super_sub2_only_head=$(git rev-parse --short super-sub2-only) &&
++ write_expected_sub2 $sub2_head $super_sub2_only_head &&
+
-+ # Use test_cmp manually because verify_fetch_result does not
-+ # consider submodule2. All the repos should be fetched, but only
-+ # submodule2 should be read from a commit
-+ cat > expect.err.combined <<-EOF &&
++ # write_expected_super cannot handle >1 branch. Since this is a
++ # one-off, construct expect.err.super manually.
++ cat >"$pwd/expect.err.super" <<-EOF &&
+ From $pwd/.
+ OLD_HEAD..$super_head super -> origin/super
+ OLD_HEAD..$super_sub2_only_head super-sub2-only -> origin/super-sub2-only
-+ Fetching submodule submodule
-+ From $pwd/submodule
-+ OLD_HEAD..$sub_head sub -> origin/sub
-+ Fetching submodule submodule/subdir/deepsubmodule
-+ From $pwd/deepsubmodule
-+ OLD_HEAD..$deep_head deep -> origin/deep
-+ Fetching submodule submodule2 at commit $super_sub2_only_head
-+ From $pwd/submodule2
-+ OLD_HEAD..$sub2_head sub2 -> origin/sub2
+ EOF
-+ sed -e "s/[0-9a-f][0-9a-f]*\.\./OLD_HEAD\.\./" actual.err >actual.err.cmp &&
-+ test_cmp expect.err.combined actual.err.cmp
++ verify_fetch_result actual.err
+'
+
test_expect_success "'--recurse-submodules=on-demand' stops when no new submodule commits are found in the superproject (and ignores config)" '
10: 362ce3c7f8 = 10: 71bb456041 submodule: fix latent check_has_commit() bugbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
-- 2.33.GIT