Re: [PATCH v6 0/6] blame: add the ability to ignore commits

From: Barret Rhoden <brho@google.com>
Date: Apr 23, 2019, 18:13 UTC
Message-ID: <533f7721-2af6-1137-17c1-065837e1321d@google.com>
In-Reply-To: <20190422222647.48628-1-michael@platin.gs>

Hi Michael -

On 4/22/19 6:26 PM, michael@platin.gs wrote:

Show 8 quoted lines

> +	int *matching_lines = fuzzy_find_matching_lines(parent->file.ptr,
> +							target->file.ptr,
> +							parent->line_starts,
> +							target->line_starts,
> +							e->s_lno + offset,
> +							e->s_lno,
> +							parent_len,
> +							e->num_lines);

Here was the issue I ran into, and it's due to translations between e->s_lno and the parent's "address space".

The short version is that "e->s_lno + offset" is not always the parent_slno, and parent_len is based off of parent_slno.

guess_line_blames() gives you parent_slno and parent_len, as well as offset. 'offset' is how you convert from the target's space to the parent's. parent_slno and parent_len describe the whole chunk given to us from the diff engine. However, there may be multiple blame_entries covering that chunk.

So e->s_lno is in the target, but it's not necessarily the beginning of the entire diff chunk. This is related to that page fault you found a while back.

Passing e->s_lno + offset for where fuzzy() starts looking in the parent is fine, but then the length in the parent needs to be adjusted. For instance, I have this at the top of my modified fuzzy_find_matching_lines() (changed to take the origins and variables from guess_line_blames()):

         // XXX conversions to michael's variable names
	int start_a = e->s_lno + offset;
         //int length_a = parent_len;    // XXX this fails the test
         int length_a = (parent_slno + parent_len) - (e->s_lno + offset);

	int start_b = e->s_lno;
         int length_b = e->num_lines;

Plus we need a check for length_a <= 0. I had to work to make it be negative, but it's possible. parent_slno = tlno + offset, so we're looking at:

	length_a = tlno + parent_len - e->s_lno;

That just requires a blame entry split such that e->s_lno > tlno, and a parent chunk that had 0 lines. I found a case that did that. Basically in one commit you add a bunch of lines. In another, you change one line in the middle of that bunch. That causes a split of the diff chunk into more than one, such that e->s_lno > tlno. That original commit only added lines, so parent_len == 0.

The intuition for the "negative length_a" isn't that the parent_len is negative, it's that the e->s_lno chunk (when offset) is outside the window of the parent's change. I have a simple test for this.

Oh, and we have to length_a == 0, due to this:

	max_search_distance = length_a - 1;

Anyway, I'll take what I've got and apply your latest and see what I come up with. =) Plus, I have fixes for all of the other stuff brought up in the v6 discussion.

Barret