Re: [PATCH v3 2/2] tests(mingw): avoid very slow `mingw_test_cmp`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 14, 2022, 14:02 UTC
Message-ID: <pss2rp96-qr48-21p4-36on-ns04nnp0944q@tzk.qr>
In-Reply-To: <221114.86tu31lnwr.gmgdl@evledraar.gmail.com>

Hi Ævar,

On Mon, 14 Nov 2022, Ævar Arnfjörð Bjarmason wrote:

> [...] before this we've been assuming that GIT_TEST_CMP is a not-ours
> binary. I.e. "diff", "cmp" etc. If it is a "that's ours" this change
> should be changing e.g. '! test_cmp' to 'test_cmp !', as the former
> would now hide segfaults.

The implied assumption of `test_cmp` is that the command is reliable, i.e. we are not expecting to test the code that runs the comparison itself.

Originally, Junio shared your concern that `diff --no-index` might not be reliable enough, but that was "a lifetime ago". In https://lore.kernel.org/git/xmqqbkrpo9or.fsf@gitster.g/ he hinted that the `diff --no-index` code has matured enough to be considered reliable. That is far from the endorsement I would wish to have received (it would have been nice to see "I consider `git diff --no-index` to be reliable enough to serve as `mingw_test_cmp`", for example), but I believe it is the clearest statement I will get in that regard.

> Additionally: We don't *need* this for an initial implementation, but
> having e.g. one of the Ubuntu CI targets run with "git diff --no-index"
> would be a nice cherry on top,

Why would this be a nice cherry on top?

From my perspective, it would co-opt unrelated test cases into the task of validating `diff --no-index`' correctness.

Such a loss of focus in test cases makes it harder to diagnose, debug and fix breakages. And it would mean that a single bug could make gazillions of test cases fail. That would be bad practice, of course.

>  * If we're trusting "git diff --no-index" to run the tests, we could
>    also get rid of "GIT_TEST_CMP_USE_COPIED_CONTEXT", whose only reason
>    for existing is to support a system "diff" that doesn't understand
>    "-u" (squashable diff below)

Be my guest to contribute this once the current patch has made it to `next`. But please only then, we have enough friction on the Git mailing list and do not need to go out of our way to add more.

Thanks, Johannes

Show 48 quoted lines

>
> 1. https://lore.kernel.org/git/220907.86v8pzl6jz.gmgdl@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/220907.86r10nl63s.gmgdl@evledraar.gmail.com/
>
> diff --git a/Makefile b/Makefile
> index 4927379184c..dea6069b5fe 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1950,10 +1950,6 @@ ifdef OVERRIDE_STRDUP
>  	COMPAT_OBJS += compat/strdup.o
>  endif
>
> -ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
> -	export GIT_TEST_CMP_USE_COPIED_CONTEXT
> -endif
> -
>  ifndef NO_MSGFMT_EXTENDED_OPTIONS
>  	MSGFMT += --check
>  endif
> @@ -3008,9 +3004,6 @@ endif
>  ifdef GIT_TEST_CMP
>  	@echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@+
>  endif
> -ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
> -	@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+
> -endif
>  ifdef GIT_TEST_UTF8_LOCALE
>  	@echo GIT_TEST_UTF8_LOCALE=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_UTF8_LOCALE)))'\' >>$@+
>  endif
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4fab1c1984c..cd6e9f797b6 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1503,12 +1503,7 @@ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
>
>  if test -z "$GIT_TEST_CMP"
>  then
> -	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
> -	then
> -		GIT_TEST_CMP="$DIFF -c"
> -	else
> -		GIT_TEST_CMP="$DIFF -u"
> -	fi
> +	GIT_TEST_CMP="$DIFF -u"
>  fi
>
>  GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>