{"thread":{"id":"42533","subject":"Coccinelle for automated refactors","startedAt":"2016-06-05T20:55:18Z","lastAt":"2016-06-06T23:09:36Z","messageCount":5,"participants":["brian m. carlson","Stefan Beller","Junio C Hamano","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"288402","messageId":"20160605205518.GA153578@vauxhall.crustytoothpaste.net","threadId":"42533","inReplyTo":null,"subject":"Coccinelle for automated refactors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2016-06-05T20:55:18Z","receivedAt":"2016-06-05T20:55:18Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"One thing that I've noticed with the struct object_id conversion is that\nmost of the work is mechanical transformations of a data member from one\ntype into another. Doing this by hand is both boring and error-prone,\nand it requires a tiresome review of nearly-identical changes.\n\nI've noticed that Coccinelle[0], a tool for automated refactors, has\nbeen used with great success on LKML, because it understands C well and\ncan perform the transformations precisely and rapidly. It also does\nnice things like indenting the code it modifies if necessary.\n\nAn example semantic patch looks like this:\n\n@@\nexpression E1;\n@@\n- is_null_sha1(E1.hash)\n+ is_null_oid(&E1)\n\n@@\nexpression E1;\n@@\n- is_null_sha1(E1->hash)\n+ is_null_oid(E1)\n\nThis does what you think it does: transforms calls to is_null_sha1 that\nuse the struct object_id hash member into calls to is_null_oid.\n\nI'd like to use this for some of the struct object_id work if others\nthink this is a good idea. I feel it's likely to reduce the reviewing\noverhead and allow people to better reason about the quality and\nbehavior of the sent patches. Of course, I would still review the\npatches manually for errors and improvements, and would still accept\nresponsibility for the content of the patches.\n\nIf there's interest, I can send a patch with a set of basic object_id\ntransforms to make it easier for others to make those changes when\nthey're doing work elsewhere in the codebase.\n\n[0] http://coccinelle.lip6.fr/\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"288462","messageId":"CAGZ79kbDgP6PJ6F6ksA33qDs==FHDBe8EwE=iBOWe8as_oFLkA@mail.gmail.com","threadId":"42533","inReplyTo":"20160605205518.GA153578@vauxhall.crustytoothpaste.net","subject":"Re: Coccinelle for automated refactors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-06T17:43:49Z","receivedAt":"2016-06-06T17:43:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Jun 5, 2016 at 1:55 PM, brian m. carlson\n wrote:\n> One thing that I've noticed with the struct object_id conversion is that\n> most of the work is mechanical transformations of a data member from one\n> type into another. Doing this by hand is both boring and error-prone,\n> and it requires a tiresome review of nearly-identical changes.\n>\n> I've noticed that Coccinelle[0], a tool for automated refactors, has\n> been used with great success on LKML, because it understands C well and\n> can perform the transformations precisely and rapidly. It also does\n> nice things like indenting the code it modifies if necessary.\n>\n> An example semantic patch looks like this:\n>\n> @@\n> expression E1;\n> @@\n> - is_null_sha1(E1.hash)\n> + is_null_oid(&E1)\n>\n> @@\n> expression E1;\n> @@\n> - is_null_sha1(E1->hash)\n> + is_null_oid(E1)\n\nSo is this all we have to review then?\n\n>\n> This does what you think it does: transforms calls to is_null_sha1 that\n> use the struct object_id hash member into calls to is_null_oid.\n>\n> I'd like to use this for some of the struct object_id work if others\n> think this is a good idea. I feel it's likely to reduce the reviewing\n> overhead and allow people to better reason about the quality and\n> behavior of the sent patches. Of course, I would still review the\n> patches manually for errors and improvements, and would still accept\n> responsibility for the content of the patches.\n>\n> If there's interest, I can send a patch with a set of basic object_id\n> transforms to make it easier for others to make those changes when\n> they're doing work elsewhere in the codebase.\n\nI would be interested in reviewing such a patch.\n\nThanks,\nStefan\n\n>\n> [0] http://coccinelle.lip6.fr/\n> --\n> brian m. carlson / brian with sandals: Houston, Texas, US\n> +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\n> OpenPGP: https://keybase.io/bk2204\n"},{"id":"288471","messageId":"xmqqy46iw3ih.fsf@gitster.mtv.corp.google.com","threadId":"42533","inReplyTo":"20160605205518.GA153578@vauxhall.crustytoothpaste.net","subject":"Re: Coccinelle for automated refactors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T18:55:50Z","receivedAt":"2016-06-06T18:55:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> An example semantic patch looks like this:\n>\n> @@\n> expression E1;\n> @@\n> - is_null_sha1(E1.hash)\n> + is_null_oid(&E1)\n>\n> @@\n> expression E1;\n> @@\n> - is_null_sha1(E1->hash)\n> + is_null_oid(E1)\n>\n> This does what you think it does: transforms calls to is_null_sha1 that\n> use the struct object_id hash member into calls to is_null_oid.\n>\n> I'd like to use this for some of the struct object_id work if others\n> think this is a good idea. I feel it's likely to reduce the reviewing\n> overhead and allow people to better reason about the quality and\n> behavior of the sent patches. Of course, I would still review the\n> patches manually for errors and improvements, and would still accept\n> responsibility for the content of the patches.\n\nIs the plan for such a \"refactor\" patch to compose such a series as\ntwo patch series:\n\n [1/2] automatic refactor\n\nwhich gives the \"semantic patch\" in the proposed log message as part\nof its description, and the automated result (with possible\nmisconversions that may have come from bugs in the automated tools),\nwith a separate\n\n [2/2] manual fixups\n\nthat corrects what was misconverted and what was missed?\n\nAs long as [2/2] can be kept to the minimum (and an automated tool\nthat is worth using should make it so), I think that is a good way\nforward. Another possibility would be to send the end-result as a\nsingle patch, with description on the manual fixups in the proposed\nlog message, but it would be a lot more work to generate and review\nsuch a patch, I would think.\n\n> If there's interest, I can send a patch with a set of basic object_id\n> transforms to make it easier for others to make those changes when\n> they're doing work elsewhere in the codebase.\n>\n> [0] http://coccinelle.lip6.fr/\n"},{"id":"288484","messageId":"CA+P7+xqszc-BRBYo7vaiQBFy8c9MMJwTY3sLrX5gOFZc21Mm8Q@mail.gmail.com","threadId":"42533","inReplyTo":"xmqqy46iw3ih.fsf@gitster.mtv.corp.google.com","subject":"Re: Coccinelle for automated refactors","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-06-06T20:38:13Z","receivedAt":"2016-06-06T20:38:13Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jun 6, 2016 at 11:55 AM, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n>\n>> An example semantic patch looks like this:\n>>\n>> @@\n>> expression E1;\n>> @@\n>> - is_null_sha1(E1.hash)\n>> + is_null_oid(&E1)\n>>\n>> @@\n>> expression E1;\n>> @@\n>> - is_null_sha1(E1->hash)\n>> + is_null_oid(E1)\n>>\n>> This does what you think it does: transforms calls to is_null_sha1 that\n>> use the struct object_id hash member into calls to is_null_oid.\n>>\n>> I'd like to use this for some of the struct object_id work if others\n>> think this is a good idea. I feel it's likely to reduce the reviewing\n>> overhead and allow people to better reason about the quality and\n>> behavior of the sent patches. Of course, I would still review the\n>> patches manually for errors and improvements, and would still accept\n>> responsibility for the content of the patches.\n>\n> Is the plan for such a \"refactor\" patch to compose such a series as\n> two patch series:\n>\n> [1/2] automatic refactor\n>\n> which gives the \"semantic patch\" in the proposed log message as part\n> of its description, and the automated result (with possible\n> misconversions that may have come from bugs in the automated tools),\n> with a separate\n>\n> [2/2] manual fixups\n>\n> that corrects what was misconverted and what was missed?\n>\n> As long as [2/2] can be kept to the minimum (and an automated tool\n> that is worth using should make it so), I think that is a good way\n> forward. Another possibility would be to send the end-result as a\n> single patch, with description on the manual fixups in the proposed\n> log message, but it would be a lot more work to generate and review\n> such a patch, I would think.\n>\n>> If there's interest, I can send a patch with a set of basic object_id\n>> transforms to make it easier for others to make those changes when\n>> they're doing work elsewhere in the codebase.\n>>\n>> [0] http://coccinelle.lip6.fr/\n> --\n\nI've used coccinelle before on the Linux Kernel, and I've found that\nit works quite well without producing any incorrect changes,\nespecially if you keep the semantic patch small and simple. I'd\ndefinitely go with the 1/2 2/2 approach but I suspect in most useful\ncases 2/2 would be very minor if at all.\n\nThanks,\nJake\n"},{"id":"288493","messageId":"20160606230936.GA60445@vauxhall.crustytoothpaste.net","threadId":"42533","inReplyTo":"xmqqy46iw3ih.fsf@gitster.mtv.corp.google.com","subject":"Re: Coccinelle for automated refactors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2016-06-06T23:09:36Z","receivedAt":"2016-06-06T23:09:36Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jun 06, 2016 at 11:55:50AM -0700, Junio C Hamano wrote:\n> Is the plan for such a \"refactor\" patch to compose such a series as\n> two patch series:\n> \n> [1/2] automatic refactor\n> \n> which gives the \"semantic patch\" in the proposed log message as part\n> of its description, and the automated result (with possible\n> misconversions that may have come from bugs in the automated tools),\n> with a separate\n> \n> [2/2] manual fixups\n> \n> that corrects what was misconverted and what was missed?\n> \n> As long as [2/2] can be kept to the minimum (and an automated tool\n> that is worth using should make it so), I think that is a good way\n> forward. Another possibility would be to send the end-result as a\n> single patch, with description on the manual fixups in the proposed\n> log message, but it would be a lot more work to generate and review\n> such a patch, I would think.\n\nMy hope is that I can make the automated changes such that manual fixups\nare more along the lines of cleaning up related functions in the module,\nfixing issues noticed during the refactor, and the like: in other words,\nthings that one might have done incidentally as part of the patch, but\ncould defensibly be done in a second patch anyway.\n\nMy goal is to make the series as much like a human-edited series as\npossible, but with less work on all sides.\n\nI can send an RFC series that demonstrates this a little later to see if\nit's an acceptable direction for work.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"}]}