On Sun, Apr 20, 2025 at 12:00:08PM +0200, Christian Couder wrote:
> On Mon, Apr 14, 2025 at 8:51 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -25,7 +25,29 @@ TEST_OUTPUT_DIRECTORY=$(pwd)
> > TEST_NO_CREATE_REPO=t
> > TEST_NO_MALLOC_CHECK=t
> >
> > -. ../test-lib.sh
> > +# While test-lib.sh computes the build directory for us, we also have to do the
> > +# same thing in order to locate the script via GIT-BUILD-OPTIONS in the first
> > +# place.
> > +GIT_BUILD_DIR="${GIT_BUILD_DIR:-$TEST_DIRECTORY/..}"
>
> Right now on 'master' there is:
>
> GIT_BUILD_DIR="${GIT_BUILD_DIR:-${TEST_DIRECTORY%/t}}"
> if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> then
> echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> exit 1
> fi
>
> so it's not exactly the same thing, even if it still probably works well.
>
> Future readers might wonder if this discrepancy results from changes
> that were made to only one of the files or if we really wanted to get
> rid of the "/t" check here. In case we do want to get rid of the "/t"
> check, I think it might be worth saying it clearly in the comment.
The "/.." is intentional here due to the way that `TEST_DIRECTORY` is
constructed. If you extend the context of this patch a bit, you can see
that `TEST_DIRECTORY=$(pwd)/..`. So stripping "/t" from the suffix
wouldn't do anything because it never has that suffix in the first
place. And neither do we want to strip "/..", because then we'd end up
in "t/perf". So the easiest fix is to just append another "/.." to end
up where we want to.
I'll try to paraphrase this in the commit message.
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > + # On Windows, we must convert Windows paths lest they contain a colon
> > + case "$(uname -s)" in
> > + *MINGW*)
> > + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > + ;;
> > + esac
> > +fi
> > +
> > +if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +then
> > + echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
>
> Maybe something like the following could help debug this:
>
> echo >&2 "error: GIT-BUILD-OPTIONS file missing from '$GIT_BUILD_DIR'"
> echo >&2 'error: (has Git been built?).'
I'd rather want to keep this as-is for now as we have the same error
message in "test-lib.sh". If we want to change it we should change both
errors, but that feels outside of the scope of this patch series.
Patrick
