Re: [PATCH] setup: trace bare repository setups

[Junio C Hamano <gitster@pobox.com>]

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.