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

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

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

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

>
> 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
>