Re: [PATCH] setup: trace bare repository setups
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 27, 2023, 22:54 UTC
- Message-ID
- <xmqqttx1gcmr.fsf@gitster.g>
- In-Reply-To
- <cb72bca46c6ff2a8cf3196408fb53411f7f91892.1682631601.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
Show 12 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 5 quoted lines
> +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.
Show 7 quoted lines
> @@ -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.
Show 25 quoted lines
> 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.