Re: [PATCH v2] setup: trace bare repository setups

From: Glen Choo <chooglen@google.com>
Date: Apr 28, 2023, 18:37 UTC
Message-ID: <kl6lbkj73laf.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <b548d96db7052dc2cb664922fa2003fe068843cf.1682702058.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 21 quoted lines

> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index 11c15a48aa..993f5bdc7d 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -7,13 +7,24 @@ TEST_PASSES_SANITIZE_LEAK=true
>  
>  pwd="$(pwd)"
>  
> -expect_accepted () {
> -	git "$@" rev-parse --git-dir
> +expect_accepted_implicit () {
> +	test_when_finished 'rm "$pwd/trace.perf"' &&
> +	GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir &&
> +	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> +}
> +
> +expect_accepted_explicit () {
> +	test_when_finished 'rm "$pwd/trace.perf"' &&
> +	GIT_DIR="$1" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir &&
> +	! grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
>  }

(Not new in v2) This wasn't obvious to me at first, but

  grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"

looks like it's asserting that we trace that the bare repository is $pwd, but it's actually only asserting that is starts with $pwd. If it might trip up other reasers, a comment here would be helpful.

Show 8 quoted lines

> @@ -21,13 +32,9 @@ test_expect_success 'setup bare repo in worktree' '
>  	git init --bare outer-repo/bare-repo
>  '
>  
> -test_expect_success 'safe.bareRepository unset' '
> -	expect_accepted -C outer-repo/bare-repo
> -'
> -

I found this surprising; I thought it was quite valuable to test the default behavior. I'd prefer to keep this test.

Perhaps there was a miscommunication? You mentioned:

  Actually, explicitly setting the variable here is equivalent to the
  following test case, so I'll just remove this one.

which is true, but I think Junio meant _un_setting the variable, something like:

  test_expect_success 'safe.bareRepository unset' '
+   test_unconfig --global safe.bareRepository &&
    expect_accepted -C outer-repo/bare-repo
  '