On Thu, Jul 25, 2013 at 5:12 AM, Johannes Sixt <j.sixt@xxxxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html