{"thread":{"id":"35597","subject":"[RFC] blame: new option to better handle merged cherry-picks","startedAt":"2014-01-02T17:55:37Z","lastAt":"2014-01-02T21:48:38Z","messageCount":4,"participants":["Bernhard R. Link","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"232564","messageId":"20140102175529.GA4669@client.brlink.eu","threadId":"35597","inReplyTo":null,"subject":"[RFC] blame: new option to better handle merged cherry-picks","fromName":"Bernhard R. Link","fromEmail":"brl+git@mail.brlink.eu","sentAt":"2014-01-02T17:55:37Z","receivedAt":"2014-01-02T17:55:37Z","isPatch":false,"sender":{"key":"brl+git@mail.brlink.eu","avatar":null},"body":"Allows to disable the git blame optimization of assuming that if there is a\nparent of a merge commit that has the exactly same file content, then\nonly this parent is to be looked at.\n\nThis optimization, while being faster in the usual case, means that in\nthe case of cherry-picks the blamed commit depends on which other commits\ntouched a file.\n\nIf for example one commit A modified both files b and c. And there are\ncommits B and C, B only modifies file b and C only modifies file c\n(so that no conflicts happen), and assume A is cherry-picked as A'\nand the two branches then merged:\n\n--o-----B---A\n   \\         \\\n    ---C---A'--M---\n\nThen without this new option git blame blames the A|A' changes of\nfile b to A while blaming the changes of c to A'.\nWith the new option --no-parent-shortcut it blames both changes to the\nsame commit.\n\nSigned-off-by: Bernhard R. Link <brlink@debian.org>\n---\n Documentation/blame-options.txt | 6 ++++++\n builtin/blame.c                 | 5 ++++-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 0cebc4f..55dd12b 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -48,6 +48,12 @@ include::line-range-format.txt[]\n \tShow the result incrementally in a format designed for\n \tmachine consumption.\n \n+--no-parent-shortcut::\n+\tAlways look at all parents of a merge and do not shortcut\n+\tto the first parent with no changes to the file looked at.\n+\tThis takes more time but produces more reliable results\n+\tif branches with cherry-picked commits were merged.\n+\n --encoding=<encoding>::\n \tSpecifies the encoding used to output author names\n \tand commit summaries. Setting it to `none` makes blame\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4916eb2..dab2c36 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -45,6 +45,7 @@ static int incremental;\n static int xdl_opts;\n static int abbrev = -1;\n static int no_whole_file_rename;\n+static int no_parent_shortcut;\n \n static enum date_mode blame_date_mode = DATE_ISO8601;\n static size_t blame_date_width;\n@@ -1248,7 +1249,8 @@ static void pass_blame(struct scoreboard *sb, struct origin *origin, int opt)\n \t\t\tporigin = find(sb, p, origin);\n \t\t\tif (!porigin)\n \t\t\t\tcontinue;\n-\t\t\tif (!hashcmp(porigin->blob_sha1, origin->blob_sha1)) {\n+\t\t\tif (!no_parent_shortcut &&\n+\t\t\t    !hashcmp(porigin->blob_sha1, origin->blob_sha1)) {\n \t\t\t\tpass_whole_blame(sb, origin, porigin);\n \t\t\t\torigin_decref(porigin);\n \t\t\t\tgoto finish;\n@@ -2247,6 +2249,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tstatic const char *contents_from = NULL;\n \tstatic const struct option options[] = {\n \t\tOPT_BOOL(0, \"incremental\", &incremental, N_(\"Show blame entries as we find them, incrementally\")),\n+\t\tOPT_BOOL(0, \"no-parent-shortcut\", &no_parent_shortcut, N_(\"Don't take shortcuts in some merges but handle cherry-picks better\")),\n \t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n \t\tOPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\n \t\tOPT_BOOL(0, \"show-stats\", &show_stats, N_(\"Show work cost statistics\")),\n-- \n1.8.5.1\n"},{"id":"232574","messageId":"xmqqlhyyp1oo.fsf@gitster.dls.corp.google.com","threadId":"35597","inReplyTo":"20140102175529.GA4669@client.brlink.eu","subject":"Re: [RFC] blame: new option to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T20:29:43Z","receivedAt":"2014-01-02T20:29:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bernhard R. Link\" <brl+git@mail.brlink.eu> writes:\n\n> Allows to disable the git blame optimization of assuming that if there is a\n> parent of a merge commit that has the exactly same file content, then\n> only this parent is to be looked at.\n\nI think this is what we usually call --full-history in \"git log\"\nfamily, but more importantly, I do not think this is solving a valid\nproblem.\n\n> This optimization, while being faster in the usual case, means that in\n> the case of cherry-picks the blamed commit depends on which other commits\n> touched a file.\n>\n> If for example one commit A modified both files b and c. And there are\n> commits B and C, B only modifies file b and C only modifies file c\n> (so that no conflicts happen), and assume A is cherry-picked as A'\n> and the two branches then merged:\n>\n> --o-----B---A\n>    \\         \\\n>     ---C---A'--M---\n\nSo the contents of b at M is as the same as in A, so following 'b'\nwill see A and B changed that path, which is correct.\n\nThe contents of c at M is?  It is different from A because at A c\nlacks the change made to it at C.  The merged result at M would\nmatch C in A', no?  So following 'c' will see A' and C changed that\npath, no?\n\nSo what is wrong about it?  If the original history were like this\ninstead, and A' were a cherry-pick of A, then what should happen?\n\n> --o-----B---A'\n>    \\         \\\n>     ---C---A---M---\n\nDon't we want to see c blamed the same way?\n\nAlso, when handling a merge, we have to handle parents sequencially,\nchecking the difference between M with its first parent first, and\nthen passing blame for the remaining common lines to the remaining\nparents.  If you flip the order of parents of M when you merge A and\nA' in your original history, and with your patch, what would you\nsee when you blame c?  Wouldn't it notice that M:c is identical to c\nin its first parent (now A') and pass the whole blame to A' anyway\nwith or without your change?\n\n\n\n> Then without this new option git blame blames the A|A' changes of\n> file b to A while blaming the changes of c to A'.\n> With the new option --no-parent-shortcut it blames both changes to the\n> same commit.\n>\n> Signed-off-by: Bernhard R. Link <brlink@debian.org>\n> ---\n>  Documentation/blame-options.txt | 6 ++++++\n>  builtin/blame.c                 | 5 ++++-\n>  2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 0cebc4f..55dd12b 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -48,6 +48,12 @@ include::line-range-format.txt[]\n>  \tShow the result incrementally in a format designed for\n>  \tmachine consumption.\n>  \n> +--no-parent-shortcut::\n> +\tAlways look at all parents of a merge and do not shortcut\n> +\tto the first parent with no changes to the file looked at.\n> +\tThis takes more time but produces more reliable results\n> +\tif branches with cherry-picked commits were merged.\n> +\n>  --encoding=<encoding>::\n>  \tSpecifies the encoding used to output author names\n>  \tand commit summaries. Setting it to `none` makes blame\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 4916eb2..dab2c36 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -45,6 +45,7 @@ static int incremental;\n>  static int xdl_opts;\n>  static int abbrev = -1;\n>  static int no_whole_file_rename;\n> +static int no_parent_shortcut;\n>  \n>  static enum date_mode blame_date_mode = DATE_ISO8601;\n>  static size_t blame_date_width;\n> @@ -1248,7 +1249,8 @@ static void pass_blame(struct scoreboard *sb, struct origin *origin, int opt)\n>  \t\t\tporigin = find(sb, p, origin);\n>  \t\t\tif (!porigin)\n>  \t\t\t\tcontinue;\n> -\t\t\tif (!hashcmp(porigin->blob_sha1, origin->blob_sha1)) {\n> +\t\t\tif (!no_parent_shortcut &&\n> +\t\t\t    !hashcmp(porigin->blob_sha1, origin->blob_sha1)) {\n>  \t\t\t\tpass_whole_blame(sb, origin, porigin);\n>  \t\t\t\torigin_decref(porigin);\n>  \t\t\t\tgoto finish;\n> @@ -2247,6 +2249,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \tstatic const char *contents_from = NULL;\n>  \tstatic const struct option options[] = {\n>  \t\tOPT_BOOL(0, \"incremental\", &incremental, N_(\"Show blame entries as we find them, incrementally\")),\n> +\t\tOPT_BOOL(0, \"no-parent-shortcut\", &no_parent_shortcut, N_(\"Don't take shortcuts in some merges but handle cherry-picks better\")),\n>  \t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n>  \t\tOPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\n>  \t\tOPT_BOOL(0, \"show-stats\", &show_stats, N_(\"Show work cost statistics\")),\n"},{"id":"232587","messageId":"20140102211507.GA6323@client.brlink.eu","threadId":"35597","inReplyTo":"xmqqlhyyp1oo.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] blame: new option to better handle merged cherry-picks","fromName":"Bernhard R. Link","fromEmail":"brl+git@mail.brlink.eu","sentAt":"2014-01-02T21:15:07Z","receivedAt":"2014-01-02T21:15:07Z","isPatch":false,"sender":{"key":"brl+git@mail.brlink.eu","avatar":null},"body":"* Junio C Hamano <gitster@pobox.com> [140102 21:29]:\n> > This optimization, while being faster in the usual case, means that in\n> > the case of cherry-picks the blamed commit depends on which other commits\n> > touched a file.\n> >\n> > If for example one commit A modified both files b and c. And there are\n> > commits B and C, B only modifies file b and C only modifies file c\n> > (so that no conflicts happen), and assume A is cherry-picked as A'\n> > and the two branches then merged:\n> >\n> > --o-----B---A\n> >    \\         \\\n> >     ---C---A'--M---\n>\n> So the contents of b at M is as the same as in A, so following 'b'\n> will see A and B changed that path, which is correct.\n>\n> The contents of c at M is?  It is different from A because at A c\n> lacks the change made to it at C.  The merged result at M would\n> match C in A', no?  So following 'c' will see A' and C changed that\n> path, no?\n>\n> So what is wrong about it?\n\nIt's not wrong (that's why I do not suggest to change the default\nbehaviour), but it's inconsistent and can be a bit confusing to\nhave either the one or the other commit blamed depending on whether\nsome file was touched or not.\nThe history I'm a bit more concerned is something like (with ...\nbeing unrelated commits not touching B or C):\n\n --o-----...---A--...---B---...--\n    \\                            \\\n     ---...---A'--...---C---...---M---\n\n\nHere having B or C touching b or c determines which of A or A' is\nblamed for which part of the patch.\n\nIt's even enough to have:\n\n       --...---A'--...---B---...--\n      /                            \\\n ---o---...---A--................---M---\n\nTo have the A/A' changes of c to be attributed to A while the b changes\nare attributed to A'. I.e. you have a master branch that has commit A,\nwhich is also cherry-picked to some previously forked side-branch.\nOnce that side-branch is merged back, parts of the change are attributed\nto A' if they are in a file that is not touched otherwise in the main\nbranch.\n\n\n> Also, when handling a merge, we have to handle parents sequencially,\n> checking the difference between M with its first parent first, and\n> then passing blame for the remaining common lines to the remaining\n> parents.  If you flip the order of parents of M when you merge A and\n> A' in your original history, and with your patch, what would you\n> see when you blame c?  Wouldn't it notice that M:c is identical to c\n> in its first parent (now A') and pass the whole blame to A' anyway\n> with or without your change?\n\nWhen giving git-blame the new option introduced with my patch, only\nthe order of parents determines which commit is blamed. Without\nthe option (i.e. the currently only possible behaviour) which commit\nis blamed depends what else touches other parts of the file.\nIf both branches make modifications to the file (or if there is\nany merge conflict resolution in the merge) then the bahaviour with\nor without the option are the same.\n\nBut in the example with one commit B touching also b and one commit C\ntouching also c, there is (without the new option) always one part\nof the cherry-picked commit is blamed on the original and one on the\ncherry-picked, no matter how you order the parents.\n(While by having your mainline always the most leftward parent, with\nthe the new option you always get those commit blamed that is the\n\"first one this was introduced to mainline\".)\n\n\tBernhard R. Link\n-- \nF8AC 04D5 0B9B 064B 3383  C3DA AFFC 96D1 151D FFDC\n"},{"id":"232591","messageId":"xmqq4n5moy15.fsf@gitster.dls.corp.google.com","threadId":"35597","inReplyTo":"20140102211507.GA6323@client.brlink.eu","subject":"Re: [RFC] blame: new option to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T21:48:38Z","receivedAt":"2014-01-02T21:48:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bernhard R. Link\" <brl+git@mail.brlink.eu> writes:\n\n> When giving git-blame the new option introduced with my patch, only\n> the order of parents determines which commit is blamed. Without\n> the option (i.e. the currently only possible behaviour) which commit\n> is blamed depends what else touches other parts of the file.\n\nI am trying to figure out why that difference matters, in other\nwords, when using the new option is actually useful.  You give the\ncommand a scenario that can be solved in two equally valid ways\n(blaming to either A or A' is equally valid), and sometimes the\ncommand gives the identical result with or without the new option,\nand some other times the user gets a different but an equally valid\nresult (but after traversing more history spending more cycles).  I\nam not sure what problem the new option solves.  I am trying to come\nup with an easy-to-understand explanation to the end users: \"If you\nwant to see blame's result with the property X, use this option---it\nmay have to spend extra cycles, but the property X is so desirable\nthat it may be worth it\".  And I am having a hard time understanding\nwhat that X is.\n\n> But in the example with one commit B touching also b and one commit C\n> touching also c, there is (without the new option) always one part\n> of the cherry-picked commit is blamed on the original and one on the\n> cherry-picked, no matter how you order the parents.\n\nYeah, the cherry-picked one will introduce the same change as the\none that was cherry-picked, so if you look at the end result and ask\n\"where did _this_ line come from?\", there are two equally plausible\ncandidates, as \"blame\" output can give only one answer to each line.\nI still do not see why the one that is picked with the new option is\nbetter.  At best, it looks to me that it is saying \"running with\nthis option may (or may not) give a different answer, so run the\ncommand with and without it and see which one you like\", which does\nnot sound too useful to the end users.  That is where my confusion\ncomes from.\n\n> (While by having your mainline always the most leftward parent, with\n> the the new option you always get those commit blamed that is the\n> \"first one this was introduced to mainline\".)\n\nYes, I vaguely recall we talked about adding --first-parent option\nto the command in the past.  I do not remember what came out of that\ndiscussion.\n"}]}