Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 17, 2018, 17:39 UTC
Message-ID: <CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com>
In-Reply-To: <xmqqr2iyc526.fsf@gitster-ct.c.googlers.com>

On Fri, Aug 17, 2018 at 12:36 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>
> Andrei Rybak <rybak.a.v@gmail.com> writes:
>
> > On 14/08/18 13:47, SZEDER Gábor wrote:
> >> ... both
> >> invocations produce empty 'pack{a,b}.objects' files, and the
> >> subsequent 'test_cmp' happily finds those two empty files identical.
> >
> > Is test_cmp ever used for empty files? Would it make sense for
> > test_cmp to issue warning when an empty file is being compared?
>
> Typically test_cmp is used to compare the actual output from a
> dubious command being tested with the expected output from a
> procedure that is known not to be broken (e.g. a run of 'echo', or a
> 'cat' of here-doc), so at least one side would not be empty.
>
> The test done here is an odd case---it compares output from two
> equally dubious processes that are being tested and sees if their
> results match.
>
> That said, since we have test_must_be_empty, we could forbid feeding
> empty files to test_cmp, after telling everybody that a test that
> expects an empty output must use test_must_be_empty.  I do not think
> it is a terrible idea.

I thought so as well, and I've looked into it; in fact this patch was one of the skeletons that fell out of our test suite "while at it". However, I had to change my mind about it, and now I don't think we should go all the way and forbid that.

See, we have quite a few tests that extract repetitive common tasks into helper functions, which sometimes includes preparing the expected results and running 'test_cmp', e.g. something like this (oversimplified) example:

  check_cmd () {
        git cmd $1 >actual &&
        echo "$2" >expect &&
        test_cmp expect actual
  }

  check_cmd --foo    FOO
  check_cmd --no-foo ""

Completely forbidding feeding empty files to 'test_cmp' would require us to add a bit of logic to cases like this to call 'test_cmp' or 'test_must_be_empty' based on the content of the second parameter. Arguably it's only a tiny bit of logic, as only a single if statement is needed, but following our coding style it would take 7 lines instead of only 2:

  if test -n "$2"
  then
        echo "$2" >expect &&
        test_cmp expect actual
  else
        test_must_be_empty actual
  fi

I don't think it's worth it in cases like this.