Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]

Hi Victoria,

On Thu, 8 Sep 2022, Victoria Dye wrote:

> Johannes Schindelin via GitGitGadget wrote:
>
> > [...]
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> >  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> >  fi
> >  GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +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
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> >  then
> >  	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> >  	exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the
> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
>
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
>  	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>  fi
>  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
>  if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>  then
>  	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
>  		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>  		;;
>  	esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> -	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> -	exit 1
>  fi
>
>  # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
>
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.

Good point!

Thank you,
Dscho