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

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

[Stephen Boyd]

Sometimes I want to blame a file starting at some point and ending at
the end of the file. In my haste I'll write something like this:

$ git blame -L5,2342343 -- builtin-blame.c

and be greeted by a die message telling me that my end range is greater
than the number of lines in the file. Obviously I can do:

$ git blame -L5, -- builtin-blame.c

and get what I want but that isn't very discoverable. If the range is
greater than the number of lines just truncate the range to go up to
the end of the file.

Update the docs to more accurately reflect the defaults for n and m too.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

I realize this is late in the game for 1.7.0 so I'll resend if this
isn't picked up.

 Documentation/blame-options.txt |    4 +++-
 builtin-blame.c                 |    4 +++-
 t/t8003-blame.sh                |    4 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 4833cac..620660d 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -9,7 +9,7 @@
 --show-stats::
 	Include additional statistics at the end of blame output.
 
--L <start>,<end>::
+-L [<start>],[<end>]::
 	Annotate only the given line range.  <start> and <end> can take
 	one of these forms:
 
@@ -31,6 +31,8 @@ starting at the line given by <start>.
 This is only valid for <end> and will specify a number
 of lines before or after the line given by <start>.
 +
+Note: if <start> is not given it defaults to 1 and if <end> is not given it
+defaults to the number of lines in the file.
 
 -l::
 	Show long rev (Default: off).
diff --git a/builtin-blame.c b/builtin-blame.c
index 10f7eac..77b7323 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1962,6 +1962,8 @@ static void prepare_blame_range(struct scoreboard *sb,
 		term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
 		if (*term)
 			usage(blame_usage);
+		if (lno < *top)
+			*top = lno;
 	}
 	if (*term)
 		usage(blame_usage);
@@ -2238,7 +2240,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"),
 		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
-		OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
+		OPT_CALLBACK('L', NULL, &bottomtop, "[n],[m]", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
 		OPT_END()
 	};
 
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index 4a8db74..0ba150e 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -161,8 +161,8 @@ test_expect_success 'blame -L with invalid start' '
 	test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
 '
 
-test_expect_success 'blame -L with invalid end' '
-	git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
+test_expect_success 'blame -L with invalid end truncates automatically' '
+	git blame -L1,5 tres
 '
 
 test_done
-- 
1.7.0.rc2.13.g8b233

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

[SZEDER Gábor]

Hi Stephen,


On Tue, Feb 09, 2010 at 11:27:44PM -0800, Stephen Boyd wrote:
> Sometimes I want to blame a file starting at some point and ending at
> the end of the file. In my haste I'll write something like this:
> 
> $ git blame -L5,2342343 -- builtin-blame.c
> 
> and be greeted by a die message telling me that my end range is greater
> than the number of lines in the file. Obviously I can do:
> 
> $ git blame -L5, -- builtin-blame.c
> 
> and get what I want but that isn't very discoverable. If the range is
> greater than the number of lines just truncate the range to go up to
> the end of the file.
> 
> Update the docs to more accurately reflect the defaults for n and m too.
> 
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
> 
> I realize this is late in the game for 1.7.0 so I'll resend if this
> isn't picked up.
> 
>  Documentation/blame-options.txt |    4 +++-
>  builtin-blame.c                 |    4 +++-
>  t/t8003-blame.sh                |    4 ++--
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index 4833cac..620660d 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -9,7 +9,7 @@
>  --show-stats::
>  	Include additional statistics at the end of blame output.
>  
> --L <start>,<end>::
> +-L [<start>],[<end>]::
>  	Annotate only the given line range.  <start> and <end> can take
>  	one of these forms:
>  
> @@ -31,6 +31,8 @@ starting at the line given by <start>.
>  This is only valid for <end> and will specify a number
>  of lines before or after the line given by <start>.
>  +
> +Note: if <start> is not given it defaults to 1 and if <end> is not given it
> +defaults to the number of lines in the file.
>  
>  -l::
>  	Show long rev (Default: off).

I agree that its too late for the behavioral change, but IMHO the
documentation update part can be considered as a bugfix, and as such
it could perhaps be included in 1.7.0.  (I never knew that <start> or
<end> can be omitted...  so thanks for the hint anyway)


Best,
Gábor

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

[Jay Soffian]

On Wed, Feb 10, 2010 at 2:27 AM, Stephen Boyd <bebarino@gmail.com> wrote:
> and get what I want but that isn't very discoverable. If the range is
> greater than the number of lines just truncate the range to go up to
> the end of the file.

I agree this is the right thing to do. I'm working on a patch to
support matching multiple times when given a regex range and made just
that change as well. :-)

j.

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

[Stephen Boyd]

On 02/10/2010 05:37 AM, Jay Soffian wrote:
> I agree this is the right thing to do. I'm working on a patch to
> support matching multiple times when given a regex range and made just
> that change as well. :-)
>    

Great! I'll split the patch into a documentation patch (for 1.7.0) and a 
behavioral patch (for post 1.7.0)? Or perhaps you can take care of the 
behavioral change in your upcoming series?

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Feb 10, 2010 at 2:27 AM, Stephen Boyd <bebarino@gmail.com> wrote:
>> and get what I want but that isn't very discoverable. If the range is
>> greater than the number of lines just truncate the range to go up to
>> the end of the file.
>
> I agree this is the right thing to do. I'm working on a patch to
> support matching multiple times when given a regex range and made just
> that change as well. :-)

I would mildly suggest against going in that direction.

This is merely "mildly", because truncating 99999 in "-L200,99999" to the
number of lines in the target blob would _not_ hurt.  But it is an ugly
hack.  I would be Ok with coding that special case, but I do not want to
see it advertised, especially if we are making "omission defaults to the
end" the documented way to explicitly say "I don't care to count, just do
it til the end".

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

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

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.

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

[Jay Soffian]

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.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

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

That is extremely sad.

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

[Jay Soffian]

On Wed, Feb 10, 2010 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> That is extremely sad.

For you, I'll see what I can do. I don't like the maintainer to be grumpy. :-)

j.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

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

I would suggest against using that syntax.

Users of different systems use A,B or A..B as range notations, but there
isn't anything that helps the unsuspecting learners to learn and memorize
that double-dot variant A..B has a repeating semantics and comma A,B does
not.