| Message ID | 20201005071954.GC2291074@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | forbidding symlinked .gitattributes and .gitignore | expand |
Jeff King wrote: > This script has already expanded beyond its original intent of ".. in > submodule names" to include other malicious submodule bits. Let's update > the name and description to reflect that, as well as the fact that we'll > soon be adding similar tests for other meta-files (.gitattributes, etc). > We'll also renumber it to move it out of the group of submodule-specific > tests. > > Signed-off-by: Jeff King <peff@peff.net> > --- > ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%) I've never heard of a "meta file" before, but I don't tend to discover test scripts based on their filename anyway. :) Thanks for updating the test_description. t7* is "the porcelainish commands concerning the working tree". Should this go in t1* (basic commands concerning database) instead? t745* is unused number space so this at least won't hit any conflicts, so fwiw Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
On Mon, Oct 05, 2020 at 12:50:20AM -0700, Jonathan Nieder wrote: > > ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%) > > I've never heard of a "meta file" before, but I don't tend to discover > test scripts based on their filename anyway. :) Thanks for updating the > test_description. I couldn't think of a better name for "files that start with .git". I almost called it "dot-git", but then I worried about confusion with the actual ".git" directory. > t7* is "the porcelainish commands concerning the working tree". Should > this go in t1* (basic commands concerning database) instead? > > t745* is unused number space so this at least won't hit any conflicts, > so fwiw We've generally tried to order tests so that basic functionality in some area comes before more advanced. So I tried to put these specialty .gitmodules tests after the basic submodule tests (and likewise after any attribute or gitignore tests). In practice, I doubt it matters that much. We don't tend to run the test suite serially in order these days anyway, so the notion that finding a bug in an early test might save you CPU time or time spent reading error messages likely no longer applies. -Peff
Jeff King wrote: > On Mon, Oct 05, 2020 at 12:50:20AM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%) >> >> I've never heard of a "meta file" before, but I don't tend to discover >> test scripts based on their filename anyway. :) Thanks for updating the >> test_description. > > I couldn't think of a better name for "files that start with .git". I > almost called it "dot-git", but then I worried about confusion with the > actual ".git" directory. t7450-dot-gitfoo-files.sh seems clear to me. [...] > In practice, I doubt it matters that much. We don't tend to run the test > suite serially in order these days anyway, so the notion that finding a > bug in an early test might save you CPU time or time spent reading error > messages likely no longer applies. I see --- the point here is that because it's using e.g. "git clone --recurse-submodules", we want it to be later than the other clone tests? I think I'd like us to move away from having the numbers at all some day (since collisions are very common), but there's probably not much to discuss there until one of us comes up with a proposal that still makes it easy to do things like "skip all git-svn tests". :) Jonathan
On Mon, Oct 05, 2020 at 01:34:41AM -0700, Jonathan Nieder wrote: > > I couldn't think of a better name for "files that start with .git". I > > almost called it "dot-git", but then I worried about confusion with the > > actual ".git" directory. > > t7450-dot-gitfoo-files.sh seems clear to me. Heh, that was actually one of the ones I thought of, but I worried that "foo" was too confusing (likewise, I almost called the test-tool function check_dotgitfoo(const char *foo)). I guess dotgitx would follow the same pattern here. > > In practice, I doubt it matters that much. We don't tend to run the test > > suite serially in order these days anyway, so the notion that finding a > > bug in an early test might save you CPU time or time spent reading error > > messages likely no longer applies. > > I see --- the point here is that because it's using e.g. "git clone > --recurse-submodules", we want it to be later than the other clone > tests? > > I think I'd like us to move away from having the numbers at all some > day (since collisions are very common), but there's probably not much > to discuss there until one of us comes up with a proposal that still > makes it easy to do things like "skip all git-svn tests". :) I'd be happy to get away from numbers, too. They're a frequent pain when dealing with duplicates, or when we run out of space in a group (I have another series to split t9001 into a few separate scripts, but I have to move either it or the unrelated bits at t9002). An obvious solution is providing some kind of name hierarchy. E.g.: t-svn-commit-diff.sh t-svn-dcommit-auto-props.sh t-svn-dcommit-clobber-series.sh t-svn-dcommit-concurrent.sh t-svn-dcommit-crlf.sh t-svn-dcommit-funky-renames.sh t-svn-dcommit-interactive.sh t-svn-dcommit-merge.sh t-svn-dcommit-new-file.sh t-svn-deep-rmdir.sh Then you could skip t-svn-*, or just t-svn-dcommit-*, or even t-svn-commit-diff.12. The "t-" is ugly, but lets you distinguish test scripts from other shell scripts in the directory. We could also use actual directories for the hierarchy. svn/dcommit/*, etc. The t/ directory is rather big. It does make it a little more work to assemble the full set of tests (you have to use `find` rather than a glob). -Peff
diff --git a/t/t7415-submodule-names.sh b/t/t7450-bad-meta-files.sh similarity index 95% rename from t/t7415-submodule-names.sh rename to t/t7450-bad-meta-files.sh index 5c95247180..6b703b12bc 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7450-bad-meta-files.sh @@ -1,9 +1,16 @@ #!/bin/sh -test_description='check handling of .. in submodule names +test_description='check forbidden or malicious patterns in .git* files -Exercise the name-checking function on a variety of names, and then give a -real-world setup that confirms we catch this in practice. +Such as: + + - presence of .. in submodule names; + Exercise the name-checking function on a variety of names, and then give a + real-world setup that confirms we catch this in practice. + + - nested submodule names + + - symlinked .gitmodules, etc ' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pack.sh
This script has already expanded beyond its original intent of ".. in submodule names" to include other malicious submodule bits. Let's update the name and description to reflect that, as well as the fact that we'll soon be adding similar tests for other meta-files (.gitattributes, etc). We'll also renumber it to move it out of the group of submodule-specific tests. Signed-off-by: Jeff King <peff@peff.net> --- ...5-submodule-names.sh => t7450-bad-meta-files.sh} | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) rename t/{t7415-submodule-names.sh => t7450-bad-meta-files.sh} (95%)