| Message ID | 20220427170649.4949-2-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f62563988fefed73d795cfaeae203421b784c334 |
| Headers | show |
| Series | t0033-safe-directory: test improvements and a documentation clarification | expand |
On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote: > - it is interpreted as a regexp by 'grep', so, because of the dot, > it matches the name of the test script and the path of the trash > directory as well. Consequently, these tests could be fooled by > any error message that would happen to include the path of the > test repository. Nice catch. I wouldn't have even thought about "could match the test directory" as a potential way for this test to generate a false positive. So I'm happy to see us tightening this down. Thanks. Taylor
Apologies for the late feedback, hoping it is still timely though. On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote: > > - it only appears in the advice part, not in the actual error > message. > > - it is interpreted as a regexp by 'grep', so, because of the dot, > it matches the name of the test script and the path of the trash > directory as well. Consequently, these tests could be fooled by > any error message that would happen to include the path of the > test repository. The subject says something else and I think it is confusing because the change you propose only seems to be removing the path as an implementation detail while the core of the change seems to be making sure that the text that is being matched is a "Fixed String" and part of the real error. I agree is an improvement, but I think it could be done better by : * using `grep -F` instead to make sure we don't interpret that string as a regex by mistake. * including the path, so we are sure we really matched the error message that was really expected. As a side effect we also validate this way that the code that does the lookup and reports the "name" of the repository that could be added to the exception list, reports the righ directory, which seems by itself an important thing we should want to avoid regressing against in future changes. Both changed are implemented by the patch below and I think might be worth considering as a replacement of this one. Carlo PS. Commit message could be improved, but wasn't sure how in the context of your series, so left it as a barebones one hoping it would be part of the patch you could adopt into your series. ----- >8 ----- Subject: t0033: verify detected worktree is accurate and part of the error Change the helper function to check for a fixed message and make sure to include the directory that we are testing from (which in all current tests is the current directory). Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index 239d93f4d2..c524b74b01 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER expect_rejected_dir () { test_must_fail git status 2>err && - grep "safe.directory" err + grep -F "unsafe repository ('$(pwd)'" err } test_expect_success 'safe.directory is not set' '
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index 239d93f4d2..6f9680e8b0 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER expect_rejected_dir () { test_must_fail git status 2>err && - grep "safe.directory" err + grep "unsafe repository" err } test_expect_success 'safe.directory is not set' '
Since 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) when git finds itself in a repository owned by someone else, it aborts with a "fatal: unsafe repository (<repo path>)" error message and an advice about how to set the 'safe.directory' config variable to mark that repository as safe. 't0033-safe-directory.sh' contains tests that check that this feature and handling said config work as intended. To ensure that git dies for the right reason, several of those tests check that its standard error contains the name of that config variable, but: - it only appears in the advice part, not in the actual error message. - it is interpreted as a regexp by 'grep', so, because of the dot, it matches the name of the test script and the path of the trash directory as well. Consequently, these tests could be fooled by any error message that would happen to include the path of the test repository. Tighten these checks to look for "unsafe repository" instead. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/t0033-safe-directory.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)