Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
> Glen Choo <chooglen@xxxxxxxxxx> writes:
>> +# For each superproject in the test setup, update its submodule, add the
>> +# submodule and create a new commit with the submodule change.
>> +#
>> +# This requires add_submodule_commits() to be called first, otherwise
>> +# the submodules will not have changed and cannot be "git add"-ed.
>> +add_superproject_commits() {
>> +(
>> + cd submodule &&
>> + (
>> + cd subdir/deepsubmodule &&
>> + git fetch &&
>> + git checkout -q FETCH_HEAD
>> + ) &&
>> + git add subdir/deepsubmodule &&
>> + git commit -m "new deep submodule"
>> + ) &&
>
> The indentation looks off. Also, no need for "-q".
Ah thanks. I think the "-q" is there to suppress the detached HEAD
warning, which is very large.
I'd prefer to keep it unless there are stronger reasons than "it's not
needed for correctness".
>> @@ -378,19 +387,7 @@ test_expect_success "Recursion picks up all submodules when necessary" '
>> '
>>
>> test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no new commits are fetched in the superproject (and ignores config)" '
>> - add_upstream_commit &&
>> - (
>> - cd submodule &&
>> - (
>> - cd subdir/deepsubmodule &&
>> - git fetch &&
>> - git checkout -q FETCH_HEAD
>> - ) &&
>> - git add subdir/deepsubmodule &&
>> - git commit -m "new deepsubmodule" &&
>> - new_head=$(git rev-parse --short HEAD) &&
>> - check_sub $new_head
>> - ) &&
>> + add_submodule_commits &&
>> (
>> cd downstream &&
>> git config fetch.recurseSubmodules true &&
>
> Hmm...I'm surprised that this still passes even when code is deleted but
> the replacement is not added. What's happening here, I guess, is that
> we're checking that nothing has happened. The test probably should be
> rewritten but that's outside the scope of this patch set. So for now,
> just add the add_superproject_commits call.
Yeah this test could use some fixing up; I spent a lot of time trying to
understand this one. It could use comments at least.
The suggestion to add the add_superproject_commits call defeats the
purpose of the test though - which is to assert that "on-demand"
recursion only fetches submodule commits if a superproject commit says
the submodule has changed, unlike "yes", which unconditionally fetches
submodule commits.
So we need to consider these cases:
1. no new upstream commits
2. new upstream submodule commits, but not superproject (call
add_submodule_commits() only)
3. new upstream submodule and superproject commits (call
add_submodule_commits() and add_superproject_commits())
(1): "on-demand" and "yes" both fetch nothing
(2): "yes" fetches submodule commits but "on-demand" doesn't
(3): "on-demand" and "yes" both fetch submodule and superproject commits
So this test can't call add_superproject_commits(), because we would no
longer be testing scenario (2) - we'd be 'testing' (3) instead (which
doesn't tell us how "on-demand" is different from "yes").
>> @@ -402,10 +399,7 @@ 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)" '
>> - git add submodule &&
>> - git commit -m "new submodule" &&
>> - new_head=$(git rev-parse --short HEAD) &&
>> - check_super $new_head &&
>> + add_superproject_commits &&
>> (
>> cd downstream &&
>> git config fetch.recurseSubmodules false &&
>
> add_superproject_commits without add_submodule_commits?
This is a silly holdover from before my rewrite.. These lines:
- git add submodule &&
- git commit -m "new submodule" &&
- new_head=$(git rev-parse --short HEAD) &&
don't make any sense either until you realize that these commits were
set up in the _previous_ test. I should clean this up though, there's no
reason for others to have to struggle with this the way I did.
The easiest approach would be to add the add_submodule_commits() call,
with a comment explaining that it's technically unnecessary work
(because the previous test already calls add_submodule_commits()) but it
makes the test easier to read.
