{"thread":{"id":"59586","subject":"[PATCH 0/2] cocci: codify authoring and reviewing practices","startedAt":"2023-04-12T20:06:01Z","lastAt":"2023-05-10T22:45:32Z","messageCount":26,"participants":["Glen Choo via GitGitGadget","Junio C Hamano","Glen Choo","Elijah Newren","SZEDER Gábor","Ævar Arnfjörð Bjarmason","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"475232","messageId":"pull.1495.git.git.1681329955.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":null,"subject":"[PATCH 0/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-12T20:05:53Z","receivedAt":"2023-04-12T20:06:01Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Here's the followup to the discussion in [1]. Sorry for the delay.\n\nI've tried to incorporate most of the responses from that thread as well as\nsuggest some guidelines that I think would make the authoring + reviewing\nprocess smoother. I've opted for stronger wording to make the guidelines\neasier to follow, but I don't feel strongly about the specifics.\n\n[1]\nhttps://lore.kernel.org/git/kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com\n\nGlen Choo (2):\n cocci: add headings to and reword README\n cocci: codify authoring and reviewing practices\n\n contrib/coccinelle/README | 33 +++++++++++++++++++++++++++++----\n 1 file changed, 29 insertions(+), 4 deletions(-)\n\n\nbase-commit: f285f68a132109c234d93490671c00218066ace9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v1\nPull-Request: https://github.com/git/git/pull/1495\n-- \ngitgitgadget\n"},{"id":"475233","messageId":"4a8b8a2a6745e791e35296e34f530b5f40f51c27.1681329955.git.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":"pull.1495.git.git.1681329955.gitgitgadget@gmail.com","subject":"[PATCH 1/2] cocci: add headings to and reword README","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-12T20:05:54Z","receivedAt":"2023-04-12T20:06:03Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n- Drop \"examples\" since we actually use the patches.\n- Drop sentences that could be headings instead\n\nSigned-off-by: Glen Choo \n---\n contrib/coccinelle/README | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\nindex d1daa1f6263..9b28ba1c57a 100644\n--- a/contrib/coccinelle/README\n+++ b/contrib/coccinelle/README\n@@ -1,7 +1,9 @@\n-This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)\n-semantic patches that might be useful to developers.\n+= coccinelle\n \n-There are two types of semantic patches:\n+This directory provides Coccinelle (http://coccinelle.lip6.fr/) semantic patches\n+that might be useful to developers.\n+\n+== Types of semantic patches\n \n * Using the semantic transformation to check for bad patterns in the code;\n The target 'make coccicheck' is designed to check for these patterns and\n@@ -42,7 +44,7 @@ There are two types of semantic patches:\n This allows to expose plans of pending large scale refactorings without\n impacting the bad pattern checks.\n \n-Git-specific tips & things to know about how we run \"spatch\":\n+== Git-specific tips & things to know about how we run \"spatch\":\n \n * The \"make coccicheck\" will piggy-back on\n \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n-- \ngitgitgadget\n\n"},{"id":"475234","messageId":"75feb18dfd8af03f5e7ba02403a16a0ed4c2edaa.1681329955.git.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":"pull.1495.git.git.1681329955.gitgitgadget@gmail.com","subject":"[PATCH 2/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-12T20:05:55Z","receivedAt":"2023-04-12T20:06:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nThis isn't set in stone; we expect this to be updated as the project\nevolves.\n\nSigned-off-by: Glen Choo \n---\n contrib/coccinelle/README | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\nindex 9b28ba1c57a..055e3622e5c 100644\n--- a/contrib/coccinelle/README\n+++ b/contrib/coccinelle/README\n@@ -92,3 +92,26 @@ that might be useful to developers.\n \n The absolute times will differ for you, but the relative speedup\n from caching should be on that order.\n+\n+== Authoring and reviewing coccinelle changes\n+\n+* When introducing and applying a new .cocci file, both the Git changes and\n+ .cocci file should be reviewed.\n+\n+* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is\n+ enough for the reviewer to get a rough understanding of the proposed rules by\n+ comparing the .cocci and Git changes, then checking that understanding\n+ with the author.\n+\n+* Conversely, authors should consider that reviewers may not be coccinelle\n+ experts. The primary aim should be to make .cocci files easy to understand,\n+ e.g. by adding comments or by using rules that are easier to understand even\n+ if they are less elegant.\n+\n+* .cocci rules should target only the problem it is trying to solve; \"collateral\n+ damage\" is not allowed.\n+\n+* .cocci files used for refactoring should be temporarily kept in-tree to aid\n+ the refactoring of out-of-tree code (e.g. in-flight topics). They should be\n+ removed when enough time has been given for others to refactor their code,\n+ i.e. ~1 release cycle.\n-- \ngitgitgadget\n"},{"id":"475241","messageId":"xmqq8rew7q9s.fsf@gitster.g","threadId":"59586","inReplyTo":"4a8b8a2a6745e791e35296e34f530b5f40f51c27.1681329955.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] cocci: add headings to and reword README","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-12T21:18:55Z","receivedAt":"2023-04-12T21:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> From: Glen Choo \n>\n> - Drop \"examples\" since we actually use the patches.\n> - Drop sentences that could be headings instead\n>\n> Signed-off-by: Glen Choo \n> ---\n> contrib/coccinelle/README | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n\nMakes sense. Will queue. Thanks.\n\n"},{"id":"475312","messageId":"kl6lleivk4r1.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59586","inReplyTo":"xmqq8rew7q9s.fsf@gitster.g","subject":"Re: [PATCH 1/2] cocci: add headings to and reword README","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-13T18:37:38Z","receivedAt":"2023-04-13T18:37:51Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n>> From: Glen Choo \n>>\n>> - Drop \"examples\" since we actually use the patches.\n>> - Drop sentences that could be headings instead\n>>\n>> Signed-off-by: Glen Choo \n>> ---\n>> contrib/coccinelle/README | 10 ++++++----\n>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> Makes sense. Will queue. Thanks.\n\nI believe this was directed at just the cleanups in this patch and not\nthe recommendations in the later patch?\n\nI was confused for a moment when I first saw this, and someone else\nmentioned off-list that they also thought you meant you'd queue both\npatches.\n"},{"id":"475313","messageId":"xmqqmt3by5sc.fsf@gitster.g","threadId":"59586","inReplyTo":"kl6lleivk4r1.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 1/2] cocci: add headings to and reword README","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T18:51:31Z","receivedAt":"2023-04-13T18:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Junio C Hamano writes:\n>\n>>> From: Glen Choo \n>>>\n>>> - Drop \"examples\" since we actually use the patches.\n>>> - Drop sentences that could be headings instead\n>>>\n>>> Signed-off-by: Glen Choo \n>>> ---\n>>> contrib/coccinelle/README | 10 ++++++----\n>>> 1 file changed, 6 insertions(+), 4 deletions(-)\n>>\n>> Makes sense. Will queue. Thanks.\n>\n> I believe this was directed at just the cleanups in this patch and not\n> the recommendations in the later patch?\n>\n> I was confused for a moment when I first saw this, and someone else\n> mentioned off-list that they also thought you meant you'd queue both\n> patches.\n\nYes, I did mean that this step made sense (not implying anything\ngood or bad about the other step).\n\nI ended up saving both on 'seen' so that we can keep track. I do\nnot think it is a problem---people can comment more on the patch and\nI expect we would update it further.\n\nTHanks.\n\n\n\n"},{"id":"475430","messageId":"CABPp-BEWaojwSpMaYT1VqNBYuhETm-QB9UyFsC-ePsu9B_e_aQ@mail.gmail.com","threadId":"59586","inReplyTo":"pull.1495.git.git.1681329955.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] cocci: codify authoring and reviewing practices","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-15T01:27:09Z","receivedAt":"2023-04-15T01:27:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 12, 2023 at 1:05 PM Glen Choo via GitGitGadget\n wrote:\n>\n> Here's the followup to the discussion in [1]. Sorry for the delay.\n>\n> I've tried to incorporate most of the responses from that thread as well as\n> suggest some guidelines that I think would make the authoring + reviewing\n> process smoother. I've opted for stronger wording to make the guidelines\n> easier to follow, but I don't feel strongly about the specifics.\n>\n> [1]\n> https://lore.kernel.org/git/kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com\n>\n> Glen Choo (2):\n> cocci: add headings to and reword README\n> cocci: codify authoring and reviewing practices\n>\n> contrib/coccinelle/README | 33 +++++++++++++++++++++++++++++----\n> 1 file changed, 29 insertions(+), 4 deletions(-)\n>\n>\n> base-commit: f285f68a132109c234d93490671c00218066ace9\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v1\n> Pull-Request: https://github.com/git/git/pull/1495\n> --\n> gitgitgadget\n\nI read through both patches, and I generally like them.\n\nI'm a little unsure on the \"To give a Reviewed-by\" bit of patch 2.\nFor example, it could be possible that .cocci changes might apply a\nfew different kinds of changes, and say only 2 of the 3 are reflected\nin the current tree, and those 2 types are handled correctly but the\nthird type of change is buggy. The .cocci files would then be a bug\nwaiting to happen. Maybe that's just a risk we take and it's okay for\nfolks to give a Reviewed-by even being unfamiliar with cocci. Maybe\nthe wording should instead be \"It's okay to give a Reviewed-by: on a\nseries that also contains cocci changes when you are unfamiliar with\ncoccinelle; just state that your Reviewed-by is limited to the other\nbits\". Or maybe the instructions should just be to give an Acked-by.\nYou should probably have someone familiar enough with coccinelle that\nthey know what is worth worrying about weigh in on that aspect.\n\nBut you can have my Acked-by on the other bits. :-)\n"},{"id":"475482","messageId":"20230416074212.GB3271@szeder.dev","threadId":"59586","inReplyTo":"75feb18dfd8af03f5e7ba02403a16a0ed4c2edaa.1681329955.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] cocci: codify authoring and reviewing practices","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2023-04-16T07:42:12Z","receivedAt":"2023-04-16T07:42:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 12, 2023 at 08:05:55PM +0000, Glen Choo via GitGitGadget wrote:\n> From: Glen Choo \n> \n> This isn't set in stone; we expect this to be updated as the project\n> evolves.\n> \n> Signed-off-by: Glen Choo \n> ---\n> contrib/coccinelle/README | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n> \n> diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\n> index 9b28ba1c57a..055e3622e5c 100644\n> --- a/contrib/coccinelle/README\n> +++ b/contrib/coccinelle/README\n> @@ -92,3 +92,26 @@ that might be useful to developers.\n> \n> The absolute times will differ for you, but the relative speedup\n> from caching should be on that order.\n> +\n> +== Authoring and reviewing coccinelle changes\n> +\n> +* When introducing and applying a new .cocci file, both the Git changes and\n> + .cocci file should be reviewed.\n> +\n> +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is\n> + enough for the reviewer to get a rough understanding of the proposed rules by\n> + comparing the .cocci and Git changes, then checking that understanding\n> + with the author.\n> +\n> +* Conversely, authors should consider that reviewers may not be coccinelle\n> + experts. The primary aim should be to make .cocci files easy to understand,\n> + e.g. by adding comments or by using rules that are easier to understand even\n> + if they are less elegant.\n> +\n> +* .cocci rules should target only the problem it is trying to solve; \"collateral\n> + damage\" is not allowed.\n> +\n> +* .cocci files used for refactoring should be temporarily kept in-tree to aid\n\nHow should such semantic patches be kept in-tree?\nAs .pending.cocci? Then I think it would be better to point this out\nhere. Or as a \"regular\" semantic patch? Then I'm not sure I agree\nwith this recommendation, but perhaps a commit message explaining the\nreasoning behind this would help me make up my mind :)\n\nIt might also be worth mentioning that before submitting a new\nsemantic patch developers should consider its cost-benefit ratio, in\nparticular its effect on the runtime of 'make coccicheck', in the hope\nthat we can avoid another 'unused.cocci' fiasco.\n\n> + the refactoring of out-of-tree code (e.g. in-flight topics). They should be\n> + removed when enough time has been given for others to refactor their code,\n> + i.e. ~1 release cycle.\n> -- \n> gitgitgadget\n"},{"id":"475489","messageId":"230416.86mt38rl2l.gmgdl@evledraar.gmail.com","threadId":"59586","inReplyTo":"75feb18dfd8af03f5e7ba02403a16a0ed4c2edaa.1681329955.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] cocci: codify authoring and reviewing practices","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-16T13:37:22Z","receivedAt":"2023-04-16T13:52:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> This isn't set in stone; we expect this to be updated as the project\n> evolves.\n>\n> Signed-off-by: Glen Choo \n> ---\n> contrib/coccinelle/README | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\n> index 9b28ba1c57a..055e3622e5c 100644\n> --- a/contrib/coccinelle/README\n> +++ b/contrib/coccinelle/README\n> @@ -92,3 +92,26 @@ that might be useful to developers.\n> \n> The absolute times will differ for you, but the relative speedup\n> from caching should be on that order.\n> +\n> +== Authoring and reviewing coccinelle changes\n> +\n> +* When introducing and applying a new .cocci file, both the Git changes and\n> + .cocci file should be reviewed.\n> +\n> +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is\n> + enough for the reviewer to get a rough understanding of the proposed rules by\n> + comparing the .cocci and Git changes, then checking that understanding\n> + with the author.\n\nMaybe it would be useful here to add something about how you can\nreproduce the application of the coccinelle rule(s).\n\nI sometimes do this on an ad-hoc basis, something like (untested):\n\n\tgit checkout HEAD^ -- ':!contrib/coccinelle' '*.[ch]'\n\tmake coccicheck\n\t\n\tgit add -A\n\nThen see if I ended up with a no-op, or if there's suggested changes.\n\nWith changes that modify both the header & source files this can be\ntricky with the default of SPATCH_USE_O_DEPENDENCIES=Y, but disabling it\nwill take care of any potential circular dependency issues. I.e. when\nthe header doesn't contain a required construct that we're replacing.\n\n> +* Conversely, authors should consider that reviewers may not be coccinelle\n> + experts. The primary aim should be to make .cocci files easy to understand,\n> + e.g. by adding comments or by using rules that are easier to understand even\n> + if they are less elegant.\n\nI agree that simple things should be kept simple, but this seems to come\nquite close (or perhaps past the line of) suggesting that we use only\nthe simpler features of the language when a more elegant solution would\nbe available with something less well-known.\n\nI think we should clarify that that's not the intent. Just as with C,\nshellscript, Perl etc. we should aim for simplicity, but ultimately we\nshould expect that we can target the full available language available\nto us.\n\n> +* .cocci rules should target only the problem it is trying to solve; \"collateral\n> + damage\" is not allowed.\n\nI think what you mean here is that you should be able to apply the rule\nand still build the project.\n\nI think that's correct, but I also think that rather than define this in\nprose, how about we just modify the current CI job to apply the result\nof non-pending rules, and do a build at the end? Wouldn't that assert\nthis going forward.\n\n> +* .cocci files used for refactoring should be temporarily kept in-tree to aid\n> + the refactoring of out-of-tree code (e.g. in-flight topics). They should be\n> + removed when enough time has been given for others to refactor their code,\n> + i.e. ~1 release cycle.\n\nMaybe s/should/can/? E.g. for my recent \"index\" and \"the_repository\"\npatches I think they can, but we often keep unused code in-repo for\nlonger than that. If e.g. that code stayed in for more than one release\nuntil someone cared to remove it we'd also be fine.\n\nI also don't know if some long-running forks (e.g. GfW?) would benefit\nfrom the rules for longer than that...\n"},{"id":"475530","messageId":"xmqqilduo4x1.fsf@gitster.g","threadId":"59586","inReplyTo":"CABPp-BEWaojwSpMaYT1VqNBYuhETm-QB9UyFsC-ePsu9B_e_aQ@mail.gmail.com","subject":"Re: [PATCH 0/2] cocci: codify authoring and reviewing practices","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T16:21:46Z","receivedAt":"2023-04-17T16:22:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> .... Maybe\n> the wording should instead be \"It's okay to give a Reviewed-by: on a\n> series that also contains cocci changes when you are unfamiliar with\n> coccinelle; just state that your Reviewed-by is limited to the other\n> bits\". Or maybe the instructions should just be to give an Acked-by.\n> You should probably have someone familiar enough with coccinelle that\n> they know what is worth worrying about weigh in on that aspect.\n>\n> But you can have my Acked-by on the other bits. :-)\n\nThe value of Reviewed-by takes two sides to determine. Even if we\nreserve a Reviewed-by to \"I have reviewed the entirety of this\npatch, and the patch is something I can stand behind\" (as opposed to\n\"my understanding of this patch is iffy in this and that area, but\nall the other parts of the patch is something I can stand behind\"),\nthe value of such a Reviewed-by is conditional to \"how well does the\nreviewer actually know the area?\" A drive-by \"Reviewed-by:\" thrown\ninto a review discussion thread by a total stranger would not carry\nmuch weight, until we know how much they are familiar with and how\ngood a taste they have.\n\nAnd honest qualifying comments like \"my understanding of this and\nthat area is iffy so I cannot endorse these parts\" helps build trust\nby others in the reviewer who gives such a partial review and we\nshould encourage such behaviour. I agree \"Acked-by:\" with comments\nis a good idea.\n\nThanks.\n"},{"id":"475709","messageId":"kl6lzg731xib.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59586","inReplyTo":"20230416074212.GB3271@szeder.dev","subject":"Re: [PATCH 2/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-19T19:29:32Z","receivedAt":"2023-04-19T19:29:46Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"SZEDER Gábor writes:\n\n>> +* .cocci rules should target only the problem it is trying to solve; \"collateral\n>> + damage\" is not allowed.\n>> +\n>> +* .cocci files used for refactoring should be temporarily kept in-tree to aid\n>\n> How should such semantic patches be kept in-tree?\n> As .pending.cocci? Then I think it would be better to point this out\n> here. Or as a \"regular\" semantic patch? Then I'm not sure I agree\n> with this recommendation, but perhaps a commit message explaining the\n> reasoning behind this would help me make up my mind :)\n\nI don't feel strongly about this, but I was envisioning keeping them as\na \"regular\" patch, e.g. what Ævar proposed in:\n\n https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/\n\nIn theory, this means that a long running fork (that didn't get updated\nduring the initial refactor) can run coccicheck, notice the failure, and\nthen automatically fix themselves with the included semantic patch. In\npractice, I don't know how many forks run coccicheck, or whether these\nrefactors are just easy enough to do by hand.\n\nFor refactors, I suspect that the impact on the 'make coccicheck'\nruntime will be low, since we're typically targeting just a few tokens\nand cocci can skip whatever files don't have those tokens, so keeping it\nas a \"regular\" patch might be okay.\n\n> It might also be worth mentioning that before submitting a new\n> semantic patch developers should consider its cost-benefit ratio, in\n> particular its effect on the runtime of 'make coccicheck',\n\nMakes sense, though I'm not sure what practical advice to give in order\nto evaluate the impact on runtime (besides just running it themselves).\n\n> in the hope\n> that we can avoid another 'unused.cocci' fiasco.\n\nMaybe this is a good starting point for discussing cost-benefit\nanalysis. I'm not familiar with this fiasco, though. Was an early\nversion of 'unused.cocci' too broad, resulting in a massive hit to\nruntime?\n"},{"id":"475719","messageId":"kl6lwn271p58.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59586","inReplyTo":"230416.86mt38rl2l.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-04-19T22:30:11Z","receivedAt":"2023-04-19T22:32:12Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote:\n>\n>> +== Authoring and reviewing coccinelle changes\n>> +\n>> +* When introducing and applying a new .cocci file, both the Git changes and\n>> + .cocci file should be reviewed.\n>> +\n>> +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is\n>> + enough for the reviewer to get a rough understanding of the proposed rules by\n>> + comparing the .cocci and Git changes, then checking that understanding\n>> + with the author.\n>\n> Maybe it would be useful here to add something about how you can\n> reproduce the application of the coccinelle rule(s).\n>\n> I sometimes do this on an ad-hoc basis, something like (untested):\n>\n> \tgit checkout HEAD^ -- ':!contrib/coccinelle' '*.[ch]'\n> \tmake coccicheck\n> \t\n> \tgit add -A\n>\n> Then see if I ended up with a no-op, or if there's suggested changes.\n>\n> With changes that modify both the header & source files this can be\n> tricky with the default of SPATCH_USE_O_DEPENDENCIES=Y, but disabling it\n> will take care of any potential circular dependency issues. I.e. when\n> the header doesn't contain a required construct that we're replacing.\n\nMakes sense. I've been thinking about sending a \"MyFirstCocci\" guide as\na follow up, and this sounds like the kind of \"Tips & Tricks\" content\nthat would belong there.\n\n>> +* Conversely, authors should consider that reviewers may not be coccinelle\n>> + experts. The primary aim should be to make .cocci files easy to understand,\n>> + e.g. by adding comments or by using rules that are easier to understand even\n>> + if they are less elegant.\n>\n> I agree that simple things should be kept simple, but this seems to come\n> quite close (or perhaps past the line of) suggesting that we use only\n> the simpler features of the language when a more elegant solution would\n> be available with something less well-known.\n>\n> I think we should clarify that that's not the intent. Just as with C,\n> shellscript, Perl etc. we should aim for simplicity, but ultimately we\n> should expect that we can target the full available language available\n> to us.\n\nMakes sense too. I think I'll adjust this to something to the effect of\n\"When using more esoteric parts of the language, be prepared to explain\nwhat the .cocci is doing.\".\n\n>> +* .cocci rules should target only the problem it is trying to solve; \"collateral\n>> + damage\" is not allowed.\n>\n> I think what you mean here is that you should be able to apply the rule\n> and still build the project.\n\nYes and no. Yes in that if the project doesn't build, the rule is\nobviously overly broad, but no in that I think it's possible to write a\nrule that affects something you didn't mean to, but still builds. I\ncan't think of a way to automatedly check for the latter case, so I\ncategorized it as something to catch at review time.\n\n>> +* .cocci files used for refactoring should be temporarily kept in-tree to aid\n>> + the refactoring of out-of-tree code (e.g. in-flight topics). They should be\n>> + removed when enough time has been given for others to refactor their code,\n>> + i.e. ~1 release cycle.\n>\n> Maybe s/should/can/? E.g. for my recent \"index\" and \"the_repository\"\n> patches I think they can, but we often keep unused code in-repo for\n> longer than that. If e.g. that code stayed in for more than one release\n> until someone cared to remove it we'd also be fine.\n>\n> I also don't know if some long-running forks (e.g. GfW?) would benefit\n> from the rules for longer than that...\n\nYeah, this is the most iffy to me too, which means it would be extra\nhelpful to decide on as many details as we can now instead of deciding\nad-hoc.\n\nPost-refactor, the .cocci file is always obsolete in-tree, so I think we\ncan either say \"always keep the patch\" or \"never keep the patch\".\n\nIf I understand you correctly, _how long_ to keep the patch is probably\na case-by-case matter, though (makes sense to me). I think this comes\ndown to the cost-benefit tradeoff mentioned by others elsewhere in the\nthread. Maybe I'll just mention that it depends on the cost-benefit\nanalysis and drop the \"~1 release cycle\" recommendation.\n"},{"id":"475762","messageId":"20230420205350.600760-1-szeder.dev@gmail.com","threadId":"59586","inReplyTo":"kl6lzg731xib.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"[PATCH] cocci: remove 'unused.cocci'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2023-04-20T20:53:50Z","receivedAt":"2023-04-20T20:54:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a\nrule to find \"unused\" strbufs, 2022-07-05) it found three unused\nstrbufs, and when it was generalized in the next commit it managed to\nfind an unused string_list as well. That's four unused variables in\nover 17 years, so apparently we rarely make this mistake.\n\nUnfortunately, applying 'unused.cocci' is quite expensive, e.g. it\nincreases the from-scratch runtime of 'make coccicheck' by over 5:30\nminutes or over 160%:\n\n $ make -s cocciclean\n $ time make -s coccicheck\n * new spatch flags\n\n real 8m56.201s\n user 0m0.420s\n sys 0m0.406s\n $ rm contrib/coccinelle/unused.cocci contrib/coccinelle/tests/unused.*\n $ make -s cocciclean\n $ time make -s coccicheck\n * new spatch flags\n\n real 3m23.893s\n user 0m0.228s\n sys 0m0.247s\n\nThat's a lot of runtime spent for not much in return, and arguably an\nunused struct instance sneaking in is not that big of a deal to\njustify the significantly increased runtime.\n\nRemove 'unused.cocci', because we are not getting our CPU cycles'\nworth.\n\nSigned-off-by: SZEDER Gábor \n---\n contrib/coccinelle/tests/unused.c | 82 -----------------------------\n contrib/coccinelle/tests/unused.res | 45 ----------------\n contrib/coccinelle/unused.cocci | 43 ---------------\n 3 files changed, 170 deletions(-)\n delete mode 100644 contrib/coccinelle/tests/unused.c\n delete mode 100644 contrib/coccinelle/tests/unused.res\n delete mode 100644 contrib/coccinelle/unused.cocci\n\ndiff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c\ndeleted file mode 100644\nindex 8294d734ba..0000000000\n--- a/contrib/coccinelle/tests/unused.c\n+++ /dev/null\n@@ -1,82 +0,0 @@\n-void test_strbuf(void)\n-{\n-\tstruct strbuf sb1 = STRBUF_INIT;\n-\tstruct strbuf sb2 = STRBUF_INIT;\n-\tstruct strbuf sb3 = STRBUF_INIT;\n-\tstruct strbuf sb4 = STRBUF_INIT;\n-\tstruct strbuf sb5;\n-\tstruct strbuf sb6 = { 0 };\n-\tstruct strbuf sb7 = STRBUF_INIT;\n-\tstruct strbuf sb8 = STRBUF_INIT;\n-\tstruct strbuf *sp1;\n-\tstruct strbuf *sp2;\n-\tstruct strbuf *sp3;\n-\tstruct strbuf *sp4 = xmalloc(sizeof(struct strbuf));\n-\tstruct strbuf *sp5 = xmalloc(sizeof(struct strbuf));\n-\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n-\tstruct strbuf *sp7;\n-\n-\tstrbuf_init(&sb5, 0);\n-\tstrbuf_init(sp1, 0);\n-\tstrbuf_init(sp2, 0);\n-\tstrbuf_init(sp3, 0);\n-\tstrbuf_init(sp4, 0);\n-\tstrbuf_init(sp5, 0);\n-\tstrbuf_init(sp6, 0);\n-\tstrbuf_init(sp7, 0);\n-\tsp7 = xmalloc(sizeof(struct strbuf));\n-\n-\tuse_before(&sb3);\n-\tuse_as_str(\"%s\", sb7.buf);\n-\tuse_as_str(\"%s\", sp1->buf);\n-\tuse_as_str(\"%s\", sp6->buf);\n-\tpass_pp(&sp3);\n-\n-\tstrbuf_release(&sb1);\n-\tstrbuf_reset(&sb2);\n-\tstrbuf_release(&sb3);\n-\tstrbuf_release(&sb4);\n-\tstrbuf_release(&sb5);\n-\tstrbuf_release(&sb6);\n-\tstrbuf_release(&sb7);\n-\tstrbuf_release(sp1);\n-\tstrbuf_release(sp2);\n-\tstrbuf_release(sp3);\n-\tstrbuf_release(sp4);\n-\tstrbuf_release(sp5);\n-\tstrbuf_release(sp6);\n-\tstrbuf_release(sp7);\n-\n-\tuse_after(&sb4);\n-\n-\tif (when_strict())\n-\t\treturn;\n-\tstrbuf_release(&sb8);\n-}\n-\n-void test_other(void)\n-{\n-\tstruct string_list l = STRING_LIST_INIT_DUP;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\n-\tstring_list_clear(&l, 0);\n-\tstring_list_clear(&sb, 0);\n-}\n-\n-void test_worktrees(void)\n-{\n-\tstruct worktree **w1 = get_worktrees();\n-\tstruct worktree **w2 = get_worktrees();\n-\tstruct worktree **w3;\n-\tstruct worktree **w4;\n-\n-\tw3 = get_worktrees();\n-\tw4 = get_worktrees();\n-\n-\tuse_it(w4);\n-\n-\tfree_worktrees(w1);\n-\tfree_worktrees(w2);\n-\tfree_worktrees(w3);\n-\tfree_worktrees(w4);\n-}\ndiff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res\ndeleted file mode 100644\nindex 6d3e745683..0000000000\n--- a/contrib/coccinelle/tests/unused.res\n+++ /dev/null\n@@ -1,45 +0,0 @@\n-void test_strbuf(void)\n-{\n-\tstruct strbuf sb3 = STRBUF_INIT;\n-\tstruct strbuf sb4 = STRBUF_INIT;\n-\tstruct strbuf sb7 = STRBUF_INIT;\n-\tstruct strbuf *sp1;\n-\tstruct strbuf *sp3;\n-\tstruct strbuf *sp6 = xmalloc(sizeof(struct strbuf));\n-\tstrbuf_init(sp1, 0);\n-\tstrbuf_init(sp3, 0);\n-\tstrbuf_init(sp6, 0);\n-\n-\tuse_before(&sb3);\n-\tuse_as_str(\"%s\", sb7.buf);\n-\tuse_as_str(\"%s\", sp1->buf);\n-\tuse_as_str(\"%s\", sp6->buf);\n-\tpass_pp(&sp3);\n-\n-\tstrbuf_release(&sb3);\n-\tstrbuf_release(&sb4);\n-\tstrbuf_release(&sb7);\n-\tstrbuf_release(sp1);\n-\tstrbuf_release(sp3);\n-\tstrbuf_release(sp6);\n-\n-\tuse_after(&sb4);\n-\n-\tif (when_strict())\n-\t\treturn;\n-}\n-\n-void test_other(void)\n-{\n-}\n-\n-void test_worktrees(void)\n-{\n-\tstruct worktree **w4;\n-\n-\tw4 = get_worktrees();\n-\n-\tuse_it(w4);\n-\n-\tfree_worktrees(w4);\n-}\ndiff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci\ndeleted file mode 100644\nindex d84046f82e..0000000000\n--- a/contrib/coccinelle/unused.cocci\n+++ /dev/null\n@@ -1,43 +0,0 @@\n-// This rule finds sequences of \"unused\" declerations and uses of a\n-// variable, where \"unused\" is defined to include only calling the\n-// equivalent of alloc, init & free functions on the variable.\n-@@\n-type T;\n-identifier I;\n-// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring)\n-constant INIT_MACRO =~ \"_INIT\";\n-identifier MALLOC1 =~ \"^x?[mc]alloc$\";\n-identifier INIT_ASSIGN1 =~ \"^get_worktrees$\";\n-identifier INIT_CALL1 =~ \"^[a-z_]*_init$\";\n-identifier REL1 =~ \"^[a-z_]*_(release|reset|clear|free)$\";\n-identifier REL2 =~ \"^(release|clear|free)_[a-z_]*$\";\n-@@\n-\n-(\n-- T I;\n-|\n-- T I = { 0 };\n-|\n-- T I = INIT_MACRO;\n-|\n-- T I = MALLOC1(...);\n-|\n-- T I = INIT_ASSIGN1(...);\n-)\n-\n-<... when != \\( I \\| &I \\)\n-(\n-- \\( INIT_CALL1 \\)( \\( I \\| &I \\), ...);\n-|\n-- I = \\( INIT_ASSIGN1 \\)(...);\n-|\n-- I = MALLOC1(...);\n-)\n-...>\n-\n-(\n-- \\( REL1 \\| REL2 \\)( \\( I \\| &I \\), ...);\n-|\n-- \\( REL1 \\| REL2 \\)( \\( &I \\| I \\) );\n-)\n- ... when != \\( I \\| &I \\)\n-- \n2.40.0.573.g2c27013916\n\n"},{"id":"475778","messageId":"xmqqmt32lzul.fsf@gitster.g","threadId":"59586","inReplyTo":"20230420205350.600760-1-szeder.dev@gmail.com","subject":"Re: [PATCH] cocci: remove 'unused.cocci'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-21T02:43:14Z","receivedAt":"2023-04-21T02:43:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a\n> rule to find \"unused\" strbufs, 2022-07-05) it found three unused\n> strbufs, and when it was generalized in the next commit it managed to\n> find an unused string_list as well. That's four unused variables in\n> over 17 years, so apparently we rarely make this mistake.\n>\n> Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it\n> increases the from-scratch runtime of 'make coccicheck' by over 5:30\n> minutes or over 160%:\n> ...\n> That's a lot of runtime spent for not much in return, and arguably an\n> unused struct instance sneaking in is not that big of a deal to\n> justify the significantly increased runtime.\n>\n> Remove 'unused.cocci', because we are not getting our CPU cycles'\n> worth.\n\nWill queue. Thanks.\n\n"},{"id":"476240","messageId":"pull.1495.v2.git.git.1682634143.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":"pull.1495.git.git.1681329955.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-27T22:22:21Z","receivedAt":"2023-04-27T22:22:29Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for the input on v1, all :)\n\nI've tried to capture all of the discussion in some form. AFAICT, the result\nis quite similar to what we are already doing, so it might not be very\nhelpful to folks who have already worked with Coccinelle, but it should\nhopefully be useful to newcomers.\n\nI suspect that we won't converge on any new practices during this\ndiscussion, but as we develop practices in the future, we can just update\nthis doc.\n\nGlen Choo (2):\n cocci: add headings to and reword README\n cocci: codify authoring and reviewing practices\n\n contrib/coccinelle/README | 40 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 36 insertions(+), 4 deletions(-)\n\n\nbase-commit: f285f68a132109c234d93490671c00218066ace9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v2\nPull-Request: https://github.com/git/git/pull/1495\n\nRange-diff vs v1:\n\n 1: 4a8b8a2a674 = 1: 4a8b8a2a674 cocci: add headings to and reword README\n 2: 75feb18dfd8 ! 2: acee642531a cocci: codify authoring and reviewing practices\n @@ Metadata\n ## Commit message ##\n cocci: codify authoring and reviewing practices\n \n - This isn't set in stone; we expect this to be updated as the project\n - evolves.\n + These practices largely reflect what we are already doing on the mailing\n + list, which should help new Coccinelle authors and reviewers get up to\n + speed.\n \n Signed-off-by: Glen Choo \n \n @@ contrib/coccinelle/README: that might be useful to developers.\n +\n +== Authoring and reviewing coccinelle changes\n +\n -+* When introducing and applying a new .cocci file, both the Git changes and\n -+ .cocci file should be reviewed.\n ++* When a .cocci is made, both the Git changes and .cocci file should be\n ++ reviewed. When reviewing such a change, do your best to understand the .cocci\n ++ changes (e.g. by asking the author to explain the change) and be explicit\n ++ about your understanding of the changes. This helps us decide whether input\n ++ from coccinelle experts is needed or not. If you aren't sure of the cocci\n ++ changes, indicate what changes you actively endorse and leave an Acked-by\n ++ (instead of Reviewed-by).\n +\n -+* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is\n -+ enough for the reviewer to get a rough understanding of the proposed rules by\n -+ comparing the .cocci and Git changes, then checking that understanding\n -+ with the author.\n -+\n -+* Conversely, authors should consider that reviewers may not be coccinelle\n -+ experts. The primary aim should be to make .cocci files easy to understand,\n -+ e.g. by adding comments or by using rules that are easier to understand even\n -+ if they are less elegant.\n ++* Authors should consider that reviewers may not be coccinelle experts, thus the\n ++ the .cocci changes may not be self-evident. A plain text description of the\n ++ changes is strongly encouraged, especially when using more esoteric features\n ++ of the language.\n +\n +* .cocci rules should target only the problem it is trying to solve; \"collateral\n -+ damage\" is not allowed.\n ++ damage\" is not allowed. Reviewers should look out and flag overly-broad rules.\n ++\n ++* Consider the cost-benefit ratio of .cocci changes. In particular, consider the\n ++ effect on the runtime of \"make coccicheck\", and how often your .cocci check\n ++ will catch something valuable. As a rule of thumb, rules that can bail early\n ++ if a file doesn't have a particular token will have a small impact on runtime,\n ++ and vice-versa.\n +\n +* .cocci files used for refactoring should be temporarily kept in-tree to aid\n -+ the refactoring of out-of-tree code (e.g. in-flight topics). They should be\n -+ removed when enough time has been given for others to refactor their code,\n -+ i.e. ~1 release cycle.\n ++ the refactoring of out-of-tree code (e.g. in-flight topics). Periodically\n ++ evaluate the cost-benefit ratio to determine when the file should be removed.\n ++ For example, consider how many out-of-tree users are left and how much this\n ++ slows down \"make coccicheck\".\n\n-- \ngitgitgadget\n"},{"id":"476241","messageId":"4a8b8a2a6745e791e35296e34f530b5f40f51c27.1682634143.git.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":"pull.1495.v2.git.git.1682634143.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-27T22:22:22Z","receivedAt":"2023-04-27T22:22:36Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n- Drop \"examples\" since we actually use the patches.\n- Drop sentences that could be headings instead\n\nSigned-off-by: Glen Choo \n---\n contrib/coccinelle/README | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\nindex d1daa1f6263..9b28ba1c57a 100644\n--- a/contrib/coccinelle/README\n+++ b/contrib/coccinelle/README\n@@ -1,7 +1,9 @@\n-This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)\n-semantic patches that might be useful to developers.\n+= coccinelle\n \n-There are two types of semantic patches:\n+This directory provides Coccinelle (http://coccinelle.lip6.fr/) semantic patches\n+that might be useful to developers.\n+\n+== Types of semantic patches\n \n * Using the semantic transformation to check for bad patterns in the code;\n The target 'make coccicheck' is designed to check for these patterns and\n@@ -42,7 +44,7 @@ There are two types of semantic patches:\n This allows to expose plans of pending large scale refactorings without\n impacting the bad pattern checks.\n \n-Git-specific tips & things to know about how we run \"spatch\":\n+== Git-specific tips & things to know about how we run \"spatch\":\n \n * The \"make coccicheck\" will piggy-back on\n \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n-- \ngitgitgadget\n\n"},{"id":"476242","messageId":"acee642531a582c0abc5d88b23476680e653314f.1682634143.git.gitgitgadget@gmail.com","threadId":"59586","inReplyTo":"pull.1495.v2.git.git.1682634143.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] cocci: codify authoring and reviewing practices","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-27T22:22:23Z","receivedAt":"2023-04-27T22:22:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nThese practices largely reflect what we are already doing on the mailing\nlist, which should help new Coccinelle authors and reviewers get up to\nspeed.\n\nSigned-off-by: Glen Choo \n---\n contrib/coccinelle/README | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/contrib/coccinelle/README b/contrib/coccinelle/README\nindex 9b28ba1c57a..055ad0e06a7 100644\n--- a/contrib/coccinelle/README\n+++ b/contrib/coccinelle/README\n@@ -92,3 +92,33 @@ that might be useful to developers.\n \n The absolute times will differ for you, but the relative speedup\n from caching should be on that order.\n+\n+== Authoring and reviewing coccinelle changes\n+\n+* When a .cocci is made, both the Git changes and .cocci file should be\n+ reviewed. When reviewing such a change, do your best to understand the .cocci\n+ changes (e.g. by asking the author to explain the change) and be explicit\n+ about your understanding of the changes. This helps us decide whether input\n+ from coccinelle experts is needed or not. If you aren't sure of the cocci\n+ changes, indicate what changes you actively endorse and leave an Acked-by\n+ (instead of Reviewed-by).\n+\n+* Authors should consider that reviewers may not be coccinelle experts, thus the\n+ the .cocci changes may not be self-evident. A plain text description of the\n+ changes is strongly encouraged, especially when using more esoteric features\n+ of the language.\n+\n+* .cocci rules should target only the problem it is trying to solve; \"collateral\n+ damage\" is not allowed. Reviewers should look out and flag overly-broad rules.\n+\n+* Consider the cost-benefit ratio of .cocci changes. In particular, consider the\n+ effect on the runtime of \"make coccicheck\", and how often your .cocci check\n+ will catch something valuable. As a rule of thumb, rules that can bail early\n+ if a file doesn't have a particular token will have a small impact on runtime,\n+ and vice-versa.\n+\n+* .cocci files used for refactoring should be temporarily kept in-tree to aid\n+ the refactoring of out-of-tree code (e.g. in-flight topics). Periodically\n+ evaluate the cost-benefit ratio to determine when the file should be removed.\n+ For example, consider how many out-of-tree users are left and how much this\n+ slows down \"make coccicheck\".\n-- \ngitgitgadget\n"},{"id":"476327","messageId":"230501.86h6swjp3r.gmgdl@evledraar.gmail.com","threadId":"59586","inReplyTo":"4a8b8a2a6745e791e35296e34f530b5f40f51c27.1682634143.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-05-01T10:53:19Z","receivedAt":"2023-05-01T10:57:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote:\n\nRe subject: I don't per-se mind the \"add headings\" formatting change,\nbut doesn't it have headings already? I.e.:\n\n> -Git-specific tips & things to know about how we run \"spatch\":\n> +== Git-specific tips & things to know about how we run \"spatch\":\n> \n> * The \"make coccicheck\" will piggy-back on\n> \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n\nI think it was clear before that that was a \"heading\", at least in the\nsense that it summarized what the indented part that followed was\ndiscussing.\n\nI think what this is really doing is converting this part of the doc to\nasciidoc, but is anything actually rendering it as asciidoc?\n\nIf we are converting it to asciidoc shouldn't the bullet-points be\nun-indented too? (I'm not sure, but couldn't find a part of our build\nthat actually feeds this through asciidoc, so spot-checking that wasn't\ntrivial...)\n\n"},{"id":"476330","messageId":"230501.864jowjh15.gmgdl@evledraar.gmail.com","threadId":"59586","inReplyTo":"20230420205350.600760-1-szeder.dev@gmail.com","subject":"Re: [PATCH] cocci: remove 'unused.cocci'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-05-01T13:27:54Z","receivedAt":"2023-05-01T13:52:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 20 2023, SZEDER Gábor wrote:\n\n> When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a\n> rule to find \"unused\" strbufs, 2022-07-05) it found three unused\n> strbufs, and when it was generalized in the next commit it managed to\n> find an unused string_list as well. That's four unused variables in\n> over 17 years, so apparently we rarely make this mistake.\n>\n> Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it\n> increases the from-scratch runtime of 'make coccicheck' by over 5:30\n> minutes or over 160%:\n>\n> $ make -s cocciclean\n> $ time make -s coccicheck\n> * new spatch flags\n>\n> real 8m56.201s\n> user 0m0.420s\n> sys 0m0.406s\n> $ rm contrib/coccinelle/unused.cocci contrib/coccinelle/tests/unused.*\n> $ make -s cocciclean\n> $ time make -s coccicheck\n> * new spatch flags\n>\n> real 3m23.893s\n> user 0m0.228s\n> sys 0m0.247s\n>\n> That's a lot of runtime spent for not much in return, and arguably an\n> unused struct instance sneaking in is not that big of a deal to\n> justify the significantly increased runtime.\n>\n> Remove 'unused.cocci', because we are not getting our CPU cycles'\n> worth.\n\nIt wasn't something I intended at the time, but arguably the main use of\nthis rule since it was added was that it served as a canary for the tree\nbecoming completely broken with coccinelle, due to adding C syntax it\ndidn't understand:\nhttps://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/\n\nSo, whatever you think of of how worthwhile it is to spot unused\nvariables, I think that weighs heavily in its favor. There *are* other\nways to detect those sorts of issues, but as it's currently our only\ncanary for that issue I don't thin we should remove it.\n\nIf we hadn't had unused.cocci we wouldn't be able to apply rules the\nfunctions that use \"UNUSED\", which have increased a lot in number since\nthen, and we wouldn't have any way of spotting similar parsing issues.\n\nBut it's unfortunate that it's this slow in the from-scratch case.\n\nWhen we last discussed this I pointed out to you that the main\ncontribution to the runtime of unused.cocci is parsing\nbuiltin/{log,rebase}.c is pathalogical, but in your reply to that you\nseem to not have spotted that (or glossed over it):\nhttps://lore.kernel.org/git/20220831180526.GA1802@szeder.dev/\n\nWhen I test this locally, doing:\n\n\ttime make contrib/coccinelle/unused.cocci.patch SPATCH=spatch SPATCH_USE_O_DEPENDENCIES=\n\nTakes ~2m, but if I first do:\n\n\t>builtin/log.c; >builtin/rebase.c\n\nIt takes ~1m.\n\nSo, even without digging into those issues, if we just skipped those two\nfiles we'd speed this part up by 100%.\n\nI think such an approach would be much better than just removing this\noutright, which feels rather heavy-handed.\n\nWe could formalize that by creating a \"coccicheck-full\" category or\nwhatever, just as we now have \"coccicheck-pending\".\n\nThen I and the CI could run \"full\", and you could run \"coccicheck\" (or\nmaybe we'd call that \"coccicheck-cheap\" or something).\n\nI also submitted patches to both make \"coccicheck\" incremental, and\nadded an \"spatchcache\", both of which have since been merged (that tool\nis in contrib/).\n\nI understand from previous discussion that you wanted to use \"make -s\"\nall the time, but does your use-case also preclude using spatchcache?\n\nI run \"coccicheck\" a lot, and haven't personally be bothered by this\nparticular slowdown since that got merged, since it'll only affect me in\nthe cases where builtin/{log,rebase}.c and a small list of other files\nare changed, and it's otherwise unnoticeable.\n\nIt would also be rather trivial to just add some way to specify patterns\non the \"make\" command-line that we'd \"$(filter-out)\", would that also\naddress your particular use-case? I.e.:\n\n\tmake coccicheck COCCI_RULES_EXCLUDE=*unused*\n\nOr whatever.\n\n\n\n"},{"id":"476335","messageId":"xmqqjzxs851d.fsf@gitster.g","threadId":"59586","inReplyTo":"230501.86h6swjp3r.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T15:06:38Z","receivedAt":"2023-05-01T15:06:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote:\n>\n> Re subject: I don't per-se mind the \"add headings\" formatting change,\n> but doesn't it have headings already? I.e.:\n>\n>> -Git-specific tips & things to know about how we run \"spatch\":\n>> +== Git-specific tips & things to know about how we run \"spatch\":\n>> \n>> * The \"make coccicheck\" will piggy-back on\n>> \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n\nI think \"add headings\" mostly refers to what the first hunk, that\nis, the hunk before that one, did. Giving the entire document the\ntitle (while removing references to \"examples\"). As a side effect,\nthe existing two sections (\"-Git-specific tips...\" we see above is\nthe second one among them) are moved down in the section hierarchy;\nin other words, I do not think the highlighted part of the patch in\nyour message is the primary change intended.\n"},{"id":"476347","messageId":"xmqqlei86o7s.fsf@gitster.g","threadId":"59586","inReplyTo":"230501.864jowjh15.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cocci: remove 'unused.cocci'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T15:55:19Z","receivedAt":"2023-05-01T15:55:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> It wasn't something I intended at the time, but arguably the main use of\n> this rule since it was added was that it served as a canary for the tree\n> becoming completely broken with coccinelle, due to adding C syntax it\n> didn't understand:\n> https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/\n\nIf it weren't Coccinelle, we could have used the much nicer looking\nUNUSED(var) notation, and the compilers were all fine.\n\nOnly because Coccinelle did not understand the \"cute\" syntax trick,\nwe couldn't. Yes, it caught us when we used a syntax it couldn't\nunderstand, but is that a good thing in the first place?\n\n"},{"id":"476365","messageId":"230501.865y9chs69.gmgdl@evledraar.gmail.com","threadId":"59586","inReplyTo":"xmqqlei86o7s.fsf@gitster.g","subject":"Re: [PATCH] cocci: remove 'unused.cocci'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-05-01T17:28:50Z","receivedAt":"2023-05-01T17:34:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 01 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> It wasn't something I intended at the time, but arguably the main use of\n>> this rule since it was added was that it served as a canary for the tree\n>> becoming completely broken with coccinelle, due to adding C syntax it\n>> didn't understand:\n>> https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/\n>\n> If it weren't Coccinelle, we could have used the much nicer looking\n> UNUSED(var) notation, and the compilers were all fine.\n>\n> Only because Coccinelle did not understand the \"cute\" syntax trick,\n> we couldn't. Yes, it caught us when we used a syntax it couldn't\n> understand, but is that a good thing in the first place?\n\nI think it's unambiguously a good thing that we spotted an otherwise\nunknown side-effect of the proposed UNUSED(var) syntax on coccinelle.\n\nWe might also say that some bit of syntax that coccinelle doesn't\nunderstand is so valuable that we'd like to make coccinelle itself\nsignificantly less useful (as it wouldn't reach into those functions),\nor stop using it altogether.\n\nBut that's a seperate question. I'm just pointing out that we'd be\nlosing a very valuable check on future syntax incompatibilities,\nparticularly when it comes to clever use of macros.\n\nA better way to spot that would be to start parsing the coccinelle logs,\nand detect when we have unknown parsing issues, and error on those. But\nuntil then...\n\n"},{"id":"476454","messageId":"6451648c11a04_1ba2d2947a@chronos.notmuch","threadId":"59586","inReplyTo":"230501.86h6swjp3r.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-02T19:29:16Z","receivedAt":"2023-05-02T19:29:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote:\n> \n> Re subject: I don't per-se mind the \"add headings\" formatting change,\n> but doesn't it have headings already? I.e.:\n> \n> > -Git-specific tips & things to know about how we run \"spatch\":\n> > +== Git-specific tips & things to know about how we run \"spatch\":\n> > \n> > * The \"make coccicheck\" will piggy-back on\n> > \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n> \n> I think it was clear before that that was a \"heading\", at least in the\n> sense that it summarized what the indented part that followed was\n> discussing.\n> \n> I think what this is really doing is converting this part of the doc to\n> asciidoc, but is anything actually rendering it as asciidoc?\n\nPersonally I write many documents in AsciiDoc format even if I'm not\nusing asciidoc, as I find them easier to read.\n\nMoreover, one can always do `:set ft=asciidoc` in vim to see some syntax\ncolors for an easier read.\n\n> If we are converting it to asciidoc shouldn't the bullet-points be\n> un-indented too? (I'm not sure, but couldn't find a part of our build\n> that actually feeds this through asciidoc, so spot-checking that wasn't\n> trivial...)\n\nYou can just do `asciidoctor doc.txt` with any document and it will\ngenerate an HTML page.\n\n-- \nFelipe Contreras"},{"id":"476455","messageId":"645164f0e6064_1ba2d294b3@chronos.notmuch","threadId":"59586","inReplyTo":"230501.86h6swjp3r.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-02T19:30:56Z","receivedAt":"2023-05-02T19:31:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"I fogot to mention:\n\nÆvar Arnfjörð Bjarmason wrote:\n> If we are converting it to asciidoc shouldn't the bullet-points be\n> un-indented too?\n\nI don't think that's necessary.\n\n-- \nFelipe Contreras"},{"id":"476907","messageId":"kl6l8rdx1jbt.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59586","inReplyTo":"230501.86h6swjp3r.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] cocci: add headings to and reword README","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-09T17:54:46Z","receivedAt":"2023-05-09T17:55:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote:\n>\n> Re subject: I don't per-se mind the \"add headings\" formatting change,\n> but doesn't it have headings already? I.e.:\n>\n>> -Git-specific tips & things to know about how we run \"spatch\":\n>> +== Git-specific tips & things to know about how we run \"spatch\":\n>> \n>> * The \"make coccicheck\" will piggy-back on\n>> \"COMPUTE_HEADER_DEPENDENCIES\". If you've built a given object file\n>\n> I think it was clear before that that was a \"heading\", at least in the\n> sense that it summarized what the indented part that followed was\n> discussing.\n\nAs Junio guessed downthread, I was primarily aiming to heading-ify the\nother parts of the doc.\n\n> I think what this is really doing is converting this part of the doc to\n> asciidoc, but is anything actually rendering it as asciidoc?\n\nAnd as Felipe mentioned downthread, I chose to author it as asciidoc\nbecause I also find structured docs easier to read, and asciidoc seems\nto be the closest thing to a standardized format we have. You're right\nthat nothing renders this as asciidoc.\n\nThanks, all :)\n\n> If we are converting it to asciidoc shouldn't the bullet-points be\n> un-indented too? (I'm not sure, but couldn't find a part of our build\n> that actually feeds this through asciidoc, so spot-checking that wasn't\n> trivial...)\n\nThanks Felipe for checking.\n"},{"id":"477000","messageId":"xmqqv8gz24c7.fsf@gitster.g","threadId":"59586","inReplyTo":"230501.865y9chs69.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] cocci: remove 'unused.cocci'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T22:45:28Z","receivedAt":"2023-05-10T22:45:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> A better way to spot that would be to start parsing the coccinelle logs,\n> and detect when we have unknown parsing issues, and error on those. But\n> until then...\n\nUntil then, I do not think a rather costly test that has found only\n4 instances of the mistakes the test was designed to find is a good\nway to stand in as a replacement.\n\nLet's drop it, as it is easy to resurrect if somebody wants to run\nit from time to time from an old version of Git. Or is it a valid\nalternative to move it to \"pending\"?\n\nThanks.\n"}]}