[PATCH v2] blame: add a range option to -L

[PATCH v2] blame: add a range option to -L

[Bill Pemberton]

In addition to <start>,<end> you can now use <center>%<radius>
to specify how many lines around <center> that you want to see.
For example: -L 20%5 would show lines 15 through 25

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---

This is like the previous patch to create a range option to -L in git-blame.
However, this one uses -L<start>%<end>

I chose to use % since it's on a standard keyboard.


 Documentation/blame-options.txt |    4 ++++
 Documentation/git-blame.txt     |    8 ++++++++
 builtin/blame.c                 |   18 +++++++++++++++---
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index d820569..f65e69c 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -32,6 +32,10 @@ This is only valid for <end> and will specify a number
 of lines before or after the line given by <start>.
 +
 
+-L <center>%<radius>::
+	This works like <start>,<end> with the annotated range
+	centered on <center> and showing <radius> lines around it.
+
 -l::
 	Show long rev (Default: off).
 
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index a27f439..73f6b83 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -110,6 +110,14 @@ line 40):
 	git blame -L 40,60 foo
 	git blame -L 40,+21 foo
 
+A range of lines around a particular line can be shown by using '%'
+instead of ','.  If you wanted to see line 20 along with the 5
+lines around it:
+
+       git blame -L 20%5 foo
+
+
+
 Also you can use a regular expression to specify the line range:
 
 	git blame -L '/^sub hello {/,/^}$/' foo
diff --git a/builtin/blame.c b/builtin/blame.c
index fc15863..eabc292 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1887,6 +1887,9 @@ static const char *parse_loc(const char *spec,
 	/* Allow "-L <something>,+20" to mean starting at <something>
 	 * for 20 lines, or "-L <something>,-5" for 5 lines ending at
 	 * <something>.
+	 *
+	 * In addition "-L <something>%5" means starting at
+	 * <something>-5 and ending at <something>+5
 	 */
 	if (1 < begin && (spec[0] == '+' || spec[0] == '-')) {
 		num = strtol(spec + 1, &term, 10);
@@ -1958,10 +1961,19 @@ static void prepare_blame_range(struct scoreboard *sb,
 	const char *term;
 
 	term = parse_loc(bottomtop, sb, lno, 1, bottom);
-	if (*term == ',') {
+	if (*term == ',')
+		term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
+	else if (*term == '%') {
+		long x;
+		/* ignore + or - if it's there */
+		if ((*(term+1) == '+') || (*(term+1) == '-'))
+			term++;
 		term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
-		if (*term)
-			usage(blame_usage);
+		x = *top;
+		*top = *bottom - x;
+		*bottom += x;
+		if (*bottom < 1)
+			*bottom = 1;
 	}
 	if (*term)
 		usage(blame_usage);
-- 
1.7.1

Re: [PATCH v2] blame: add a range option to -L

[Michael Witten]

On Mon, May 3, 2010 at 13:06, Bill Pemberton <wfp5p@virginia.edu> wrote:
> +-L <center>%<radius>::
> +       This works like <start>,<end> with the annotated range
> +       centered on <center> and showing <radius> lines around it.

I think the baggage of '%' might trip people up unless you add
something like: "Here, % symbolizes the notion of context above and
below the center line".

Re: [PATCH v2] blame: add a range option to -L

[Junio C Hamano]

Bill Pemberton <wfp5p@virginia.edu> writes:

> In addition to <start>,<end> you can now use <center>%<radius>
> to specify how many lines around <center> that you want to see.
> For example: -L 20%5 would show lines 15 through 25
>
> Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
> ---

Please retitle, as (1) -L has always been about "range", and (2) what you
are adding now is a "radius" option ;-)

> +-L <center>%<radius>::
> +	This works like <start>,<end> with the annotated range
> +	centered on <center> and showing <radius> lines around it.

I am not sure how "like <start>,<end>" in this sentence helps the readers.
If you bring up the similarity, shouldn't you at least be saying that it
is an shorthand to give "<radius> lines before <center>" as <start>, and
"<radius> lines after <center>" as <end>, or somesuch?

> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> index a27f439..73f6b83 100644
> --- a/Documentation/git-blame.txt
> +++ b/Documentation/git-blame.txt
> @@ -110,6 +110,14 @@ line 40):
>  	git blame -L 40,60 foo
>  	git blame -L 40,+21 foo
>  
> +A range of lines around a particular line can be shown by using '%'
> +instead of ','.  If you wanted to see line 20 along with the 5
> +lines around it:
> +
> +       git blame -L 20%5 foo
> +
> +
> +

Why this many blank lines around the example?

I see this at the beginning of parse_loc() in builtin/blame.c:

	/* Allow "-L <something>,+20" to mean starting at <something>
	 * for 20 lines, or "-L <something>,-5" for 5 lines ending at
	 * <something>.
	 */

which means that it is not "-L <start>,<end>" to begin with.  I wonder if
it makes the interface more consistent to rewrite the above comment like
this:

	/*
	 * Allow "-L <something>,+20" to mean starting at <something>
	 * for 20 lines; "-L <something>,-5" for 5 lines ending at
	 * <something>; and "-L <something>,+-5" for 5 lines around
         * <something>.
	 */

and the match the code.

Re: [PATCH v2] blame: add a range option to -L

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> I wonder if it makes the interface more consistent to rewrite the
> above comment like this:
> 
> 	/*
> 	 * Allow "-L <something>,+20" to mean starting at <something>
> 	 * for 20 lines; "-L <something>,-5" for 5 lines ending at
> 	 * <something>; and "-L <something>,+-5" for 5 lines around
>          * <something>.
> 	 */
> 
> and the match the code.

I like this.


The other approach would be to use -B <num> / -A <num> / -C [num], -<num>
convention from 'grep'... but git-blame uses -C and -<num> for other
things.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH v2] blame: add a range option to -L

[Matthieu Moy]

Bill Pemberton <wfp5p@virginia.edu> writes:

>  		term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
> -		if (*term)
> -			usage(blame_usage);
> +		x = *top;

Why not use parse_loc(..., &x) if you want the value to end up in x ?

> +		*top = *bottom - x;
> +		*bottom += x;

The existing code seems to assume that top >= bottom, but swaps top
and bottom otherwise:

	if (bottom && top && top < bottom) {
		long tmp;
		tmp = top; top = bottom; bottom = tmp;
	}

So, I'd write

*top = *bottom + x;
*bottom -= x;

> +		if (*bottom < 1)
> +			*bottom = 1;

I guess you've assumed that bottom was the small number here,
otherwise, you're checking for overflow, not for actually negative
numbers. Either you apply my proposal above or you should
s/bottom/top/ here, right?

(the existing code already have this a few lines after the call to
this functions, it doesn't harm to do it again, but better do it on
the right function)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/