Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: May 10, 2022, 06:04 UTC
Message-ID: <20220510060453.d6t5q5lqssvba6tg@carlos-mbp.lan>
In-Reply-To: <20220427170649.4949-2-szeder.dev@gmail.com>

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:

Show 9 quoted lines

> 
>   - 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' '