[Added Cc:Thomas Rast] On Sun, Jul 7, 2013 at 5:58 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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 -- 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