{"thread":{"id":"33873","subject":"[PATCH v6] Add new git-related helper to contrib","startedAt":"2013-05-19T15:53:43Z","lastAt":"2013-05-24T00:22:53Z","messageCount":27,"participants":["Felipe Contreras","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"217918","messageId":"1368978823-18247-1-git-send-email-felipe.contreras@gmail.com","threadId":"33873","inReplyTo":null,"subject":"[PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-19T15:53:43Z","receivedAt":"2013-05-19T15:53:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This script find people that might be interested in a patch, by going\nback through the history for each single hunk modified, and finding\npeople that reviewed, acknowledge, signed, or authored the code the\npatch is modifying.\n\nIt does this by running 'git blame' incrementally on each hunk, and then\nparsing the commit message. After gathering all the relevant people, it\ngroups them to show what exactly was their role when the participated in\nthe development of the relevant commit, and on how many relevant commits\nthey participated. They are only displayed if they pass a minimum\nthreshold of participation.\n\nFor example:\n\n % git related 0001-remote-hg-trivial-cleanups.patch\n Felipe Contreras \n Jeff King \n Max Horn \n Junio C Hamano \n\nThus it can be used for 'git send-email' as a cc-cmd.\n\nThere might be some other related functions to this script, not just to\nbe used as a cc-cmd.\n\nComments-by: Ramkumar Ramachandra \nSigned-off-by: Felipe Contreras \n---\n\nSames as v5, with a few tiny modifications. I'm tired of sending the whole\nseries multiple times over the years, only to get stuck at the first patch, so\nI'll send only the first one.\n\n contrib/related/git-related | 124 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 124 insertions(+)\n create mode 100755 contrib/related/git-related\n\ndiff --git a/contrib/related/git-related b/contrib/related/git-related\nnew file mode 100755\nindex 0000000..b96dcdd\n--- /dev/null\n+++ b/contrib/related/git-related\n@@ -0,0 +1,124 @@\n+#!/usr/bin/env ruby\n+\n+# This script finds people that might be interested in a patch\n+# usage: git related \n+\n+$since = '5-years-ago'\n+$min_percent = 10\n+\n+def fmt_person(name, email)\n+ '%s <%s>' % [name, email]\n+end\n+\n+class Commit\n+\n+ attr_reader :persons\n+\n+ def initialize(id)\n+ @id = id\n+ @persons = []\n+ end\n+\n+ def parse(data)\n+ msg = nil\n+ data.each_line do |line|\n+ if not msg\n+ case line\n+ when /^author ([^<>]+) <(\\S+)> (.+)$/\n+ @persons << fmt_person($1, $2)\n+ when /^$/\n+ msg = true\n+ end\n+ else\n+ if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\\S+?)>$/\n+ @persons << fmt_person($2, $3)\n+ end\n+ end\n+ end\n+ @persons.uniq!\n+ end\n+\n+end\n+\n+class Commits\n+\n+ def initialize\n+ @items = {}\n+ end\n+\n+ def size\n+ @items.size\n+ end\n+\n+ def each(&block)\n+ @items.each(&block)\n+ end\n+\n+ def import\n+ return if @items.empty?\n+ File.popen(%w[git cat-file --batch], 'r+') do |p|\n+ p.write(@items.keys.join(\"\\n\"))\n+ p.close_write\n+ p.each do |line|\n+ if line =~ /^(\\h{40}) commit (\\d+)/\n+ id, len = $1, $2\n+ data = p.read($2.to_i)\n+ @items[id].parse(data)\n+ end\n+ end\n+ end\n+ end\n+\n+ def get_blame(source, start, len, from)\n+ return if len == 0\n+ len ||= 1\n+ File.popen(['git', 'blame', '--incremental', '-CCC',\n+ '-L', '%u,+%u' % [start, len],\n+ '--since', $since, from + '^',\n+ '--', source]) do |p|\n+ p.each do |line|\n+ if line =~ /^(\\h{40})/\n+ id = $&\n+ @items[id] = Commit.new(id)\n+ end\n+ end\n+ end\n+ end\n+\n+ def from_patch(file)\n+ from = source = nil\n+ File.open(file) do |f|\n+ f.each do |line|\n+ case line\n+ when /^From (\\h+) (.+)$/\n+ from = $1\n+ when /^---\\s+(\\S+)/\n+ source = $1 != '/dev/null' ? $1[2..-1] : nil\n+ when /^@@ -(\\d+)(?:,(\\d+))?/\n+ get_blame(source, $1, $2, from)\n+ end\n+ end\n+ end\n+ end\n+\n+end\n+\n+exit 1 if ARGV.size != 1\n+\n+commits = Commits.new\n+commits.from_patch(ARGV[0])\n+commits.import\n+\n+count_per_person = Hash.new(0)\n+\n+commits.each do |id, commit|\n+ commit.persons.each do |person|\n+ count_per_person[person] += 1\n+ end\n+end\n+\n+count_per_person.each do |person, count|\n+ percent = count.to_f * 100 / commits.size\n+ next if percent < $min_percent\n+ puts person\n+end\n-- \n1.8.3.rc3.286.g3d43083\n"},{"id":"217951","messageId":"CAPig+cQV4goutofOhHBYtc1SfEChdOwYvXpcv+pxyZd8qVNZdQ@mail.gmail.com","threadId":"33873","inReplyTo":"1368978823-18247-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-20T01:47:58Z","receivedAt":"2013-05-20T01:47:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 19, 2013 at 11:53 AM, Felipe Contreras\n wrote:\n> This script find people that might be interested in a patch, by going\n\ns/find/finds/\n\n> back through the history for each single hunk modified, and finding\n> people that reviewed, acknowledge, signed, or authored the code the\n> patch is modifying.\n>\n> It does this by running 'git blame' incrementally on each hunk, and then\n> parsing the commit message. After gathering all the relevant people, it\n> groups them to show what exactly was their role when the participated in\n> the development of the relevant commit, and on how many relevant commits\n> they participated. They are only displayed if they pass a minimum\n> threshold of participation.\n>\n> For example:\n>\n> % git related 0001-remote-hg-trivial-cleanups.patch\n> Felipe Contreras \n> Jeff King \n> Max Horn \n> Junio C Hamano \n>\n> Thus it can be used for 'git send-email' as a cc-cmd.\n>\n> There might be some other related functions to this script, not just to\n> be used as a cc-cmd.\n>\n> Comments-by: Ramkumar Ramachandra \n> Signed-off-by: Felipe Contreras \n"},{"id":"218202","messageId":"7vr4gyizti.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"1368978823-18247-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T19:23:21Z","receivedAt":"2013-05-22T19:23:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> diff --git a/contrib/related/git-related b/contrib/related/git-related\n> new file mode 100755\n> index 0000000..b96dcdd\n> --- /dev/null\n> +++ b/contrib/related/git-related\n> @@ -0,0 +1,124 @@\n> +#!/usr/bin/env ruby\n> +\n> +# This script finds people that might be interested in a patch\n> +# usage: git related \n> +\n> +$since = '5-years-ago'\n> +$min_percent = 10\n> +\n> +def fmt_person(name, email)\n> + '%s <%s>' % [name, email]\n> +end\n\nMicronit. I suspect you do not need this helper, unless later\npatches start using it.\n\n> + def import\n> + return if @items.empty?\n> + File.popen(%w[git cat-file --batch], 'r+') do |p|\n> + p.write(@items.keys.join(\"\\n\"))\n> + p.close_write\n> + p.each do |line|\n> + if line =~ /^(\\h{40}) commit (\\d+)/\n> + id, len = $1, $2\n> + data = p.read($2.to_i)\n> + @items[id].parse(data)\n> + end\n> + end\n> + end\n> + end\n> +\n> + def get_blame(source, start, len, from)\n> + return if len == 0\n> + len ||= 1\n> + File.popen(['git', 'blame', '--incremental', '-CCC',\n\nI am torn on the hardcoded use of \"-CCC\" here.\n\nDepending on the nature of the change in question, it may match well\nor worse to what you are trying to find out. When you are trying to\nsay \"What were you smoking when you implemented this broken logic?\",\nusing -C may be good, but when your question is \"Even though all the\ncallers of this function live in that other file, somebody moved\nthis function that used to be file static in that file to here and\nmade it public. Why?\", you do not want to use -C.\n\nI am reasonably sure that in the finished code later in the series\nit will become configurable, but a fallback default is better to be\nnot so expensive one.\n\n> + '-L', '%u,+%u' % [start, len],\n> + '--since', $since, from + '^',\n\nIs \"from\" unconditionally set?\n\nPerhaps that nil + '^' magically disappear and this code is relying\non that, but it smells like a too much magic to me.\n\n> + '--', source]) do |p|\n> + p.each do |line|\n> + if line =~ /^(\\h{40})/\n> + id = $&\n> + @items[id] = Commit.new(id)\n> + end\n> + end\n> + end\n> + end\n> +\n> + def from_patch(file)\n> + from = source = nil\n> + File.open(file) do |f|\n> + f.each do |line|\n> + case line\n> + when /^From (\\h+) (.+)$/\n> + from = $1\n> + when /^---\\s+(\\S+)/\n> + source = $1 != '/dev/null' ? $1[2..-1] : nil\n> + when /^@@ -(\\d+)(?:,(\\d+))?/\n> + get_blame(source, $1, $2, from)\n> + end\n\nMakes sense to start from the preimage so that you can find out who\nwrote the original block of lines your patch is removing.\n\nBut then if source is /dev/null, wouldn't you be able to stop\nwithout running blame at all? You know the patch is creating a new\nfile at that point and there is nobody to point a finger at.\n\n> + end\n> + end\n> + end\n> +\n> +end\n> +\n> +exit 1 if ARGV.size != 1\n> +\n> +commits = Commits.new\n> +commits.from_patch(ARGV[0])\n> +commits.import\n> +\n> +count_per_person = Hash.new(0)\n> +\n> +commits.each do |id, commit|\n> + commit.persons.each do |person|\n> + count_per_person[person] += 1\n> + end\n> +end\n> +\n> +count_per_person.each do |person, count|\n> + percent = count.to_f * 100 / commits.size\n> + next if percent < $min_percent\n> + puts person\n> +end\n"},{"id":"218203","messageId":"7vk3mqiycx.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"7vr4gyizti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T19:54:54Z","receivedAt":"2013-05-22T19:54:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Felipe Contreras writes:\n>\n>> diff --git a/contrib/related/git-related b/contrib/related/git-related\n>> new file mode 100755\n>> index 0000000..b96dcdd\n>> --- /dev/null\n>> +++ b/contrib/related/git-related\n>> @@ -0,0 +1,124 @@\n>> +#!/usr/bin/env ruby\n>> +\n>> +# This script finds people that might be interested in a patch\n>> +# usage: git related \n>> +\n>> +$since = '5-years-ago'\n>> +$min_percent = 10\n>> +\n>> +def fmt_person(name, email)\n>> + '%s <%s>' % [name, email]\n>> +end\n>\n> Micronit. I suspect you do not need this helper, unless later\n> patches start using it.\n\nNot that matters terribly, but \"later patches start using it in a\ndifferent way\" may be needed as a clarification.\n\nThe current two callers capture both \"%s <%s>\" as two separate\ngroups but they do not need to; they can do \"(%s <%s>)\" and pass $1\nto this function, I think.\n"},{"id":"218215","messageId":"CAMP44s0pXLmjVUwsVWiN2G5QReh80spXjvy_6GuUGRz1RmUbLQ@mail.gmail.com","threadId":"33873","inReplyTo":"7vr4gyizti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-22T22:23:28Z","receivedAt":"2013-05-22T22:23:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 2:23 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> diff --git a/contrib/related/git-related b/contrib/related/git-related\n>> new file mode 100755\n>> index 0000000..b96dcdd\n>> --- /dev/null\n>> +++ b/contrib/related/git-related\n>> @@ -0,0 +1,124 @@\n>> +#!/usr/bin/env ruby\n>> +\n>> +# This script finds people that might be interested in a patch\n>> +# usage: git related \n>> +\n>> +$since = '5-years-ago'\n>> +$min_percent = 10\n>> +\n>> +def fmt_person(name, email)\n>> + '%s <%s>' % [name, email]\n>> +end\n>\n> Micronit. I suspect you do not need this helper, unless later\n> patches start using it.\n\nIt's not *needed*, but it makes if fulfills the role of a function: to\navoid typing that code in multiple places.\n\n>> + def import\n>> + return if @items.empty?\n>> + File.popen(%w[git cat-file --batch], 'r+') do |p|\n>> + p.write(@items.keys.join(\"\\n\"))\n>> + p.close_write\n>> + p.each do |line|\n>> + if line =~ /^(\\h{40}) commit (\\d+)/\n>> + id, len = $1, $2\n>> + data = p.read($2.to_i)\n>> + @items[id].parse(data)\n>> + end\n>> + end\n>> + end\n>> + end\n>> +\n>> + def get_blame(source, start, len, from)\n>> + return if len == 0\n>> + len ||= 1\n>> + File.popen(['git', 'blame', '--incremental', '-CCC',\n>\n> I am torn on the hardcoded use of \"-CCC\" here.\n>\n> Depending on the nature of the change in question, it may match well\n> or worse to what you are trying to find out. When you are trying to\n> say \"What were you smoking when you implemented this broken logic?\",\n> using -C may be good, but when your question is \"Even though all the\n> callers of this function live in that other file, somebody moved\n> this function that used to be file static in that file to here and\n> made it public. Why?\", you do not want to use -C.\n>\n> I am reasonably sure that in the finished code later in the series\n> it will become configurable, but a fallback default is better to be\n> not so expensive one.\n\nThe script's purpose is to find related commits, -CCC does that, does it not?\n\n>> + '-L', '%u,+%u' % [start, len],\n>> + '--since', $since, from + '^',\n>\n> Is \"from\" unconditionally set?\n>\n> Perhaps that nil + '^' magically disappear and this code is relying\n> on that, but it smells like a too much magic to me.\n\nI personally don't care. You decide what's the behavior when no 'From\n' line is available in the patch. I don't see the point in worrying\nabout non-git patches.\n\n>> + '--', source]) do |p|\n>> + p.each do |line|\n>> + if line =~ /^(\\h{40})/\n>> + id = $&\n>> + @items[id] = Commit.new(id)\n>> + end\n>> + end\n>> + end\n>> + end\n>> +\n>> + def from_patch(file)\n>> + from = source = nil\n>> + File.open(file) do |f|\n>> + f.each do |line|\n>> + case line\n>> + when /^From (\\h+) (.+)$/\n>> + from = $1\n>> + when /^---\\s+(\\S+)/\n>> + source = $1 != '/dev/null' ? $1[2..-1] : nil\n>> + when /^@@ -(\\d+)(?:,(\\d+))?/\n>> + get_blame(source, $1, $2, from)\n>> + end\n>\n> Makes sense to start from the preimage so that you can find out who\n> wrote the original block of lines your patch is removing.\n>\n> But then if source is /dev/null, wouldn't you be able to stop\n> without running blame at all? You know the patch is creating a new\n> file at that point and there is nobody to point a finger at.\n\nA patch can touch multiple files.\n\n-- \nFelipe Contreras\n"},{"id":"218217","messageId":"7vd2siiqsr.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s0pXLmjVUwsVWiN2G5QReh80spXjvy_6GuUGRz1RmUbLQ@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T22:38:12Z","receivedAt":"2013-05-22T22:38:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Depending on the nature of the change in question, it may match well\n>> or worse to what you are trying to find out. When you are trying to\n>> say \"What were you smoking when you implemented this broken logic?\",\n>> using -C may be good, but when your question is \"Even though all the\n>> callers of this function live in that other file, somebody moved\n>> this function that used to be file static in that file to here and\n>> made it public. Why?\", you do not want to use -C.\n>>\n>> I am reasonably sure that in the finished code later in the series\n>> it will become configurable, but a fallback default is better to be\n>> not so expensive one.\n>\n> The script's purpose is to find related commits, -CCC does that, does it not?\n\nAs I already said in the above, the answer is no, when you are\ntrying to find who moved the code from the original place.\n\n>> Makes sense to start from the preimage so that you can find out who\n>> wrote the original block of lines your patch is removing.\n>>\n>> But then if source is /dev/null, wouldn't you be able to stop\n>> without running blame at all? You know the patch is creating a new\n>> file at that point and there is nobody to point a finger at.\n>\n> A patch can touch multiple files.\n\nSo?\n\nWhat line range would you be feeding \"blame\" with, for such a file\ncreation event?\n"},{"id":"218218","messageId":"CAMP44s2ETSxebGxXJV_BYEqTC5gJMVdQLbnTUomrie2rHhdmDg@mail.gmail.com","threadId":"33873","inReplyTo":"7vd2siiqsr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-22T22:43:34Z","receivedAt":"2013-05-22T22:43:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 5:38 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> Depending on the nature of the change in question, it may match well\n>>> or worse to what you are trying to find out. When you are trying to\n>>> say \"What were you smoking when you implemented this broken logic?\",\n>>> using -C may be good, but when your question is \"Even though all the\n>>> callers of this function live in that other file, somebody moved\n>>> this function that used to be file static in that file to here and\n>>> made it public. Why?\", you do not want to use -C.\n>>>\n>>> I am reasonably sure that in the finished code later in the series\n>>> it will become configurable, but a fallback default is better to be\n>>> not so expensive one.\n>>\n>> The script's purpose is to find related commits, -CCC does that, does it not?\n>\n> As I already said in the above, the answer is no, when you are\n> trying to find who moved the code from the original place.\n\nBut I'm not trying to find who moved the code, I'm trying to find\nrelated commits; hence the name 'git related'.\n\nThe person who moved the code will be on the list regardless, so 'git\nrelated' does find the person who moved the code indirectly; by\nfinding the persons that touched the code.\n\n>>> Makes sense to start from the preimage so that you can find out who\n>>> wrote the original block of lines your patch is removing.\n>>>\n>>> But then if source is /dev/null, wouldn't you be able to stop\n>>> without running blame at all? You know the patch is creating a new\n>>> file at that point and there is nobody to point a finger at.\n>>\n>> A patch can touch multiple files.\n>\n> So?\n>\n> What line range would you be feeding \"blame\" with, for such a file\n> creation event?\n\nI thought it was skipping git blame in such cases, perhaps it got\ndropped in one of the other countless versions of this script.\n\nGood catch.\n\n-- \nFelipe Contreras\n"},{"id":"218219","messageId":"7v8v36iq3e.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s2ETSxebGxXJV_BYEqTC5gJMVdQLbnTUomrie2rHhdmDg@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T22:53:25Z","receivedAt":"2013-05-22T22:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, May 22, 2013 at 5:38 PM, Junio C Hamano wrote:\n> ...\n>> As I already said in the above, the answer is no, when you are\n>> trying to find who moved the code from the original place.\n>\n> But I'm not trying to find who moved the code, I'm trying to find\n> related commits; hence the name 'git related'.\n>\n> The person who moved the code will be on the list regardless,\n\nThat is exactly the point I have been trying to raise. Does the\nperson appear in the list when you run blame with -CCC? You ask for\nthe body of the function, and the -C mode of blame sees through the\nblock-of-line movement across file boundaries, and goes straight to\nthe last commit that touched the body of the function in its original\nfile, no?\n"},{"id":"218220","messageId":"7v4nduipvb.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"7v8v36iq3e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T22:58:16Z","receivedAt":"2013-05-22T22:58:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Felipe Contreras writes:\n>\n>> On Wed, May 22, 2013 at 5:38 PM, Junio C Hamano wrote:\n>> ...\n>>> As I already said in the above, the answer is no, when you are\n>>> trying to find who moved the code from the original place.\n>>\n>> But I'm not trying to find who moved the code, I'm trying to find\n>> related commits; hence the name 'git related'.\n>>\n>> The person who moved the code will be on the list regardless,\n>\n> That is exactly the point I have been trying to raise. Does the\n> person appear in the list when you run blame with -CCC? You ask for\n\ns/person/commit/;\n\n> the body of the function, and the -C mode of blame sees through the\n> block-of-line movement across file boundaries, and goes straight to\n> the last commit that touched the body of the function in its original\n> file, no?\n"},{"id":"218221","messageId":"CAMP44s38=kQrCLOEuSmCqDYzCDic5SXaZN3rKY+4t3LXke11=g@mail.gmail.com","threadId":"33873","inReplyTo":"7v8v36iq3e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-22T23:19:19Z","receivedAt":"2013-05-22T23:19:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 5:53 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Wed, May 22, 2013 at 5:38 PM, Junio C Hamano wrote:\n>> ...\n>>> As I already said in the above, the answer is no, when you are\n>>> trying to find who moved the code from the original place.\n>>\n>> But I'm not trying to find who moved the code, I'm trying to find\n>> related commits; hence the name 'git related'.\n>>\n>> The person who moved the code will be on the list regardless,\n>\n> That is exactly the point I have been trying to raise. Does the\n> person appear in the list when you run blame with -CCC? You ask for\n> the body of the function, and the -C mode of blame sees through the\n> block-of-line movement across file boundaries, and goes straight to\n> the last commit that touched the body of the function in its original\n> file, no?\n\nI'm not familiar how the different -C options work, but I'm testing\nright now and I see the commit that moved a file with both -C and\n-CCC, but strangely enough, not if it's the previous commit (with\nboth).\n\n-- \nFelipe Contreras\n"},{"id":"218222","messageId":"7vzjvmh98r.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"7v4nduipvb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T23:42:44Z","receivedAt":"2013-05-22T23:42:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> The person who moved the code will be on the list regardless,\n>>\n>> That is exactly the point I have been trying to raise. Does the\n>> person appear in the list when you run blame with -CCC? You ask for\n>\n> s/person/commit/;\n>\n>> the body of the function, and the -C mode of blame sees through the\n>> block-of-line movement across file boundaries, and goes straight to\n>> the last commit that touched the body of the function in its original\n>> file, no?\n\n-- >8 --\ncd /var/tmp/\ngit init blame\ncd blame\n\ncp /src/git/COPYING COPYING\ngit add COPYING\ngit commit -m initial\n\nsed -ne 120,140p COPYING >EXTRACTING\ngit add EXTRACTING\ngit commit -m second\n\ngit blame -C -C -C EXTRACTING\n-- 8< --\n\nwill show that all lines from EXTRACTING came from the original\nrevision of COPYING, and we will miss the line-move event.\n\n\"blame -C\" with a single -C does not look at the files that did not\nchange in the commit that added these lines to EXTRACTING\n(i.e. COPYING), so the digging stops there.\n\nAfter this, if you do this:\n\n-- >8 --\necho >>COPYING\ngit commit --amend -a --no-edit\n\ngit blame -C EXTRACTING\n-- 8< --\n\nthen the commit that did add these lines to EXTRACTING touched\nCOPYING, and the origin of these lines are traced to it (this is\ndesigned to be useful in a typical \"refactor by moving code\";\n\"cut and paste without changing the original\" people need heavier\ncopy detection with more -C).\n\nIIRC, git-gui runs two blames, one without any -C and one with (I do\nnot offhand recall how many -C it uses) to show both.\n\nHTH.\n"},{"id":"218223","messageId":"CAMP44s1_=HSgZA=abHJ3sgDbUyfkF78qO+sR1+Aact3o7bAgLA@mail.gmail.com","threadId":"33873","inReplyTo":"7vzjvmh98r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-22T23:57:33Z","receivedAt":"2013-05-22T23:57:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 6:42 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>>>> The person who moved the code will be on the list regardless,\n>>>\n>>> That is exactly the point I have been trying to raise. Does the\n>>> person appear in the list when you run blame with -CCC? You ask for\n>>\n>> s/person/commit/;\n>>\n>>> the body of the function, and the -C mode of blame sees through the\n>>> block-of-line movement across file boundaries, and goes straight to\n>>> the last commit that touched the body of the function in its original\n>>> file, no?\n>\n> -- >8 --\n> cd /var/tmp/\n> git init blame\n> cd blame\n>\n> cp /src/git/COPYING COPYING\n> git add COPYING\n> git commit -m initial\n>\n> sed -ne 120,140p COPYING >EXTRACTING\n> git add EXTRACTING\n> git commit -m second\n>\n> git blame -C -C -C EXTRACTING\n> -- 8< --\n\nI just realized that -CCC does not do the same thing as -C -C -C.\n\n> then the commit that did add these lines to EXTRACTING touched\n> COPYING, and the origin of these lines are traced to it (this is\n> designed to be useful in a typical \"refactor by moving code\";\n> \"cut and paste without changing the original\" people need heavier\n> copy detection with more -C).\n>\n> IIRC, git-gui runs two blames, one without any -C and one with (I do\n> not offhand recall how many -C it uses) to show both.\n\n'git blame' is a very expensive operation, perhaps we should add\nanother option so users don't need to run two blames to find this.\n\n-- \nFelipe Contreras\n"},{"id":"218224","messageId":"7vvc6ah81g.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s1_=HSgZA=abHJ3sgDbUyfkF78qO+sR1+Aact3o7bAgLA@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T00:08:43Z","receivedAt":"2013-05-23T00:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> IIRC, git-gui runs two blames, one without any -C and one with (I do\n>> not offhand recall how many -C it uses) to show both.\n>\n> 'git blame' is a very expensive operation, perhaps we should add\n> another option so users don't need to run two blames to find this.\n\nYes, you could add a \"struct origin *suspect_in_the_same_file\" next\nto the current \"struct origin *suspect\" to the blame_entry in order\nto represent the origin you would get if you were to stop at a \"move\nacross files\" event, and keep digging, when you are operating under\n\"-C -C\" or higher mode.\n\nThat would make the result just as expensive as a single run of \"-C\n-C\" (or higher mode) [*1*] without having to run an extra \"git\nblame\" without any -C (even though that mode is much cheaper).\n\nRepresenting the result for a terminal with reasonable width might\nbecome unwieldy, but for --porcelain output format that should not\nbe a problem.\n\n\n[Footnote]\n\n*1* It would make it slightly more expensive than the current code,\n as it is very likely that you have to split a single blame_entry\n into many separate pieces.\n"},{"id":"218226","messageId":"CAMP44s20njF9u9tRKM6J92-oqeLHoLqd30R-A7opTG7bdrTJzA@mail.gmail.com","threadId":"33873","inReplyTo":"7vzjvmh98r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T03:23:59Z","receivedAt":"2013-05-23T03:23:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This doesn't make any sense:\n\n---\ncd /tmp\nrm -rf blame\n\ngit init blame\ncd blame\n\ncp ~/dev/git/COPYING COPYING\ngit add COPYING\ngit commit -m initial\n\nsed -ne 120,140p COPYING >EXTRACTING\ngit add EXTRACTING\ngit commit -m second\n\ngit log --oneline\ngit blame -C EXTRACTING\n\necho >>COPYING\ngit commit --amend -a --no-edit\n\ngit log --oneline\ngit blame -C EXTRACTING\n---\n\nWhy would the first instance find the blame in commit #2, and the\nsecond one at commit #1? If anything, the second instance should have\nmore trouble finding the original commit.\n\n-- \nFelipe Contreras\n"},{"id":"218227","messageId":"CAMP44s1e-kjVnAHQx3GOAG6ge_GZBmOBaCnr8YeyU6t5CveuzA@mail.gmail.com","threadId":"33873","inReplyTo":"CAMP44s20njF9u9tRKM6J92-oqeLHoLqd30R-A7opTG7bdrTJzA@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T03:50:39Z","receivedAt":"2013-05-23T03:50:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 10:23 PM, Felipe Contreras\n wrote:\n> This doesn't make any sense:\n\nAh, never mind, it's COPYING the one being modified, not EXTRACTING.\n\n-- \nFelipe Contreras\n"},{"id":"218228","messageId":"CAMP44s37a0_wVVef+QVnLNvc8Uos_XpCZAtpX0RmQyMcuJHUHw@mail.gmail.com","threadId":"33873","inReplyTo":"7vvc6ah81g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T04:07:32Z","receivedAt":"2013-05-23T04:07:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 7:08 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> IIRC, git-gui runs two blames, one without any -C and one with (I do\n>>> not offhand recall how many -C it uses) to show both.\n>>\n>> 'git blame' is a very expensive operation, perhaps we should add\n>> another option so users don't need to run two blames to find this.\n>\n> Yes, you could add a \"struct origin *suspect_in_the_same_file\" next\n> to the current \"struct origin *suspect\" to the blame_entry in order\n> to represent the origin you would get if you were to stop at a \"move\n> across files\" event, and keep digging, when you are operating under\n> \"-C -C\" or higher mode.\n\nNo, this is what I meant:\n\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1509,16 +1509,6 @@ static void found_guilty_entry(struct blame_entry *ent)\n if (ent->guilty)\n return;\n ent->guilty = 1;\n- if (incremental) {\n- struct origin *suspect = ent->suspect;\n-\n- printf(\"%s %d %d %d\\n\",\n- sha1_to_hex(suspect->commit->object.sha1),\n- ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n- emit_one_suspect_detail(suspect, 0);\n- write_filename_info(suspect->path);\n- maybe_flush_or_die(stdout, \"stdout\");\n- }\n }\n\n /*\n@@ -1536,12 +1526,24 @@ static void assign_blame(struct scoreboard *sb, int opt)\n struct origin *suspect = NULL;\n\n /* find one suspect to break down */\n- for (ent = sb->ent; !suspect && ent; ent = ent->next)\n- if (!ent->guilty)\n+ for (ent = sb->ent; ent; ent = ent->next)\n+ if (!ent->guilty) {\n suspect = ent->suspect;\n+ break;\n+ }\n+\n if (!suspect)\n return; /* all done */\n\n+ if (incremental &&\n!is_null_sha1(suspect->commit->object.sha1)) {\n+ printf(\"%s %d %d %d\\n\",\n+\nsha1_to_hex(suspect->commit->object.sha1),\n+ ent->s_lno + 1, ent->lno + 1,\nent->num_lines);\n+ emit_one_suspect_detail(suspect, 0);\n+ write_filename_info(suspect->path);\n+ maybe_flush_or_die(stdout, \"stdout\");\n+ }\n+\n /*\n * We will use this suspect later in the loop,\n * so hold onto it in the meantime.\n\n-- \nFelipe Contreras\n"},{"id":"218230","messageId":"CAMP44s18ps9JtY7+mvXVbuZu3cm7bHnK1vCzMWqBdvawLr+zzA@mail.gmail.com","threadId":"33873","inReplyTo":"CAMP44s37a0_wVVef+QVnLNvc8Uos_XpCZAtpX0RmQyMcuJHUHw@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T05:22:07Z","receivedAt":"2013-05-23T05:22:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 22, 2013 at 11:07 PM, Felipe Contreras\n wrote:\n> On Wed, May 22, 2013 at 7:08 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>>> IIRC, git-gui runs two blames, one without any -C and one with (I do\n>>>> not offhand recall how many -C it uses) to show both.\n>>>\n>>> 'git blame' is a very expensive operation, perhaps we should add\n>>> another option so users don't need to run two blames to find this.\n>>\n>> Yes, you could add a \"struct origin *suspect_in_the_same_file\" next\n>> to the current \"struct origin *suspect\" to the blame_entry in order\n>> to represent the origin you would get if you were to stop at a \"move\n>> across files\" event, and keep digging, when you are operating under\n>> \"-C -C\" or higher mode.\n>\n> No, this is what I meant:\n\nBut that would not print the right line numbers, so perhaps:\n\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1500,25 +1500,16 @@ static int emit_one_suspect_detail(struct\norigin *suspect, int repeat)\n \treturn 1;\n }\n\n-/*\n- * The blame_entry is found to be guilty for the range. Mark it\n- * as such, and show it in incremental output.\n- */\n-static void found_guilty_entry(struct blame_entry *ent)\n+static void print_guilty_entry(struct blame_entry *ent)\n {\n-\tif (ent->guilty)\n-\t\treturn;\n-\tent->guilty = 1;\n-\tif (incremental) {\n-\t\tstruct origin *suspect = ent->suspect;\n-\n-\t\tprintf(\"%s %d %d %d\\n\",\n-\t\t sha1_to_hex(suspect->commit->object.sha1),\n-\t\t ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n-\t\temit_one_suspect_detail(suspect, 0);\n-\t\twrite_filename_info(suspect->path);\n-\t\tmaybe_flush_or_die(stdout, \"stdout\");\n-\t}\n+\tstruct origin *suspect = ent->suspect;\n+\n+\tprintf(\"%s %d %d %d\\n\",\n+\t\t\tsha1_to_hex(suspect->commit->object.sha1),\n+\t\t\tent->s_lno + 1, ent->lno + 1, ent->num_lines);\n+\temit_one_suspect_detail(suspect, 0);\n+\twrite_filename_info(suspect->path);\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n }\n\n /*\n@@ -1531,14 +1522,19 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \tstruct rev_info *revs = sb->revs;\n\n \twhile (1) {\n-\t\tstruct blame_entry *ent;\n+\t\tstruct blame_entry *ent, tmp_ent;\n \t\tstruct commit *commit;\n \t\tstruct origin *suspect = NULL;\n+\t\tint found_guilty = 0;\n\n \t\t/* find one suspect to break down */\n-\t\tfor (ent = sb->ent; !suspect && ent; ent = ent->next)\n-\t\t\tif (!ent->guilty)\n+\t\tfor (ent = sb->ent; ent; ent = ent->next)\n+\t\t\tif (!ent->guilty) {\n+\t\t\t\ttmp_ent = *ent;\n \t\t\t\tsuspect = ent->suspect;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n \t\tif (!suspect)\n \t\t\treturn; /* all done */\n\n@@ -1564,9 +1560,20 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t\tcommit->object.flags |= UNINTERESTING;\n\n \t\t/* Take responsibility for the remaining entries */\n-\t\tfor (ent = sb->ent; ent; ent = ent->next)\n-\t\t\tif (same_suspect(ent->suspect, suspect))\n-\t\t\t\tfound_guilty_entry(ent);\n+\t\tfor (ent = sb->ent; ent; ent = ent->next) {\n+\t\t\tif (same_suspect(ent->suspect, suspect)) {\n+\t\t\t\tif (ent->guilty)\n+\t\t\t\t\tcontinue;\n+\t\t\t\tfound_guilty = ent->guilty = 1;\n+\t\t\t\tif (incremental)\n+\t\t\t\t\tprint_guilty_entry(ent);\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (incremental && !found_guilty &&\n+\t\t\t\t!is_null_sha1(suspect->commit->object.sha1))\n+\t\t\tprint_guilty_entry(&tmp_ent);\n+\n \t\torigin_decref(suspect);\n\n \t\tif (DEBUG) /* sanity */\n\n-- \nFelipe Contreras\n"},{"id":"218281","messageId":"7v8v35hc11.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s18ps9JtY7+mvXVbuZu3cm7bHnK1vCzMWqBdvawLr+zzA@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T16:54:50Z","receivedAt":"2013-05-23T16:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, May 22, 2013 at 11:07 PM, Felipe Contreras\n> wrote:\n>> On Wed, May 22, 2013 at 7:08 PM, Junio C Hamano wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>>> IIRC, git-gui runs two blames, one without any -C and one with (I do\n>>>>> not offhand recall how many -C it uses) to show both.\n>>>>\n>>>> 'git blame' is a very expensive operation, perhaps we should add\n>>>> another option so users don't need to run two blames to find this.\n>>>\n>>> Yes, you could add a \"struct origin *suspect_in_the_same_file\" next\n>>> to the current \"struct origin *suspect\" to the blame_entry in order\n>>> to represent the origin you would get if you were to stop at a \"move\n>>> across files\" event, and keep digging, when you are operating under\n>>> \"-C -C\" or higher mode.\n>>\n>> No, this is what I meant:\n>\n> But that would not print the right line numbers, so perhaps:\n\nUsers of full-output may want to be able to see the same thing.\n\nI have a suspicion that you misread what assign_blame() does. The\nfirst inner loop to find one \"suspect\" is really what it says. It\nloops over blame entries in the scoreboard but it is not about\nfinding one \"entry\", but is to find one \"suspect\". The \"ent\" found\nin the loop that you store in tmp_ent is no more special than any\nother \"ent\" that shares the same \"suspect\" as its origin.\n\nImagine that your scoreboard originally has three blocks of text\n(i.e. blame_entry) A, B, and C, and the current suspect for A and C\nare the same, while we already know what the final guilty party for\nB is (which may be some descendant of the \"suspect\").\n\nOnce we find one \"suspect\" in the first inner loop, the call to\npass_blame() does everything it can do to exonerate that \"suspect\".\n\nIt runs a single diff between the suspect and each of its parents to\nfind lines in both A and C that can be blamed to one of these\nparents, and blame entries A and C are split into pieces, some of\nwhich still have the same suspect as their origin, which are where\ntheir lines originate from, while others are attributable to these\nparents or their ancestors.\n\nIf you keep the original entry for A to do something special, like\nprinting what the original range of A was before it was split by\npass_blame(), wouldn't you need to do the same for C? We will not\nbe visiting C later in the outer while(1) loop, as a single call to\npass_blame() for \"suspect\" we did when we found it in A has already\ntaken care of it.\n\nI am not sure what you are trying to achieve with that found-guilty\nlogic, even if the \"save away in tmp_ent\" logic is changed to keep\nall the blame entries that have \"suspect\" we are looking at as their\norigin. When the current suspect is found to have passed all lines\nintact from its parents, we will see found-guilty left to be false.\n\nHow would it make the original blame_entry (perhaps halfway) guilty\nin that situation?\n\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1500,25 +1500,16 @@ static int emit_one_suspect_detail(struct\n> origin *suspect, int repeat)\n> \treturn 1;\n> }\n>\n> -/*\n> - * The blame_entry is found to be guilty for the range. Mark it\n> - * as such, and show it in incremental output.\n> - */\n> -static void found_guilty_entry(struct blame_entry *ent)\n> +static void print_guilty_entry(struct blame_entry *ent)\n> {\n> -\tif (ent->guilty)\n> -\t\treturn;\n> -\tent->guilty = 1;\n> -\tif (incremental) {\n> -\t\tstruct origin *suspect = ent->suspect;\n> -\n> -\t\tprintf(\"%s %d %d %d\\n\",\n> -\t\t sha1_to_hex(suspect->commit->object.sha1),\n> -\t\t ent->s_lno + 1, ent->lno + 1, ent->num_lines);\n> -\t\temit_one_suspect_detail(suspect, 0);\n> -\t\twrite_filename_info(suspect->path);\n> -\t\tmaybe_flush_or_die(stdout, \"stdout\");\n> -\t}\n> +\tstruct origin *suspect = ent->suspect;\n> +\n> +\tprintf(\"%s %d %d %d\\n\",\n> +\t\t\tsha1_to_hex(suspect->commit->object.sha1),\n> +\t\t\tent->s_lno + 1, ent->lno + 1, ent->num_lines);\n> +\temit_one_suspect_detail(suspect, 0);\n> +\twrite_filename_info(suspect->path);\n> +\tmaybe_flush_or_die(stdout, \"stdout\");\n> }\n>\n> /*\n> @@ -1531,14 +1522,19 @@ static void assign_blame(struct scoreboard *sb, int opt)\n> \tstruct rev_info *revs = sb->revs;\n>\n> \twhile (1) {\n> -\t\tstruct blame_entry *ent;\n> +\t\tstruct blame_entry *ent, tmp_ent;\n> \t\tstruct commit *commit;\n> \t\tstruct origin *suspect = NULL;\n> +\t\tint found_guilty = 0;\n>\n> \t\t/* find one suspect to break down */\n> -\t\tfor (ent = sb->ent; !suspect && ent; ent = ent->next)\n> -\t\t\tif (!ent->guilty)\n> +\t\tfor (ent = sb->ent; ent; ent = ent->next)\n> +\t\t\tif (!ent->guilty) {\n> +\t\t\t\ttmp_ent = *ent;\n> \t\t\t\tsuspect = ent->suspect;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> \t\tif (!suspect)\n> \t\t\treturn; /* all done */\n>\n> @@ -1564,9 +1560,20 @@ static void assign_blame(struct scoreboard *sb, int opt)\n> \t\t\tcommit->object.flags |= UNINTERESTING;\n>\n> \t\t/* Take responsibility for the remaining entries */\n> -\t\tfor (ent = sb->ent; ent; ent = ent->next)\n> -\t\t\tif (same_suspect(ent->suspect, suspect))\n> -\t\t\t\tfound_guilty_entry(ent);\n> +\t\tfor (ent = sb->ent; ent; ent = ent->next) {\n> +\t\t\tif (same_suspect(ent->suspect, suspect)) {\n> +\t\t\t\tif (ent->guilty)\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tfound_guilty = ent->guilty = 1;\n> +\t\t\t\tif (incremental)\n> +\t\t\t\t\tprint_guilty_entry(ent);\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (incremental && !found_guilty &&\n> +\t\t\t\t!is_null_sha1(suspect->commit->object.sha1))\n> +\t\t\tprint_guilty_entry(&tmp_ent);\n> +\n> \t\torigin_decref(suspect);\n>\n> \t\tif (DEBUG) /* sanity */\n"},{"id":"218284","messageId":"7v38tdhbjd.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s1e-kjVnAHQx3GOAG6ge_GZBmOBaCnr8YeyU6t5CveuzA@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T17:05:26Z","receivedAt":"2013-05-23T17:05:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, May 22, 2013 at 10:23 PM, Felipe Contreras\n> wrote:\n>> This doesn't make any sense:\n>\n> Ah, never mind, it's COPYING the one being modified, not EXTRACTING.\n\nYes.\n\nThe different levels of -C happens to correspond to the software\nengineering practice from best to sloppy:\n\n - When you refactor to have a block of lines in a file , the\n original of that block would have come from another file (or the\n same file) you touched to remove duplication, so a single -C\n looks for an origin only from modified files (best).\n\n - When you start a new file, you may have to start from some\n boilerplate material that already exists in another file that is\n not (and does not have to be) changed in the commit you add that\n new file, so double -C -C looks for an origin of lines from other\n files, even unmodified ones, when looking at the lines in a new\n file (unfortunate but acceptable).\n\n - When you copy and paste without making any effort to refactor,\n you modify a file by adding new lines that match identically from\n another existing file, the latter of which does not change in the\n commit you do that copy and paste. To find this, you need to\n look for an origin for any file from all the other files, and\n triple -C -C -C lets you do so (sloppy).\n"},{"id":"218295","messageId":"7vzjvleea5.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"7v8v35hc11.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T18:34:26Z","receivedAt":"2013-05-23T18:34:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Users of full-output may want to be able to see the same thing.\n\n... but I agree we may be able to cheat if we only want to support\ninteractive, and we do want to find a way to cheat when we are\nrunning interactive without having to store much more information in\neach blame_entry.\n\n> Imagine that your scoreboard originally has three blocks of text\n> ...\n> in that situation?\n\n(I snipped everything I said in the previous reply only for\nbrevity but they are still relevant).\n\nI _think_ one way to \"cheat\" without storing too much information in\neach blame_entry is to first make a copy of all the original blame\nentries that share the \"suspect\", see if any line in the line range\nrepresented by each of the blame entries ended up being blamed to an\norigin with a path different from that of the \"suspect\". And print\nthose original blame entries that fits the criterion as additional\n\"these are not the final blame as you are digging with the option to\ndetect copy across files, but at this commit this line appeared anew\nas far as the origin->path is concerned\" output.\n\nSo I think you were going in the right direction (in other words,\nthe patch inserted new code to the right places), even though you\nmay have got the details in what the new code should do wrong.\n"},{"id":"218317","messageId":"CAMP44s0X-79Pn4gq_rqBbti-xUobyYDc8Gy-3-g=sq8siZqq8g@mail.gmail.com","threadId":"33873","inReplyTo":"7v8v35hc11.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T21:33:13Z","receivedAt":"2013-05-23T21:33:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 23, 2013 at 11:54 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Wed, May 22, 2013 at 11:07 PM, Felipe Contreras\n>> wrote:\n>>> On Wed, May 22, 2013 at 7:08 PM, Junio C Hamano wrote:\n>>>> Felipe Contreras writes:\n>>>>\n>>>>>> IIRC, git-gui runs two blames, one without any -C and one with (I do\n>>>>>> not offhand recall how many -C it uses) to show both.\n>>>>>\n>>>>> 'git blame' is a very expensive operation, perhaps we should add\n>>>>> another option so users don't need to run two blames to find this.\n>>>>\n>>>> Yes, you could add a \"struct origin *suspect_in_the_same_file\" next\n>>>> to the current \"struct origin *suspect\" to the blame_entry in order\n>>>> to represent the origin you would get if you were to stop at a \"move\n>>>> across files\" event, and keep digging, when you are operating under\n>>>> \"-C -C\" or higher mode.\n>>>\n>>> No, this is what I meant:\n>>\n>> But that would not print the right line numbers, so perhaps:\n>\n> Users of full-output may want to be able to see the same thing.\n>\n> I have a suspicion that you misread what assign_blame() does. The\n> first inner loop to find one \"suspect\" is really what it says. It\n> loops over blame entries in the scoreboard but it is not about\n> finding one \"entry\", but is to find one \"suspect\". The \"ent\" found\n> in the loop that you store in tmp_ent is no more special than any\n> other \"ent\" that shares the same \"suspect\" as its origin.\n\nIt is stored because it's the only structure that has the line\nnumbers. If the blame is passed to another 'ent', the original line\nnumbers are gone. We could manually copy the line numbers to three\nvariables, or we could copy the whole thing to one variable. The rest\nof the 'ent' doesn't matter.\n\n> Imagine that your scoreboard originally has three blocks of text\n> (i.e. blame_entry) A, B, and C, and the current suspect for A and C\n> are the same, while we already know what the final guilty party for\n> B is (which may be some descendant of the \"suspect\").\n\nI don't see how that's possible, but whatever.\n\n> Once we find one \"suspect\" in the first inner loop, the call to\n> pass_blame() does everything it can do to exonerate that \"suspect\".\n>\n> It runs a single diff between the suspect and each of its parents to\n> find lines in both A and C that can be blamed to one of these\n> parents, and blame entries A and C are split into pieces, some of\n> which still have the same suspect as their origin, which are where\n> their lines originate from, while others are attributable to these\n> parents or their ancestors.\n>\n> If you keep the original entry for A to do something special, like\n> printing what the original range of A was before it was split by\n> pass_blame(), wouldn't you need to do the same for C?\n\ntmp_ent is only used when there's no entry taken the guilt away from\nA. If the blame entry of A is split, and the result of the split has a\ndifferent filename, found_guilty() would not be called, and thus we\nwon't show the blame entry, and we want to show it.\n\nAnd yes, we would need to do the same for C, and we would once the\nturn of C comes along. If it's possible that A descendant from A takes\nthe blame for C, then sure, we would need to do ensure C is printed\nbefore it's gone, but pass_blame(A) would not destroy C.\n\n> We will not\n> be visiting C later in the outer while(1) loop, as a single call to\n> pass_blame() for \"suspect\" we did when we found it in A has already\n> taken care of it.\n\nIt took care of A's suspect, but not C. Isn't it possible that C is\nsplit further and creates another blame entry? Either way, in\n--incremental we want to print C as well, whether it's the guilty one\nor not.\n\n> I am not sure what you are trying to achieve with that found-guilty\n> logic, even if the \"save away in tmp_ent\" logic is changed to keep\n> all the blame entries that have \"suspect\" we are looking at as their\n> origin.\n\n> When the current suspect is found to have passed all lines\n> intact from its parents, we will see found-guilty left to be false.\n\nWhat? When a 'blame entry' passes the whole blame to a parent,\nfound_guilty is false, so we print the entry, which is exactly what we\nwant to do.\n\n> How would it make the original blame_entry (perhaps halfway) guilty\n> in that situation?\n\nI thought 'guilty' meant that it was guilty of adding those lines to\nthe 'final' text, so it ends up in the non-incremental blame. In that\nsense those blame entries are not guilty.\n\nBut they are still blame entries, and we want to print all blame\nentries in incremental, guilty or not.\n\n-- \nFelipe Contreras\n"},{"id":"218319","messageId":"7vppwhcqjq.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s0X-79Pn4gq_rqBbti-xUobyYDc8Gy-3-g=sq8siZqq8g@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T21:52:25Z","receivedAt":"2013-05-23T21:52:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Imagine that your scoreboard originally has three blocks of text\n>> (i.e. blame_entry) A, B, and C, and the current suspect for A and C\n>> are the same, while we already know what the final guilty party for\n>> B is (which may be some descendant of the \"suspect\").\n>\n> I don't see how that's possible, but whatever.\n\nThe tree in your latest commit HEAD has a file with block of text A\nfollowed by block of text B followed by block of text C. The latest\ncommit was the one that added B (perhaps new lines were inserted, or\nperhaps existing contiguous block of text was replaced, there is no\ndifference between these two cases). You start digging the history\nof this file from HEAD.\n\nYour scoreboard begins with a single blame-entry that covers all\nthree segments, with its suspect set to HEAD. Then pass_blame()\ndiscovers that top and bottom segments are older than this latest\ncommit, and splits the originally-one blame entry into three blame\nentries. The first and the third blame entries cover the block A\nand the block C respectively, and their suspect fields are both set\nto HEAD^. The second blame entry covers the block B and its suspect\ndoes not change (it still is HEAD). Then it returns to the caller.\n\nThe caller of pass_blame() looks at the scoreboard and finds these\nthree blame entries. The second one's supect is still the original\nsuspect the caller asked pass_blame() to exonerate blames for, and\nthe suspect failed to do so for block B. The final blame for the\nmiddle block is now known to be HEAD.\n\nAfter all of the above, the next iteration of while(1) loop begins.\nThat is how you arrive to the \"whatever\" situation. You have three\nblame entries, A, B and C, and suspect of A and C are the same,\nwhile B has a different suspect.\n\nThen in that \"next iteration\", we pick blame entry for A and decide\nto see if HEAD^, which is the suspect for A, can be exonerated of\nblames for _any_ (not just A) blame entry it currently is suspected\nfor. We call pass_blame() and it will find and process both A and\nC with a single \"git diff-tree\", attempting to pass blame to HEAD^^\nand its ancestors.\n"},{"id":"218321","messageId":"CAMP44s3LcEkTghg-cBRux7sA54L-U93w5Znqa1jz2MnUWj8Rhw@mail.gmail.com","threadId":"33873","inReplyTo":"7vppwhcqjq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T21:58:12Z","receivedAt":"2013-05-23T21:58:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 23, 2013 at 4:52 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> Imagine that your scoreboard originally has three blocks of text\n>>> (i.e. blame_entry) A, B, and C, and the current suspect for A and C\n>>> are the same, while we already know what the final guilty party for\n>>> B is (which may be some descendant of the \"suspect\").\n>>\n>> I don't see how that's possible, but whatever.\n>\n> The tree in your latest commit HEAD has a file with block of text A\n> followed by block of text B followed by block of text C. The latest\n> commit was the one that added B (perhaps new lines were inserted, or\n> perhaps existing contiguous block of text was replaced, there is no\n> difference between these two cases). You start digging the history\n> of this file from HEAD.\n>\n> Your scoreboard begins with a single blame-entry that covers all\n> three segments, with its suspect set to HEAD. Then pass_blame()\n> discovers that top and bottom segments are older than this latest\n> commit, and splits the originally-one blame entry into three blame\n> entries. The first and the third blame entries cover the block A\n> and the block C respectively, and their suspect fields are both set\n> to HEAD^. The second blame entry covers the block B and its suspect\n> does not change (it still is HEAD). Then it returns to the caller.\n>\n> The caller of pass_blame() looks at the scoreboard and finds these\n> three blame entries. The second one's supect is still the original\n> suspect the caller asked pass_blame() to exonerate blames for, and\n> the suspect failed to do so for block B. The final blame for the\n> middle block is now known to be HEAD.\n>\n> After all of the above, the next iteration of while(1) loop begins.\n> That is how you arrive to the \"whatever\" situation. You have three\n> blame entries, A, B and C, and suspect of A and C are the same,\n> while B has a different suspect.\n>\n> Then in that \"next iteration\", we pick blame entry for A and decide\n> to see if HEAD^, which is the suspect for A, can be exonerated of\n> blames for _any_ (not just A) blame entry it currently is suspected\n> for. We call pass_blame() and it will find and process both A and\n> C with a single \"git diff-tree\", attempting to pass blame to HEAD^^\n> and its ancestors.\n\nAll right, my code still works in that situation.\n\n-- \nFelipe Contreras\n"},{"id":"218325","messageId":"7vhahtco5b.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s3LcEkTghg-cBRux7sA54L-U93w5Znqa1jz2MnUWj8Rhw@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T22:44:16Z","receivedAt":"2013-05-23T22:44:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Then in that \"next iteration\", we pick blame entry for A and decide\n>> to see if HEAD^, which is the suspect for A, can be exonerated of\n>> blames for _any_ (not just A) blame entry it currently is suspected\n>> for. We call pass_blame() and it will find and process both A and\n>> C with a single \"git diff-tree\", attempting to pass blame to HEAD^^\n>> and its ancestors.\n>\n> All right, my code still works in that situation.\n\nWhen HEAD^ was processed, pass_blame() finds that the first line in\nA is attributable to HEAD^ (our current suspect) while the rest were\ncopied from a different file in HEAD^^ . All lines in C are found\nto be copy from a different file in HEAD^^.\n\nThen your scoreboard would have:\n\n 1. a blame entry that failed to pass blame to parents of HEAD^ (the\n first line in A), which still has suspect == HEAD^\n\n 2. a blame entry that covers the remainder of A, blaming HEAD^^.\n\n 3. a blame entry that covers all of B, whose final guilty party is\n known already.\n\n 4. a blame entry that covers all of C, blaming a different file in\n HEAD^^.\n\nYour \"Take responsibility\" loop goes over these blame entries, sets\nfound_guilty to true because of the first blame entry (the first\nline of A), and calls print_guilty_entry() for blame entry 1,\nshowing that HEAD^ is guilty for the first line of A.\n\nAfter the loop, your \"if we did not find anybody guilty\" logic does\nnot kick in, and the original line range for block A you saved in\ntmp_ent is not used.\n\nYou lost the fact that the second and remainder of A were in this\nfile at the commit HEAD^ but not in HEAD^^ (i.e. these lines were\nmoved by HEAD^ from elsewhere).\n\nThe fact that HEAD^ touched _something_ is not lost, so if _all_ you\ncare about is \"list all the commits, but I do not care about what\nline range was modified how\", you can claim it \"working\", but that\nis a very limited definition of \"working\" and is not very reusable\nor extensible in order to help those like gitk that currently have\nto run two separate blames. They need an output that lets them tell\nbetween\n\n - this is the earliest we saw these lines in the path (it may have\n been copied from another path, but this entry in the incremental\n output stream is not about that final blame); and\n\n - this is the final blame where these lines came from, possibly\n from different path\"\n\nand coalesce both kind of origin..\n\nAlso the fact that the entire C was copied from elsewhere by HEAD^\nis lost but that is the same issue. The next round will not find\nany blame entry that is !ent->guity because the call to pass_blame()\nfor HEAD^ already handled the final blame not just for blame entries\n1 and 2, but also for 4 during this round.\n\nIf the change in HEAD^ in the above example were to copy the whole A\nand C from a different file, then your !found_guilty logic would\nkick in and say all lines of A where copied from elsewhere in HEAD^,\nbut again we would not learn the same information for C.\n\nI do not think \"I care only about a single bit per commit, i.e. if\nthe commit touched the path; screw everybody else who wants to\nactually know where each line came from\" can be called \"working\".\n"},{"id":"218330","messageId":"CAMP44s14Gdadmgy2zi6dRnKLYdZ=HO85OiPSVxGZ8Hquv_58=g@mail.gmail.com","threadId":"33873","inReplyTo":"7vhahtco5b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-23T22:59:37Z","receivedAt":"2013-05-23T22:59:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 23, 2013 at 5:44 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> Then in that \"next iteration\", we pick blame entry for A and decide\n>>> to see if HEAD^, which is the suspect for A, can be exonerated of\n>>> blames for _any_ (not just A) blame entry it currently is suspected\n>>> for. We call pass_blame() and it will find and process both A and\n>>> C with a single \"git diff-tree\", attempting to pass blame to HEAD^^\n>>> and its ancestors.\n>>\n>> All right, my code still works in that situation.\n>\n> When HEAD^ was processed, pass_blame() finds that the first line in\n> A is attributable to HEAD^ (our current suspect) while the rest were\n> copied from a different file in HEAD^^ . All lines in C are found\n> to be copy from a different file in HEAD^^.\n>\n> Then your scoreboard would have:\n>\n> 1. a blame entry that failed to pass blame to parents of HEAD^ (the\n> first line in A), which still has suspect == HEAD^\n>\n> 2. a blame entry that covers the remainder of A, blaming HEAD^^.\n>\n> 3. a blame entry that covers all of B, whose final guilty party is\n> known already.\n>\n> 4. a blame entry that covers all of C, blaming a different file in\n> HEAD^^.\n>\n> Your \"Take responsibility\" loop goes over these blame entries, sets\n> found_guilty to true because of the first blame entry (the first\n> line of A), and calls print_guilty_entry() for blame entry 1,\n> showing that HEAD^ is guilty for the first line of A.\n>\n> After the loop, your \"if we did not find anybody guilty\" logic does\n> not kick in, and the original line range for block A you saved in\n> tmp_ent is not used.\n>\n> You lost the fact that the second and remainder of A were in this\n> file at the commit HEAD^ but not in HEAD^^ (i.e. these lines were\n> moved by HEAD^ from elsewhere).\n\n> The fact that HEAD^ touched _something_ is not lost, so if _all_ you\n> care about is \"list all the commits, but I do not care about what\n> line range was modified how\", you can claim it \"working\",\n\nThe line range printed is correct.\n\n> but that\n> is a very limited definition of \"working\" and is not very reusable\n> or extensible in order to help those like gitk that currently have\n> to run two separate blames. They need an output that lets them tell\n> between\n>\n> - this is the earliest we saw these lines in the path (it may have\n> been copied from another path, but this entry in the incremental\n> output stream is not about that final blame); and\n>\n> - this is the final blame where these lines came from, possibly\n> from different path\"\n>\n> and coalesce both kind of origin..\n\nThey can already do that by looking at the lines; if they get replaced\nby another entry; they are not guilty.\n\nOr we can add a 'guilty' line to the incremental output, that's trivial.\n\n> Also the fact that the entire C was copied from elsewhere by HEAD^\n> is lost but that is the same issue. The next round will not find\n> any blame entry that is !ent->guity because the call to pass_blame()\n> for HEAD^ already handled the final blame not just for blame entries\n> 1 and 2, but also for 4 during this round.\n\nNo, that's not true. If it's a different file the blame entry is not\nfound guilty.\n\n> If the change in HEAD^ in the above example were to copy the whole A\n> and C from a different file, then your !found_guilty logic would\n> kick in and say all lines of A where copied from elsewhere in HEAD^,\n> but again we would not learn the same information for C.\n\nWe would, when it's the turn for C, which is not guilty at this point.\n\n> I do not think \"I care only about a single bit per commit, i.e. if\n> the commit touched the path; screw everybody else who wants to\n> actually know where each line came from\" can be called \"working\".\n\nThey can know where each line came from; the lines of each entry are\nprinted properly; that's the whole point of 'tmp_ent'.\n\n-- \nFelipe Contreras\n"},{"id":"218335","messageId":"7vvc69b6nl.fsf@alter.siamese.dyndns.org","threadId":"33873","inReplyTo":"CAMP44s14Gdadmgy2zi6dRnKLYdZ=HO85OiPSVxGZ8Hquv_58=g@mail.gmail.com","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-23T23:47:26Z","receivedAt":"2013-05-23T23:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> If the change in HEAD^ in the above example were to copy the whole A\n>> and C from a different file, then your !found_guilty logic would\n>> kick in and say all lines of A where copied from elsewhere in HEAD^,\n>> but again we would not learn the same information for C.\n>\n> We would, when it's the turn for C, which is not guilty at this point.\n\nIn _this_ round of the while(1) loop, pass_blame_to_parent() gets\nthe scoreboard and two origins (HEAD^ that we are looking at and\nHEAD^^ that is its parent); it does not even know what blame entry\nthis request came from.\n\nIt runs a single diff using diff_hunks(), and asks blame_chunk() to\nsplit all the blame entries in the scoreboard that suspect it is\nlooking at may be guilty for. Blame entry for A and C are both\nprocessed exactly the same way when HEAD^ is given to pass_blame()\nfor the first time, which is when assign_blame() decided to call it\nwith HEAD^ because it happened to have seen A before seeing C. At\nthat point, both A and C are processed, and the post-processing loop\n\"Take responsibility for the remaining\" will clean up remnants from\nboth A and C. After this round ends, the suspect for A and C are\nboth set to HEAD^^.\n\nIn the next round of the while(1) loop, C already forgot that its\nline movement happened in HEAD^. Its suspect is now HEAD^^. When\n\"it's the turn for C\" [*1*], you can say \"These lines originate in\nthat different path in HEAD^^\", but it is too late to say \"But the\nfirst time they appeared in the original file was HEAD^\" (which is\nwhen they were moved from the different path in HEAD^^), isn't it?\n\n\n[Footnote]\n\n*1* It does not make much sense to say \"turn for C\", by the way.\n pass_blame() work per suspect (not per blame entry), so what you\n are talking about really is \"when it is turn for HEAD^^\", at\n which point we will discover that these lines in C originate in\n that different path in the commit. And in the scenario we are\n discussing, we will decide to check HEAD^^ because the blame ent\n for A is also the same origin as C's suspect in HEAD^^, so\n technically it is still \"turn for A\" ;-).\n\n The sample would be like\n\n\t(cat A; cat C) >file0; git commit file0\t ; HEAD^^\n\t(cat A; cat C) >file1; git commit file1\t ; HEAD^\n (cat A; cat B; cat C) >file1; git commit file1 ; HEAD\n git blame -C -C -C file1 HEAD\n\n\n \n"},{"id":"218340","messageId":"CAMP44s1GrtCFBdCRVDAAG+J24uDFiBc+mQ-jOesLdwVp-Qg_Dg@mail.gmail.com","threadId":"33873","inReplyTo":"7vvc69b6nl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-24T00:22:53Z","receivedAt":"2013-05-24T00:22:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 23, 2013 at 6:47 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> If the change in HEAD^ in the above example were to copy the whole A\n>>> and C from a different file, then your !found_guilty logic would\n>>> kick in and say all lines of A where copied from elsewhere in HEAD^,\n>>> but again we would not learn the same information for C.\n>>\n>> We would, when it's the turn for C, which is not guilty at this point.\n>\n> In _this_ round of the while(1) loop, pass_blame_to_parent() gets\n> the scoreboard and two origins (HEAD^ that we are looking at and\n> HEAD^^ that is its parent); it does not even know what blame entry\n> this request came from.\n>\n> It runs a single diff using diff_hunks(), and asks blame_chunk() to\n> split all the blame entries in the scoreboard that suspect it is\n> looking at may be guilty for. Blame entry for A and C are both\n> processed exactly the same way when HEAD^ is given to pass_blame()\n> for the first time, which is when assign_blame() decided to call it\n> with HEAD^ because it happened to have seen A before seeing C. At\n> that point, both A and C are processed, and the post-processing loop\n> \"Take responsibility for the remaining\" will clean up remnants from\n> both A and C. After this round ends, the suspect for A and C are\n> both set to HEAD^^.\n>\n> In the next round of the while(1) loop, C already forgot that its\n> line movement happened in HEAD^. Its suspect is now HEAD^^. When\n> \"it's the turn for C\" [*1*], you can say \"These lines originate in\n> that different path in HEAD^^\", but it is too late to say \"But the\n> first time they appeared in the original file was HEAD^\" (which is\n> when they were moved from the different path in HEAD^^), isn't it?\n\nIf that's the case then we'll need another list of blame entries where\neach discarded blame entry goes. Given the luck of my previous obvious\npatches, I'm not interested in implementing this non-obvious one in\nthe least.\n\nThe point is 'git related' should do -C -C -C, if 'git blame' doesn't\nthrow the right output, that's a bug in 'git blame' not 'git related.\n\n-- \nFelipe Contreras\n"}]}