Re: [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs
- From
Glen Choo <chooglen@google.com>
- Date
- Mar 6, 2023, 23:34 UTC
- Message-ID
- <kl6lsfehihdv.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20230228185642.2357806-3-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 5 quoted lines
> This commit continues the previous work of updating the test suite to > use `git submodule add` to create submodules instead of using `git add` > to include embedded repositories. Specifically, in this commit we update > test cases where expected diffs must change due to the presence of a > .gitmodules file.
Adjusting the diff makes sense when the .gitmodules file is relevant to the diff being tested.
Show 18 quoted lines
> diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh > index 61da7e3b94..a0f14db3d2 100755 > --- a/t/t3040-subprojects-basic.sh > +++ b/t/t3040-subprojects-basic.sh > @@ -19,11 +19,12 @@ test_expect_success 'setup: create subprojects' ' > ( cd sub2 && git init && : >Makefile && git add * && > git commit -q -m "subproject 2" ) && > git update-index --add sub1 && > - git add sub2 && > + git submodule add ./sub2 && > git commit -q -m "subprojects added" && > GIT_PRINT_SHA1_ELLIPSIS="yes" git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && > git branch save HEAD && > cat >expected <<-\EOF && > + :000000 100644 00000... A .gitmodules > :000000 160000 00000... A sub1 > :000000 160000 00000... A sub2 > EOF
e.g. this change makes sense
Show 25 quoted lines
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > index 2aa12243bd..f5074071a4 100755 > --- a/t/t4041-diff-submodule-option.sh > +++ b/t/t4041-diff-submodule-option.sh > @@ -50,9 +50,19 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'added submodule' ' > - git add sm1 && > + git submodule add ./sm1 && > + gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && > git diff-index -p --submodule=log HEAD >actual && > cat >expected <<-EOF && > + diff --git a/.gitmodules b/.gitmodules > + new file mode 100644 > + index 0000000..$gitmodules_hash1 > + --- /dev/null > + +++ b/.gitmodules > + @@ -0,0 +1,3 @@ > + +[submodule "sm1"] > + + path = sm1 > + + url = ./sm1 > Submodule sm1 0000000...$head1 (new submodule) > EOF > test_cmp expected actual
But in this file and the next (t4041 and t4060), we are checking submodule diffing behavior, so wouldn't it make sense to ignore non-submodule changes in the diff?
E.g. we could have ignored .gitmodules during the diff like so
test_expect_success 'added submodule' '
git submodule add ./sm1 &&
gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) &&
- git diff-index -p --submodule=log HEAD >actual &&
+ git diff-index -p --submodule=log HEAD -- :!.gitmodules >actual &&and then we wouldn't have to adjust the diff. That would be my preferred approach, since it keeps the irrelevant details out of the test.
To play devil's advocate, there's a small integration test-style benefit to testing both a regular file diff and a submodule diff together. I haven't checked if these are the only files that are testing this, but even if not, checking .gitmodules repeatedly seems like a suboptimal way to do this.
Show 8 quoted lines
> @@ -243,7 +244,7 @@ test_expect_success 'status -a clean (empty submodule dir)' ' > ' > > cat >status_expect <<\EOF > -AA .gitmodules > +UU .gitmodules > A sub1 > EOF
_Maybe_ this is worth modernizing too as a 'while we're at it' kind of change, though it's far less important than the earlier patch (where the setup actually touches the Git repo and submodules). Idk.