Re: [PATCH/RFC] blame: accept multiple -L ranges

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 9, 2013, 15:04 UTC
Message-ID: <CAPig+cQ9sfV+PRqn5vrL0=xOZrKDDmsKdAPiB4PsCFb=jnk4kQ@mail.gmail.com>
In-Reply-To: <7vk3l26695.fsf@alter.siamese.dyndns.org>

[Added Cc:Thomas Rast]

On Sun, Jul 7, 2013 at 5:58 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> git-blame accepts only zero or one -L option. Clients requiring blame
>> information for multiple disjoint ranges are therefore forced either to
>> invoke git-blame multiple times, once for each range, or only once with
>> no -L option to cover the entire file, which can be costly. Teach
>> git-blame to accept multiple -L ranges.
>>
>> Overlapping and out-of-order ranges are accepted and handled gracefully.
>> For example:
>>
>>   git blame -L 3,+4 -L 91,+7 -L 2,3 -L 89,100 source.c
>>
>> emits blame information for lines 2-6 and 89-100.
>> ---
>>
>> This is RFC because it lacks documentation and test updates, and because
>> I want to make sure the approach is sound and not abusive of the blame
>> machinery.
>
> A few commments (without reading too deep in the patch, so do not
> take any of these as complaint---if you did it the way I said "I'd
> prefer", take it as a praise ;-).
>
>  - I'd prefer to see the command parser for multiple -L options to
>    ensure that they are in strictly increasing order without
>    overlap.  Error out with a message if the input ranges are out of
>    order or with overlap.  Doing it that way, it would be easier to
>    explain to the users how "blame -L /A/,/B/ -L /C/,/D/" should
>    work.  It would find the first line that matches C _after_ the
>    end of the first range.  This is in line with the way we find the
>    end of the range (e.g. the line that matches B) starting from the
>    last line previously specified (e.g. the line that matches A).

As implemented by this patch, the behavior of git-blame with multiple -L's is consistent with that of git-log with multiple -L's. The implemented behavior feels intuitive to me, but I can see how the behavior you suggest could feel intuitive to others.

If I re-do the patch to work the way you describe above, how should we deal with the inconsistent behaviors between the two commands?

>  - I'd be somewhat unhappy to see coalesce() butchered to blindly
>    accept overlapping ranges (if anything, I'd rather see it
>    tightened to detect such input as a programming error), but this
>    is a minor point.

Loosening the behavior bothered enough that I mentioned it centrally in the patch commentary. I can re-implement without (ab)using coalesce().

(In fact, I already have an implementation which re-uses the machinery employed by git-log -L.)

-- ES