Re: [PATCH 0/5] range-set and line-log bug fixes

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 25, 2013, 13:09 UTC
Message-ID: <CAPig+cToEX1b+3q-Mhw_ukvZzqSDnRR7bJymC962t+qkZE25fw@mail.gmail.com>
In-Reply-To: <51F0EBF5.80105@viscovery.net>

On Thu, Jul 25, 2013 at 5:12 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 18 quoted lines

> Am 7/25/2013 10:03, schrieb Eric Sunshine:
>> The tests in this series identify real bugs in dealing with empty
>> ranges, which the subsequent patches fix. The test are possible
>> because one can specify an empty range via blame/log -L, however, I
>> now realize that the ability for -L to create empty ranges was never
>> intended or part of the design, but is in fact itself a bug.
> ...
>> * Should we drop these new t4211 tests which guard against real potential bugs?
>>
>> * Should we add custom C code to the test suite to make the
>> empty-range testing possible?
>>
>> * Should we introduce another (undocumented) loophole just for the
>> sake of the tests?
>
> IIUC, the tests you added are protecting the *implementation* of range-set
> functions. For tests of the implementation, we usually write test-foo
> programs that call the functions directly.

You understand correctly. The added t4211 tests check range-set and line-log functionality.

range-set is an implementation detail of git-log's -L and is entirely private (static to the implementation file), so there's no API to test via a test-foo program. It is sufficiently generic that its API could (some day) be published, thus allowing a test-foo program, however, doing so would involve writing documentation and covering its entire API with tests: a large enough task in itself, and quite orthogonal to fixing the log/blame -L loophole.

line-log is partially public, however, the code in which the bug was discovered is private (static) and likely always will be since it is not generic. Moreover, once the -L loophole is closed, there will be no way to trigger the case under consideration via its public API, so again there is no opportunity for a test-foo program.

Thus, the question remains: What to do with these two tests once the -L loophole is closed? Remove them?

> Tests invoking git should test the observable behavior. Therefore, if
> calling a git utility with "-Lfoo,+0" should be an error, then the test
> suite should mark such a call with test_must_fail. I guess this rules out
> the loophole approach.

Indeed, nicely stated.