git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Taylor Blau <me@ttaylorr.com>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir
Date: Mon, 9 May 2022 23:04:53 -0700	[thread overview]
Message-ID: <20220510060453.d6t5q5lqssvba6tg@carlos-mbp.lan> (raw)
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:
> 
>   - 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' '

  parent reply	other threads:[~2022-05-10  6:05 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-13 15:32 [PATCH 0/3] Updates to the safe.directory config option Derrick Stolee via GitGitGadget
2022-04-13 15:32 ` [PATCH 1/3] t0033: add tests for safe.directory Derrick Stolee via GitGitGadget
2022-04-13 16:24   ` Junio C Hamano
2022-04-13 16:29     ` Derrick Stolee
2022-04-13 19:16   ` Ævar Arnfjörð Bjarmason
2022-04-13 19:46     ` Junio C Hamano
2022-04-13 19:52     ` Derrick Stolee
2022-04-13 15:32 ` [PATCH 2/3] setup: fix safe.directory key not being checked Matheus Valadares via GitGitGadget
2022-04-13 16:34   ` Junio C Hamano
2022-04-13 15:32 ` [PATCH 3/3] setup: opt-out of check with safe.directory=* Derrick Stolee via GitGitGadget
2022-04-13 16:40   ` Junio C Hamano
2022-04-13 16:15 ` [PATCH 0/3] Updates to the safe.directory config option Junio C Hamano
2022-04-13 16:25   ` Derrick Stolee
2022-04-13 16:44     ` Junio C Hamano
2022-04-13 20:27       ` Junio C Hamano
2022-04-13 21:25         ` Taylor Blau
2022-04-13 21:45           ` Junio C Hamano
2022-04-27 17:06 ` [PATCH 0/3] t0033-safe-directory: test improvements and a documentation clarification SZEDER Gábor
2022-04-27 17:06   ` [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir SZEDER Gábor
2022-05-09 22:27     ` Taylor Blau
2022-05-10  6:04     ` Carlo Marcelo Arenas Belón [this message]
2022-04-27 17:06   ` [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored SZEDER Gábor
2022-04-27 20:37     ` Junio C Hamano
2022-04-29 16:12       ` Derrick Stolee
2022-04-29 17:57         ` Junio C Hamano
2022-04-29 19:06           ` SZEDER Gábor
2022-04-29 19:19             ` Derrick Stolee
2022-05-10 18:33               ` SZEDER Gábor
2022-05-10 19:55                 ` Taylor Blau
2022-05-10 20:06                   ` Carlo Marcelo Arenas Belón
2022-05-10 20:11                     ` Taylor Blau
2022-05-10 20:18                   ` Eric Sunshine
2022-04-27 17:06   ` [PATCH 3/3] safe.directory: document and check that it's ignored in the environment SZEDER Gábor
2022-04-27 20:42     ` Junio C Hamano
2022-04-27 20:49       ` Junio C Hamano
2022-04-29 16:13         ` Derrick Stolee
2022-05-09 21:39         ` SZEDER Gábor
2022-05-09 21:45           ` Junio C Hamano
2022-05-10 18:48             ` SZEDER Gábor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220510060453.d6t5q5lqssvba6tg@carlos-mbp.lan \
    --to=carenas@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).