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

On Thu, Jul 25, 2013 at 5:12 AM, Johannes Sixt [off-list ref] wrote:

Am 7/25/2013 10:03, schrieb Eric Sunshine:

quoted

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.

...

quoted

* 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.