| Message ID | YI124aZ1dbY5O67J@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leftover bits from symlinked gitattributes, etc topics | expand |
On Sat, May 1, 2021 at 11:42 AM Jeff King <peff@peff.net> wrote: > Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules, > 2018-05-04) made it impossible to load a symlink .gitmodules file into > the index. However, there are no tests of this behavior. Let's make sure > this case is covered. We can easily reuse the test setup created by > the matching b7b1fca175 (fsck: complain when .gitmodules is a symlink, > 2018-05-04). > > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh > @@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' ' > -test_expect_success 'fsck detects symlinked .gitmodules file' ' > +test_expect_success 'set up repo with symlinked .gitmodules file' ' > git init symlink && > ( > cd symlink && > @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' > { > printf "100644 blob $content\t$tricky\n" && > printf "120000 blob $target\t.gitmodules\n" > - } >bad-tree && > - tree=$(git mktree <bad-tree) && > + } >bad-tree > + ) && > + tree=$(git -C symlink mktree <symlink/bad-tree) > +' `tree` is an unusually generic name for this now-global variable. One can easily imagine it being re-used by some unrelated test arbitrarily inserted into this script, thus potentially breaking the following tests which depend upon it. I wonder if a name such as `BAD_TREE` would be more appropriate.
On Sat, May 1, 2021 at 2:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Sat, May 1, 2021 at 11:42 AM Jeff King <peff@peff.net> wrote: > > + tree=$(git -C symlink mktree <symlink/bad-tree) > > `tree` is an unusually generic name for this now-global variable. One > can easily imagine it being re-used by some unrelated test arbitrarily > inserted into this script, thus potentially breaking the following > tests which depend upon it. I wonder if a name such as `BAD_TREE` > would be more appropriate. I see that all `$tree` references get encapsulated into a shell function by the next patch, so perhaps the generic name `tree` isn't a big deal after all.
On Sat, May 01 2021, Jeff King wrote: > +test_expect_success 'refuse to load symlinked .gitmodules into index' ' > + test_must_fail git -C symlink read-tree $tree 2>err && > + test_i18ngrep "invalid path.*gitmodules" err && > + git -C symlink ls-files >out && > + test_must_be_empty out > +' > + In 1/9 a test_i18ngrep is removed, but here's a new one.
On Sat, May 01, 2021 at 03:03:56PM -0400, Eric Sunshine wrote: > On Sat, May 1, 2021 at 2:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, May 1, 2021 at 11:42 AM Jeff King <peff@peff.net> wrote: > > > + tree=$(git -C symlink mktree <symlink/bad-tree) > > > > `tree` is an unusually generic name for this now-global variable. One > > can easily imagine it being re-used by some unrelated test arbitrarily > > inserted into this script, thus potentially breaking the following > > tests which depend upon it. I wonder if a name such as `BAD_TREE` > > would be more appropriate. > > I see that all `$tree` references get encapsulated into a shell > function by the next patch, so perhaps the generic name `tree` isn't a > big deal after all. Yeah. I'd like to think it is not that big a deal between even just adjacent tests, because anybody adding tests in the middle of a script would take care not to split related ones. But that may be too optimistic. ;) At any rate, it seems OK to assume the function will all be used together. -Peff
On Mon, May 03, 2021 at 12:12:32PM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Sat, May 01 2021, Jeff King wrote: > > > +test_expect_success 'refuse to load symlinked .gitmodules into index' ' > > + test_must_fail git -C symlink read-tree $tree 2>err && > > + test_i18ngrep "invalid path.*gitmodules" err && > > + git -C symlink ls-files >out && > > + test_must_be_empty out > > +' > > + > > In 1/9 a test_i18ngrep is removed, but here's a new one. Thanks. Most of these patches were written either in 2018 or 2020 and pulled forward (and back then, this call would have been necessary). I fixed up a few cases as I was rebasing, but obviously missed this one. -Peff
diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index 34d4dc6def..c021d4e752 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -139,7 +139,7 @@ test_expect_success 'index-pack --strict works for non-repo pack' ' grep gitmodulesName output ' -test_expect_success 'fsck detects symlinked .gitmodules file' ' +test_expect_success 'set up repo with symlinked .gitmodules file' ' git init symlink && ( cd symlink && @@ -155,8 +155,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' { printf "100644 blob $content\t$tricky\n" && printf "120000 blob $target\t.gitmodules\n" - } >bad-tree && - tree=$(git mktree <bad-tree) && + } >bad-tree + ) && + tree=$(git -C symlink mktree <symlink/bad-tree) +' + +test_expect_success 'fsck detects symlinked .gitmodules file' ' + ( + cd symlink && # Check not only that we fail, but that it is due to the # symlink detector @@ -165,6 +171,13 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' ) ' +test_expect_success 'refuse to load symlinked .gitmodules into index' ' + test_must_fail git -C symlink read-tree $tree 2>err && + test_i18ngrep "invalid path.*gitmodules" err && + git -C symlink ls-files >out && + test_must_be_empty out +' + test_expect_success 'fsck detects non-blob .gitmodules' ' git init non-blob && (
Commit 10ecfa7649 (verify_path: disallow symlinks in .gitmodules, 2018-05-04) made it impossible to load a symlink .gitmodules file into the index. However, there are no tests of this behavior. Let's make sure this case is covered. We can easily reuse the test setup created by the matching b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04). Signed-off-by: Jeff King <peff@peff.net> --- t/t7450-bad-git-dotfiles.sh | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)