| Message ID | patch-v4-1.9-f479003941b-20221215T083502Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Get rid of "git --super-prefix" | expand |
No comment on the structure of the tests themselves; those look good to me. Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > index 2859695c6d2..d556342ea57 100755 > --- a/t/t7412-submodule-absorbgitdirs.sh > +++ b/t/t7412-submodule-absorbgitdirs.sh > @@ -10,6 +10,7 @@ TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'setup a real submodule' ' > + cwd="$(pwd)" && > git init sub1 && > test_commit -C sub1 first && > git submodule add ./sub1 && [...] > @@ -18,13 +19,21 @@ test_expect_success 'setup a real submodule' ' > ' > > test_expect_success 'absorb the git dir' ' > + >expect && > + >actual && > >expect.1 && > >expect.2 && > >actual.1 && > >actual.2 && > git status >expect.1 && > git -C sub1 rev-parse HEAD >expect.2 && > - git submodule absorbgitdirs && > + cat >expect <<-EOF && > + Migrating git directory of '\''sub1'\'' from > + '\''$cwd/sub1/.git'\'' to > + '\''$cwd/.git/modules/sub1'\'' > + EOF > + git submodule absorbgitdirs 2>actual && > + test_cmp expect actual && > git fsck && > test -f sub1/.git && > test -d .git/modules/sub1 && I thought that we typically avoid setting environment variables in the test cases themselves, so when we set environment variables to be read in later tests, we typically set them outside of the test case (e.g. t/t5526-fetch-submodules.sh). > @@ -97,6 +119,27 @@ test_expect_success 'absorb the git dir in a nested submodule' ' > test_cmp expect.2 actual.2 > ' > > +test_expect_success 'absorb the git dir outside of primary worktree' ' > + test_when_finished "rm -rf repo-bare.git" && > + git clone --bare . repo-bare.git && > + test_when_finished "rm -rf repo-wt" && > + git -C repo-bare.git worktree add ../repo-wt && > + > + test_when_finished "rm -f .gitconfig" && > + test_config_global protocol.file.allow always && > + git -C repo-wt submodule update --init && > + git init repo-wt/sub2 && > + test_commit -C repo-wt/sub2 A && > + git -C repo-wt submodule add ./sub2 sub2 && > + cat >expect <<-EOF && > + Migrating git directory of '\''sub2'\'' from > + '\''$cwd/repo-wt/sub2/.git'\'' to > + '\''$cwd/repo-bare.git/worktrees/repo-wt/modules/sub2'\'' > + EOF > + DO_IT=1 git -C repo-wt submodule absorbgitdirs 2>actual && DO_IT is a leftover from dev? (I'm also curious as to what it does :)).
On Thu, Dec 15 2022, Glen Choo wrote: > No comment on the structure of the tests themselves; those look good to > me. > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh >> index 2859695c6d2..d556342ea57 100755 >> --- a/t/t7412-submodule-absorbgitdirs.sh >> +++ b/t/t7412-submodule-absorbgitdirs.sh >> @@ -10,6 +10,7 @@ TEST_PASSES_SANITIZE_LEAK=true >> . ./test-lib.sh >> >> test_expect_success 'setup a real submodule' ' >> + cwd="$(pwd)" && >> git init sub1 && >> test_commit -C sub1 first && >> git submodule add ./sub1 && > > [...] > >> @@ -18,13 +19,21 @@ test_expect_success 'setup a real submodule' ' >> ' >> >> test_expect_success 'absorb the git dir' ' >> + >expect && >> + >actual && >> >expect.1 && >> >expect.2 && >> >actual.1 && >> >actual.2 && >> git status >expect.1 && >> git -C sub1 rev-parse HEAD >expect.2 && >> - git submodule absorbgitdirs && >> + cat >expect <<-EOF && >> + Migrating git directory of '\''sub1'\'' from >> + '\''$cwd/sub1/.git'\'' to >> + '\''$cwd/.git/modules/sub1'\'' >> + EOF >> + git submodule absorbgitdirs 2>actual && >> + test_cmp expect actual && >> git fsck && >> test -f sub1/.git && >> test -d .git/modules/sub1 && > > I thought that we typically avoid setting environment variables in the > test cases themselves, so when we set environment variables to be read > in later tests, we typically set them outside of the test case (e.g. > t/t5526-fetch-submodules.sh). We could do it either way, but no, I think the preferred style is to do such setup/assignment in a test_expect_success, we don't run our tests as "set -e", so we'd miss any errors (however unlikely in this case) from the commands outside test bodies. See e.g. t0002-gitfile.sh for the same pattern, i.e. setting the "$REAL" variable in the setup "test_expect_success", then using it later. >> @@ -97,6 +119,27 @@ test_expect_success 'absorb the git dir in a nested submodule' ' >> test_cmp expect.2 actual.2 >> ' >> >> +test_expect_success 'absorb the git dir outside of primary worktree' ' >> + test_when_finished "rm -rf repo-bare.git" && >> + git clone --bare . repo-bare.git && >> + test_when_finished "rm -rf repo-wt" && >> + git -C repo-bare.git worktree add ../repo-wt && >> + >> + test_when_finished "rm -f .gitconfig" && >> + test_config_global protocol.file.allow always && >> + git -C repo-wt submodule update --init && >> + git init repo-wt/sub2 && >> + test_commit -C repo-wt/sub2 A && >> + git -C repo-wt submodule add ./sub2 sub2 && >> + cat >expect <<-EOF && >> + Migrating git directory of '\''sub2'\'' from >> + '\''$cwd/repo-wt/sub2/.git'\'' to >> + '\''$cwd/repo-bare.git/worktrees/repo-wt/modules/sub2'\'' >> + EOF >> + DO_IT=1 git -C repo-wt submodule absorbgitdirs 2>actual && > > DO_IT is a leftover from dev? > > (I'm also curious as to what it does :)). Oops, will fix! It was just something for ad-hoc getenv()-debugging that escaped the lab.
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 2859695c6d2..d556342ea57 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -10,6 +10,7 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a real submodule' ' + cwd="$(pwd)" && git init sub1 && test_commit -C sub1 first && git submodule add ./sub1 && @@ -18,13 +19,21 @@ test_expect_success 'setup a real submodule' ' ' test_expect_success 'absorb the git dir' ' + >expect && + >actual && >expect.1 && >expect.2 && >actual.1 && >actual.2 && git status >expect.1 && git -C sub1 rev-parse HEAD >expect.2 && - git submodule absorbgitdirs && + cat >expect <<-EOF && + Migrating git directory of '\''sub1'\'' from + '\''$cwd/sub1/.git'\'' to + '\''$cwd/.git/modules/sub1'\'' + EOF + git submodule absorbgitdirs 2>actual && + test_cmp expect actual && git fsck && test -f sub1/.git && test -d .git/modules/sub1 && @@ -37,7 +46,8 @@ test_expect_success 'absorb the git dir' ' test_expect_success 'absorbing does not fail for deinitialized submodules' ' test_when_finished "git submodule update --init" && git submodule deinit --all && - git submodule absorbgitdirs && + git submodule absorbgitdirs 2>err && + test_must_be_empty err && test -d .git/modules/sub1 && test -d sub1 && ! test -e sub1/.git @@ -56,7 +66,13 @@ test_expect_success 'setup nested submodule' ' test_expect_success 'absorb the git dir in a nested submodule' ' git status >expect.1 && git -C sub1/nested rev-parse HEAD >expect.2 && - git submodule absorbgitdirs && + cat >expect <<-EOF && + Migrating git directory of '\''sub1/nested'\'' from + '\''$cwd/sub1/nested/.git'\'' to + '\''$cwd/.git/modules/sub1/modules/nested'\'' + EOF + git submodule absorbgitdirs 2>actual && + test_cmp expect actual && test -f sub1/nested/.git && test -d .git/modules/sub1/modules/nested && git status >actual.1 && @@ -87,7 +103,13 @@ test_expect_success 're-setup nested submodule' ' test_expect_success 'absorb the git dir in a nested submodule' ' git status >expect.1 && git -C sub1/nested rev-parse HEAD >expect.2 && - git submodule absorbgitdirs && + cat >expect <<-EOF && + Migrating git directory of '\''sub1'\'' from + '\''$cwd/sub1/.git'\'' to + '\''$cwd/.git/modules/sub1'\'' + EOF + git submodule absorbgitdirs 2>actual && + test_cmp expect actual && test -f sub1/.git && test -f sub1/nested/.git && test -d .git/modules/sub1/modules/nested && @@ -97,6 +119,27 @@ test_expect_success 'absorb the git dir in a nested submodule' ' test_cmp expect.2 actual.2 ' +test_expect_success 'absorb the git dir outside of primary worktree' ' + test_when_finished "rm -rf repo-bare.git" && + git clone --bare . repo-bare.git && + test_when_finished "rm -rf repo-wt" && + git -C repo-bare.git worktree add ../repo-wt && + + test_when_finished "rm -f .gitconfig" && + test_config_global protocol.file.allow always && + git -C repo-wt submodule update --init && + git init repo-wt/sub2 && + test_commit -C repo-wt/sub2 A && + git -C repo-wt submodule add ./sub2 sub2 && + cat >expect <<-EOF && + Migrating git directory of '\''sub2'\'' from + '\''$cwd/repo-wt/sub2/.git'\'' to + '\''$cwd/repo-bare.git/worktrees/repo-wt/modules/sub2'\'' + EOF + DO_IT=1 git -C repo-wt submodule absorbgitdirs 2>actual && + test_cmp expect actual +' + test_expect_success 'setup a gitlink with missing .gitmodules entry' ' git init sub2 && test_commit -C sub2 first && @@ -107,7 +150,11 @@ test_expect_success 'setup a gitlink with missing .gitmodules entry' ' test_expect_success 'absorbing the git dir fails for incomplete submodules' ' git status >expect.1 && git -C sub2 rev-parse HEAD >expect.2 && - test_must_fail git submodule absorbgitdirs && + cat >expect <<-\EOF && + fatal: could not lookup name for submodule '\''sub2'\'' + EOF + test_must_fail git submodule absorbgitdirs 2>actual && + test_cmp expect actual && git -C sub2 fsck && test -d sub2/.git && git status >actual && @@ -127,8 +174,11 @@ test_expect_success 'setup a submodule with multiple worktrees' ' ' test_expect_success 'absorbing fails for a submodule with multiple worktrees' ' - test_must_fail git submodule absorbgitdirs sub3 2>error && - test_i18ngrep "not supported" error + cat >expect <<-\EOF && + fatal: could not lookup name for submodule '\''sub2'\'' + EOF + test_must_fail git submodule absorbgitdirs 2>actual && + test_cmp expect actual ' test_done
Fix a blind spots in the tests surrounding "submodule absorbgitdirs" and test what output we emit, and how emitted the message and behavior interacts with a "git worktree" where the repository isn't at the base of the working directory. The "$(pwd)" instead of "$PWD" here is needed due to Windows, where the latter will be a path like "/d/a/git/[...]", whereas we need "D:/a/git/[...]". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t7412-submodule-absorbgitdirs.sh | 64 ++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 7 deletions(-)