Re: [RFC] blame: new option to better handle...

Re: [RFC] blame: new option to better handle merged cherry-picks

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:32

"Bernhard R. Link" [off-list ref] writes:

Allows to disable the git blame optimization of assuming that if there is a
parent of a merge commit that has the exactly same file content, then
only this parent is to be looked at.

I think this is what we usually call --full-history in "git log"
family, but more importantly, I do not think this is solving a valid
problem.

This optimization, while being faster in the usual case, means that in
the case of cherry-picks the blamed commit depends on which other commits
touched a file.

If for example one commit A modified both files b and c. And there are
commits B and C, B only modifies file b and C only modifies file c
(so that no conflicts happen), and assume A is cherry-picked as A'
and the two branches then merged:

--o-----B---A
   \         \
    ---C---A'--M---

So the contents of b at M is as the same as in A, so following 'b'
will see A and B changed that path, which is correct.

The contents of c at M is?  It is different from A because at A c
lacks the change made to it at C.  The merged result at M would
match C in A', no?  So following 'c' will see A' and C changed that
path, no?

So what is wrong about it?  If the original history were like this
instead, and A' were a cherry-pick of A, then what should happen?

--o-----B---A'
   \         \
    ---C---A---M---

Don't we want to see c blamed the same way?

Also, when handling a merge, we have to handle parents sequencially,
checking the difference between M with its first parent first, and
then passing blame for the remaining common lines to the remaining
parents.  If you flip the order of parents of M when you merge A and
A' in your original history, and with your patch, what would you
see when you blame c?  Wouldn't it notice that M:c is identical to c
in its first parent (now A') and pass the whole blame to A' anyway
with or without your change?

quoted hunk

Then without this new option git blame blames the A|A' changes of
file b to A while blaming the changes of c to A'.
With the new option --no-parent-shortcut it blames both changes to the
same commit.

Signed-off-by: Bernhard R. Link <redacted>
---
 Documentation/blame-options.txt | 6 ++++++
 builtin/blame.c                 | 5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 0cebc4f..55dd12b 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt

@@ -48,6 +48,12 @@ include::line-range-format.txt[]
 	Show the result incrementally in a format designed for
 	machine consumption.
 
+--no-parent-shortcut::
+	Always look at all parents of a merge and do not shortcut
+	to the first parent with no changes to the file looked at.
+	This takes more time but produces more reliable results
+	if branches with cherry-picked commits were merged.
+
 --encoding=<encoding>::
 	Specifies the encoding used to output author names
 	and commit summaries. Setting it to `none` makes blame

diff --git a/builtin/blame.c b/builtin/blame.c
index 4916eb2..dab2c36 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -45,6 +45,7 @@ static int incremental;
 static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
+static int no_parent_shortcut;
 
 static enum date_mode blame_date_mode = DATE_ISO8601;
 static size_t blame_date_width;

@@ -1248,7 +1249,8 @@ static void pass_blame(struct scoreboard *sb, struct origin *origin, int opt)
 			porigin = find(sb, p, origin);
 			if (!porigin)
 				continue;
-			if (!hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
+			if (!no_parent_shortcut &&
+			    !hashcmp(porigin->blob_sha1, origin->blob_sha1)) {
 				pass_whole_blame(sb, origin, porigin);
 				origin_decref(porigin);
 				goto finish;

@@ -2247,6 +2249,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	static const char *contents_from = NULL;
 	static const struct option options[] = {
 		OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")),
+		OPT_BOOL(0, "no-parent-shortcut", &no_parent_shortcut, N_("Don't take shortcuts in some merges but handle cherry-picks better")),
 		OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
 		OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),
 		OPT_BOOL(0, "show-stats", &show_stats, N_("Show work cost statistics")),

Re: [RFC] blame: new option to better handle merged cherry-picks

From: Bernhard R. Link <hidden>
Date: 2016-06-15 22:59:32

* Junio C Hamano [off-list ref] [140102 21:29]:

quoted

This optimization, while being faster in the usual case, means that in
the case of cherry-picks the blamed commit depends on which other commits
touched a file.

If for example one commit A modified both files b and c. And there are
commits B and C, B only modifies file b and C only modifies file c
(so that no conflicts happen), and assume A is cherry-picked as A'
and the two branches then merged:

--o-----B---A
   \         \
    ---C---A'--M---

So the contents of b at M is as the same as in A, so following 'b'
will see A and B changed that path, which is correct.

The contents of c at M is?  It is different from A because at A c
lacks the change made to it at C.  The merged result at M would
match C in A', no?  So following 'c' will see A' and C changed that
path, no?

So what is wrong about it?

It's not wrong (that's why I do not suggest to change the default
behaviour), but it's inconsistent and can be a bit confusing to
have either the one or the other commit blamed depending on whether
some file was touched or not.
The history I'm a bit more concerned is something like (with ...
being unrelated commits not touching B or C):

 --o-----...---A--...---B---...--
    \                            \
     ---...---A'--...---C---...---M---


Here having B or C touching b or c determines which of A or A' is
blamed for which part of the patch.

It's even enough to have:

       --...---A'--...---B---...--
      /                            \
 ---o---...---A--................---M---

To have the A/A' changes of c to be attributed to A while the b changes
are attributed to A'. I.e. you have a master branch that has commit A,
which is also cherry-picked to some previously forked side-branch.
Once that side-branch is merged back, parts of the change are attributed
to A' if they are in a file that is not touched otherwise in the main
branch.

Also, when handling a merge, we have to handle parents sequencially,
checking the difference between M with its first parent first, and
then passing blame for the remaining common lines to the remaining
parents.  If you flip the order of parents of M when you merge A and
A' in your original history, and with your patch, what would you
see when you blame c?  Wouldn't it notice that M:c is identical to c
in its first parent (now A') and pass the whole blame to A' anyway
with or without your change?

When giving git-blame the new option introduced with my patch, only
the order of parents determines which commit is blamed. Without
the option (i.e. the currently only possible behaviour) which commit
is blamed depends what else touches other parts of the file.
If both branches make modifications to the file (or if there is
any merge conflict resolution in the merge) then the bahaviour with
or without the option are the same.

But in the example with one commit B touching also b and one commit C
touching also c, there is (without the new option) always one part
of the cherry-picked commit is blamed on the original and one on the
cherry-picked, no matter how you order the parents.
(While by having your mainline always the most leftward parent, with
the the new option you always get those commit blamed that is the
"first one this was introduced to mainline".)

	Bernhard R. Link
-- 
F8AC 04D5 0B9B 064B 3383  C3DA AFFC 96D1 151D FFDC

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help