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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2013, 09:58 UTC
Message-ID: <7vk3l26695.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1373186706-19284-1-git-send-email-sunshine@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 19 quoted lines

> 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.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> 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.

Thanks.  Procrastination (and being down sick) pays off.

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 ;-).

 - The general concept to start from not just one but multiple blame
   entries that share the same source (and initial suspect) is the
   right way to implement multiple ranges.

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

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