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

[Josh Steadmon <steadmon@google.com>]

On 2023.04.28 11:37, Glen Choo wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > 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.

Added a note in V3.


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

Yeah, misunderstood. However, see my reply to Junio about this being a
"change detector" test.