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@xxxxxxxxxxxxxxxxxxx/ > 2. https://lore.kernel.org/git/220907.86r10nl63s.gmgdl@xxxxxxxxxxxxxxxxxxx/ > > 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 >
