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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jul 25, 2013 at 5:12 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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.