{"thread":{"id":"44886","subject":"[RFC for GIT] pull-request: add praise to people doing QA","startedAt":"2017-01-15T18:31:29Z","lastAt":"2017-01-20T00:14:35Z","messageCount":10,"participants":["Wolfram Sang","Junio C Hamano","Jacob Keller","Jeff King","Joe Perches"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"309444","messageId":"20170115183051.3565-1-wsa@the-dreams.de","threadId":"44886","inReplyTo":null,"subject":"[RFC for GIT] pull-request: add praise to people doing QA","fromName":"Wolfram Sang","fromEmail":"wsa@the-dreams.de","sentAt":"2017-01-15T18:30:51Z","receivedAt":"2017-01-15T18:31:29Z","isPatch":false,"sender":{"key":"wsa@the-dreams.de","avatar":null},"body":"Asking for opinions on lkml and git...\n\nGetting enough quality assurance is likely one of the bigger upcoming tasks in\nthe near future. To improve the situation, praise the people already doing that\nby adding their names to pull requests in the same manner that patch authors\nare credited. Here is an example, I sent out today [1]:\n\n=== old stuff\n\nThe following changes since commit a121103c922847ba5010819a3f250f1f7fc84ab8:\n\n...\n\nVlad Tsyrklevich (1):\n      i2c: fix kernel memory disclosure in dev interface\n\n=== new stuff starts here\n\nwith much appreciated quality assurance from\n----------------------------------------------------------------\nAndy Shevchenko (1):\n      (Rev.) i2c: piix4: Avoid race conditions with IMC\n\nBenjamin Tissoires (1):\n      (Test) i2c: do not enable fall back to Host Notify by default\n\nVladimir Zapolskiy (1):\n      (Rev.) i2c: print correct device invalid address\n\n=== diffstat, ...\n\nThis patch is a very early RFC to collect opinions. I am not very familiar with\nthe git codebase, but I guess using a filter needs to be reworked, the\ndependency on GNU awk may be frowned upon (though 'asorti' is really useful\nhere), the reg-ex are not super-solid, and it should be a command-line option,\nof course. That all being said, it was a fast way to produce what I would like\nto add to my pull requests for the i2c subsystem and to see if other kernel/git\nmaintainers are interested in something like this.\n\nDisclaimer: while this patch applies to the git codebase, I have to admit that\nI simply patched around in /usr/lib/git-core of my Debian machine :)\n\nSo much for now, let me know what you think,\n\n   Wolfram\n\n[1] http://lkml.org/lkml/2017/1/15/55\n\nSigned-off-by: Wolfram Sang <wsa@the-dreams.de>\n---\n git-praise-qa.awk   |   33 +++++++++++++++++++++++++++++++++\n git-request-pull.sh |    1 +\n 2 files changed, 34 insertions(+)\n\nIndex: git-2.11.0/git-request-pull.sh\n===================================================================\n--- git-2.11.0.orig/git-request-pull.sh\n+++ git-2.11.0/git-request-pull.sh\n@@ -155,6 +155,7 @@ then\n fi &&\n \n git shortlog ^$baserev $headrev &&\n+git log --no-merges ^$baserev $headrev | git-praise-qa.awk &&\n git diff -M --stat --summary $patch $merge_base..$headrev || status=1\n \n exit $status\nIndex: git-2.11.0/git-praise-qa.awk\n===================================================================\n--- /dev/null\n+++ git-2.11.0/git-praise-qa.awk\n@@ -0,0 +1,33 @@\n+#! /usr/bin/gawk -f\n+\n+# New commit found, empty subject variable\n+/^commit / { subject = \"\" }\n+\n+# Grab the subject line\n+!subject && /^    / { subject = substr($0, 5); }\n+\n+# Scan for tags and get the type\n+/^    Reviewed-by:/ { type = \"Rev.\" }\n+/^    Tested-by:/ { type = \"Test\" }\n+\n+type && subject {\n+\t# Extract the name\n+\tsub(/^.*: /, \"\"); sub(/ <.*/, \"\"); name = $0;\n+\t# Collect tags given by 'name'\n+\ttags[name] = tags[name] \"      (\" type \") \" subject \"\\n\";\n+\tcount[name]++;\n+\t# Done, clear flag\n+\ttype = \"\";\n+}\n+\n+END {\n+\tprint \"\\nwith much appreciated quality assurance from\"\n+\tprint \"----------------------------------------------------------------\"\n+\t# Sort by names\n+\tasorti(tags, sorted_names);\n+\t# printout in git style\n+\tfor (i in sorted_names) {\n+\t\tname = sorted_names[i];\n+\t\tprint name \" (\" count[name] \"):\" \"\\n\" tags[name];\n+\t}\n+}\n"},{"id":"309462","messageId":"xmqqlgubc04z.fsf@gitster.mtv.corp.google.com","threadId":"44886","inReplyTo":"20170115183051.3565-1-wsa@the-dreams.de","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-16T00:35:24Z","receivedAt":"2017-01-16T00:35:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wolfram Sang <wsa@the-dreams.de> writes:\n\n> === new stuff starts here\n>\n> with much appreciated quality assurance from\n> ----------------------------------------------------------------\n> Andy Shevchenko (1):\n>       (Rev.) i2c: piix4: Avoid race conditions with IMC\n>\n> Benjamin Tissoires (1):\n>       (Test) i2c: do not enable fall back to Host Notify by default\n>\n> Vladimir Zapolskiy (1):\n>       (Rev.) i2c: print correct device invalid address\n>\n> === diffstat, ...\n>\n> This patch is a very early RFC to collect opinions. I am not very familiar with\n> the git codebase, but I guess using a filter needs to be reworked, the\n> dependency on GNU awk may be frowned upon (though 'asorti' is really useful\n> here), the reg-ex are not super-solid, and it should be a command-line option,\n> of course. That all being said, it was a fast way to produce what I would like\n> to add to my pull requests for the i2c subsystem and to see if other kernel/git\n> maintainers are interested in something like this.\n>\n> Disclaimer: while this patch applies to the git codebase, I have to admit that\n> I simply patched around in /usr/lib/git-core of my Debian machine :)\n>\n> So much for now, let me know what you think,\n\nSo the idea is to have list of those whose names appear on\nReviewed-by: and Tested-by: collected and listed after the list of\ncommit titles and author names.  I personally do not see much\ndownsides in doing so, but I do not consume that many PRs myself, so\nlet's hear from those who actually do process many of them.\n\nAs to the implementation, I am wondering if we can make this somehow\nwork well with the \"trailers\" code we already have, instead of\ninventing yet another parser of trailers.  \n\nIn its current shape, \"interpret-trailers\" focuses on \"editing\" an\nexisting commit log message to tweak the trailer lines.  That mode\nof operation would help amending and rebasing, and to do that it\nneeds to parse the commit log message, identify trailer blocks,\nparse out each trailer lines, etc.  \n\nThere is no fundamental reason why its output must be an edited\noriginal commit log message---it should be usable as a filter that\npicks trailer lines of the selected trailer type, like \"Tested-By\",\netc.\n"},{"id":"309467","messageId":"CA+P7+xojtk4QD2rtAy90ytHCpuaSAOy2SE-aXC4A863N020Efw@mail.gmail.com","threadId":"44886","inReplyTo":"xmqqlgubc04z.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-16T04:05:26Z","receivedAt":"2017-01-16T04:06:05Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Jan 15, 2017 at 4:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> As to the implementation, I am wondering if we can make this somehow\n> work well with the \"trailers\" code we already have, instead of\n> inventing yet another parser of trailers.\n>\n> In its current shape, \"interpret-trailers\" focuses on \"editing\" an\n> existing commit log message to tweak the trailer lines.  That mode\n> of operation would help amending and rebasing, and to do that it\n> needs to parse the commit log message, identify trailer blocks,\n> parse out each trailer lines, etc.\n>\n> There is no fundamental reason why its output must be an edited\n> original commit log message---it should be usable as a filter that\n> picks trailer lines of the selected trailer type, like \"Tested-By\",\n> etc.\n\nI have been looking at ways to use the interpret-trailers as a way to\nfilter commits and print out trailers, and this sort of feature would\nbe useful to me if it were generic. (and then pull-request could use\nthe generic interface to grab the data and then parse it into a praise\nformat)\n\nThanks,\nJake\n"},{"id":"309777","messageId":"20170119204343.xtotmjddhbum2mvr@ninjato","threadId":"44886","inReplyTo":"xmqqlgubc04z.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Wolfram Sang","fromEmail":"wsa@the-dreams.de","sentAt":"2017-01-19T20:43:45Z","receivedAt":"2017-01-19T20:44:46Z","isPatch":false,"sender":{"key":"wsa@the-dreams.de","avatar":null},"body":"\n> So the idea is to have list of those whose names appear on\n> Reviewed-by: and Tested-by: collected and listed after the list of\n> commit titles and author names.  I personally do not see much\n> downsides in doing so, but I do not consume that many PRs myself, so\n> let's hear from those who actually do process many of them.\n\nSadly, no further responses so far. Let's see if they will come if I\nkeep posting my pull requests with that information attached.\n\n> As to the implementation, I am wondering if we can make this somehow\n> work well with the \"trailers\" code we already have, instead of\n> inventing yet another parser of trailers.  \n> \n> In its current shape, \"interpret-trailers\" focuses on \"editing\" an\n> existing commit log message to tweak the trailer lines.  That mode\n> of operation would help amending and rebasing, and to do that it\n> needs to parse the commit log message, identify trailer blocks,\n> parse out each trailer lines, etc.  \n> \n> There is no fundamental reason why its output must be an edited\n> original commit log message---it should be usable as a filter that\n> picks trailer lines of the selected trailer type, like \"Tested-By\",\n> etc.\n\nI didn't know about trailers before. As I undestand it, I could use\n\"Tested-by\" as the key, and the commit subject as the value. This list\nthen could be parsed and brought into proper output shape. It would\nsimplify the subject parsing, but most things my AWK script currently\ndoes would still need to stay or to be reimplemented (extracting names\nfrom tags, creating arrays of tags given by $name). Am I correct?\n\nAll under the assumption that trailers work on a range of commits. I\nhave to admit that adding this to git is beyond my scope.\n\nThanks for looking into it,\n\n   Wolfram\n\n"},{"id":"309783","messageId":"xmqq7f5qzqx3.fsf@gitster.mtv.corp.google.com","threadId":"44886","inReplyTo":"20170119204343.xtotmjddhbum2mvr@ninjato","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T21:22:00Z","receivedAt":"2017-01-19T21:27:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wolfram Sang <wsa@the-dreams.de> writes:\n\n> I didn't know about trailers before. As I undestand it, I could use\n> \"Tested-by\" as the key, and the commit subject as the value. This list\n> then could be parsed and brought into proper output shape. It would\n> simplify the subject parsing, but most things my AWK script currently\n> does would still need to stay or to be reimplemented (extracting names\n> from tags, creating arrays of tags given by $name). Am I correct?\n\nThat is not exactly what I had in mind.  I was wondering if we can\ndo without any external script, implementing the logic you added\ninside shortlog with an extra option that triggers the whole thing,\nwhich may call into the same trailers API as used by the\ninterpret-trailers command to do the parsing and picking out parts.\n"},{"id":"309785","messageId":"20170119212039.3gixsrk7qco45wjo@sigill.intra.peff.net","threadId":"44886","inReplyTo":"20170119204343.xtotmjddhbum2mvr@ninjato","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T21:20:40Z","receivedAt":"2017-01-19T21:27:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:\n\n> > As to the implementation, I am wondering if we can make this somehow\n> > work well with the \"trailers\" code we already have, instead of\n> > inventing yet another parser of trailers.  \n> > \n> > In its current shape, \"interpret-trailers\" focuses on \"editing\" an\n> > existing commit log message to tweak the trailer lines.  That mode\n> > of operation would help amending and rebasing, and to do that it\n> > needs to parse the commit log message, identify trailer blocks,\n> > parse out each trailer lines, etc.  \n> > \n> > There is no fundamental reason why its output must be an edited\n> > original commit log message---it should be usable as a filter that\n> > picks trailer lines of the selected trailer type, like \"Tested-By\",\n> > etc.\n> \n> I didn't know about trailers before. As I undestand it, I could use\n> \"Tested-by\" as the key, and the commit subject as the value. This list\n> then could be parsed and brought into proper output shape. It would\n> simplify the subject parsing, but most things my AWK script currently\n> does would still need to stay or to be reimplemented (extracting names\n> from tags, creating arrays of tags given by $name). Am I correct?\n> \n> All under the assumption that trailers work on a range of commits. I\n> have to admit that adding this to git is beyond my scope.\n\nThis sounds a lot like the shortlog-trailers work I did about a year\nago:\n\n  http://public-inbox.org/git/20151229073832.GN8842@sigill.intra.peff.net/\n\n  http://public-inbox.org/git/20151229075013.GA9191@sigill.intra.peff.net/\n\nNobody seemed to really find it useful, so I didn't pursue it.\n\nSome of the preparatory patches in that series bit-rotted in the\nmeantime, but you can play with a version based on v2.7.0 by fetching\nthe \"shortlog-trailers-historical\" branch from\nhttps://github.com/peff/git.git.\n\nAnd then things like:\n\n  git shortlog --ident=tested-by --format='...tested a patch by %an'\n\nwork (and you can put whatever commit items you want into the --format,\nincluding just dumping the hash if you want to do more analysis).\n\n-Peff\n"},{"id":"309798","messageId":"20170119220257.GB1747@katana","threadId":"44886","inReplyTo":"xmqq7f5qzqx3.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Wolfram Sang","fromEmail":"wsa@the-dreams.de","sentAt":"2017-01-19T22:02:58Z","receivedAt":"2017-01-19T22:04:00Z","isPatch":false,"sender":{"key":"wsa@the-dreams.de","avatar":null},"body":"\n> > I didn't know about trailers before. As I undestand it, I could use\n> > \"Tested-by\" as the key, and the commit subject as the value. This list\n> > then could be parsed and brought into proper output shape. It would\n> > simplify the subject parsing, but most things my AWK script currently\n> > does would still need to stay or to be reimplemented (extracting names\n> > from tags, creating arrays of tags given by $name). Am I correct?\n> \n> That is not exactly what I had in mind.  I was wondering if we can\n> do without any external script, implementing the logic you added\n> inside shortlog with an extra option that triggers the whole thing,\n> which may call into the same trailers API as used by the\n> interpret-trailers command to do the parsing and picking out parts.\n\nSorry for being unclear. That's what I meant with \"or to be\nreimplemented\". I should have added \"in C\".\n\nI am afraid this also requires more time than I am willing to\nspend on this issue. Seems my hack is going to stay for a while here.\n\nHowever, thank you for your time and assisting me with pointers!\n\n"},{"id":"309805","messageId":"CA+P7+xo5N66a8-PeNRLBgwRN3rJZRbQuDnx8wCnW7L-0tz10Fg@mail.gmail.com","threadId":"44886","inReplyTo":"20170119212039.3gixsrk7qco45wjo@sigill.intra.peff.net","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-19T23:42:14Z","receivedAt":"2017-01-19T23:43:25Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Jan 19, 2017 at 1:20 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:\n>\n>> > As to the implementation, I am wondering if we can make this somehow\n>> > work well with the \"trailers\" code we already have, instead of\n>> > inventing yet another parser of trailers.\n>> >\n>> > In its current shape, \"interpret-trailers\" focuses on \"editing\" an\n>> > existing commit log message to tweak the trailer lines.  That mode\n>> > of operation would help amending and rebasing, and to do that it\n>> > needs to parse the commit log message, identify trailer blocks,\n>> > parse out each trailer lines, etc.\n>> >\n>> > There is no fundamental reason why its output must be an edited\n>> > original commit log message---it should be usable as a filter that\n>> > picks trailer lines of the selected trailer type, like \"Tested-By\",\n>> > etc.\n>>\n>> I didn't know about trailers before. As I undestand it, I could use\n>> \"Tested-by\" as the key, and the commit subject as the value. This list\n>> then could be parsed and brought into proper output shape. It would\n>> simplify the subject parsing, but most things my AWK script currently\n>> does would still need to stay or to be reimplemented (extracting names\n>> from tags, creating arrays of tags given by $name). Am I correct?\n>>\n>> All under the assumption that trailers work on a range of commits. I\n>> have to admit that adding this to git is beyond my scope.\n>\n> This sounds a lot like the shortlog-trailers work I did about a year\n> ago:\n>\n>   http://public-inbox.org/git/20151229073832.GN8842@sigill.intra.peff.net/\n>\n>   http://public-inbox.org/git/20151229075013.GA9191@sigill.intra.peff.net/\n>\n> Nobody seemed to really find it useful, so I didn't pursue it.\n>\n> Some of the preparatory patches in that series bit-rotted in the\n> meantime, but you can play with a version based on v2.7.0 by fetching\n> the \"shortlog-trailers-historical\" branch from\n> https://github.com/peff/git.git.\n>\n> And then things like:\n>\n>   git shortlog --ident=tested-by --format='...tested a patch by %an'\n>\n> work (and you can put whatever commit items you want into the --format,\n> including just dumping the hash if you want to do more analysis).\n>\n> -Peff\n\nThis sounds interesting to me! When I have some more time to take a\nlook at this i might see if I can revive it.\n\nThanks,\nJake\n"},{"id":"309808","messageId":"1484870777.2707.2.camel@perches.com","threadId":"44886","inReplyTo":"CA+P7+xo5N66a8-PeNRLBgwRN3rJZRbQuDnx8wCnW7L-0tz10Fg@mail.gmail.com","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2017-01-20T00:06:17Z","receivedAt":"2017-01-20T00:08:03Z","isPatch":false,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Thu, 2017-01-19 at 15:42 -0800, Jacob Keller wrote:\n> On Thu, Jan 19, 2017 at 1:20 PM, Jeff King <peff@peff.net> wrote:\n> > On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:\n> > \n> > > > As to the implementation, I am wondering if we can make this somehow\n> > > > work well with the \"trailers\" code we already have, instead of\n> > > > inventing yet another parser of trailers.\n> > > > \n> > > > In its current shape, \"interpret-trailers\" focuses on \"editing\" an\n> > > > existing commit log message to tweak the trailer lines.  That mode\n> > > > of operation would help amending and rebasing, and to do that it\n> > > > needs to parse the commit log message, identify trailer blocks,\n> > > > parse out each trailer lines, etc.\n> > > > \n> > > > There is no fundamental reason why its output must be an edited\n> > > > original commit log message---it should be usable as a filter that\n> > > > picks trailer lines of the selected trailer type, like \"Tested-By\",\n> > > > etc.\n> > > \n> > > I didn't know about trailers before. As I undestand it, I could use\n> > > \"Tested-by\" as the key, and the commit subject as the value. This list\n> > > then could be parsed and brought into proper output shape. It would\n> > > simplify the subject parsing, but most things my AWK script currently\n> > > does would still need to stay or to be reimplemented (extracting names\n> > > from tags, creating arrays of tags given by $name). Am I correct?\n> > > \n> > > All under the assumption that trailers work on a range of commits. I\n> > > have to admit that adding this to git is beyond my scope.\n> > \n> > This sounds a lot like the shortlog-trailers work I did about a year\n> > ago:\n> > \n> >   http://public-inbox.org/git/20151229073832.GN8842@sigill.intra.peff.net/\n> > \n> >   http://public-inbox.org/git/20151229075013.GA9191@sigill.intra.peff.net/\n> > \n> > Nobody seemed to really find it useful, so I didn't pursue it.\n> > \n> > Some of the preparatory patches in that series bit-rotted in the\n> > meantime, but you can play with a version based on v2.7.0 by fetching\n> > the \"shortlog-trailers-historical\" branch from\n> > https://github.com/peff/git.git.\n> > \n> > And then things like:\n> > \n> >   git shortlog --ident=tested-by --format='...tested a patch by %an'\n> > \n> > work (and you can put whatever commit items you want into the --format,\n> > including just dumping the hash if you want to do more analysis).\n> > \n> > -Peff\n> \n> This sounds interesting to me! When I have some more time to take a\n> look at this i might see if I can revive it.\n\nCan the terminology please be standardized to what\nwas once called bylines?\n\nhttps://patchwork.kernel.org/patch/9307703/\n\n"},{"id":"309809","messageId":"CA+P7+xo8rZT_C=guKZYg1smRa+poh5v6a91xDjn=BUukgf-bXQ@mail.gmail.com","threadId":"44886","inReplyTo":"1484870777.2707.2.camel@perches.com","subject":"Re: [RFC for GIT] pull-request: add praise to people doing QA","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-20T00:13:15Z","receivedAt":"2017-01-20T00:14:35Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Jan 19, 2017 at 4:06 PM, Joe Perches <joe@perches.com>\nwrote:>> This sounds interesting to me! When I have some more time to\ntake a\n>> look at this i might see if I can revive it.\n>\n> Can the terminology please be standardized to what\n> was once called bylines?\n>\n> https://patchwork.kernel.org/patch/9307703/\n>\n\nI am fairly certain we've settled on \"trailers\" at this point. I don't\nhave an objection to either name, but most of the code today uses\ntrailers.\n\nThanks,\nJake\n"}]}