{"thread":{"id":"35646","subject":"[RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","startedAt":"2014-01-13T06:30:25Z","lastAt":"2014-01-14T19:12:21Z","messageCount":7,"participants":["Bernhard R. Link","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233048","messageId":"20140113063008.GA3072@client.brlink.eu","threadId":"35646","inReplyTo":null,"subject":"[RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Bernhard R. Link","fromEmail":"brlink@debian.org","sentAt":"2014-01-13T06:30:25Z","receivedAt":"2014-01-13T06:30:25Z","isPatch":false,"sender":{"key":"brlink@debian.org","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 --prefer-first it blames both changes to the\nsame commit and to the one more on the \"left\" side of the graph.\n\nSigned-off-by: Bernhard R. Link \n---\n Documentation/blame-options.txt | 6 ++++++\n builtin/blame.c | 7 +++++--\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\n Differences to first round: rename option and describe the effect\n instead of the implementation in documentation.\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 0cebc4f..b2e7fb8 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+--prefer-first::\n+\tIf a line was introduced by two commits (for example via\n+\ta merged cherry-pick), prefer the commit that was\n+\tfirst merged in the history of always following the\n+\tfirst parent.\n+\n --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..8ea34cf 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 prefer_first;\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 (!prefer_first &&\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,7 +2249,8 @@ 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('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n+\t\tOPT_BOOL(0, \"prefer-first\", &prefer_first, N_(\"Prefer blaming commits merged earlier\")),\n+\t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: ff)\")),\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 \t\tOPT_BIT(0, \"score-debug\", &output_option, N_(\"Show output score for blame entries\"), OUTPUT_SHOW_SCORE),\n-- \n1.8.5.1\n\n\tBernhard R. Link\n-- \nF8AC 04D5 0B9B 064B 3383 C3DA AFFC 96D1 151D FFDC\n"},{"id":"233067","messageId":"xmqqfvor5xil.fsf@gitster.dls.corp.google.com","threadId":"35646","inReplyTo":"20140113063008.GA3072@client.brlink.eu","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-13T22:26:26Z","receivedAt":"2014-01-13T22:26:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bernhard R. Link\" 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>\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> 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 --prefer-first it blames both changes to the\n> same commit and to the one more on the \"left\" side of the graph.\n>\n> Signed-off-by: Bernhard R. Link \n> ---\n> Documentation/blame-options.txt | 6 ++++++\n> builtin/blame.c | 7 +++++--\n> 2 files changed, 11 insertions(+), 2 deletions(-)\n>\n> Differences to first round: rename option and describe the effect\n> instead of the implementation in documentation.\n\nI read the updated documentation three times but it still does not\nanswer any of my questions I had in $gmane/239888, the most\nimportant part of which was:\n\n Yeah, the cherry-picked one will introduce the same change as\n the one that was cherry-picked, so if you look at the end result\n and ask \"where did _this_ line come from?\", there are two\n equally plausible candidates, as \"blame\" output can give only\n one answer to each line. I still do not see why the one that is\n picked with the new option is better. At best, it looks to me\n that it is saying \"running with this option may (or may not)\n give a different answer, so run the command with and without it\n and see which one you like\", which does not sound too useful to\n the end users.\n\nTo put it another way, why/when would an end user choose to use this\noption? If the result of using this option is always better than\nwithout, why/when would an end user choose not to use this option?\n\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index 0cebc4f..b2e7fb8 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> +--prefer-first::\n> +\tIf a line was introduced by two commits (for example via\n> +\ta merged cherry-pick), prefer the commit that was\n> +\tfirst merged in the history of always following the\n> +\tfirst parent.\n> +\n> --encoding=::\n> \tSpecifies the encoding used to output author names\n> \tand commit summaries. Setting it to `none` makes blame\n"},{"id":"233068","messageId":"20140113225229.GA3418@client.brlink.eu","threadId":"35646","inReplyTo":"xmqqfvor5xil.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Bernhard R. Link","fromEmail":"brl+git@mail.brlink.eu","sentAt":"2014-01-13T22:52:29Z","receivedAt":"2014-01-13T22:52:29Z","isPatch":false,"sender":{"key":"brl+git@mail.brlink.eu","avatar":null},"body":"* Junio C Hamano [140113 23:31]:\n> I read the updated documentation three times but it still does not\n> answer any of my questions I had in $gmane/239888, the most\n> important part of which was:\n>\n> Yeah, the cherry-picked one will introduce the same change as\n> the one that was cherry-picked, so if you look at the end result\n> and ask \"where did _this_ line come from?\", there are two\n> equally plausible candidates, as \"blame\" output can give only\n> one answer to each line. I still do not see why the one that is\n> picked with the new option is better.\n\nBecause:\n - it will blame the modifications of merged cherry-picked commit\n to only one commit. Without the option parts of the modification\n will be reported as coming from the one, parts will be reported\n to be from the other. With the option only one of those two commits\n is reported as the origin at the same time and not both.\n - it is more predictable which commit is blamed, so if one is\n interested in where some commit was introduced first into a\n \"mainline\", one gets this information, and not somtimes a different\n one due to unrelated reasons.\n\n> To put it another way, why/when would an end user choose to use this\n> option? If the result of using this option is always better than\n> without, why/when would an end user choose not to use this option?\n\nWhile the result is more consistent and more predictable in the case\nof merged cherry picks, it is also slower in every case. Usually speed\nwill be more important than this exactness, especially as the result\nwill not differ for the common case (if there are no cherry-picked\ncommits merged or when those commits do not touch any files that are\notherwise only modified in the merged branch).\n\n\tBernhard R. Link\n-- \nF8AC 04D5 0B9B 064B 3383 C3DA AFFC 96D1 151D FFDC\n"},{"id":"233069","messageId":"xmqqbnzf5vvu.fsf@gitster.dls.corp.google.com","threadId":"35646","inReplyTo":"20140113225229.GA3418@client.brlink.eu","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-13T23:01:41Z","receivedAt":"2014-01-13T23:01:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bernhard R. Link\" writes:\n\n> * Junio C Hamano [140113 23:31]:\n>> I read the updated documentation three times but it still does not\n>> answer any of my questions I had in $gmane/239888, the most\n>> important part of which was:\n>>\n>> Yeah, the cherry-picked one will introduce the same change as\n>> the one that was cherry-picked, so if you look at the end result\n>> and ask \"where did _this_ line come from?\", there are two\n>> equally plausible candidates, as \"blame\" output can give only\n>> one answer to each line. I still do not see why the one that is\n>> picked with the new option is better.\n>\n> Because:\n> - it will blame the modifications of merged cherry-picked commit\n> to only one commit. Without the option parts of the modification\n> will be reported as coming from the one, parts will be reported\n> to be from the other. With the option only one of those two commits\n> is reported as the origin at the same time and not both.\n> - it is more predictable which commit is blamed, so if one is\n> interested in where some commit was introduced first into a\n> \"mainline\", one gets this information, and not somtimes a different\n> one due to unrelated reasons.\n>\n>> To put it another way, why/when would an end user choose to use this\n>> option? If the result of using this option is always better than\n>> without, why/when would an end user choose not to use this option?\n>\n> While the result is more consistent and more predictable in the case\n> of merged cherry picks, it is also slower in every case.\n\nConsistent and predictable, perhaps, but I am not sure \"exact\" would\nbe a good word.\n\nWouldn't the result depend on which way the cherry pick went, and\nthen the later merge went? In the particular topology you depicted\nin the log message, the end result may happen to point at the same\ncommit for these two paths, but I am not sure how the change\nguarantees that \"we always point at the same original commit not the\ncherry-picked one\", which was implied by the log message, if your\ncherry-pick and merge went in different direction in similar\ntopologies.\n\nAnd that is why I said:\n\n At best, it looks to me that it is saying \"running with this\n option may (or may not) give a different answer, so run the\n command with and without it and see which one you like\"\n\nWith the stress on \"different\" answer; it the change were \"with the\noption the result is always better, albeit you will have to wait\nlonger\", I would not have this much trouble accepting the change,\nthough.\n"},{"id":"233073","messageId":"xmqq7ga35qdd.fsf@gitster.dls.corp.google.com","threadId":"35646","inReplyTo":"xmqqbnzf5vvu.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-14T01:00:46Z","receivedAt":"2014-01-14T01:00:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> While the result is more consistent and more predictable in the case\n>> of merged cherry picks, it is also slower in every case.\n>\n> Consistent and predictable, perhaps, but I am not sure \"exact\" would\n> be a good word.\n\nAnother thing I am not enthusiasitc about this change is that I am\nafraid that this may make \"git blame -- path\" and \"git log -- path\"\nwork inconsistenly. The both cull side branches whenever one of the\nparents gave the resulting blob, even that parent is not the first\none. But \"git blame --prefer-first -- path\", afaict, behaves quite\ndifferently from \"git log --first-parent -- path\", even though they\nshare similar option names, adding more to the confusion.\n"},{"id":"233083","messageId":"7va9ez0xji.fsf@alter.siamese.dyndns.org","threadId":"35646","inReplyTo":"xmqq7ga35qdd.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-14T08:37:05Z","receivedAt":"2014-01-14T08:37:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>> While the result is more consistent and more predictable in the case\n>>> of merged cherry picks, it is also slower in every case.\n>>\n>> Consistent and predictable, perhaps, but I am not sure \"exact\" would\n>> be a good word.\n>\n> Another thing I am not enthusiasitc about this change is that I am\n> afraid that this may make \"git blame -- path\" and \"git log -- path\"\n> work inconsistenly. The both cull side branches whenever one of the\n> parents gave the resulting blob, even that parent is not the first\n> one. But \"git blame --prefer-first -- path\", afaict, behaves quite\n> differently from \"git log --first-parent -- path\", even though they\n> share similar option names, adding more to the confusion.\n\nI think I am starting to understand why this patch felt wrong to me.\nThis wasn't about \"--first-parent\" at all, and you are correct that\nyou didn't call the option as such), but I somehow thought that they\nwere related; perhaps the fact that both disable the \"if the result\nexactly matches one parent, all the other parents can be culled to\nsimplify the history\" logic blinded me.\n\nIn reality, the new flag is a lot closer in spirit to the total\nopposite of \"--first-parent\", i.e. \"--full-history\". That option\nalso disables that \"if same to one parent, other parents do not\nmatter\" logic, but its effect is quite different. It makes the\nother histories that did not have to have contributed the end result\nshown in the output.\n\nNow, when we step back and think about how the normal \"git blame\"\nlogic apportions the blame to multiple parents when there is no\nexact match, it does so in a pretty arbitrary way. It lets earlier\nparents to claim the responsibility and later parents only get\nleftover contents that weren't claimed by the earlier ones. We can\ncall that \"favouring earler ones\", i.e. \"--prefer-first\".\n\nIt was implemented this way, not because this order makes any sense,\nbut primarily because no order is particularly better than any\nother, and the designer (me) happened to have picked the easiest one\nat random.\n\nThe \"pick the one that exactly matches if exists\" can be thought of\nan easy hack to hide the problems that come from this arbitrary\nchoice. Without it, if the result matches the second parent (i.e. a\ntypical merge of a work done on the topic branch while the mainline\nhas been quiescent in the same area), the \"give earlier parents a\nchance to claim responsiblity before later ones\" rule would have\nsplit the blame for parts that weren't changed in the side branch\ntopic to the mainline and blame would have been passed to the side\nbranch only for the portion that were changed by the side branch.\nInstead, \"pass the whole blame to the one that exactly matches\" hack\nkeeps larger blocks of text unsplit, clumping related contents\ntogether as long as possible while we traverse the history.\n\nIt is an \"easy hack\", because we only need to compare the object\nname, but a logical extension to it would have been to compute the\nsimilarity scores between the result and each of the parents, sort\nthe parents by that similarity score order, and give more similar\nones a chance to claim responsibility before less similar ones.\nWe could call it \"favouring similar ones\", i.e. \"--prefer-similar\"\nor something.\n\nThat would have made the result more stable. Imagine that in one\nhistory, a merge's result matchs exactly the second parent, and in\nanother history, a merge's result almost matches exactly the second\nparent but the difference is the result adds one blank line at the\nend of the file relative to what the second parent has.\n\nWith the current code, blaming the file will get quite a different\nresult.\n\nIn the former history, the sub-history leading to the second parent\nof the merge will get all the blame, but in the latter history, the\nsub-history leading to the first parent of the merge will have a\nchance to claim the responsibility for the shared part before the\nsecond parent has a say in the output.\n\nIf we sorted the parents in the similarity order and gave the first\nrefusal right to more similar parents before less similar ones, then\nthe resulting output from \"git blame\" would be very similar in these\ntwo histories, which would be a very desirable property. If the\nonly difference between the results of the merge in the former and\nthe latter histories is one blank line at the end of the file in\nquestion, blames for the remaining part of the file should be\nassigned the same between the two histories, but the \"pass the\nentire blame to the second parent only when the second parent\nexactly matches\" hack gets in the way for that ideal, and \"sort the\nparents in similarity order\" will fix that.\n\nOf course, it would make the computation a lot more costly, but it\nwould make the behaviour more predictable and understandable.\n\nBut that is a different tangent.\n\nI think the new feature introduced by your change can be explained\nas \"'git blame' uses the same history simplification as the commands\nin the 'git log' family that culls other side branches when the\nmerge result exactly matches one parent, and in all other cases, it\nlets earlier parents claim responsibility before the later ones.\nThis option disables the culling of the irrelevant side branches, in\na way similar to how '--full-history' option to the commands in the\n'git log' family works, and lets earlier parents claim\nresponsibility to the merge result (even when the later parents\ncontributed a lot more to the result) before the later parents.\".\n\nAnd if it were sold that way, I think I could at least understand it\n(I do not necessarily buy it as a useful feature, though---at least\nnot yet).\n\nIn any case, \"--prefer-first\" is not particularly a good name, as\nthat is the default mode of operation for \"git blame\". If we were\never going to implement the \"sort parents by similarity\", that would\nbe triggered with \"--prefer-similar\" and \"--prefer-first\" would\nbecomeq a way to choose the current algorithm (i.e. not sort the\nparents by similarity but go from earlier to later parents). We\nwould regret if we gave that option name to the feature proposed by\nthe patch under discussion. How about calling it \"--full-history\",\nwhich is a way to tell Git not to cull side branches when the result\nmatches one of the parents? It is even plausible that we may later\ncome up with \"--prefer-\" (sort the parents not in the\noriginal parent order nor in the similarity order but with some\nother heuristics), and I suspect \"--full-history\" would be an\northogonal axis to the order in which the parents are given a chance\nto claim responsiblity.\n\nThanks; I'll queue the patch on 'pu' and wait for others to comment.\n"},{"id":"233098","messageId":"xmqqr48a4bu2.fsf@gitster.dls.corp.google.com","threadId":"35646","inReplyTo":"7va9ez0xji.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC v2] blame: new option --prefer-first to better handle merged cherry-picks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-14T19:12:21Z","receivedAt":"2014-01-14T19:12:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The \"pick the one that exactly matches if exists\" can be thought of\n> an easy hack to hide the problems that come from this arbitrary\n> choice. ...\n> Instead, \"pass the whole blame to the one that exactly matches\" hack\n> keeps larger blocks of text unsplit, clumping related contents\n> together as long as possible while we traverse the history.\n>\n> It is an \"easy hack\", because we only need to compare the object\n> name, but a logical extension to it would have been to compute the\n> similarity scores between the result and each of the parents, sort\n> the parents by that similarity score order, and give more similar\n> ones a chance to claim responsibility before less similar ones.\n> We could call it \"favouring similar ones\", i.e. \"--prefer-similar\"\n> or something.\n\nExtending along the tangent further.\n\nAnother thing that I found the argument in the proposed log message\nof the patch weak was that the claim that changed code will assign\nthe blame to the \"same\" commit for both path b and c. There are two\nreasons why. One is that we do not look at b while chasing the\nancestry of c, so if a different traverse order assigns the blame to\nthe same commit for them, it is a mere happenstance. But a more\nimportant reason is that the changed code will still assign the\nblame for \"different\" commits if the final merge were made in the\nopposite direction. In your original topology, we skip over the\nfirst parent and give the whole blame to the second parent without\nthe change, and with the change, we stop doing so and instead give\nsome blame to the first parent and then allow the second parent a\nchance to claim the blame for the remainder. But in a history where\nthe final merge went in the opposite direction, even with the\nchange, we compare with the \"first\" parent (which was the \"second\"\none in your original topology) with the result, find out that the\ncontents exactly match, and that parent grabs the whole blame. So\nin that sense, the updated code that \"consistently\" gives earlier\nparents chance to claim the blame before later ones does not behave\nconsitently on the same history with different merge parent order.\n\nThat makes me think that the reason why the result you got with the\nchange is better (assuming it is better) is _not_ because the\nupdated code lets earlier parents give chance to claim the blame; it\ncould be an indication that the \"keep larger blocks of text unsplit,\nclumping related contents together as long as possible\" heuristics\nis what prevents us from having a better result.\n\nIf that is really the case, that would mean that letting the blame\nsplit early would give us a better result. I alluded to \"give more\nsimilar parents first chance to claim responsibility before less\nsimilar ones\" in the previous message, but perhaps this is\nindicating that we might get a better result if we did the\nopposite---instead of assigning blames to earlier parents and then\nto later ones, compare the result with each parent, order the\nparents by how few lines of blame they could claim if each of them\nwere allowed to go first, and then actually compute and assign the\nblame in that order, \"favouring dissimilar ones\". That may produce\nthe result you are after in a more consistent way, regardless of the\nmerge order.\n\nI think I've done thinking about this issue, at least for now.\n"}]}