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

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

[Eric Sunshine]

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.

Rather than sorting and coalescing input -L ranges manually, existing
add_blame_range() and coalesce() are (ab)used to normalize the input.
This requires a small change to coalesce() to deal with potentially
overlapping ranges since it never otherwise encounters overlap during
normal blame operation.

This patch is somewhat less scary when whitespace changes are ignored.


 builtin/blame.c | 70 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 34 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 079dcd3..f26ff44 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -278,8 +278,11 @@ static void coalesce(struct scoreboard *sb)
 	for (ent = sb->ent; ent && (next = ent->next); ent = next) {
 		if (same_suspect(ent->suspect, next->suspect) &&
 		    ent->guilty == next->guilty &&
-		    ent->s_lno + ent->num_lines == next->s_lno) {
-			ent->num_lines += next->num_lines;
+		    ent->s_lno + ent->num_lines >= next->s_lno) {
+			int ent_top = ent->lno + ent->num_lines;
+			int next_top = next->lno + next->num_lines;
+			if (ent_top < next_top)
+				ent->num_lines = next_top - ent->s_lno;
 			ent->next = next->next;
 			if (ent->next)
 				ent->next->prev = ent;
@@ -2245,17 +2248,6 @@ static int blame_move_callback(const struct option *option, const char *arg, int
 	return 0;
 }
 
-static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset)
-{
-	const char **bottomtop = option->value;
-	if (!arg)
-		return -1;
-	if (*bottomtop)
-		die("More than one '-L n,m' option given");
-	*bottomtop = arg;
-	return 0;
-}
-
 int cmd_blame(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -2263,11 +2255,11 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct scoreboard sb;
 	struct origin *o;
 	struct blame_entry *ent;
-	long dashdash_pos, bottom, top, lno;
+	long dashdash_pos, lno;
 	const char *final_commit_name = NULL;
 	enum object_type type;
 
-	static const char *bottomtop = NULL;
+	static struct string_list ranges;
 	static int output_option = 0, opt = 0;
 	static int show_stats = 0;
 	static const char *revs_file = NULL;
@@ -2293,13 +2285,14 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("Use <file>'s contents as the final image")),
 		{ OPTION_CALLBACK, 'C', NULL, &opt, N_("score"), N_("Find line copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, N_("score"), N_("Find line movements within and across files"), PARSE_OPT_OPTARG, blame_move_callback },
-		OPT_CALLBACK('L', NULL, &bottomtop, N_("n,m"), N_("Process only line range n,m, counting from 1"), blame_bottomtop_callback),
+		OPT_STRING_LIST('L', NULL, &ranges, N_("n,m"), N_("Process only line range n,m, counting from 1")),
 		OPT__ABBREV(&abbrev),
 		OPT_END()
 	};
 
 	struct parse_opt_ctx_t ctx;
 	int cmd_is_annotate = !strcmp(argv[0], "annotate");
+	unsigned int range_i;
 
 	git_config(git_blame_config, NULL);
 	init_revisions(&revs, NULL);
@@ -2492,24 +2485,33 @@ parse_done:
 	num_read_blob++;
 	lno = prepare_lines(&sb);
 
-	bottom = top = 0;
-	if (bottomtop)
-		prepare_blame_range(&sb, bottomtop, lno, &bottom, &top);
-	if (bottom < 1)
-		bottom = 1;
-	if (top < 1)
-		top = lno;
-	bottom--;
-	if (lno < top || lno < bottom)
-		die("file %s has only %lu lines", path, lno);
-
-	ent = xcalloc(1, sizeof(*ent));
-	ent->lno = bottom;
-	ent->num_lines = top - bottom;
-	ent->suspect = o;
-	ent->s_lno = bottom;
-
-	sb.ent = ent;
+	if (!ranges.nr)
+		string_list_append(&ranges, xstrdup("0,0"));
+
+	for (range_i = 0; range_i < ranges.nr; ++range_i) {
+		long bottom, top;
+		prepare_blame_range(&sb, ranges.items[range_i].string,
+				    lno, &bottom, &top);
+		if (bottom < 1)
+			bottom = 1;
+		if (top < 1)
+			top = lno;
+		bottom--;
+		if (lno < top || lno < bottom)
+			die("file %s has only %lu lines", path, lno);
+
+		ent = xcalloc(1, sizeof(*ent));
+		ent->lno = bottom;
+		ent->num_lines = top - bottom;
+		ent->suspect = o;
+		ent->s_lno = bottom;
+		add_blame_entry(&sb, ent);
+	}
+	coalesce(&sb);
+
+	origin_decref(o);
+	string_list_clear(&ranges, 0);
+
 	sb.path = path;
 
 	read_mailmap(&mailmap, NULL);
-- 
1.8.3.2

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> 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.
>
> 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.

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

[Eric Sunshine]

[Added Cc:Thomas Rast]

On Sun, Jul 7, 2013 at 5:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> 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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

To be extremely honest, I do not care too deeply about what log -L
does today, because it is still in "may have rough edges but is an
interesting toy to play with" state in my mind ;-)

The suggestion to error out was more about "start simple, strict and
obvious to make it easy to explain" and nothing else.  If we start
with a simple and strict version, we can later loosen it without
making an input that was valid earlier invalid.  If we start with
too loose, on the other hand, it would be hard to tighten it later.

But the only two things I care deeply about are, in a file whose
contents is:

	C
        B
        A
        B
        C
        D

 (1) The range "-L /A/,/B/" finds the first A from the beginning,
     and then chooses B that comes _after_ it, making it equivalent
     to -L3,4 (not -L3,2 or -L2,3).

 (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
     beginning of the second range is found by choosing C that comes
     _after_ the end of the previous range (/B/ may choose either
     the second or the 4th line, and the only C that comes after
     either of them is the 5th line and that is where the second
     range should begin, not at the beginning of the file).  The
     same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
     to be the beginning of the second range).

I view it as a nice addition to coalesce two overlapping ranges
given exactly by numbers, e.g. "-L 100,200 -L 50,102".  I do not
have a strong objection to it, as long as it does not interfere
negatively with ranges specified by patterns.

Thanks.

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

[Eric Sunshine]

On Tue, Jul 9, 2013 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> 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?
>
> The suggestion to error out was more about "start simple, strict and
> obvious to make it easy to explain" and nothing else.  If we start
> with a simple and strict version, we can later loosen it without
> making an input that was valid earlier invalid.  If we start with
> too loose, on the other hand, it would be hard to tighten it later.

Makes sense.

> But the only two things I care deeply about are, in a file whose
> contents is:
>
>         C
>         B
>         A
>         B
>         C
>         D
>
>  (1) The range "-L /A/,/B/" finds the first A from the beginning,
>      and then chooses B that comes _after_ it, making it equivalent
>      to -L3,4 (not -L3,2 or -L2,3).
>
>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>      beginning of the second range is found by choosing C that comes
>      _after_ the end of the previous range (/B/ may choose either
>      the second or the 4th line, and the only C that comes after
>      either of them is the 5th line and that is where the second
>      range should begin, not at the beginning of the file).  The
>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>      to be the beginning of the second range).

Thinking aloud: Thus, for "-L 1,5 -L /C/", no C would be found.

> I view it as a nice addition to coalesce two overlapping ranges
> given exactly by numbers, e.g. "-L 100,200 -L 50,102".  I do not
> have a strong objection to it, as long as it does not interfere
> negatively with ranges specified by patterns.

Okay, I will base my rewrite upon the above constraints, along with
other observations from your initial response.

-- ES

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

[Thomas Rast]

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

>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>      beginning of the second range is found by choosing C that comes
>      _after_ the end of the previous range (/B/ may choose either
>      the second or the 4th line, and the only C that comes after
>      either of them is the 5th line and that is where the second
>      range should begin, not at the beginning of the file).  The
>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>      to be the beginning of the second range).

So passing several -L arguments does not blame the union of what each
argument would blame individually?  Doesn't that make it rather harder
to explain?

In any case, if you define it like that for blame, log -L should be
changed to match.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Eric Sunshine]

On Tue, Jul 9, 2013 at 1:42 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>>      beginning of the second range is found by choosing C that comes
>>      _after_ the end of the previous range (/B/ may choose either
>>      the second or the 4th line, and the only C that comes after
>>      either of them is the 5th line and that is where the second
>>      range should begin, not at the beginning of the file).  The
>>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>>      to be the beginning of the second range).
>
> So passing several -L arguments does not blame the union of what each
> argument would blame individually?  Doesn't that make it rather harder
> to explain?

I don't think Junio meant to imply that. Collecting the blame ranges
can/should be a distinct step from coalescing them. Junio is saying
that an -L /re/ range search should start after the maximum line
number already specified by any preceding range. Once all input ranges
are collected, they can be coalesced. (If a -L /re/ range happens to
be coalesced with or into some other range, that's fine: you're still
seeing blame output for the requested lines.)

-- ES

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jul 9, 2013 at 1:42 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>>>      beginning of the second range is found by choosing C that comes
>>>      _after_ the end of the previous range (/B/ may choose either
>>>      the second or the 4th line, and the only C that comes after
>>>      either of them is the 5th line and that is where the second
>>>      range should begin, not at the beginning of the file).  The
>>>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>>>      to be the beginning of the second range).
>>
>> So passing several -L arguments does not blame the union of what each
>> argument would blame individually?  Doesn't that make it rather harder
>> to explain?
>
> I don't think Junio meant to imply that. Collecting the blame ranges
> can/should be a distinct step from coalescing them. Junio is saying
> that an -L /re/ range search should start after the maximum line
> number already specified by any preceding range.

I am not sure if I want "maximum specified so far". I meant "start
searching at the last location", e.g.

	-L 100,200 -L 4,6 -L /A/,+20

would want to find the first A after line 6, not after line 200.

> Once all input ranges
> are collected, they can be coalesced. (If a -L /re/ range happens to
> be coalesced with or into some other range, that's fine: you're still
> seeing blame output for the requested lines.)

Yes.

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

[Eric Sunshine]

On Tue, Jul 9, 2013 at 2:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Jul 9, 2013 at 1:42 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>>>>      beginning of the second range is found by choosing C that comes
>>>>      _after_ the end of the previous range (/B/ may choose either
>>>>      the second or the 4th line, and the only C that comes after
>>>>      either of them is the 5th line and that is where the second
>>>>      range should begin, not at the beginning of the file).  The
>>>>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>>>>      to be the beginning of the second range).
>>>
>>> So passing several -L arguments does not blame the union of what each
>>> argument would blame individually?  Doesn't that make it rather harder
>>> to explain?
>>
>> I don't think Junio meant to imply that. Collecting the blame ranges
>> can/should be a distinct step from coalescing them. Junio is saying
>> that an -L /re/ range search should start after the maximum line
>> number already specified by any preceding range.
>
> I am not sure if I want "maximum specified so far". I meant "start
> searching at the last location", e.g.
>
>         -L 100,200 -L 4,6 -L /A/,+20
>
> would want to find the first A after line 6, not after line 200.

Okay.

>
>> Once all input ranges
>> are collected, they can be coalesced. (If a -L /re/ range happens to
>> be coalesced with or into some other range, that's fine: you're still
>> seeing blame output for the requested lines.)
>
> Yes.

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

[Thomas Rast]

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Jul 9, 2013 at 1:42 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>>>>      beginning of the second range is found by choosing C that comes
>>>>      _after_ the end of the previous range (/B/ may choose either
>>>>      the second or the 4th line, and the only C that comes after
>>>>      either of them is the 5th line and that is where the second
>>>>      range should begin, not at the beginning of the file).  The
>>>>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>>>>      to be the beginning of the second range).
>>>
>>> So passing several -L arguments does not blame the union of what each
>>> argument would blame individually?  Doesn't that make it rather harder
>>> to explain?
>>
>> I don't think Junio meant to imply that. Collecting the blame ranges
>> can/should be a distinct step from coalescing them. Junio is saying
>> that an -L /re/ range search should start after the maximum line
>> number already specified by any preceding range.
>
> I am not sure if I want "maximum specified so far". I meant "start
> searching at the last location", e.g.
>
> 	-L 100,200 -L 4,6 -L /A/,+20
>
> would want to find the first A after line 6, not after line 200.

Ok, so my point (in new words, since the old one was apparently too
terse) is:

If you define it that way, the output of

  git blame -L 4,6; git blame -L /A/,+20

is significantly different from

  git blame -L 4,6 -L /A/,+20

Not just in the presentation or any possible coalescing, but in the
meaning of the ranges.

Do you really want to make it that way?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Jonathan Nieder]

Thomas Rast wrote:

> If you define it that way, the output of
>
>   git blame -L 4,6; git blame -L /A/,+20
>
> is significantly different from
>
>   git blame -L 4,6 -L /A/,+20
>
> Not just in the presentation or any possible coalescing, but in the
> meaning of the ranges.

I can see both meanings being useful.  I suspect that the "restart at
the top of the file for each LHS regex" meaning would be more useful,
though.

Thanks,
Jonathan

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> If you define it that way, the output of
>
>   git blame -L 4,6; git blame -L /A/,+20
>
> is significantly different from
>
>   git blame -L 4,6 -L /A/,+20
>
> Not just in the presentation or any possible coalescing, but in the
> meaning of the ranges.
>
> Do you really want to make it that way?

Absolutely.  The primary reason I want to be able to specify two
ranges at the same time is to follow two functions in a file that
appear in separate places, and /A/ might not be unique.  When I want
to say "I want to see from here to there, and then from here to
there, and then from here to there", it would be very frustrating if
"and then" resets what I mean by "here" every time and make these
three evaluated independently.

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

[Michael Haggerty]

On 07/09/2013 09:31 PM, Junio C Hamano wrote:
> Thomas Rast <trast@inf.ethz.ch> writes:
> 
>> If you define it that way, the output of
>>
>>   git blame -L 4,6; git blame -L /A/,+20
>>
>> is significantly different from
>>
>>   git blame -L 4,6 -L /A/,+20
>>
>> Not just in the presentation or any possible coalescing, but in the
>> meaning of the ranges.
>>
>> Do you really want to make it that way?
> 
> Absolutely.  The primary reason I want to be able to specify two
> ranges at the same time is to follow two functions in a file that
> appear in separate places, and /A/ might not be unique.  When I want
> to say "I want to see from here to there, and then from here to
> there, and then from here to there", it would be very frustrating if
> "and then" resets what I mean by "here" every time and make these
> three evaluated independently.

It would be more general to support "follow the second match to /A/"
*independent* of whether the first match is also followed.  I think your
proposal only allows the second to be followed if the first is also
followed.  Therefore it seems to me that your wish is to add a
side-effect to one feature so that you can use it to obtain a simulacrum
of a second feature, instead of building the second feature directly.

Perhaps allow <start> and <end> to be a sequence of forms like

/A//A/,+20

    Start at the second occurrence of /A/ an continue for 20 lines

/A/+20,/B/

    Start 20 lines after the first match of /A/ until the subsequent
match of /B/

E.g., the body of function "foo" would be '/^int foo//^{/+1,/^}/-1'.
That should provide hours of amusement to baffled users ;-)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It would be more general to support "follow the second match to /A/"
> *independent* of whether the first match is also followed.  I think your
> proposal only allows the second to be followed if the first is also
> followed.  Therefore it seems to me that your wish is to add a
> side-effect to one feature so that you can use it to obtain a simulacrum
> of a second feature, instead of building the second feature directly.
>
> Perhaps allow <start> and <end> to be a sequence of forms like
>
> /A//A/,+20

Remember "A" is just a placeholder and in real life it would be more
than one character.  It is just as annoying as hell you have to type
it again.

I am not saying that a mode that resets the "start searching from
here" pointer to the beginning of the file is useless.  For example,
I would not mind typing a special character, e.g.

    -L <begin1>,<end1> -L !<begin2>,<end2>

that resets the search pointer to the beginning, for a rare case
where I want the search for <begin2> to restart at the top.

But the thing is, the default matters.  And it is far more common,
at least to me, when I want to say "from here to there, and then
from here to there", to expect the second "from here" would be below
the first one I already specified, while I am looking at the current
state of a single file from top to bottom and notice two places I am
interested in.

> /A/+20,/B/
>
>     Start 20 lines after the first match of /A/ until the subsequent
> match of /B/

Yes, I think -L "/^{/-4,/^}/" would be a nice thing to have, but I
think it is orthogonal to the issue of "where the search for /^{/

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

[Eric Sunshine]

On Tue, Jul 9, 2013 at 4:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> It would be more general to support "follow the second match to /A/"
>> *independent* of whether the first match is also followed.  I think your
>> proposal only allows the second to be followed if the first is also
>> followed.  Therefore it seems to me that your wish is to add a
>> side-effect to one feature so that you can use it to obtain a simulacrum
>> of a second feature, instead of building the second feature directly.
>>
>> Perhaps allow <start> and <end> to be a sequence of forms like
>>
>> /A//A/,+20
>
> Remember "A" is just a placeholder and in real life it would be more
> than one character.  It is just as annoying as hell you have to type
> it again.
>
> I am not saying that a mode that resets the "start searching from
> here" pointer to the beginning of the file is useless.  For example,
> I would not mind typing a special character, e.g.
>
>     -L <begin1>,<end1> -L !<begin2>,<end2>
>
> that resets the search pointer to the beginning, for a rare case
> where I want the search for <begin2> to restart at the top.
>
> But the thing is, the default matters.  And it is far more common,
> at least to me, when I want to say "from here to there, and then
> from here to there", to expect the second "from here" would be below
> the first one I already specified, while I am looking at the current
> state of a single file from top to bottom and notice two places I am
> interested in.

The proposal currently is only for "-L /RE/,whatever" to behave in a
relative fashion, beginning the search at the end of the last range
specified via -L (or line 1 if there is no previous -L).

Would it also make sense to support "-L +N,whatever" as relative to
the end of the last range specified via -L (or 1 if none).

I ask because the implementation changes needed to also support "-L
+N,whatever" appear to be less invasive than those only allowing "-L
/RE/,whatever/" to be relative. On the other hand, supporting "-L
+N,whatever" requires more documentation. I don't necessarily consider
less invasive changes as a good reason to support "-L +N,whatever" but
it got me thinking about it.

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

[Thomas Rast]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The proposal currently is only for "-L /RE/,whatever" to behave in a
> relative fashion, beginning the search at the end of the last range
> specified via -L (or line 1 if there is no previous -L).
>
> Would it also make sense to support "-L +N,whatever" as relative to
> the end of the last range specified via -L (or 1 if none).

Sounds reasonable.

I'm still not sure I am super-happy with /RE/ always being relative,
though I see Junio's problem space as something worth solving.  How does
it interact with -L:RE?  Do you now have to know in what order the
functions appear in the source to correctly specify -L:foo -L:bar or
similarly, -L/foo/,/^}/ -L/bar/,/^}/?  What if we supported +/RE/ as the
relative version?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> The proposal currently is only for "-L /RE/,whatever" to behave in a
>> relative fashion, beginning the search at the end of the last range
>> specified via -L (or line 1 if there is no previous -L).
>>
>> Would it also make sense to support "-L +N,whatever" as relative to
>> the end of the last range specified via -L (or 1 if none).
>
> Sounds reasonable.
>
> I'm still not sure I am super-happy with /RE/ always being relative,
> though I see Junio's problem space as something worth solving.  How does
> it interact with -L:RE?  Do you now have to know in what order the
> functions appear in the source to correctly specify -L:foo -L:bar or
> similarly, -L/foo/,/^}/ -L/bar/,/^}/?  What if we supported +/RE/ as the
> relative version?

Two gripes I have are:

 (1) That sounds like making common things more cumbersome to ask.

 (2) In "-L /RE1/,/RE2/", you do not have to say +/RE2/ (and you
     shouldn't have to).  /RE3/ without any magic that starts
     searching after the last match in "-L /RE1/,/RE2/ -L /RE3/,+4"
     feels a lot more consistent than requiring a prefix plus.

I am OK if you made /RE/, which starts searching immediately after
the last match, wrap around and continue the search at the beginning
upon finding nothing through the end of the file (and make sure you
stop if you passed the last match again).  That would solve your "I
have two functions, and I can give them in any order", while keeping
the consistency with "In /RE1/,/RE2/, the latter is already relative
to the former".

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

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> The proposal currently is only for "-L /RE/,whatever" to behave in a
>>> relative fashion, beginning the search at the end of the last range
>>> specified via -L (or line 1 if there is no previous -L).
>>>
>>> Would it also make sense to support "-L +N,whatever" as relative to
>>> the end of the last range specified via -L (or 1 if none).
>>
>> Sounds reasonable.
>>
>> I'm still not sure I am super-happy with /RE/ always being relative,
>> though I see Junio's problem space as something worth solving.  How does
>> it interact with -L:RE?  Do you now have to know in what order the
>> functions appear in the source to correctly specify -L:foo -L:bar or
>> similarly, -L/foo/,/^}/ -L/bar/,/^}/?  What if we supported +/RE/ as the
>> relative version?
>
> Two gripes I have are:
>
>  (1) That sounds like making common things more cumbersome to ask.
>
>  (2) In "-L /RE1/,/RE2/", you do not have to say +/RE2/ (and you
>      shouldn't have to).  /RE3/ without any magic that starts
>      searching after the last match in "-L /RE1/,/RE2/ -L /RE3/,+4"
>      feels a lot more consistent than requiring a prefix plus.
>
> I am OK if you made /RE/, which starts searching immediately after
> the last match, wrap around and continue the search at the beginning
> upon finding nothing through the end of the file (and make sure you
> stop if you passed the last match again).  That would solve your "I
> have two functions, and I can give them in any order", while keeping
> the consistency with "In /RE1/,/RE2/, the latter is already relative
> to the former".

Dunno.  It still doesn't really solve the order-of-L problem if there
are multiple matches.

I can't really say how they fare against each other.  I have a bad
feeling about the consistency of what results, but as you say, doing it
the way I suggested isn't very consistent either.  Perhaps in retrospect
even -L/foo/,/bar/ should have required the + in +/bar/ to make it
relative.

I'll just leave it at that and let you decide what to do (presumably go
ahead as you already outlined).  I've never actually ever used multiple
-L in the same log/blame invocation, anyway.  I just think that when it
comes to it, I'll have to read the manpage before I try...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> I'll just leave it at that and let you decide what to do (presumably go
> ahead as you already outlined).  I've never actually ever used multiple
> -L in the same log/blame invocation, anyway.

Nobody has ;-).

It is just between my "I often wished while looking at 'less foo.c'
I wanted to give more than on -L to cover two ranges I see in the
file" and your "I do not know if the user can give the first -L to
cover the first range and the second -L to cover the second range".
I do not think neither of us successfully gave more than one -L and
got any useful result.

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

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> If you define it that way, the output of
>>
>>   git blame -L 4,6; git blame -L /A/,+20
>>
>> is significantly different from
>>
>>   git blame -L 4,6 -L /A/,+20
>>
>> Not just in the presentation or any possible coalescing, but in the
>> meaning of the ranges.
>>
>> Do you really want to make it that way?
>
> Absolutely.  The primary reason I want to be able to specify two
> ranges at the same time is to follow two functions in a file that
> appear in separate places, and /A/ might not be unique.  When I want
> to say "I want to see from here to there, and then from here to
> there, and then from here to there", it would be very frustrating if
> "and then" resets what I mean by "here" every time and make these
> three evaluated independently.

Ok, fair enough.  That is at least an argument other than "trust me, I
care deeply" :-)

But still, log -L should then be changed to match this behavior (for all
args affecting a single file).  Currently it always does the scan for
the start of the range from line 1 of the file.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> But still, log -L should then be changed to match this behavior (for all
> args affecting a single file).  Currently it always does the scan for
> the start of the range from line 1 of the file.

Thanks, I think that makes sense.

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

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>  (2) In the ranges "-L <anything>,/B/ -L /C/,<anything>", the
>>      beginning of the second range is found by choosing C that comes
>>      _after_ the end of the previous range (/B/ may choose either
>>      the second or the 4th line, and the only C that comes after
>>      either of them is the 5th line and that is where the second
>>      range should begin, not at the beginning of the file).  The
>>      same for "-L 1,3 -L /C/" (only C that comes after 3 is eligible
>>      to be the beginning of the second range).
>
> So passing several -L arguments does not blame the union of what each
> argument would blame individually?  Doesn't that make it rather harder
> to explain?
>
> In any case, if you define it like that for blame, log -L should be
> changed to match.

I thought "log -L" was fundamnetally different as it is per path.
For example, -L1,100:hello.c and -L5,105:goodbye.c would not
intersect/overlap.

If you mean to coalesce -L1,100:hello.c and -L5,105:hello.c into a
single -L1,105:hello.c, then yes I think it makes sense.