git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Platings <michael@platin.gs>
To: Barret Rhoden <brho@google.com>
Cc: "Git mailing list" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"David Kastrup" <dak@gnu.org>, "Jeff King" <peff@peff.net>,
	"Jeff Smith" <whydoubt@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Junio C Hamano" <gitster@pobox.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Stefan Beller" <stefanbeller@gmail.com>
Subject: Re: [PATCH v6 0/6] blame: add the ability to ignore commits
Date: Sun, 14 Apr 2019 22:10:30 +0100	[thread overview]
Message-ID: <CAJDYR9SL9JCJjdARejV=NCf9GYn72=bfszXx84iDc416sZm31A@mail.gmail.com> (raw)
In-Reply-To: <20190410162409.117264-1-brho@google.com>

Hi Barret,

This works pretty well for the typical reformatting use case now. I've
run it over every commit of every .c file in the git project root,
both forwards and backwards with every combination of -w/-M/-C and
can't get it to crash so I think it's good in that respect.

However, it can still attribute lines to the wrong parent line. See
https://pypi.org/project/autopep8/#usage for an example reformatting
that it gets a bit confused on. The patch I submitted handles this
case correctly because it uses information about the more similar
lines to decide how more ambiguous lines should be matched.

You also gave an example of:

        commit-a 11) void new_func_1(void *x, void *y);
        commit-b 12) void new_func_2(void *x, void *y);

Being reformatted to:

        commit-a 11) void new_func_1(void *x,
        commit-b 12)                 void *y);
        commit-b 13) void new_func_2(void *x,
        commit-b 14)                 void *y);

The patch I submitted handles this case correctly, assigning line 12
to commit-a because it scales the parent line numbers according to the
relative diff chunk sizes instead of assuming a 1-1 mapping.

So I do ask that you incorporate more of my patch, including the test
code. It is more complex but I hope this demonstrates that there are
reasons for that. Happy to provide more examples or explanation if it
would help. On the other hand if you have examples where it falls
short then I'd be interested to know.

The other major use case that I'm interested in is renaming. In this
case, the git-hyper-blame approach of mapping line numbers 1-1 works
perfectly. Here's an example. Before:

        commit-a 11) Position MyClass::location(Offset O) {
        commit-b 12)    return P + O;
        commit-c 13) }

After:

        commit-a 11) Position MyClass::location(Offset offset) {
        commit-a 12)    return position + offset;
        commit-c 13) }

With the fuzzy matching, line 12 gets incorrectly matched to parent
line 11 because the similarity of "position" and "offset" outweighs
the similarity of "return". I'm considering adding even more
complexity to my patch such that parts of a line that have already
been matched can't be matched again by other lines.

But the other possibility is that we let the user choose the
heuristic. For a commit where they know that line numbers haven't
changed they could choose 1-1 matching, while for a reformatting
commit they could use fuzzy matching. I welcome your thoughts.

-Michael

  parent reply	other threads:[~2019-04-14 21:10 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-10 16:24 [PATCH v6 0/6] blame: add the ability to ignore commits Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 1/6] Move init_skiplist() outside of fsck Barret Rhoden
2019-04-10 19:04   ` Ævar Arnfjörð Bjarmason
2019-04-15 13:32     ` Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 2/6] blame: use a helper function in blame_chunk() Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 3/6] blame: add the ability to ignore commits and their changes Barret Rhoden
2019-04-10 19:00   ` Ævar Arnfjörð Bjarmason
2019-04-14 10:42     ` Michael Platings
2019-04-15 13:32       ` Barret Rhoden
2019-04-15 13:34     ` Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 4/6] blame: add config options to handle output for ignored lines Barret Rhoden
2019-04-14  3:45   ` Junio C Hamano
2019-04-14 10:09     ` Michael Platings
2019-04-14 10:24       ` Junio C Hamano
2019-04-14 11:27         ` Michael Platings
2019-04-15 13:51           ` Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 5/6] blame: optionally track line fingerprints during fill_blame_origin() Barret Rhoden
2019-04-10 16:24 ` [PATCH v6 6/6] blame: use a fingerprint heuristic to match ignored lines Barret Rhoden
2019-04-14  3:54   ` Junio C Hamano
2019-04-14  9:41     ` Michael Platings
2019-04-15 14:03     ` Barret Rhoden
2019-04-16  4:10       ` Junio C Hamano
2019-04-14 21:10 ` Michael Platings [this message]
2019-04-15 13:23   ` [PATCH v6 0/6] blame: add the ability to ignore commits Barret Rhoden
2019-04-15 21:54     ` Michael Platings
     [not found] <[PATCH v6 0/6] blame: add the ability to ignore commits>
2019-04-22 22:26 ` michael
2019-04-23 14:23   ` Barret Rhoden
2019-04-23 18:13   ` Barret Rhoden
2019-04-23 20:17   ` Barret Rhoden
2019-04-23 21:21   ` Barret Rhoden
2019-04-24 21:07   ` Barret Rhoden

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='CAJDYR9SL9JCJjdARejV=NCf9GYn72=bfszXx84iDc416sZm31A@mail.gmail.com' \
    --to=michael@platin.gs \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=brho@google.com \
    --cc=dak@gnu.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --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).