{"thread":{"id":"27499","subject":"blame --reverse selecting wrong commit","startedAt":"2011-05-30T02:21:33Z","lastAt":"2011-05-30T20:01:25Z","messageCount":6,"participants":["Shawn Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168960","messageId":"BANLkTikuFEg686VH_RojEe-zmBZRBOx6eA@mail.gmail.com","threadId":"27499","inReplyTo":null,"subject":"blame --reverse selecting wrong commit","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-30T02:21:33Z","receivedAt":"2011-05-30T02:21:33Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Although blame shows Stefan Lay removed the block in commit 05fa1713,\nthis isn't what happened. It was actually removed in commit 2302a6d3\nby Christian Halstrick. It looks like blame gets confused around this\nsection of the JGit history.\n\nRepository URL: git://egit.eclipse.jgit/jgit.git\n\n$ git blame -L 1080, --reverse 40fa75feb..master --\norg.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java\n283a60d1 (Shawn O. Pearce 2011-05-26 17:25:59 -0700 1080) }\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1081)\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1082) /**\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1083) *\nKick the timestamp of a local file.\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1084) *

\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1085) * We\nshouldn't have to make these method calls. The cache is using file\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1086) *\nsystem timestamps, and on many systems unit tests run faster than the\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1087) *\nmodification clock. Dumping the cache after we make an edit behind\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1088) *\nRefDirectory's back allows the tests to pass.\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1089) *\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1090) * @param name\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1091) *\n the file in the repository to force a time change on.\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1092) */\n05fa1713 (Stefan Lay 2011-05-24 01:38:59 -0700 1093)\nprivate void BUG_WorkAroundRacyGitIssues(String name) {\n\n\n-- \nShawn.\n"},{"id":"168961","messageId":"BANLkTimAij28ibW3DV2HaHPK5z0D-CrDHQ@mail.gmail.com","threadId":"27499","inReplyTo":"BANLkTikuFEg686VH_RojEe-zmBZRBOx6eA@mail.gmail.com","subject":"Re: blame --reverse selecting wrong commit","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-30T02:33:07Z","receivedAt":"2011-05-30T02:33:07Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 29, 2011 at 19:21, Shawn Pearce wrote:\n> Although blame shows Stefan Lay removed the block in commit 05fa1713,\n> this isn't what happened. It was actually removed in commit 2302a6d3\n> by Christian Halstrick. It looks like blame gets confused around this\n> section of the JGit history.\n>\n> Repository URL:  git://egit.eclipse.jgit/jgit.git\n>\n> $ git blame -L 1080, --reverse 40fa75feb..master --\n> org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java\n\nActually, you can use a narrower history range of 16e810b2..aa05559.\n\nI found this glitch while working on the new JGit implementation of\nreverse blame. It just so happens you and I both took the same rule on\na merge commit, follow the parent that has exactly the blob of the\nmerge itself... passing all blame onto it.\n\nExcept that in reverse mode this isn't true. One of the \"parents\" is\nthe descendent that deleted this method, so of course its not\nidentical content. The other parent is a different side-branch that\ndidn't touch the file at all. Later blame finds these branches merge\ntogether, and lays the blame on the wrong side.\n\nIt seems that we may want to avoid \"parents\" that have identical file\ncontent when in reverse mode if there is another parent that has\ndifferent content for the current target path.\n\n-- \nShawn.\n"},{"id":"168964","messageId":"BANLkTimXppE9sCW8tMntbD8p5-bRVFLHmQ@mail.gmail.com","threadId":"27499","inReplyTo":"BANLkTimAij28ibW3DV2HaHPK5z0D-CrDHQ@mail.gmail.com","subject":"Re: blame --reverse selecting wrong commit","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-30T03:11:19Z","receivedAt":"2011-05-30T03:11:19Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 29, 2011 at 19:33, Shawn Pearce wrote:\n> On Sun, May 29, 2011 at 19:21, Shawn Pearce wrote:\n>> Although blame shows Stefan Lay removed the block in commit 05fa1713,\n>> this isn't what happened. It was actually removed in commit 2302a6d3\n>> by Christian Halstrick. It looks like blame gets confused around this\n>> section of the JGit history.\n\nOh. Re-reading the man page for git blame helps. It says blame shows\nthe last revision that a line exists in, rather than the revision that\nremoved the line. IMHO, I expected reverse to show me the revision\nthat deleted (or replaced) that line, so I can inspect the commit\nmessage and the contents of the patch. Showing me one of the potential\nparents of that revision seems to be nearly useless.\n\nRereading commit 85af7929ee (\"git-blame --reverse\"), it seems you left\nthis an \"exercise for the reader\"... and in the past 3 years, no\nreader has stepped forward to implement the exercise as a patch to\nblame. *sigh*\n\n-- \nShawn.\n"},{"id":"168977","messageId":"7vlixoofaj.fsf@alter.siamese.dyndns.org","threadId":"27499","inReplyTo":"BANLkTimXppE9sCW8tMntbD8p5-bRVFLHmQ@mail.gmail.com","subject":"Re: blame --reverse selecting wrong commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-30T06:47:48Z","receivedAt":"2011-05-30T06:47:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> Rereading commit 85af7929ee (\"git-blame --reverse\"), it seems you left\n> this an \"exercise for the reader\"... and in the past 3 years, no\n> reader has stepped forward to implement the exercise as a patch to\n> blame. *sigh*\n\nYeah.\n\nIt has always been my opinion that asking for \"one commit past the blamed\none\" is a undefined request (after all, blame for the line fell on that\ncommit exactly why the next commit does _not_ have any corresponding\nline), so that is why I punted there.\n\nNow we seem to have found one interested reader, eh? ;-)\n"},{"id":"169014","messageId":"BANLkTikvmQgW=x0GYY6Yu=BnOgG+RXOCug@mail.gmail.com","threadId":"27499","inReplyTo":"7vlixoofaj.fsf@alter.siamese.dyndns.org","subject":"Re: blame --reverse selecting wrong commit","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-05-30T17:57:54Z","receivedAt":"2011-05-30T17:57:54Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Sun, May 29, 2011 at 23:47, Junio C Hamano wrote:\n> Shawn Pearce writes:\n>\n>> Rereading commit 85af7929ee (\"git-blame --reverse\"), it seems you left\n>> this an \"exercise for the reader\"... and in the past 3 years, no\n>> reader has stepped forward to implement the exercise as a patch to\n>> blame. *sigh*\n>\n> Yeah.\n>\n> It has always been my opinion that asking for \"one commit past the blamed\n> one\" is a undefined request (after all, blame for the line fell on that\n> commit exactly why the next commit does _not_ have any corresponding\n> line), so that is why I punted there.\n\nI don't think its undefined. Normally with blame/annotate we want to\ndiscover who put this line here, that is who did the insertion or\nreplacement that made this line show up in the result file. Under\nthese circumstances its clear to everyone that this is the commit with\nthe \"+\" in its unified diff on that line. :-)\n\nA reverse blame/annotate would want to say who removed this line in\nhistory. That's very well defined by everyone as the commit with the\n\"-\" in its unified diff on that line.\n\nThe notion of who deleted the line may seem less well defined than who\nadded it, because when you flip the history graph around you have\nthese forks exiting a single revision (an anti-merge as it were)...\nand any of those forks could have caused the deletion in question. Or\nall of them may have caused it. If more than one is responsible for\nthe deletion, who do you blame?\n\nThe same problem of multiple parties at fault exists in the normal\nforward case too. When a normal merge commit is reached, both sides\nmay have added the exact same line independently... and the merge\nresult will now contain identical content. Currently blame chooses to\ntraverse only the first parent history in this case, ignoring the\nother parents, and never showing that side branch. Its not any\ndifferent than the fork problem during deletion.\n\n> Now we seem to have found one interested reader, eh? ;-)\n\nWell, I implemented --reverse \"correctly\" in JGit last night. With my\npatch series applied, `jgit blame --reverse` will produce annotations\nfor who deleted the line, rather than the last surviving revision.\n\nThe catch is, its slightly more expensive than forward because we pass\nblame down *all* paths of a fork, rather than only the one that was\nidentical. This prevents pruning of side branches like we do in the\nnormal forward case, making for a larger amount of history to examine\nduring the reverse traversal. The incremental output also may produce\nmore than one \"source\" for a given line, if different side branches\ndeleted that line independently of one another. We smooth that out in\nour final result object for the command line case by showing only the\nearliest deletion, and discarding the other candidates.\n\nI still need to write a bunch of unit tests around the Java code, but\nthe algorithm worked as expected on the section of graph I started\nthis thread with. It may be worth back-porting to C Git, but now that\n--reverse is already shipping with its current \"last surviving\nrevision\" results, I'm not sure we can change the results without\ncausing some major confusion to users.\n\n-- \nShawn.\n"},{"id":"169022","messageId":"7vei3glzze.fsf@alter.siamese.dyndns.org","threadId":"27499","inReplyTo":"BANLkTikvmQgW=x0GYY6Yu=BnOgG+RXOCug@mail.gmail.com","subject":"Re: blame --reverse selecting wrong commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-30T20:01:25Z","receivedAt":"2011-05-30T20:01:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n>> Yeah.\n>>\n>> It has always been my opinion that asking for \"one commit past the blamed\n>> one\" is a undefined request (after all, blame for the line fell on that\n>> commit exactly why the next commit does _not_ have any corresponding\n>> line), so that is why I punted there.\n>\n> I don't think its undefined. Normally with blame/annotate we want to\n> discover who put this line here, that is who did the insertion or\n> replacement that made this line show up in the result file.\n\nVery true. The question that does not have a defined answer is \"what\ncontent was before this commit on that blamed line\", and that is very\ndifferent from \"what commit was there before this commit the line is\nblamed for\". The topic of this thread is the latter, and I was referring\nthe former which is offtopic because I was confused and tired.\n\n> The catch is, its slightly more expensive than forward because we pass\n> blame down *all* paths of a fork, rather than only the one that was\n> identical.\n\nHmm, I don't know. Isn't it just the matter of running the current\n\"reverse blame\" to find the newest revision that had the line (without any\n\"dig wider\"), and then dig a single level wider by comparing that revision\nand all its children to see which child has the line in question removed?\n"}]}