Re: [PATCH v3 03/10] t5526: create superproject commits with test helper
- From
Glen Choo <chooglen@google.com>
- Date
- Feb 25, 2022, 02:52 UTC
- Message-ID
- <kl6lfso74pzm.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20220224231420.2878760-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 19 quoted lines
> Glen Choo <chooglen@google.com> 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".
Show 27 quoted lines
>> @@ -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").
Show 14 quoted lines
>> @@ -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.