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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 25, 2013, 08:03 UTC
Message-ID: <CAPig+cRWaKv6UN5pZLwonXBpHnFE7+4ELXTnE+2nLr8h83bX6Q@mail.gmail.com>
In-Reply-To: <1374589688-27751-1-git-send-email-sunshine@sunshineco.com>

On Tue, Jul 23, 2013 at 10:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 10 quoted lines

> 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