| Message ID | 20201005072015.GD2291074@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | forbidding symlinked .gitattributes and .gitignore | expand |
Hi, Jeff King 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> > --- > t/t7450-bad-meta-files.sh | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh > index 6b703b12bc..b73985157f 100755 > --- a/t/t7450-bad-meta-files.sh > +++ b/t/t7450-bad-meta-files.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 'create 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) > +' This is super nitpicky, but: test scripts can be hard to maintain when there's this kind of state carried from assertion to assertion without it being made obvious. Can this include "setup" or "set up" in the name to do that? E.g. test_expect_success 'set up repo with symlinked .gitmodules file' ' ... ' Thanks, Jonathan
On Mon, Oct 05, 2020 at 12:53:11AM -0700, Jonathan Nieder wrote: > > @@ -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) > > +' > > This is super nitpicky, but: test scripts can be hard to maintain when > there's this kind of state carried from assertion to assertion without > it being made obvious. > > Can this include "setup" or "set up" in the name to do that? E.g. > > test_expect_success 'set up repo with symlinked .gitmodules file' ' > ... > ' Hmph. I specifically _tried_ to do that by breaking it into a separate test with the name "create" in it, which I thought was one of the code-words for "I'm doing stuff that will be used in another test". But I guess there's no official rule on that. I dug up: https://lore.kernel.org/git/20130826173501.GS4110@google.com/ but I guess I mis-remembered "create" being present there. -Peff
Jeff King wrote: > On Mon, Oct 05, 2020 at 12:53:11AM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> @@ -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) >>> +' >> >> This is super nitpicky, but: test scripts can be hard to maintain when >> there's this kind of state carried from assertion to assertion without >> it being made obvious. >> >> Can this include "setup" or "set up" in the name to do that? E.g. >> >> test_expect_success 'set up repo with symlinked .gitmodules file' ' >> ... >> ' > > Hmph. I specifically _tried_ to do that by breaking it into a separate > test with the name "create" in it, which I thought was one of the > code-words for "I'm doing stuff that will be used in another test". But > I guess there's no official rule on that. I dug up: > > https://lore.kernel.org/git/20130826173501.GS4110@google.com/ > > but I guess I mis-remembered "create" being present there. I can try to find some time today to introduce a test_setup helper. Having to figure out and rely on this kind of ad hoc convention is not something I really want to ask of patch authors and reviewers. The reason I find "set up" clearer than "create" is that the latter is something I can easily imagine myself genuinely wanting to test. "Set up for a later test" is more explicit about what the commands are being run for. Jonathan
diff --git a/t/t7450-bad-meta-files.sh b/t/t7450-bad-meta-files.sh index 6b703b12bc..b73985157f 100755 --- a/t/t7450-bad-meta-files.sh +++ b/t/t7450-bad-meta-files.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 'create 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; this grep string comes from the config @@ -166,6 +172,11 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' ) ' +test_expect_success 'refuse to load symlinked .gitmodule into index' ' + test_must_fail git -C symlink read-tree $tree 2>err && + test_i18ngrep "invalid path.*gitmodules" err +' + 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-meta-files.sh | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)