{"thread":{"id":"21248","subject":"My custom cccmd","startedAt":"2009-10-15T13:20:02Z","lastAt":"2009-11-02T14:25:57Z","messageCount":8,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"125074","messageId":"94a0d4530910150620g733bdf0aq88660053f869b0a9@mail.gmail.com","threadId":"21248","inReplyTo":null,"subject":"My custom cccmd","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-10-15T13:20:02Z","receivedAt":"2009-10-15T13:20:02Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nI love the new option to run a cccmd and how good it works on the\nlinux kernel, but I couldn't find a generic script. So I decided to\nwrite my own.\n\nIt's very simple, it just looks into the authors of the commits that\nmodified the lines being overridden (git blame). It's not checking for\ns-o-b, or anything fancy.\n\nComments?\n\n#!/usr/bin/env ruby\n\n@commits = {} # keeps a count of commits per author\n\nARGV.each do |filename|\n  File.open(filename) do |patch_file|\n    patch_file.each_line do |patch_line|\n      case patch_line\n      when /^---\\s+(\\S+)/\n        @source = $1[2..-1]\n      when /^@@\\s-(\\d+),(\\d+)/\n        blame = `git blame -p -L #{$1},+#{$2} #{@source} | grep author`\n        blame.each_line do |al|\n          key, value = al.chomp.split(\" \", 2)\n          case key\n          when \"author\"\n            @name = value\n          when \"author-mail\"\n            @mail = value\n            author = \"\\\"#{@name}\\\" #{@mail}\"\n            @commits[author] ||= 0\n            @commits[author] += 1\n          end\n        end\n      end\n    end\n  end\nend\n\n@commits.each_key do |a|\n  puts a\nend\n\n-- \nFelipe Contreras\n"},{"id":"125101","messageId":"7vk4yw4dy3.fsf@alter.siamese.dyndns.org","threadId":"21248","inReplyTo":"94a0d4530910150620g733bdf0aq88660053f869b0a9@mail.gmail.com","subject":"Re: My custom cccmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-15T20:09:08Z","receivedAt":"2009-10-15T20:09:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Hi,\n>\n> I love the new option to run a cccmd and how good it works on the\n> linux kernel, but I couldn't find a generic script. So I decided to\n> write my own.\n>\n> It's very simple, it just looks into the authors of the commits that\n> modified the lines being overridden (git blame). It's not checking for\n> s-o-b, or anything fancy.\n>\n> Comments?\n\n> #!/usr/bin/env ruby\n>\n> @commits = {} # keeps a count of commits per author\n>\n> ARGV.each do |filename|\n>   File.open(filename) do |patch_file|\n>     patch_file.each_line do |patch_line|\n>       case patch_line\n>       when /^---\\s+(\\S+)/\n>         @source = $1[2..-1]\n>       when /^@@\\s-(\\d+),(\\d+)/\n>         blame = `git blame -p -L #{$1},+#{$2} #{@source} | grep author`\n>         blame.each_line do |al|\n>           key, value = al.chomp.split(\" \", 2)\n>           case key\n>           when \"author\"\n>             @name = value\n>           when \"author-mail\"\n>             @mail = value\n>             author = \"\\\"#{@name}\\\" #{@mail}\"\n>             @commits[author] ||= 0\n>             @commits[author] += 1\n>           end\n>         end\n>       end\n>     end\n>   end\n> end\n\nComments.\n\n #0. Gaahhh, my eyes, my eyes!!  Can't you do this ugly run of infinite\n     number of \"end\"s?\n\n #1. You are not making sure that you start blaming from the commit the\n     patch is based on, so your -La,b line numbers can be off.  If you can\n     assume that you are always reading format-patch output, you can learn\n     which commit to start from by reading the first \"magic\" line.\n     \n #2. If you have two patch series that updates one file twice, some\n     changes in your second patch could even be an update to the changes\n     you introduced in your first patch.  After you fix issue #1, you\n     would probably want to fix this by excluding the commits you have\n     already sent the blames for.\n\n #3. Does the number of commits you keep per author have any significance?\n     I know it doesn't in the implementation you posted, but should it,\n     and if so how?\n\n> @commits.each_key do |a|\n>   puts a\n> end\n>\n> -- \n> Felipe Contreras\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"125107","messageId":"94a0d4530910151437s780bd96anca82d2b26ef99e0a@mail.gmail.com","threadId":"21248","inReplyTo":"7vk4yw4dy3.fsf@alter.siamese.dyndns.org","subject":"Re: My custom cccmd","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-10-15T21:37:36Z","receivedAt":"2009-10-15T21:37:36Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 15, 2009 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Hi,\n>>\n>> I love the new option to run a cccmd and how good it works on the\n>> linux kernel, but I couldn't find a generic script. So I decided to\n>> write my own.\n>>\n>> It's very simple, it just looks into the authors of the commits that\n>> modified the lines being overridden (git blame). It's not checking for\n>> s-o-b, or anything fancy.\n>>\n\n<snip/>\n\n> Comments.\n>\n>  #0. Gaahhh, my eyes, my eyes!!  Can't you do this ugly run of infinite\n>     number of \"end\"s?\n\nHehe, sure. Will do.\n\n>  #1. You are not making sure that you start blaming from the commit the\n>     patch is based on, so your -La,b line numbers can be off.  If you can\n>     assume that you are always reading format-patch output, you can learn\n>     which commit to start from by reading the first \"magic\" line.\n\nThe 'From' magic line points to the actual commit the patch was\ngenerated from, so it would actually be @from^.\n\nThis of course would only work if the patches have the corresponding\ncommits in the current tree (which is the case most of the time).\n\nAnd makes sense only for the first patch, the rest of the patches\nwould use a wrong commit as a base. See below.\n\n>  #2. If you have two patch series that updates one file twice, some\n>     changes in your second patch could even be an update to the changes\n>     you introduced in your first patch.  After you fix issue #1, you\n>     would probably want to fix this by excluding the commits you have\n>     already sent the blames for.\n\nHow exactly? By looking at the commit from 'git blame' and discarding\nit? That would be a bit tricky since each instance of 'cccmd' is not\naware of the previous ones.\n\n>  #3. Does the number of commits you keep per author have any significance?\n>     I know it doesn't in the implementation you posted, but should it,\n>     and if so how?\n\nNot currently. Once I add support for s-o-b it might be useful.\nCurrently I left it in order to order the CC's by the count, but it\nturned out to be a bit messier than I thought, and the advantage is\nalmost nothing.\n\nI'll clean it up.\n\nTaking in consideration the previous comments, here is v2:\n\n#!/usr/bin/env ruby\n\n@authors = {}\n\ndef parse_blame(line)\n  key, value = line.split(\" \", 2)\n  case key\n  when \"author\"\n    @name = value\n  when \"author-mail\"\n    @mail = value\n    author = \"\\\"#{@name}\\\" #{@mail}\"\n    @authors[author] = true\n  end\nend\n\nARGV.each do |filename|\n  patch_file = File.open(filename)\n  patch_file.each_line do |patch_line|\n    case patch_line\n    when /^---\\s+(\\S+)/\n      @source = $1[2..-1]\n    when /^@@\\s-(\\d+),(\\d+)/\n      blame = `git blame -p -L #{$1},+#{$2} #{@source} | grep author`\n      blame.each_line { |l| parse_blame(l.chomp) }\n    end\n  end\nend\n\n@authors.each_key do |a|\n  puts a\nend\n\n-- \nFelipe Contreras\n"},{"id":"125885","messageId":"94a0d4530910250804w3a7da36eke10710eb1cbb03c1@mail.gmail.com","threadId":"21248","inReplyTo":"94a0d4530910151437s780bd96anca82d2b26ef99e0a@mail.gmail.com","subject":"Re: My custom cccmd","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-10-25T15:04:44Z","receivedAt":"2009-10-25T15:04:44Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Oct 15, 2009 at 11:37 PM, Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n> On Thu, Oct 15, 2009 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>  #2. If you have two patch series that updates one file twice, some\n>>     changes in your second patch could even be an update to the changes\n>>     you introduced in your first patch.  After you fix issue #1, you\n>>     would probably want to fix this by excluding the commits you have\n>>     already sent the blames for.\n>\n> How exactly? By looking at the commit from 'git blame' and discarding\n> it? That would be a bit tricky since each instance of 'cccmd' is not\n> aware of the previous ones.\n\nI explored this a bit more and I've come to the conclusion that we\nactually don't wand to discard the previous commits in the patch\nseries. Let's think about this example:\n0001 <- John\n0002 <- Me overriding some changes from John\n\nIn this case we want John to appear in the CC list of 0002, because we\nare changing his code. If it turns out that the whole patch series was\nwritten by me, it's still ok for me to appear on the CC list, which\nwould essentially be \"discarding\" those commits.\n\nSo all we need to do is pick @from^ as the base of the git blame:\n@@ -17,11 +17,12 @@ end\n ARGV.each do |filename|\n   patch_file = File.open(filename)\n   patch_file.each_line do |patch_line|\n+    @from ||= patch_line[/From (\\w+)/, 1]\n     case patch_line\n     when /^---\\s+(\\S+)/\n       @source = $1[2..-1]\n     when /^@@\\s-(\\d+),(\\d+)/\n-      blame = `git blame -p -L #{$1},+#{$2} #{@source} | grep author`\n+      blame = `git blame --incremental -L #{$1},+#{$2} #{@source}\n#{@from}^ | grep author`\n       blame.each_line { |l| parse_blame(l.chomp) }\n     end\n   end\n\nYou can find the whole script here:\nhttp://gist.github.com/218093\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"126061","messageId":"7vzl7c4i81.fsf@alter.siamese.dyndns.org","threadId":"21248","inReplyTo":"94a0d4530910250804w3a7da36eke10710eb1cbb03c1@mail.gmail.com","subject":"Re: My custom cccmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-27T21:53:02Z","receivedAt":"2009-10-27T21:53:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> I explored this a bit more and I've come to the conclusion that we\n> actually don't wand to discard the previous commits in the patch\n> series. Let's think about this example:\n> 0001 <- John\n> 0002 <- Me overriding some changes from John\n>\n> In this case we want John to appear in the CC list of 0002, because we\n> are changing his code.\n\nThere are two cases: your patch partially overrides his code, and your\npatch completely rewrites/removes his code.\n\nObviously in the former case you do want to Cc, but there are parts of his\nchange that remain in the final result, so this case does not matter in\nthis discussion.  You will Cc him because of these remaining lines anyway.\n\nIn the latter case, the only contribution that remains from him is a\ncommit with his log message that does not help describing anything in the\nend result, cluttering the history.\n\nIn such a case, I would imagine that the reviewers would want to see a\ncleaned up series that does not have his patch that is irrelevant for\nunderstanding the final result.  John might want to know about it, if only\nto raise objection to the way how you arranged your series.  For that\nreason, I think it makes sense to Cc him.\n\nBut it is a rather a convoluted logic, if you ask me.  You find John and\nCc him, primarily so that he can point out that you should be redoing the\nseries not to have his patch as an intermediate state in the series to\nbegin with, because his commit does not contribute to the end result.\n\nIt might make more sense if your tool told you about such a case directly,\nrather than helping you find John so that he can tell you ;-).\n"},{"id":"126361","messageId":"94a0d4530910300139l2f20e3aaw2f89e0b809a7246c@mail.gmail.com","threadId":"21248","inReplyTo":"7vzl7c4i81.fsf@alter.siamese.dyndns.org","subject":"Re: My custom cccmd","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-10-30T08:39:18Z","receivedAt":"2009-10-30T08:39:18Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Oct 27, 2009 at 11:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> I explored this a bit more and I've come to the conclusion that we\n>> actually don't wand to discard the previous commits in the patch\n>> series. Let's think about this example:\n>> 0001 <- John\n>> 0002 <- Me overriding some changes from John\n>>\n>> In this case we want John to appear in the CC list of 0002, because we\n>> are changing his code.\n>\n> There are two cases: your patch partially overrides his code, and your\n> patch completely rewrites/removes his code.\n>\n> Obviously in the former case you do want to Cc, but there are parts of his\n> change that remain in the final result, so this case does not matter in\n> this discussion.  You will Cc him because of these remaining lines anyway.\n>\n> In the latter case, the only contribution that remains from him is a\n> commit with his log message that does not help describing anything in the\n> end result, cluttering the history.\n>\n> In such a case, I would imagine that the reviewers would want to see a\n> cleaned up series that does not have his patch that is irrelevant for\n> understanding the final result.  John might want to know about it, if only\n> to raise objection to the way how you arranged your series.  For that\n> reason, I think it makes sense to Cc him.\n>\n> But it is a rather a convoluted logic, if you ask me.  You find John and\n> Cc him, primarily so that he can point out that you should be redoing the\n> series not to have his patch as an intermediate state in the series to\n> begin with, because his commit does not contribute to the end result.\n>\n> It might make more sense if your tool told you about such a case directly,\n> rather than helping you find John so that he can tell you ;-).\n\nBut that's not the purpose of the cccmd tool.\n\nI agree that such \"patch series simplificator\" tool would be very\nuseful, but that's out of scope for this. Isn't it?\n\n-- \nFelipe Contreras\n"},{"id":"126438","messageId":"7vr5skim76.fsf@alter.siamese.dyndns.org","threadId":"21248","inReplyTo":"94a0d4530910300139l2f20e3aaw2f89e0b809a7246c@mail.gmail.com","subject":"Re: My custom cccmd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-30T21:52:29Z","receivedAt":"2009-10-30T21:52:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> On Tue, Oct 27, 2009 at 11:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>\n>>> I explored this a bit more and I've come to the conclusion that we\n>>> actually don't wand to discard the previous commits in the patch\n>>> series. Let's think about this example:\n>>> 0001 <- John\n>>> 0002 <- Me overriding some changes from John\n>>>\n>>> In this case we want John to appear in the CC list of 0002, because we\n>>> are changing his code.\n>> ...\n>> In such a case, I would imagine that the reviewers would want to see a\n>> cleaned up series that does not have his patch that is irrelevant for\n>> understanding the final result.  John might want to know about it, if only\n>> to raise objection to the way how you arranged your series.  For that\n>> reason, I think it makes sense to Cc him.\n>>\n>> But it is a rather a convoluted logic, if you ask me.  You find John and\n>> Cc him, primarily so that he can point out that you should be redoing the\n>> series not to have his patch as an intermediate state in the series to\n>> begin with, because his commit does not contribute to the end result.\n>>\n>> It might make more sense if your tool told you about such a case directly,\n>> rather than helping you find John so that he can tell you ;-).\n>\n> But that's not the purpose of the cccmd tool.\n>\n> I agree that such \"patch series simplificator\" tool would be very\n> useful, but that's out of scope for this. Isn't it?\n\nExactly.\n\nSo you agree that you _do_ want to \"discard the previous commits in the\npatch series\", because not doing so would mean the result would be a\nhalf-cooked \"patch series simplificator\" that tries to do something that\nis outside the scope of cccmd, right?\n\nThe \"discarding the previous commits\" happens to match what I suggested\nearlier that lead to your \"explored this a bit more\":\n\nOn Thu, Oct 15, 2009 at 11:37 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:\n> On Thu, Oct 15, 2009 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>  #2. If you have two patch series that updates one file twice, some\n>>     changes in your second patch could even be an update to the changes\n>>     you introduced in your first patch.  After you fix issue #1, you\n>>     would probably want to fix this by excluding the commits you have\n>>     already sent the blames for.\n\nso I think we are in agreement.\n"},{"id":"126588","messageId":"94a0d4530911020625i250c5d44rbf6dfc68fc69b520@mail.gmail.com","threadId":"21248","inReplyTo":"7vr5skim76.fsf@alter.siamese.dyndns.org","subject":"Re: My custom cccmd","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-02T14:25:57Z","receivedAt":"2009-11-02T14:25:57Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Oct 30, 2009 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> On Tue, Oct 27, 2009 at 11:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> It might make more sense if your tool told you about such a case directly,\n>>> rather than helping you find John so that he can tell you ;-).\n>>\n>> But that's not the purpose of the cccmd tool.\n>>\n>> I agree that such \"patch series simplificator\" tool would be very\n>> useful, but that's out of scope for this. Isn't it?\n>\n> Exactly.\n>\n> So you agree that you _do_ want to \"discard the previous commits in the\n> patch series\", because not doing so would mean the result would be a\n> half-cooked \"patch series simplificator\" that tries to do something that\n> is outside the scope of cccmd, right?\n>\n> The \"discarding the previous commits\" happens to match what I suggested\n> earlier that lead to your \"explored this a bit more\":\n>\n> On Thu, Oct 15, 2009 at 11:37 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:\n>> On Thu, Oct 15, 2009 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>  #2. If you have two patch series that updates one file twice, some\n>>>     changes in your second patch could even be an update to the changes\n>>>     you introduced in your first patch.  After you fix issue #1, you\n>>>     would probably want to fix this by excluding the commits you have\n>>>     already sent the blames for.\n>\n> so I think we are in agreement.\n\nCool. Is this script something that would make sense in the contrib section?\n\n-- \nFelipe Contreras\n"}]}