{"thread":{"id":"33981","subject":"[PATCH v7] Add new git-related helper to contrib","startedAt":"2013-05-30T03:32:57Z","lastAt":"2013-05-31T08:28:28Z","messageCount":9,"participants":["Felipe Contreras","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":7,"patchTotal":null},"messages":[{"id":"218931","messageId":"1369884777-7227-1-git-send-email-felipe.contreras@gmail.com","threadId":"33981","inReplyTo":null,"subject":"[PATCH v7] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T03:32:57Z","receivedAt":"2013-05-30T03:32:57Z","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\nSmall changes since v6:\n\n--- a/contrib/related/git-related\n+++ b/contrib/related/git-related\n@@ -6,10 +6,6 @@\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@@ -25,13 +21,13 @@ class Commit\n if not msg\n case line\n when /^author ([^<>]+) <(\\S+)> (.+)$/\n- @persons << fmt_person($1, $2)\n+ @persons << '%s <%s>' % [$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+ @persons << '%s <%s>' % [$2, $3]\n end\n end\n end\n@@ -72,12 +68,12 @@ class Commits\n def get_blame(source, start, len, from)\n return if len == 0\n len ||= 1\n- File.popen(['git', 'blame', '--incremental', '-CCC',\n+ File.popen(['git', 'blame', '--incremental', '-C', '-C',\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+ if line =~ /^\\h{40}/\n id = $&\n @items[id] = Commit.new(id)\n end\n@@ -95,7 +91,7 @@ class Commits\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+ get_blame(source, $1, $2, from) if source and from\n end\n end\n end\n\n contrib/related/git-related | 120 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 120 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..1b9b1e7\n--- /dev/null\n+++ b/contrib/related/git-related\n@@ -0,0 +1,120 @@\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+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 << '%s <%s>' % [$1, $2]\n+ when /^$/\n+ msg = true\n+ end\n+ else\n+ if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\\S+?)>$/\n+ @persons << '%s <%s>' % [$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', '-C', '-C',\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) if source and 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.312.g47657de\n"},{"id":"218956","messageId":"CALkWK0=ZbOy6sXOvnTNAqz_UBsUymY1CR_WczT-O3Q+18HJjzQ@mail.gmail.com","threadId":"33981","inReplyTo":"1369884777-7227-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-30T09:01:11Z","receivedAt":"2013-05-30T09:01:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Let's do one more review.\n\nFelipe Contreras wrote:\n> diff --git a/contrib/related/git-related b/contrib/related/git-related\n> new file mode 100755\n> index 0000000..1b9b1e7\n> --- /dev/null\n> +++ b/contrib/related/git-related\n> @@ -0,0 +1,120 @@\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> +class Commit\n> +\n> + attr_reader :persons\n\nUnless you plan to introduce many more fields (I haven't looked at the\nlater patches), you might as well implement an #each, like in Commits.\n\n> + def initialize(id)\n> + @id = id\n> + @persons = []\n> + end\n> +\n> + def parse(data)\n> + msg = nil\n\nmsg = false, to indicate that it is a boolean.\n\n> + data.each_line do |line|\n> + if not msg\n> + case line\n> + when /^author ([^<>]+) <(\\S+)> (.+)$/\n> + @persons << '%s <%s>' % [$1, $2]\n\nWhy capture the third group when $3 is unused?\n\n> + when /^$/\n> + msg = true\n> + end\n> + else\n> + if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\\S+?)>$/\n> + @persons << '%s <%s>' % [$2, $3]\n\nWhy capture the first group when $1 is unused?\n\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\nDon't you need rb+ to suppress the CRLF nonsense on Windows?\n\n> + p.write(@items.keys.join(\"\\n\"))\n\nAs you might have realized, the parentheses are optional everywhere\n(except when it is required for disambiguation). I'm merely pointing\nit out here, because this line looks especially ugly.\n\n> + p.close_write\n> + p.each do |line|\n> + if line =~ /^(\\h{40}) commit (\\d+)/\n> + id, len = $1, $2\n\nid, len = $1, Integer $2. And drop the .to_i on the next line.\n\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\nI asked you to use 'len =1 if not len' for clarity, but you didn't like it.\n\n> + File.popen(['git', 'blame', '--incremental', '-C', '-C',\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\nFile.readlines(file).each do |line|.\n\n> + case line\n> + when /^From (\\h+) (.+)$/\n> + from = $1\n\nUseless capture.\n\n> + when /^---\\s+(\\S+)/\n> + source = $1 != '/dev/null' ? $1[2..-1] : nil\n> + when /^@@ -(\\d+)(?:,(\\d+))?/\n> + get_blame(source, $1, $2, from) if source and from\n\nUseless capture. When is len ($2) going to be nil?\n\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\ncommits.each do |_, commit|, since you're not using id.\n\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\nI prefer 'Float count' over count.to_f, but that's just a matter of taste.\n\n> + next if percent < $min_percent\n> + puts person\n> +end\n> --\n> 1.8.3.rc3.312.g47657de\n"},{"id":"218965","messageId":"CAMP44s25vX1p1Np7yqc9_AqVBme+MCTY88hjhfWdL6KZkxgs7Q@mail.gmail.com","threadId":"33981","inReplyTo":"CALkWK0=ZbOy6sXOvnTNAqz_UBsUymY1CR_WczT-O3Q+18HJjzQ@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:31:20Z","receivedAt":"2013-05-30T11:31:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 4:01 AM, Ramkumar Ramachandra\n wrote:\n> Let's do one more review.\n>\n> Felipe Contreras wrote:\n>> diff --git a/contrib/related/git-related b/contrib/related/git-related\n>> new file mode 100755\n>> index 0000000..1b9b1e7\n>> --- /dev/null\n>> +++ b/contrib/related/git-related\n>> @@ -0,0 +1,120 @@\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>> +class Commit\n>> +\n>> + attr_reader :persons\n>\n> Unless you plan to introduce many more fields (I haven't looked at the\n> later patches), you might as well implement an #each, like in Commits.\n\ncommit.each doesn't make sense; each what?\n\nWe could do 'commit.each_person', but why is that so better than\ncommit.persons.each? It's not.\n\n>> + data.each_line do |line|\n>> + if not msg\n>> + case line\n>> + when /^author ([^<>]+) <(\\S+)> (.+)$/\n>> + @persons << '%s <%s>' % [$1, $2]\n>\n> Why capture the third group when $3 is unused?\n\nCompleteness.\n\n>> + when /^$/\n>> + msg = true\n>> + end\n>> + else\n>> + if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\\S+?)>$/\n>> + @persons << '%s <%s>' % [$2, $3]\n>\n> Why capture the first group when $1 is unused?\n\nYou want to complicate the regex even more with:\n\n/^(?:Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\\S+?)>$/\n\nFor what purpose?\n\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>\n> Don't you need rb+ to suppress the CRLF nonsense on Windows?\n\nWho knows.\n\n>> + p.write(@items.keys.join(\"\\n\"))\n>\n> As you might have realized, the parentheses are optional everywhere\n> (except when it is required for disambiguation). I'm merely pointing\n> it out here, because this line looks especially ugly.\n\nI suspect most Git developers would prefer the traditional function call style.\n\n>> + p.close_write\n>> + p.each do |line|\n>> + if line =~ /^(\\h{40}) commit (\\d+)/\n>> + id, len = $1, $2\n>\n> id, len = $1, Integer $2. And drop the .to_i on the next line.\n\nThis is way is better.\n\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>\n> I asked you to use 'len =1 if not len' for clarity, but you didn't like it.\n\ngit grep \"||=\" disagrees.\n\n>> + File.popen(['git', 'blame', '--incremental', '-C', '-C',\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>\n> File.readlines(file).each do |line|.\n\nThat's less efficient.\n\n>> + when /^---\\s+(\\S+)/\n>> + source = $1 != '/dev/null' ? $1[2..-1] : nil\n>> + when /^@@ -(\\d+)(?:,(\\d+))?/\n>> + get_blame(source, $1, $2, from) if source and from\n>\n> Useless capture. When is len ($2) going to be nil?\n\nJunio already went over it, see the diff format.\n\nWhat's your objective? Block this patch from ever going in?\n\nNot a single one of these comments makes a difference at all, all of\nthem can wait until after the patch is merged, many of them are a\nmatter of preferences, and some of them have already been addressed as\nprecisely that: disagreements in style.\n\n-- \nFelipe Contreras\n"},{"id":"218983","messageId":"CALkWK0=LfhDtOAgsq2KYSCbTocf2R8FjTWg4S6jxGiGxDxeg1A@mail.gmail.com","threadId":"33981","inReplyTo":"CAMP44s25vX1p1Np7yqc9_AqVBme+MCTY88hjhfWdL6KZkxgs7Q@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-30T12:08:25Z","receivedAt":"2013-05-30T12:08:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> What's your objective? Block this patch from ever going in?\n>\n> Not a single one of these comments makes a difference at all, all of\n> them can wait until after the patch is merged, many of them are a\n> matter of preferences, and some of them have already been addressed as\n> precisely that: disagreements in style.\n\nYou posted a patch, and I reviewed it. End of story. I never\nexplicitly or implicitly indicated that I want to block the patch, so\nstop pulling stuff out of your arse.\n\nIf you don't want a review, write \"DO NOT REVIEW\" (or better yet,\ndon't hit my inbox). I'm not interested in wasting my time either.\n"},{"id":"218984","messageId":"CAMP44s0ystM--SJgY-zgM+KKC9NKfHK_jF-w+nUN2tZ1dXmzJw@mail.gmail.com","threadId":"33981","inReplyTo":"CALkWK0=LfhDtOAgsq2KYSCbTocf2R8FjTWg4S6jxGiGxDxeg1A@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T12:12:13Z","receivedAt":"2013-05-30T12:12:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 7:08 AM, Ramkumar Ramachandra\n wrote:\n> Felipe Contreras wrote:\n>> What's your objective? Block this patch from ever going in?\n>>\n>> Not a single one of these comments makes a difference at all, all of\n>> them can wait until after the patch is merged, many of them are a\n>> matter of preferences, and some of them have already been addressed as\n>> precisely that: disagreements in style.\n>\n> You posted a patch, and I reviewed it. End of story. I never\n> explicitly or implicitly indicated that I want to block the patch, so\n> stop pulling stuff out of your arse.\n\nThat's exactly what you are doing.\n\nDo you see any other reason for not merging this if not your comments?\n\n-- \nFelipe Contreras\n"},{"id":"219080","messageId":"CAMP44s0peXDtFB31pEqF49xM8z5OtDKAWaOR65aZSkCndL2bhQ@mail.gmail.com","threadId":"33981","inReplyTo":"CAMP44s25vX1p1Np7yqc9_AqVBme+MCTY88hjhfWdL6KZkxgs7Q@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-31T07:49:30Z","receivedAt":"2013-05-31T07:49:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 6:31 AM, Felipe Contreras\n wrote:\n> On Thu, May 30, 2013 at 4:01 AM, Ramkumar Ramachandra\n\n> Not a single one of these comments makes a difference at all, all of\n> them can wait until after the patch is merged, many of them are a\n> matter of preferences, and some of them have already been addressed as\n> precisely that: disagreements in style.\n\nI was going to make these stylistic changes to make you happy, but\nthen I realized the only that does really make sense is to change msg\n= nil to msg = false, and it's not even worth to waste a thought on\nchanges like that.\n\nSo I'll move on to the next patches, hopefully Duy or Junio would have\nsome comments of actual significance, or maybe you would too, but for\nthe moment it seems pretty clear you are only stating opinions about\nwhat Ruby code-style you like best.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"219081","messageId":"CALkWK0nVDBv9bfFpAJ+pzON-uD_4HQ0Bcg=CN32fgFvNJnnjgA@mail.gmail.com","threadId":"33981","inReplyTo":"CAMP44s0peXDtFB31pEqF49xM8z5OtDKAWaOR65aZSkCndL2bhQ@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-31T08:03:14Z","receivedAt":"2013-05-31T08:03:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> I was going to make these stylistic changes to make you happy, but\n> then I realized the only that does really make sense is to change msg\n> = nil to msg = false, and it's not even worth to waste a thought on\n> changes like that.\n\nWe don't have existing Ruby code in git.git to follow, so what I say\ncan obviously not have more weightage than \"personal opinion\". Don't\ndo things to \"make me happy\"; I am nobody. Have a good sense of style\nand defend it instead of flaming me because you thought I was stalling\nwork.\n\n\"Some of these style changes seem to make sense, and I'll fix them\nafter this patch gets merged.\"\n\nor\n\n\"I have a different opinion on these issues, and here's why: \"\n\nwould have both been appropriate responses.\n\n> So I'll move on to the next patches, hopefully Duy or Junio would have\n> some comments of actual significance, or maybe you would too, but for\n> the moment it seems pretty clear you are only stating opinions about\n> what Ruby code-style you like best.\n\nI've done reviews of several iterations of this patch, and stylistic\ncomments were all that I had left: I might not be adequately\nintelligent to come up with \"comments of actual significance\", but\nthat's a limitation you're going to have to deal with.\n"},{"id":"219083","messageId":"CAMP44s1z8CvF_K4uTn=p2c2pbjv8TKRyMo7QXR2CAqopOnyg_g@mail.gmail.com","threadId":"33981","inReplyTo":"CALkWK0nVDBv9bfFpAJ+pzON-uD_4HQ0Bcg=CN32fgFvNJnnjgA@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-31T08:14:33Z","receivedAt":"2013-05-31T08:14:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, May 31, 2013 at 3:03 AM, Ramkumar Ramachandra\n wrote:\n> Felipe Contreras wrote:\n>> I was going to make these stylistic changes to make you happy, but\n>> then I realized the only that does really make sense is to change msg\n>> = nil to msg = false, and it's not even worth to waste a thought on\n>> changes like that.\n>\n> We don't have existing Ruby code in git.git to follow, so what I say\n> can obviously not have more weightage than \"personal opinion\". Don't\n> do things to \"make me happy\"; I am nobody. Have a good sense of style\n> and defend it instead of flaming me because you thought I was stalling\n> work.\n\nI already defended the style, only to hear the same comment again in\nthe next iteration.\n\n>> So I'll move on to the next patches, hopefully Duy or Junio would have\n>> some comments of actual significance, or maybe you would too, but for\n>> the moment it seems pretty clear you are only stating opinions about\n>> what Ruby code-style you like best.\n>\n> I've done reviews of several iterations of this patch, and stylistic\n> comments were all that I had left:\n\nWhen a reviewer reaches that point, (s)he usually says: other than\ncosmetic preferences: Reviewed-by: me.\n\n-- \nFelipe Contreras\n"},{"id":"219088","messageId":"CALkWK0=N85vBSOxSh87Z6vKiu7JZFDzrxTQurn3dE4o6N2gbRw@mail.gmail.com","threadId":"33981","inReplyTo":"CAMP44s1z8CvF_K4uTn=p2c2pbjv8TKRyMo7QXR2CAqopOnyg_g@mail.gmail.com","subject":"Re: [PATCH v7] Add new git-related helper to contrib","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-31T08:28:28Z","receivedAt":"2013-05-31T08:28:28Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> When a reviewer reaches that point, (s)he usually says: other than\n> cosmetic preferences: Reviewed-by: me.\n\nYes, you can have a Reviewed-by from me if you want.\n\nAnd yes, it is appropriate not to CC me on v8 and v9, since I have no\nfurther comments.\n"}]}