Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Would it make sense to integrate this in git shortlog, which already does something similar? Conceptually, yes, but the end result will be much larger in scope. I am not sure if shortlog is still a good label for it. since we are throwing ideas around... The first place where I would logically look for such a feature would be in git-blame --cc-list or something like that. git-blame seems to me as a logical place for all look at history and give me a list of names type commands -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Johannes Sixt j...@kdbg.org writes: But I think it can be useful outside the context of send-email as well, and having one independent tool that does one single job well is a better design. Perhaps it is better to name it less specific to send-email's cc-cmd option. git people? git whom? git reviewers? I dunno, but along those lines. Would it make sense to integrate this in git shortlog, which already does something similar? Conceptually, yes, but the end result will be much larger in scope. I am not sure if shortlog is still a good label for it. shortlog, when it internally runs log [*1*], is still about the commits within the range given to the command; shortlog A..B talks only about commits within A..B range. This new thing is about what happened to the part of the code A..B touches in the past (i.e. before A happened), which feels a bit different. [Footnote] *1* It can be used as a filter to git log output, which is a bit different animal, but it still is about shortening that incoming log, not about independently digging the history using the input as a starting point. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Felipe Contreras wrote: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. Finally, it calculates what percentage of the total relevant commits each person was involved in, and show only the ones that pass the threshold. Um, this didn't really explain it to me. How about: This command takes a patch prepared by 'git format-patch' as an argument, and runs 'git blame' on every hunk of the diff to determine the commits that added/removed those lines. It then aggregates the authors and signers of all the commits, and prints out these people in descending order of the percentage of commits they were responsible for. It omits people are below a certain threshold. % git cc-cmd 0001-remote-hg-trivial-cleanups.patch Felipe Contreras felipe.contre...@gmail.com (author: 100%) Jeff King p...@peff.net (signer: 83%) Max Horn m...@quendi.de (signer: 16%) Junio C Hamano gits...@pobox.com (signer: 16%) Won't my name appear as the first one on each of my patches? Will I be CC'ed on every patch that I send out? contrib/cc-cmd/git-cc-cmd | 140 ++ 1 file changed, 140 insertions(+) create mode 100755 contrib/cc-cmd/git-cc-cmd No README? diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd new file mode 100755 index 000..c7ecf79 --- /dev/null +++ b/contrib/cc-cmd/git-cc-cmd @@ -0,0 +1,140 @@ +#!/usr/bin/env ruby What is the minimum required version of Ruby, by the way? I see you haven't used any fancy lazy enumerators or optional arguments introduced in 2.0, so we should be okay practically speaking. +$since = '3-years-ago' +$min_percent = 5 Do I get to configure these two? Also, the '$' prefix from your Perl habit? It's not idiomatic in Ruby afaik. +class Commit + + attr_reader :id + attr_accessor :roles + + def initialize(id) +@id = id +@roles = [] What are id, roles exactly? This isn't C where we have types, so you're going to have to use some (rdoc-parseable) comments, if you want me to be able to read the code without jumping around too much. + end + + def self.parse(data) +id = author = msg = nil This is not C. You don't need to initialize stuff, unless you're going to start accessing it using a .append() or similar. In that case, you need to initialize it as an empty list, so the compiler knows what you're appending to. +roles = {} Shouldn't this be @roles? Didn't you initialize it as an empty list in initialize()? Why did you suddenly change your mind and make it a hashtable now? +data.each_line do |line| + if not msg +case line +when /^commit (.+)$/ + id = $1 Okay, I have no idea what you're parsing yet. There's some commit line, so I know this is not a raw commit object, as the name of the class Commit would've indicated. +when /^author ([^]+) (\S+)$/ + author = $1, $2 + roles[author] = 'author' Huh? the key is a two-item list, and the value is a hard-coded 'author' string? If it's meant to be a sematic value, use a symbol. Also, why don't you just collect the authors and signers in two separate lists, instead of doing this and burdening yourself with accumulation later? +when /^$/ + msg = true I can't see msg being used later in the function, so I don't know what you're doing. +roles = roles.map do |person, role| + address = %s %s % person + [person, role] What?! If you wanted address in the first place, why did you stuff a two-member list into roles as the key? Where is address being used? + def import +return if @items.empty? +format = [ 'commit %H', 'author %an %ae', '', '%B' ].join('%n') +File.popen(['git', 'show', '-z', '-s', '--format=format:' + format] + @items.keys) do |p| Ah, you're using 'git show'. I thought cat-file --batch was especially well-suited for this task. What do you need the pretty-printing for? + p.each(\0) do |data| +next if data == \0 # bug in git show? What is the bug exactly? It displays two consecutive \0 characters sometimes, when given a bunch of items to display? +id, roles = Commit.parse(data) +commit = @items[id] +commit.roles = roles @items[id].roles = roles + def each_person_role +commit_roles = @items.values.map { |commit| commit.roles }.flatten(1) +commit_roles.group_by { |person, role| person }.each do |person, commit_roles| + commit_roles.group_by { |person, role| role }.each do |role, commit_roles| +yield person, role, commit_roles.size Unnecessary work if you'd chosen a better way to store the person = role mapping in the first place. + def get_blame(source, start, offset, from) +return unless source +File.popen(['git', 'blame', '--incremental', '-C', + '-L', '%u,+%u' %
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
On Fri, Apr 19, 2013 at 8:26 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Felipe Contreras wrote: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. Finally, it calculates what percentage of the total relevant commits each person was involved in, and show only the ones that pass the threshold. Um, this didn't really explain it to me. How about: This command takes a patch prepared by 'git format-patch' as an argument, and runs 'git blame' on every hunk of the diff to determine the commits that added/removed those lines. It then aggregates the authors and signers of all the commits, and prints out these people in descending order of the percentage of commits they were responsible for. It omits people are below a certain threshold. Fine by me. % git cc-cmd 0001-remote-hg-trivial-cleanups.patch Felipe Contreras felipe.contre...@gmail.com (author: 100%) Jeff King p...@peff.net (signer: 83%) Max Horn m...@quendi.de (signer: 16%) Junio C Hamano gits...@pobox.com (signer: 16%) Won't my name appear as the first one on each of my patches? Will I be CC'ed on every patch that I send out? No. The patches being analyzed are ignored. Either way, you are the s-o-b, so send-email Cc's you by default. contrib/cc-cmd/git-cc-cmd | 140 ++ 1 file changed, 140 insertions(+) create mode 100755 contrib/cc-cmd/git-cc-cmd No README? No README. Half the stuff in contrib doesn't have one. And I don't see what stuff of value we could have there. diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd new file mode 100755 index 000..c7ecf79 --- /dev/null +++ b/contrib/cc-cmd/git-cc-cmd @@ -0,0 +1,140 @@ +#!/usr/bin/env ruby What is the minimum required version of Ruby, by the way? I don't know. I see you haven't used any fancy lazy enumerators I did, but they are implicit. +$since = '3-years-ago' +$min_percent = 5 Do I get to configure these two? Patience. That's for another patch. Also, the '$' prefix from your Perl habit? It's not idiomatic in Ruby afaik. It is for global variables. +class Commit + + attr_reader :id + attr_accessor :roles + + def initialize(id) +@id = id +@roles = [] What are id, roles exactly? This isn't C where we have types, so you're going to have to use some (rdoc-parseable) comments, if you want me to be able to read the code without jumping around too much. The id of the commit, of course (aka. SHA-1), and the roles people had in this commit. We are after all in the Commit class. + end + + def self.parse(data) +id = author = msg = nil This is not C. You don't need to initialize stuff, unless you're going to start accessing it using a .append() or similar. In that case, you need to initialize it as an empty list, so the compiler knows what you're appending to. You need to define them if you are going to modify them in a block, and access them after the block. +roles = {} Shouldn't this be @roles? No, we are in a class method, we don't have instance variables; we don't have an instance. Didn't you initialize it as an empty list in initialize()? No, that's for the instance; we don't have an instance yet. Why did you suddenly change your mind and make it a hashtable now? It's a completely different variable; it's local to the method. +data.each_line do |line| + if not msg +case line +when /^commit (.+)$/ + id = $1 Okay, I have no idea what you're parsing yet. There's some commit line, so I know this is not a raw commit object, as the name of the class Commit would've indicated. Yeah, it's parsing a raw commit object. +when /^author ([^]+) (\S+)$/ + author = $1, $2 + roles[author] = 'author' Huh? the key is a two-item list, and the value is a hard-coded 'author' string? If it's meant to be a sematic value, use a symbol. Yeah, that might make sense. Also, why don't you just collect the authors and signers in two separate lists, instead of doing this and burdening yourself with accumulation later? Go ahead and try this change. How are you going to group persons by the role they took in the relevant commits? It's much simpler to have generic code that is agnostic of the types of roles there are. Also, this would make it simpler to add other roles. +when /^$/ + msg = true I can't see msg being used later in the function, so I don't know what you're doing. It's used before. +roles = roles.map do |person, role| + address = %s %s % person + [person, role] What?! If you wanted address in the first place, why did you stuff a two-member list into roles as the key? What difference does it make? 'person' could be an object, it could be anything. Where is address being used? In a later
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Felipe Contreras felipe.contre...@gmail.com writes: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. In general, I am not all that interested in adding anything new to contrib/ as git.git has matured enough, but even if this will stay outside my tree, there are a few interesting things to note to help its eventual users. +roles = roles.map do |person, role| + address = %s %s % person + [person, role] +end Is address being used elsewhere, or is this a remnant from an earlier debugging or something? +[id, roles] + end + +end ... +File.open(file) do |f| + f.each do |line| +case line +when /^From (\h+) (.+)$/ + from = $1 +when /^---\s+(\S+)/ + source = $1 != '/dev/null' ? $1[2..-1] : nil This may need to be tightened if you want to use this on a real-world project (git.git itself does not count ;-); you may see something like: diff --git a/a\b b/a\b (I did an insane pathname 'ab' to get the above example, but a more realistic is a character outside ASCII). +when /^@@\s-(\d+),(\d+)/ + get_blame(source, $1, $2, from) This may want to be a bit more careful for a hunk that adds to an empty file, which will give you something like @@ -0,0 +1 @@ @@ -0,0 +1,200 @@ Nobody sane would use -U0 when doing a format-patch, but if this wants to accomodate such a patch as well, it needs to ignore a hunk that only adds new lines. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
On Fri, Apr 19, 2013 at 12:08 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. In general, I am not all that interested in adding anything new to contrib/ as git.git has matured enough, but even if this will stay outside my tree, there are a few interesting things to note to help its eventual users. Why not add it to mainline git then? This tool, or a similar one, would certainly be useful in the git arsenal. +roles = roles.map do |person, role| + address = %s %s % person + [person, role] +end Is address being used elsewhere, or is this a remnant from an earlier debugging or something? It's used later on; it creeped in. +[id, roles] + end + +end ... +File.open(file) do |f| + f.each do |line| +case line +when /^From (\h+) (.+)$/ + from = $1 +when /^---\s+(\S+)/ + source = $1 != '/dev/null' ? $1[2..-1] : nil This may need to be tightened if you want to use this on a real-world project (git.git itself does not count ;-); you may see something like: diff --git a/a\b b/a\b (I did an insane pathname 'ab' to get the above example, but a more realistic is a character outside ASCII). Suggestions on how to do that are welcome. +when /^@@\s-(\d+),(\d+)/ + get_blame(source, $1, $2, from) This may want to be a bit more careful for a hunk that adds to an empty file, which will give you something like @@ -0,0 +1 @@ @@ -0,0 +1,200 @@ Simple: return unless source and start and offset Nobody sane would use -U0 when doing a format-patch, but if this wants to accomodate such a patch as well, it needs to ignore a hunk that only adds new lines. I'm not going to worry about it now. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Felipe Contreras felipe.contre...@gmail.com writes: On Fri, Apr 19, 2013 at 12:08 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. In general, I am not all that interested in adding anything new to contrib/ as git.git has matured enough, but even if this will stay outside my tree, there are a few interesting things to note to help its eventual users. Why not add it to mainline git then? This tool, or a similar one, would certainly be useful in the git arsenal. As to this particular feature (the goal it tries to achieve, not necessarily the implementation), that actually was the first thing that came to my mind. It helps the develop, review locally, format-patch, decide whom to ask reviews and then send it out workflow in general to have a tool that tells you who are the people involved in the code you are touching. If this were _only_ to be used within send-email (i.e. replacing the then send it out above with then use send-email to limit the usecase), git cc-cmd would be a reasonable name. But if that is the intended use case, it would even be more reasonable to make this logic part of send-email and trigger it with --auto-cc-reviewers option or something. But I think it can be useful outside the context of send-email as well, and having one independent tool that does one single job well is a better design. Perhaps it is better to name it less specific to send-email's cc-cmd option. git people? git whom? git reviewers? I dunno, but along those lines. It is OK for a design demonstration prototype to be written in any language others (who can comment on the design) can read, but the version to be a first-class citizen needs to be written in one of the languages such as C, POSIX shell, or Perl to avoid adding extra dependencies to the users. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
On Fri, Apr 19, 2013 at 2:24 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, Apr 19, 2013 at 12:08 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. In general, I am not all that interested in adding anything new to contrib/ as git.git has matured enough, but even if this will stay outside my tree, there are a few interesting things to note to help its eventual users. Why not add it to mainline git then? This tool, or a similar one, would certainly be useful in the git arsenal. As to this particular feature (the goal it tries to achieve, not necessarily the implementation), that actually was the first thing that came to my mind. It helps the develop, review locally, format-patch, decide whom to ask reviews and then send it out workflow in general to have a tool that tells you who are the people involved in the code you are touching. If this were _only_ to be used within send-email (i.e. replacing the then send it out above with then use send-email to limit the usecase), git cc-cmd would be a reasonable name. But if that is the intended use case, it would even be more reasonable to make this logic part of send-email and trigger it with --auto-cc-reviewers option or something. Yeap, but I wouldn't want to be the one that implements that in perl. But I think it can be useful outside the context of send-email as well, and having one independent tool that does one single job well is a better design. Perhaps it is better to name it less specific to send-email's cc-cmd option. git people? git whom? git reviewers? I dunno, but along those lines. 'git relevant'? 'git related'? It's not only people, also commits. It is OK for a design demonstration prototype to be written in any language others (who can comment on the design) can read, but the version to be a first-class citizen needs to be written in one of the languages such as C, POSIX shell, or Perl to avoid adding extra dependencies to the users. That is going to be though. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Felipe Contreras felipe.contre...@gmail.com writes: If this were _only_ to be used within send-email (i.e. replacing the then send it out above with then use send-email to limit the usecase), git cc-cmd would be a reasonable name. But if that is the intended use case, it would even be more reasonable to make this logic part of send-email and trigger it with --auto-cc-reviewers option or something. Yeap, but I wouldn't want to be the one that implements that in perl. That is OK. None of this has to be done by you. And we seem to be in agreement that the feature deserves to be its own command, so it does not have to be in Perl, either. But I think it can be useful outside the context of send-email as well, and having one independent tool that does one single job well is a better design. Perhaps it is better to name it less specific to send-email's cc-cmd option. git people? git whom? git reviewers? I dunno, but along those lines. 'git relevant'? 'git related'? It's not only people, also commits. Let's let it simmer on the list for a few days so that other people can come up with a better name. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Am 19.04.2013 21:24, schrieb Junio C Hamano: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, Apr 19, 2013 at 12:08 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. But I think it can be useful outside the context of send-email as well, and having one independent tool that does one single job well is a better design. Perhaps it is better to name it less specific to send-email's cc-cmd option. git people? git whom? git reviewers? I dunno, but along those lines. Would it make sense to integrate this in git shortlog, which already does something similar? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] Add new git-cc-cmd helper to contrib
Felipe Contreras felipe.contre...@gmail.com writes: +File.open(file) do |f| + f.each do |line| +case line +when /^From (\h+) (.+)$/ + from = $1 +when /^---\s+(\S+)/ + source = $1 != '/dev/null' ? $1[2..-1] : nil This may need to be tightened if you want to use this on a real-world project (git.git itself does not count ;-); you may see something like: diff --git a/a\b b/a\b (I did an insane pathname 'ab' to get the above example, but a more realistic is a character outside ASCII). Suggestions on how to do that are welcome. Check gitweb and find unquote. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/8] Add new git-cc-cmd helper to contrib
The code finds the changes of a commit, runs 'git blame' for each chunk to see which other commits are relevant, and then reports the author and signers. Finally, it calculates what percentage of the total relevant commits each person was involved in, and show only the ones that pass the threshold. For example: % git cc-cmd 0001-remote-hg-trivial-cleanups.patch Felipe Contreras felipe.contre...@gmail.com (author: 100%) Jeff King p...@peff.net (signer: 83%) Max Horn m...@quendi.de (signer: 16%) Junio C Hamano gits...@pobox.com (signer: 16%) Thus it can be used for 'git send-email' as a cc-cmd. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- contrib/cc-cmd/git-cc-cmd | 140 ++ 1 file changed, 140 insertions(+) create mode 100755 contrib/cc-cmd/git-cc-cmd diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd new file mode 100755 index 000..c7ecf79 --- /dev/null +++ b/contrib/cc-cmd/git-cc-cmd @@ -0,0 +1,140 @@ +#!/usr/bin/env ruby + +$since = '3-years-ago' +$min_percent = 5 + +class Commit + + attr_reader :id + attr_accessor :roles + + def initialize(id) +@id = id +@roles = [] + end + + def self.parse(data) +id = author = msg = nil +roles = {} +data.each_line do |line| + if not msg +case line +when /^commit (.+)$/ + id = $1 +when /^author ([^]+) (\S+)$/ + author = $1, $2 + roles[author] = 'author' +when /^$/ + msg = true +end + else +if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/ + person = $2, $3 + roles[person] = 'signer' if person != author +end + end +end +roles = roles.map do |person, role| + address = %s %s % person + [person, role] +end +[id, roles] + end + +end + +class Commits + + attr_reader :items + + def initialize() +@items = {} + end + + def size +@items.size + end + + def import +return if @items.empty? +format = [ 'commit %H', 'author %an %ae', '', '%B' ].join('%n') +File.popen(['git', 'show', '-z', '-s', '--format=format:' + format] + @items.keys) do |p| + p.each(\0) do |data| +next if data == \0 # bug in git show? +id, roles = Commit.parse(data) +commit = @items[id] +commit.roles = roles + end +end + end + + def each_person_role +commit_roles = @items.values.map { |commit| commit.roles }.flatten(1) +commit_roles.group_by { |person, role| person }.each do |person, commit_roles| + commit_roles.group_by { |person, role| role }.each do |role, commit_roles| +yield person, role, commit_roles.size + end +end + end + + def get_blame(source, start, offset, from) +return unless source +File.popen(['git', 'blame', '--incremental', '-C', + '-L', '%u,+%u' % [start, offset], + '--since', $since, from + '^', + '--', source]) do |p| + p.each do |line| +if line =~ /^(\h{40})/ + id = $1 + @items[id] = Commit.new(id) +end + end +end + end + + def from_patch(file) +source = nil +from = nil +File.open(file) do |f| + f.each do |line| +case line +when /^From (\h+) (.+)$/ + from = $1 +when /^---\s+(\S+)/ + source = $1 != '/dev/null' ? $1[2..-1] : nil +when /^@@\s-(\d+),(\d+)/ + get_blame(source, $1, $2, from) +end + end +end +import + end + +end + +exit 1 if ARGV.size != 1 + +commits = Commits.new() +commits.from_patch(ARGV[0]) + +# hash of hashes +persons = Hash.new { |hash, key| hash[key] = {} } + +commits.each_person_role do |person, role, count| + persons[person][role] = count +end + +persons.each do |person, roles| + roles = roles.map do |role, count| +percent = count.to_f * 100 / commits.size +next if percent $min_percent +%s: %u%% % [role, percent] + end.compact + next if roles.empty? + + name, email = person + # must quote chars? + name = '%s' % name if name =~ /[^\w \-]/i + person = name ? %s %s % [name, email] : email + puts %s (%s) % [person, roles.join(', ')] +end -- 1.8.2.1.790.g4588561 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html