Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 17, 2020, 06:06 UTC
Message-ID: <CAPig+cRqnRKUdmwX3K+hbYs8+T0CCpatkPi4T-bu8ofH6jTnNg@mail.gmail.com>
In-Reply-To: <xmqqy2k5931k.fsf@gitster.c.googlers.com>

On Fri, Oct 16, 2020 at 7:14 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> >  test_cmp() {
> > -     test $# -eq 2 || BUG "test_cmp requires two arguments"
> > -     if ! eval "$GIT_TEST_CMP" '"$@"'
>
> Looking at this again, I think we could keep the "we should have two
> arguments, no more than, no less than, but exactly two".  But I think
> those who write new tests are working to eventually make them pass,
> so hopefully they'll notice and investigate test_cmp that yields false
> anyway, I guess.

The purpose of the change in question[1] was specifically to help catch bugs in the test_expect_failure() case, which is quite rare in the Git test suite (and hopefully becomes rarer over time). So, I think it's fine to revert the change fully, including the 2-argument check since, as you say, test writers are normally trying to make their tests pass, and the 2-argument check doesn't provide much, if any, value for the test_expect_success() case.

[1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)