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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 23, 2013 at 10:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> While implementing multiple -L support for git-blame, I encountered
> several bugs in range-set and line-log resulting in crashes. This
> series fixes those bugs.
>
> Eric Sunshine (5):
>   range-set: fix sort_and_merge_range_set() corner case bug
>   t4211: demonstrate empty -L range crash
>   range-set: satisfy non-empty ranges invariant
>   t4211: demonstrate crash when first -L encountered is empty range
>   line-log: fix "log -LN" crash when N is last line of file

I've run into a bit of a conundrum relating to the tests added by this
series, for which I could use some input.

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. An
example manifestation of this bug, given a 5-line file:

  % git blame -L5 foo  # OK, blames line 5
  % git blame -L6 foo  # accepted, no error, no output, huh?
  % git blame -L7 foo  # error: "fatal: file foo has only 5 lines"

I believe that it is correct to fix this bug (and already have a fix
locally). The example -L6 should error out just like -L7 rather than
creating an empty range.

Fixing this bug closes the empty-range-creation loophole which is used
by the t4211 tests added in this series and, unfortunately, there is
no other way to create an empty range, hence no way to keep these
tests operational. What to do?

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

For the last point, I was considering -Lfoo,+0. It is currently
undocumented and its behavior undefined. In fact, -Lfoo,+0 and
-Lfoo,-0 presently are interpreted as -Lfoo,+2 (definitely undefined
behavior). It would be possible to make -Lfoo,+0 mean empty-range and
keep it undocumented, which would create the loophole these tests
require.

I personally dislike this idea for several reasons: (1) we should be
closing loopholes rather than creating them intentionally; (2)
generally, an empty range has no useful meaning in conjunction with
-L; (3) if not an empty range, then -Lfoo,+0 conveys nothing and
should be reported as an error.

The only possible minor advantage I can see to interpreting -Lfoo,+0
as an empty range is that it could provide an anchor for relative
-L/RE/ searches once blame accepts multiple -L options. For example,
"blame -L42,+0 -L/^struct/,/^}/ foo.c" would blame the first struct
starting at line 42, without also showing blame for line 42. I don't
really consider this a good argument in favor of -Lfoo,+0 representing
an empty range, and it's a very poor substitute for Michael Haggerty's
more expressive proposal [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/229755/focus=230967