Re: [PATCH] setup: trace bare repository setups

[Josh Steadmon <steadmon@google.com>]

On 2023.04.27 15:54, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > diff --git a/setup.c b/setup.c
> > index 59abc16ba6..458582207e 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -1352,6 +1352,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> >  		}
> >  
> >  		if (is_git_directory(dir->buf)) {
> > +			trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
> >  			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
> >  				return GIT_DIR_DISALLOWED_BARE;
> >  			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
> 
> That is kind of obvious.
> 
> > diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> > index 11c15a48aa..a1f3b5a4d6 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\"" &&
> 
> Why not
> 
> 	test_when_finished 'rm "$pwd/trace.perf"' &&
> 
> instead?  
> 
> In your version, $pwd is expanded before test_when_finished sees it,
> so you'd have to worry about things like backslashes and double quotes
> in it.  You can of course quote the '$' like so
> 
> 	test_when_finished "rm \"\$pwd/trace.perf\"" &&
> 
> to work it around, but it is equivalent to enclosing everything
> inside a pair of single quotes.  Either way your $pwd will be
> interpolated when "eval" sees the $test_cleanup script.
> 
> And it would be much easier to read without backslash and
> backslashed double quotes.
> 
> > +	GIT_TRACE2_PERF="$pwd/trace.perf" git "$@" rev-parse --git-dir &&
> > +	grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> > +}
> 
> We ensure that we positively have such a trace in the output.  Good.
> 
> > +expect_accepted_explicit () {
> > +	test_when_finished "rm \"$pwd/trace.perf\"" &&
> > +	GIT_DIR="$@" GIT_TRACE2_PERF="$pwd/trace.perf" git rev-parse --git-dir &&
> > +	! grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
> >  }
> 
> When not asking for the magic behaviour of "$@" and instead doing a
> "squash everything into a single string, using the first letter in
> $IFS as the separator in between", please write "$*" instead.
> 
>     GIT_DIR="$*" GIT_TRACE2_PERF="..." git rev-parse --git-dir
> 
> But in this case, I do not think you are ever planning to send a
> directory name split into two or more, to be concatenated with SP,
> so writing it like
> 
>     GIT_DIR="$1" GIT_TRACE2_PERF="..." git rev-parse --git-dir
> 
> would be much less error prone and easier to follow, I think.
> 
> > @@ -22,12 +33,12 @@ test_expect_success 'setup bare repo in worktree' '
> >  '
> >  
> >  test_expect_success 'safe.bareRepository unset' '
> > -	expect_accepted -C outer-repo/bare-repo
> > +	expect_accepted_implicit -C outer-repo/bare-repo
> >  '
> 
> Perhaps futureproof this test piece by explicitly unsetting the
> variable before starting the test?  That way, this piece will not be
> broken even if earlier tests gets modified to set some value to
> safe.bareRepository in the future.
> 
> >  test_expect_success 'safe.bareRepository=all' '
> >  	test_config_global safe.bareRepository all &&
> > -	expect_accepted -C outer-repo/bare-repo
> > +	expect_accepted_implicit -C outer-repo/bare-repo
> >  '
> >  
> >  test_expect_success 'safe.bareRepository=explicit' '
> > @@ -47,7 +58,7 @@ test_expect_success 'safe.bareRepository in the repository' '
> >  
> >  test_expect_success 'safe.bareRepository on the command line' '
> >  	test_config_global safe.bareRepository explicit &&
> > -	expect_accepted -C outer-repo/bare-repo \
> > +	expect_accepted_implicit -C outer-repo/bare-repo \
> >  		-c safe.bareRepository=all
> >  '
> >  
> > @@ -60,4 +71,8 @@ test_expect_success 'safe.bareRepository in included file' '
> >  	expect_rejected -C outer-repo/bare-repo
> >  '
> >  
> > +test_expect_success 'no trace when GIT_DIR is explicitly provided' '
> > +	expect_accepted_explicit "$pwd/outer-repo/bare-repo"
> > +'
> > +
> >  test_done
> 
> All the expectations look sensible.  Thanks for a pleasant read.

Agreed with all the feedback points, will fix in V2. Thanks!