Re: [PATCH] blame: allow -L n,m to have an m bigger than the file's line count

[Jay Soffian <jaysoffian@gmail.com>]

On Wed, Feb 10, 2010 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> While we are talking about touching the vicinity, I think we should
> tighten the -L s,e parsing rules a bit further.
>
> There is an undocumented code that swaps start and end if the given end is
> smaller than the start.  This triggers even when "-L280,300" is mis-typed
> as "-L280,30".  I was bitten by this more than once---when the input does
> not make sense, we should actively error out, instead of doing a wrong
> thing.  I suspect that I coded it that way _only_ to support this pattern:
>
>        -L'/^#endif \/\* !WINDOWS \/\*/,-30'
>
> i.e. "blame 30 lines before the '#endif' line".  But the code also
> internally turns "-L50,-20" into "-L 50,30" and then swaps them to may
> make it "-L30,50"; this was merely an unintended side effect.

I was curious what sed does. At least on my system, sed -n -e '10,1p'
prints just line 1. Seems a bit odd. sed -n -e '1,10000p' just prints
to the end, and doesn't error out if there are less than 10k lines.

> I do want to see -L'/regexp/,-offset' keep working, I do not mind if we
> keep taking "-L50,-20" as an unintuitive way to spell "-L30,50", or reject
> "-L50,-20" as a nonsense.  But I do want to see us reject "-L280,30" as a
> typo.
>
> As to use of more than one -L option, especially when the start (or end
> for that matter) is specified with an regexp, I am of two minds.

Actually, I was not planning on supporting multiple -L options, but rather...

> When annotating the body of two functions, frotz and nitfol, I might
> expect this to work:
>
>    -L'/^int frotz(/,/^}/'  -L'/^int nitfol(/,/^}/'
>
> regardless of the order these functions appear in the blob (i.e. nitfol
> may be defined first).  This requires that parsing of "regexp" needs to
> reset to the beginning of blob for each -L option (iow, multiple -L are
> parsed independently from each other).
>
> But at the same time, if I am actually looking at the blob contents in one
> terminal while spelling the blame command line in another, it would be
> nicer if the multiple -L looked for patterns incrementally.  I may
> appreciate if I can write the above command line as:
>
>    -L'/^int frotz(/,/^}/'  -L'/nitfol/,/^}/'
>
> when I can see in my "less" of the blob contents in the other terminal
> that the first line that has string "nitfol" after the end of the
> definition of "frotz" is the beginning of function "nitfol".
>
> Another thing we _might_ want to consider doing is something like:
>
>    -L'*/^#ifdef WINDOWS/,/^#endif \/\* WINDOWS \/\*/'
>
> to tell it "I don't care to count how many WINDOWS ifdef blocks there are;
> grab all of them".

That was my aim, but the syntax I'd settled on was to use
-L/pattern/..END where END is either a numerical argument or another
pattern. IOW, ".." instead of ",".

> Regardless of how parsing of multiple -L goes, you need to be careful to
> sort the resulting line ranges and possibly coalesce them when there are
> overlaps (e.g. "-L12,+7 -L10,+5" should become "-L10,17").  And be careful
> about refcounting of origin.  You'll be making multiple blame_ent and
> queuing them to the scoreboard when starting, all pointing at the blame
> target blob; the origin blob needs to start with the right number of
> references to keep origin_decref() discarding it.

Understood.

j.