From: Barret Rhoden <brho@google.com>
To: michael@platin.gs, git@vger.kernel.org
Cc: "Jeff King" <peff@peff.net>,
"Stefan Beller" <stefanbeller@gmail.com>,
"Jeff Smith" <whydoubt@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>,
"René Scharfe" <l.s.r@web.de>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"David Kastrup" <dak@gnu.org>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v6 0/6] blame: add the ability to ignore commits
Date: Tue, 23 Apr 2019 14:13:05 -0400 [thread overview]
Message-ID: <533f7721-2af6-1137-17c1-065837e1321d@google.com> (raw)
In-Reply-To: <20190422222647.48628-1-michael@platin.gs>
Hi Michael -
On 4/22/19 6:26 PM, michael@platin.gs wrote:
> + 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
next prev parent reply other threads:[~2019-04-23 18:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <[PATCH v6 0/6] blame: add the ability to ignore commits>
2019-04-22 22:26 ` [PATCH v6 0/6] blame: add the ability to ignore commits michael
2019-04-23 14:23 ` Barret Rhoden
2019-04-23 18:13 ` Barret Rhoden [this message]
2019-04-23 20:17 ` Barret Rhoden
2019-04-23 21:21 ` Barret Rhoden
2019-04-24 21:07 ` Barret Rhoden
2019-04-10 16:24 Barret Rhoden
2019-04-14 21:10 ` Michael Platings
2019-04-15 13:23 ` Barret Rhoden
2019-04-15 21:54 ` Michael Platings
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=533f7721-2af6-1137-17c1-065837e1321d@google.com \
--to=brho@google.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=dak@gnu.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=michael@platin.gs \
--cc=peff@peff.net \
--cc=stefanbeller@gmail.com \
--cc=whydoubt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).