On 2023.04.27 15:54, Junio C Hamano wrote:
> Josh Steadmon <steadmon@xxxxxxxxxx> 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!
