* [PATCH 0/2] cocci: codify authoring and reviewing practices @ 2023-04-12 20:05 Glen Choo via GitGitGadget 2023-04-12 20:05 ` [PATCH 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-12 20:05 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, Glen Choo Here's the followup to the discussion in [1]. Sorry for the delay. I've tried to incorporate most of the responses from that thread as well as suggest some guidelines that I think would make the authoring + reviewing process smoother. I've opted for stronger wording to make the guidelines easier to follow, but I don't feel strongly about the specifics. [1] https://lore.kernel.org/git/kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com Glen Choo (2): cocci: add headings to and reword README cocci: codify authoring and reviewing practices contrib/coccinelle/README | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) base-commit: f285f68a132109c234d93490671c00218066ace9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v1 Pull-Request: https://github.com/git/git/pull/1495 -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] cocci: add headings to and reword README 2023-04-12 20:05 [PATCH 0/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget @ 2023-04-12 20:05 ` Glen Choo via GitGitGadget 2023-04-12 21:18 ` Junio C Hamano 2023-04-12 20:05 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-12 20:05 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, Glen Choo, Glen Choo From: Glen Choo <chooglen@google.com> - Drop "examples" since we actually use the patches. - Drop sentences that could be headings instead Signed-off-by: Glen Choo <chooglen@google.com> --- contrib/coccinelle/README | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index d1daa1f6263..9b28ba1c57a 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -1,7 +1,9 @@ -This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) -semantic patches that might be useful to developers. += coccinelle -There are two types of semantic patches: +This directory provides Coccinelle (http://coccinelle.lip6.fr/) semantic patches +that might be useful to developers. + +== Types of semantic patches * Using the semantic transformation to check for bad patterns in the code; The target 'make coccicheck' is designed to check for these patterns and @@ -42,7 +44,7 @@ There are two types of semantic patches: This allows to expose plans of pending large scale refactorings without impacting the bad pattern checks. -Git-specific tips & things to know about how we run "spatch": +== Git-specific tips & things to know about how we run "spatch": * The "make coccicheck" will piggy-back on "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] cocci: add headings to and reword README 2023-04-12 20:05 ` [PATCH 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget @ 2023-04-12 21:18 ` Junio C Hamano 2023-04-13 18:37 ` Glen Choo 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-04-12 21:18 UTC (permalink / raw) To: Glen Choo via GitGitGadget Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, Glen Choo "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Glen Choo <chooglen@google.com> > > - Drop "examples" since we actually use the patches. > - Drop sentences that could be headings instead > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > contrib/coccinelle/README | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Makes sense. Will queue. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] cocci: add headings to and reword README 2023-04-12 21:18 ` Junio C Hamano @ 2023-04-13 18:37 ` Glen Choo 2023-04-13 18:51 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Glen Choo @ 2023-04-13 18:37 UTC (permalink / raw) To: Junio C Hamano, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren Junio C Hamano <gitster@pobox.com> writes: >> From: Glen Choo <chooglen@google.com> >> >> - Drop "examples" since we actually use the patches. >> - Drop sentences that could be headings instead >> >> Signed-off-by: Glen Choo <chooglen@google.com> >> --- >> contrib/coccinelle/README | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > > Makes sense. Will queue. Thanks. I believe this was directed at just the cleanups in this patch and not the recommendations in the later patch? I was confused for a moment when I first saw this, and someone else mentioned off-list that they also thought you meant you'd queue both patches. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] cocci: add headings to and reword README 2023-04-13 18:37 ` Glen Choo @ 2023-04-13 18:51 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-04-13 18:51 UTC (permalink / raw) To: Glen Choo Cc: Glen Choo via GitGitGadget, git, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren Glen Choo <chooglen@google.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >>> From: Glen Choo <chooglen@google.com> >>> >>> - Drop "examples" since we actually use the patches. >>> - Drop sentences that could be headings instead >>> >>> Signed-off-by: Glen Choo <chooglen@google.com> >>> --- >>> contrib/coccinelle/README | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> Makes sense. Will queue. Thanks. > > I believe this was directed at just the cleanups in this patch and not > the recommendations in the later patch? > > I was confused for a moment when I first saw this, and someone else > mentioned off-list that they also thought you meant you'd queue both > patches. Yes, I did mean that this step made sense (not implying anything good or bad about the other step). I ended up saving both on 'seen' so that we can keep track. I do not think it is a problem---people can comment more on the patch and I expect we would update it further. THanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/2] cocci: codify authoring and reviewing practices 2023-04-12 20:05 [PATCH 0/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 2023-04-12 20:05 ` [PATCH 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget @ 2023-04-12 20:05 ` Glen Choo via GitGitGadget 2023-04-16 7:42 ` SZEDER Gábor 2023-04-16 13:37 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Ævar Arnfjörð Bjarmason 2023-04-15 1:27 ` [PATCH 0/2] " Elijah Newren 2023-04-27 22:22 ` [PATCH v2 " Glen Choo via GitGitGadget 3 siblings, 2 replies; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-12 20:05 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, Glen Choo, Glen Choo From: Glen Choo <chooglen@google.com> This isn't set in stone; we expect this to be updated as the project evolves. Signed-off-by: Glen Choo <chooglen@google.com> --- contrib/coccinelle/README | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index 9b28ba1c57a..055e3622e5c 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -92,3 +92,26 @@ that might be useful to developers. The absolute times will differ for you, but the relative speedup from caching should be on that order. + +== Authoring and reviewing coccinelle changes + +* When introducing and applying a new .cocci file, both the Git changes and + .cocci file should be reviewed. + +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is + enough for the reviewer to get a rough understanding of the proposed rules by + comparing the .cocci and Git changes, then checking that understanding + with the author. + +* Conversely, authors should consider that reviewers may not be coccinelle + experts. The primary aim should be to make .cocci files easy to understand, + e.g. by adding comments or by using rules that are easier to understand even + if they are less elegant. + +* .cocci rules should target only the problem it is trying to solve; "collateral + damage" is not allowed. + +* .cocci files used for refactoring should be temporarily kept in-tree to aid + the refactoring of out-of-tree code (e.g. in-flight topics). They should be + removed when enough time has been given for others to refactor their code, + i.e. ~1 release cycle. -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] cocci: codify authoring and reviewing practices 2023-04-12 20:05 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget @ 2023-04-16 7:42 ` SZEDER Gábor 2023-04-19 19:29 ` Glen Choo 2023-04-16 13:37 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 26+ messages in thread From: SZEDER Gábor @ 2023-04-16 7:42 UTC (permalink / raw) To: Glen Choo via GitGitGadget Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, Glen Choo On Wed, Apr 12, 2023 at 08:05:55PM +0000, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> > > This isn't set in stone; we expect this to be updated as the project > evolves. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > contrib/coccinelle/README | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README > index 9b28ba1c57a..055e3622e5c 100644 > --- a/contrib/coccinelle/README > +++ b/contrib/coccinelle/README > @@ -92,3 +92,26 @@ that might be useful to developers. > > The absolute times will differ for you, but the relative speedup > from caching should be on that order. > + > +== Authoring and reviewing coccinelle changes > + > +* When introducing and applying a new .cocci file, both the Git changes and > + .cocci file should be reviewed. > + > +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is > + enough for the reviewer to get a rough understanding of the proposed rules by > + comparing the .cocci and Git changes, then checking that understanding > + with the author. > + > +* Conversely, authors should consider that reviewers may not be coccinelle > + experts. The primary aim should be to make .cocci files easy to understand, > + e.g. by adding comments or by using rules that are easier to understand even > + if they are less elegant. > + > +* .cocci rules should target only the problem it is trying to solve; "collateral > + damage" is not allowed. > + > +* .cocci files used for refactoring should be temporarily kept in-tree to aid How should such semantic patches be kept in-tree? As .pending.cocci? Then I think it would be better to point this out here. Or as a "regular" semantic patch? Then I'm not sure I agree with this recommendation, but perhaps a commit message explaining the reasoning behind this would help me make up my mind :) It might also be worth mentioning that before submitting a new semantic patch developers should consider its cost-benefit ratio, in particular its effect on the runtime of 'make coccicheck', in the hope that we can avoid another 'unused.cocci' fiasco. > + the refactoring of out-of-tree code (e.g. in-flight topics). They should be > + removed when enough time has been given for others to refactor their code, > + i.e. ~1 release cycle. > -- > gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] cocci: codify authoring and reviewing practices 2023-04-16 7:42 ` SZEDER Gábor @ 2023-04-19 19:29 ` Glen Choo 2023-04-20 20:53 ` [PATCH] cocci: remove 'unused.cocci' SZEDER Gábor 0 siblings, 1 reply; 26+ messages in thread From: Glen Choo @ 2023-04-19 19:29 UTC (permalink / raw) To: SZEDER Gábor, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren SZEDER Gábor <szeder.dev@gmail.com> writes: >> +* .cocci rules should target only the problem it is trying to solve; "collateral >> + damage" is not allowed. >> + >> +* .cocci files used for refactoring should be temporarily kept in-tree to aid > > How should such semantic patches be kept in-tree? > As .pending.cocci? Then I think it would be better to point this out > here. Or as a "regular" semantic patch? Then I'm not sure I agree > with this recommendation, but perhaps a commit message explaining the > reasoning behind this would help me make up my mind :) I don't feel strongly about this, but I was envisioning keeping them as a "regular" patch, e.g. what Ævar proposed in: https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/ In theory, this means that a long running fork (that didn't get updated during the initial refactor) can run coccicheck, notice the failure, and then automatically fix themselves with the included semantic patch. In practice, I don't know how many forks run coccicheck, or whether these refactors are just easy enough to do by hand. For refactors, I suspect that the impact on the 'make coccicheck' runtime will be low, since we're typically targeting just a few tokens and cocci can skip whatever files don't have those tokens, so keeping it as a "regular" patch might be okay. > It might also be worth mentioning that before submitting a new > semantic patch developers should consider its cost-benefit ratio, in > particular its effect on the runtime of 'make coccicheck', Makes sense, though I'm not sure what practical advice to give in order to evaluate the impact on runtime (besides just running it themselves). > in the hope > that we can avoid another 'unused.cocci' fiasco. Maybe this is a good starting point for discussing cost-benefit analysis. I'm not familiar with this fiasco, though. Was an early version of 'unused.cocci' too broad, resulting in a massive hit to runtime? ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] cocci: remove 'unused.cocci' 2023-04-19 19:29 ` Glen Choo @ 2023-04-20 20:53 ` SZEDER Gábor 2023-04-21 2:43 ` Junio C Hamano 2023-05-01 13:27 ` Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 26+ messages in thread From: SZEDER Gábor @ 2023-04-20 20:53 UTC (permalink / raw) To: git Cc: Glen Choo, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, SZEDER Gábor When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a rule to find "unused" strbufs, 2022-07-05) it found three unused strbufs, and when it was generalized in the next commit it managed to find an unused string_list as well. That's four unused variables in over 17 years, so apparently we rarely make this mistake. Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it increases the from-scratch runtime of 'make coccicheck' by over 5:30 minutes or over 160%: $ make -s cocciclean $ time make -s coccicheck * new spatch flags real 8m56.201s user 0m0.420s sys 0m0.406s $ rm contrib/coccinelle/unused.cocci contrib/coccinelle/tests/unused.* $ make -s cocciclean $ time make -s coccicheck * new spatch flags real 3m23.893s user 0m0.228s sys 0m0.247s That's a lot of runtime spent for not much in return, and arguably an unused struct instance sneaking in is not that big of a deal to justify the significantly increased runtime. Remove 'unused.cocci', because we are not getting our CPU cycles' worth. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- contrib/coccinelle/tests/unused.c | 82 ----------------------------- contrib/coccinelle/tests/unused.res | 45 ---------------- contrib/coccinelle/unused.cocci | 43 --------------- 3 files changed, 170 deletions(-) delete mode 100644 contrib/coccinelle/tests/unused.c delete mode 100644 contrib/coccinelle/tests/unused.res delete mode 100644 contrib/coccinelle/unused.cocci diff --git a/contrib/coccinelle/tests/unused.c b/contrib/coccinelle/tests/unused.c deleted file mode 100644 index 8294d734ba..0000000000 --- a/contrib/coccinelle/tests/unused.c +++ /dev/null @@ -1,82 +0,0 @@ -void test_strbuf(void) -{ - struct strbuf sb1 = STRBUF_INIT; - struct strbuf sb2 = STRBUF_INIT; - struct strbuf sb3 = STRBUF_INIT; - struct strbuf sb4 = STRBUF_INIT; - struct strbuf sb5; - struct strbuf sb6 = { 0 }; - struct strbuf sb7 = STRBUF_INIT; - struct strbuf sb8 = STRBUF_INIT; - struct strbuf *sp1; - struct strbuf *sp2; - struct strbuf *sp3; - struct strbuf *sp4 = xmalloc(sizeof(struct strbuf)); - struct strbuf *sp5 = xmalloc(sizeof(struct strbuf)); - struct strbuf *sp6 = xmalloc(sizeof(struct strbuf)); - struct strbuf *sp7; - - strbuf_init(&sb5, 0); - strbuf_init(sp1, 0); - strbuf_init(sp2, 0); - strbuf_init(sp3, 0); - strbuf_init(sp4, 0); - strbuf_init(sp5, 0); - strbuf_init(sp6, 0); - strbuf_init(sp7, 0); - sp7 = xmalloc(sizeof(struct strbuf)); - - use_before(&sb3); - use_as_str("%s", sb7.buf); - use_as_str("%s", sp1->buf); - use_as_str("%s", sp6->buf); - pass_pp(&sp3); - - strbuf_release(&sb1); - strbuf_reset(&sb2); - strbuf_release(&sb3); - strbuf_release(&sb4); - strbuf_release(&sb5); - strbuf_release(&sb6); - strbuf_release(&sb7); - strbuf_release(sp1); - strbuf_release(sp2); - strbuf_release(sp3); - strbuf_release(sp4); - strbuf_release(sp5); - strbuf_release(sp6); - strbuf_release(sp7); - - use_after(&sb4); - - if (when_strict()) - return; - strbuf_release(&sb8); -} - -void test_other(void) -{ - struct string_list l = STRING_LIST_INIT_DUP; - struct strbuf sb = STRBUF_INIT; - - string_list_clear(&l, 0); - string_list_clear(&sb, 0); -} - -void test_worktrees(void) -{ - struct worktree **w1 = get_worktrees(); - struct worktree **w2 = get_worktrees(); - struct worktree **w3; - struct worktree **w4; - - w3 = get_worktrees(); - w4 = get_worktrees(); - - use_it(w4); - - free_worktrees(w1); - free_worktrees(w2); - free_worktrees(w3); - free_worktrees(w4); -} diff --git a/contrib/coccinelle/tests/unused.res b/contrib/coccinelle/tests/unused.res deleted file mode 100644 index 6d3e745683..0000000000 --- a/contrib/coccinelle/tests/unused.res +++ /dev/null @@ -1,45 +0,0 @@ -void test_strbuf(void) -{ - struct strbuf sb3 = STRBUF_INIT; - struct strbuf sb4 = STRBUF_INIT; - struct strbuf sb7 = STRBUF_INIT; - struct strbuf *sp1; - struct strbuf *sp3; - struct strbuf *sp6 = xmalloc(sizeof(struct strbuf)); - strbuf_init(sp1, 0); - strbuf_init(sp3, 0); - strbuf_init(sp6, 0); - - use_before(&sb3); - use_as_str("%s", sb7.buf); - use_as_str("%s", sp1->buf); - use_as_str("%s", sp6->buf); - pass_pp(&sp3); - - strbuf_release(&sb3); - strbuf_release(&sb4); - strbuf_release(&sb7); - strbuf_release(sp1); - strbuf_release(sp3); - strbuf_release(sp6); - - use_after(&sb4); - - if (when_strict()) - return; -} - -void test_other(void) -{ -} - -void test_worktrees(void) -{ - struct worktree **w4; - - w4 = get_worktrees(); - - use_it(w4); - - free_worktrees(w4); -} diff --git a/contrib/coccinelle/unused.cocci b/contrib/coccinelle/unused.cocci deleted file mode 100644 index d84046f82e..0000000000 --- a/contrib/coccinelle/unused.cocci +++ /dev/null @@ -1,43 +0,0 @@ -// This rule finds sequences of "unused" declerations and uses of a -// variable, where "unused" is defined to include only calling the -// equivalent of alloc, init & free functions on the variable. -@@ -type T; -identifier I; -// STRBUF_INIT, but also e.g. STRING_LIST_INIT_DUP (so no anchoring) -constant INIT_MACRO =~ "_INIT"; -identifier MALLOC1 =~ "^x?[mc]alloc$"; -identifier INIT_ASSIGN1 =~ "^get_worktrees$"; -identifier INIT_CALL1 =~ "^[a-z_]*_init$"; -identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$"; -identifier REL2 =~ "^(release|clear|free)_[a-z_]*$"; -@@ - -( -- T I; -| -- T I = { 0 }; -| -- T I = INIT_MACRO; -| -- T I = MALLOC1(...); -| -- T I = INIT_ASSIGN1(...); -) - -<... when != \( I \| &I \) -( -- \( INIT_CALL1 \)( \( I \| &I \), ...); -| -- I = \( INIT_ASSIGN1 \)(...); -| -- I = MALLOC1(...); -) -...> - -( -- \( REL1 \| REL2 \)( \( I \| &I \), ...); -| -- \( REL1 \| REL2 \)( \( &I \| I \) ); -) - ... when != \( I \| &I \) -- 2.40.0.573.g2c27013916 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] cocci: remove 'unused.cocci' 2023-04-20 20:53 ` [PATCH] cocci: remove 'unused.cocci' SZEDER Gábor @ 2023-04-21 2:43 ` Junio C Hamano 2023-05-01 13:27 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-04-21 2:43 UTC (permalink / raw) To: SZEDER Gábor Cc: git, Glen Choo, Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren SZEDER Gábor <szeder.dev@gmail.com> writes: > When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a > rule to find "unused" strbufs, 2022-07-05) it found three unused > strbufs, and when it was generalized in the next commit it managed to > find an unused string_list as well. That's four unused variables in > over 17 years, so apparently we rarely make this mistake. > > Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it > increases the from-scratch runtime of 'make coccicheck' by over 5:30 > minutes or over 160%: > ... > That's a lot of runtime spent for not much in return, and arguably an > unused struct instance sneaking in is not that big of a deal to > justify the significantly increased runtime. > > Remove 'unused.cocci', because we are not getting our CPU cycles' > worth. Will queue. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] cocci: remove 'unused.cocci' 2023-04-20 20:53 ` [PATCH] cocci: remove 'unused.cocci' SZEDER Gábor 2023-04-21 2:43 ` Junio C Hamano @ 2023-05-01 13:27 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:55 ` Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-05-01 13:27 UTC (permalink / raw) To: SZEDER Gábor Cc: git, Glen Choo, Taylor Blau, Elijah Newren, Junio C Hamano On Thu, Apr 20 2023, SZEDER Gábor wrote: > When 'unused.cocci' was added in 4f40f6cb73 (cocci: add and apply a > rule to find "unused" strbufs, 2022-07-05) it found three unused > strbufs, and when it was generalized in the next commit it managed to > find an unused string_list as well. That's four unused variables in > over 17 years, so apparently we rarely make this mistake. > > Unfortunately, applying 'unused.cocci' is quite expensive, e.g. it > increases the from-scratch runtime of 'make coccicheck' by over 5:30 > minutes or over 160%: > > $ make -s cocciclean > $ time make -s coccicheck > * new spatch flags > > real 8m56.201s > user 0m0.420s > sys 0m0.406s > $ rm contrib/coccinelle/unused.cocci contrib/coccinelle/tests/unused.* > $ make -s cocciclean > $ time make -s coccicheck > * new spatch flags > > real 3m23.893s > user 0m0.228s > sys 0m0.247s > > That's a lot of runtime spent for not much in return, and arguably an > unused struct instance sneaking in is not that big of a deal to > justify the significantly increased runtime. > > Remove 'unused.cocci', because we are not getting our CPU cycles' > worth. It wasn't something I intended at the time, but arguably the main use of this rule since it was added was that it served as a canary for the tree becoming completely broken with coccinelle, due to adding C syntax it didn't understand: https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ So, whatever you think of of how worthwhile it is to spot unused variables, I think that weighs heavily in its favor. There *are* other ways to detect those sorts of issues, but as it's currently our only canary for that issue I don't thin we should remove it. If we hadn't had unused.cocci we wouldn't be able to apply rules the functions that use "UNUSED", which have increased a lot in number since then, and we wouldn't have any way of spotting similar parsing issues. But it's unfortunate that it's this slow in the from-scratch case. When we last discussed this I pointed out to you that the main contribution to the runtime of unused.cocci is parsing builtin/{log,rebase}.c is pathalogical, but in your reply to that you seem to not have spotted that (or glossed over it): https://lore.kernel.org/git/20220831180526.GA1802@szeder.dev/ When I test this locally, doing: time make contrib/coccinelle/unused.cocci.patch SPATCH=spatch SPATCH_USE_O_DEPENDENCIES= Takes ~2m, but if I first do: >builtin/log.c; >builtin/rebase.c It takes ~1m. So, even without digging into those issues, if we just skipped those two files we'd speed this part up by 100%. I think such an approach would be much better than just removing this outright, which feels rather heavy-handed. We could formalize that by creating a "coccicheck-full" category or whatever, just as we now have "coccicheck-pending". Then I and the CI could run "full", and you could run "coccicheck" (or maybe we'd call that "coccicheck-cheap" or something). I also submitted patches to both make "coccicheck" incremental, and added an "spatchcache", both of which have since been merged (that tool is in contrib/). I understand from previous discussion that you wanted to use "make -s" all the time, but does your use-case also preclude using spatchcache? I run "coccicheck" a lot, and haven't personally be bothered by this particular slowdown since that got merged, since it'll only affect me in the cases where builtin/{log,rebase}.c and a small list of other files are changed, and it's otherwise unnoticeable. It would also be rather trivial to just add some way to specify patterns on the "make" command-line that we'd "$(filter-out)", would that also address your particular use-case? I.e.: make coccicheck COCCI_RULES_EXCLUDE=*unused* Or whatever. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] cocci: remove 'unused.cocci' 2023-05-01 13:27 ` Ævar Arnfjörð Bjarmason @ 2023-05-01 15:55 ` Junio C Hamano 2023-05-01 17:28 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2023-05-01 15:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: SZEDER Gábor, git, Glen Choo, Taylor Blau, Elijah Newren, Junio C Hamano Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > It wasn't something I intended at the time, but arguably the main use of > this rule since it was added was that it served as a canary for the tree > becoming completely broken with coccinelle, due to adding C syntax it > didn't understand: > https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ If it weren't Coccinelle, we could have used the much nicer looking UNUSED(var) notation, and the compilers were all fine. Only because Coccinelle did not understand the "cute" syntax trick, we couldn't. Yes, it caught us when we used a syntax it couldn't understand, but is that a good thing in the first place? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] cocci: remove 'unused.cocci' 2023-05-01 15:55 ` Junio C Hamano @ 2023-05-01 17:28 ` Ævar Arnfjörð Bjarmason 2023-05-10 22:45 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-05-01 17:28 UTC (permalink / raw) To: Junio C Hamano Cc: SZEDER Gábor, git, Glen Choo, Taylor Blau, Elijah Newren, Junio C Hamano On Mon, May 01 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> It wasn't something I intended at the time, but arguably the main use of >> this rule since it was added was that it served as a canary for the tree >> becoming completely broken with coccinelle, due to adding C syntax it >> didn't understand: >> https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/ > > If it weren't Coccinelle, we could have used the much nicer looking > UNUSED(var) notation, and the compilers were all fine. > > Only because Coccinelle did not understand the "cute" syntax trick, > we couldn't. Yes, it caught us when we used a syntax it couldn't > understand, but is that a good thing in the first place? I think it's unambiguously a good thing that we spotted an otherwise unknown side-effect of the proposed UNUSED(var) syntax on coccinelle. We might also say that some bit of syntax that coccinelle doesn't understand is so valuable that we'd like to make coccinelle itself significantly less useful (as it wouldn't reach into those functions), or stop using it altogether. But that's a seperate question. I'm just pointing out that we'd be losing a very valuable check on future syntax incompatibilities, particularly when it comes to clever use of macros. A better way to spot that would be to start parsing the coccinelle logs, and detect when we have unknown parsing issues, and error on those. But until then... ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] cocci: remove 'unused.cocci' 2023-05-01 17:28 ` Ævar Arnfjörð Bjarmason @ 2023-05-10 22:45 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-05-10 22:45 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: SZEDER Gábor, git, Glen Choo, Taylor Blau, Elijah Newren, Junio C Hamano Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > A better way to spot that would be to start parsing the coccinelle logs, > and detect when we have unknown parsing issues, and error on those. But > until then... Until then, I do not think a rather costly test that has found only 4 instances of the mistakes the test was designed to find is a good way to stand in as a replacement. Let's drop it, as it is easy to resurrect if somebody wants to run it from time to time from an old version of Git. Or is it a valid alternative to move it to "pending"? Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] cocci: codify authoring and reviewing practices 2023-04-12 20:05 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 2023-04-16 7:42 ` SZEDER Gábor @ 2023-04-16 13:37 ` Ævar Arnfjörð Bjarmason 2023-04-19 22:30 ` Glen Choo 1 sibling, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-04-16 13:37 UTC (permalink / raw) To: Glen Choo via GitGitGadget; +Cc: git, Taylor Blau, Elijah Newren, Glen Choo On Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> > > This isn't set in stone; we expect this to be updated as the project > evolves. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > contrib/coccinelle/README | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README > index 9b28ba1c57a..055e3622e5c 100644 > --- a/contrib/coccinelle/README > +++ b/contrib/coccinelle/README > @@ -92,3 +92,26 @@ that might be useful to developers. > > The absolute times will differ for you, but the relative speedup > from caching should be on that order. > + > +== Authoring and reviewing coccinelle changes > + > +* When introducing and applying a new .cocci file, both the Git changes and > + .cocci file should be reviewed. > + > +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is > + enough for the reviewer to get a rough understanding of the proposed rules by > + comparing the .cocci and Git changes, then checking that understanding > + with the author. Maybe it would be useful here to add something about how you can reproduce the application of the coccinelle rule(s). I sometimes do this on an ad-hoc basis, something like (untested): git checkout HEAD^ -- ':!contrib/coccinelle' '*.[ch]' make coccicheck <apply any suggested patches> git add -A Then see if I ended up with a no-op, or if there's suggested changes. With changes that modify both the header & source files this can be tricky with the default of SPATCH_USE_O_DEPENDENCIES=Y, but disabling it will take care of any potential circular dependency issues. I.e. when the header doesn't contain a required construct that we're replacing. > +* Conversely, authors should consider that reviewers may not be coccinelle > + experts. The primary aim should be to make .cocci files easy to understand, > + e.g. by adding comments or by using rules that are easier to understand even > + if they are less elegant. I agree that simple things should be kept simple, but this seems to come quite close (or perhaps past the line of) suggesting that we use only the simpler features of the language when a more elegant solution would be available with something less well-known. I think we should clarify that that's not the intent. Just as with C, shellscript, Perl etc. we should aim for simplicity, but ultimately we should expect that we can target the full available language available to us. > +* .cocci rules should target only the problem it is trying to solve; "collateral > + damage" is not allowed. I think what you mean here is that you should be able to apply the rule and still build the project. I think that's correct, but I also think that rather than define this in prose, how about we just modify the current CI job to apply the result of non-pending rules, and do a build at the end? Wouldn't that assert this going forward. > +* .cocci files used for refactoring should be temporarily kept in-tree to aid > + the refactoring of out-of-tree code (e.g. in-flight topics). They should be > + removed when enough time has been given for others to refactor their code, > + i.e. ~1 release cycle. Maybe s/should/can/? E.g. for my recent "index" and "the_repository" patches I think they can, but we often keep unused code in-repo for longer than that. If e.g. that code stayed in for more than one release until someone cared to remove it we'd also be fine. I also don't know if some long-running forks (e.g. GfW?) would benefit from the rules for longer than that... ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] cocci: codify authoring and reviewing practices 2023-04-16 13:37 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Ævar Arnfjörð Bjarmason @ 2023-04-19 22:30 ` Glen Choo 0 siblings, 0 replies; 26+ messages in thread From: Glen Choo @ 2023-04-19 22:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Elijah Newren Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote: > >> +== Authoring and reviewing coccinelle changes >> + >> +* When introducing and applying a new .cocci file, both the Git changes and >> + .cocci file should be reviewed. >> + >> +* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is >> + enough for the reviewer to get a rough understanding of the proposed rules by >> + comparing the .cocci and Git changes, then checking that understanding >> + with the author. > > Maybe it would be useful here to add something about how you can > reproduce the application of the coccinelle rule(s). > > I sometimes do this on an ad-hoc basis, something like (untested): > > git checkout HEAD^ -- ':!contrib/coccinelle' '*.[ch]' > make coccicheck > <apply any suggested patches> > git add -A > > Then see if I ended up with a no-op, or if there's suggested changes. > > With changes that modify both the header & source files this can be > tricky with the default of SPATCH_USE_O_DEPENDENCIES=Y, but disabling it > will take care of any potential circular dependency issues. I.e. when > the header doesn't contain a required construct that we're replacing. Makes sense. I've been thinking about sending a "MyFirstCocci" guide as a follow up, and this sounds like the kind of "Tips & Tricks" content that would belong there. >> +* Conversely, authors should consider that reviewers may not be coccinelle >> + experts. The primary aim should be to make .cocci files easy to understand, >> + e.g. by adding comments or by using rules that are easier to understand even >> + if they are less elegant. > > I agree that simple things should be kept simple, but this seems to come > quite close (or perhaps past the line of) suggesting that we use only > the simpler features of the language when a more elegant solution would > be available with something less well-known. > > I think we should clarify that that's not the intent. Just as with C, > shellscript, Perl etc. we should aim for simplicity, but ultimately we > should expect that we can target the full available language available > to us. Makes sense too. I think I'll adjust this to something to the effect of "When using more esoteric parts of the language, be prepared to explain what the .cocci is doing.". >> +* .cocci rules should target only the problem it is trying to solve; "collateral >> + damage" is not allowed. > > I think what you mean here is that you should be able to apply the rule > and still build the project. Yes and no. Yes in that if the project doesn't build, the rule is obviously overly broad, but no in that I think it's possible to write a rule that affects something you didn't mean to, but still builds. I can't think of a way to automatedly check for the latter case, so I categorized it as something to catch at review time. >> +* .cocci files used for refactoring should be temporarily kept in-tree to aid >> + the refactoring of out-of-tree code (e.g. in-flight topics). They should be >> + removed when enough time has been given for others to refactor their code, >> + i.e. ~1 release cycle. > > Maybe s/should/can/? E.g. for my recent "index" and "the_repository" > patches I think they can, but we often keep unused code in-repo for > longer than that. If e.g. that code stayed in for more than one release > until someone cared to remove it we'd also be fine. > > I also don't know if some long-running forks (e.g. GfW?) would benefit > from the rules for longer than that... Yeah, this is the most iffy to me too, which means it would be extra helpful to decide on as many details as we can now instead of deciding ad-hoc. Post-refactor, the .cocci file is always obsolete in-tree, so I think we can either say "always keep the patch" or "never keep the patch". If I understand you correctly, _how long_ to keep the patch is probably a case-by-case matter, though (makes sense to me). I think this comes down to the cost-benefit tradeoff mentioned by others elsewhere in the thread. Maybe I'll just mention that it depends on the cost-benefit analysis and drop the "~1 release cycle" recommendation. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] cocci: codify authoring and reviewing practices 2023-04-12 20:05 [PATCH 0/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 2023-04-12 20:05 ` [PATCH 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget 2023-04-12 20:05 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget @ 2023-04-15 1:27 ` Elijah Newren 2023-04-17 16:21 ` Junio C Hamano 2023-04-27 22:22 ` [PATCH v2 " Glen Choo via GitGitGadget 3 siblings, 1 reply; 26+ messages in thread From: Elijah Newren @ 2023-04-15 1:27 UTC (permalink / raw) To: Glen Choo via GitGitGadget Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason, Glen Choo On Wed, Apr 12, 2023 at 1:05 PM Glen Choo via GitGitGadget <gitgitgadget@gmail.com> wrote: > > Here's the followup to the discussion in [1]. Sorry for the delay. > > I've tried to incorporate most of the responses from that thread as well as > suggest some guidelines that I think would make the authoring + reviewing > process smoother. I've opted for stronger wording to make the guidelines > easier to follow, but I don't feel strongly about the specifics. > > [1] > https://lore.kernel.org/git/kl6l7cuycd3n.fsf@chooglen-macbookpro.roam.corp.google.com > > Glen Choo (2): > cocci: add headings to and reword README > cocci: codify authoring and reviewing practices > > contrib/coccinelle/README | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > > base-commit: f285f68a132109c234d93490671c00218066ace9 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v1 > Pull-Request: https://github.com/git/git/pull/1495 > -- > gitgitgadget I read through both patches, and I generally like them. I'm a little unsure on the "To give a Reviewed-by" bit of patch 2. For example, it could be possible that .cocci changes might apply a few different kinds of changes, and say only 2 of the 3 are reflected in the current tree, and those 2 types are handled correctly but the third type of change is buggy. The .cocci files would then be a bug waiting to happen. Maybe that's just a risk we take and it's okay for folks to give a Reviewed-by even being unfamiliar with cocci. Maybe the wording should instead be "It's okay to give a Reviewed-by: on a series that also contains cocci changes when you are unfamiliar with coccinelle; just state that your Reviewed-by is limited to the other bits". Or maybe the instructions should just be to give an Acked-by. You should probably have someone familiar enough with coccinelle that they know what is worth worrying about weigh in on that aspect. But you can have my Acked-by on the other bits. :-) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] cocci: codify authoring and reviewing practices 2023-04-15 1:27 ` [PATCH 0/2] " Elijah Newren @ 2023-04-17 16:21 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-04-17 16:21 UTC (permalink / raw) To: Elijah Newren Cc: Glen Choo via GitGitGadget, git, Taylor Blau, Ævar Arnfjörð Bjarmason, Glen Choo Elijah Newren <newren@gmail.com> writes: > .... Maybe > the wording should instead be "It's okay to give a Reviewed-by: on a > series that also contains cocci changes when you are unfamiliar with > coccinelle; just state that your Reviewed-by is limited to the other > bits". Or maybe the instructions should just be to give an Acked-by. > You should probably have someone familiar enough with coccinelle that > they know what is worth worrying about weigh in on that aspect. > > But you can have my Acked-by on the other bits. :-) The value of Reviewed-by takes two sides to determine. Even if we reserve a Reviewed-by to "I have reviewed the entirety of this patch, and the patch is something I can stand behind" (as opposed to "my understanding of this patch is iffy in this and that area, but all the other parts of the patch is something I can stand behind"), the value of such a Reviewed-by is conditional to "how well does the reviewer actually know the area?" A drive-by "Reviewed-by:" thrown into a review discussion thread by a total stranger would not carry much weight, until we know how much they are familiar with and how good a taste they have. And honest qualifying comments like "my understanding of this and that area is iffy so I cannot endorse these parts" helps build trust by others in the reviewer who gives such a partial review and we should encourage such behaviour. I agree "Acked-by:" with comments is a good idea. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/2] cocci: codify authoring and reviewing practices 2023-04-12 20:05 [PATCH 0/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget ` (2 preceding siblings ...) 2023-04-15 1:27 ` [PATCH 0/2] " Elijah Newren @ 2023-04-27 22:22 ` Glen Choo via GitGitGadget 2023-04-27 22:22 ` [PATCH v2 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget 2023-04-27 22:22 ` [PATCH v2 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 3 siblings, 2 replies; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-27 22:22 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, SZEDER Gábor, Glen Choo Thanks for the input on v1, all :) I've tried to capture all of the discussion in some form. AFAICT, the result is quite similar to what we are already doing, so it might not be very helpful to folks who have already worked with Coccinelle, but it should hopefully be useful to newcomers. I suspect that we won't converge on any new practices during this discussion, but as we develop practices in the future, we can just update this doc. Glen Choo (2): cocci: add headings to and reword README cocci: codify authoring and reviewing practices contrib/coccinelle/README | 40 +++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) base-commit: f285f68a132109c234d93490671c00218066ace9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1495%2Fchooglen%2Fpush-lsxuouxyokwo-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1495/chooglen/push-lsxuouxyokwo-v2 Pull-Request: https://github.com/git/git/pull/1495 Range-diff vs v1: 1: 4a8b8a2a674 = 1: 4a8b8a2a674 cocci: add headings to and reword README 2: 75feb18dfd8 ! 2: acee642531a cocci: codify authoring and reviewing practices @@ Metadata ## Commit message ## cocci: codify authoring and reviewing practices - This isn't set in stone; we expect this to be updated as the project - evolves. + These practices largely reflect what we are already doing on the mailing + list, which should help new Coccinelle authors and reviewers get up to + speed. Signed-off-by: Glen Choo <chooglen@google.com> @@ contrib/coccinelle/README: that might be useful to developers. + +== Authoring and reviewing coccinelle changes + -+* When introducing and applying a new .cocci file, both the Git changes and -+ .cocci file should be reviewed. ++* When a .cocci is made, both the Git changes and .cocci file should be ++ reviewed. When reviewing such a change, do your best to understand the .cocci ++ changes (e.g. by asking the author to explain the change) and be explicit ++ about your understanding of the changes. This helps us decide whether input ++ from coccinelle experts is needed or not. If you aren't sure of the cocci ++ changes, indicate what changes you actively endorse and leave an Acked-by ++ (instead of Reviewed-by). + -+* Reviewers do not need to be coccinelle experts. To give a Reviewed-By, it is -+ enough for the reviewer to get a rough understanding of the proposed rules by -+ comparing the .cocci and Git changes, then checking that understanding -+ with the author. -+ -+* Conversely, authors should consider that reviewers may not be coccinelle -+ experts. The primary aim should be to make .cocci files easy to understand, -+ e.g. by adding comments or by using rules that are easier to understand even -+ if they are less elegant. ++* Authors should consider that reviewers may not be coccinelle experts, thus the ++ the .cocci changes may not be self-evident. A plain text description of the ++ changes is strongly encouraged, especially when using more esoteric features ++ of the language. + +* .cocci rules should target only the problem it is trying to solve; "collateral -+ damage" is not allowed. ++ damage" is not allowed. Reviewers should look out and flag overly-broad rules. ++ ++* Consider the cost-benefit ratio of .cocci changes. In particular, consider the ++ effect on the runtime of "make coccicheck", and how often your .cocci check ++ will catch something valuable. As a rule of thumb, rules that can bail early ++ if a file doesn't have a particular token will have a small impact on runtime, ++ and vice-versa. + +* .cocci files used for refactoring should be temporarily kept in-tree to aid -+ the refactoring of out-of-tree code (e.g. in-flight topics). They should be -+ removed when enough time has been given for others to refactor their code, -+ i.e. ~1 release cycle. ++ the refactoring of out-of-tree code (e.g. in-flight topics). Periodically ++ evaluate the cost-benefit ratio to determine when the file should be removed. ++ For example, consider how many out-of-tree users are left and how much this ++ slows down "make coccicheck". -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/2] cocci: add headings to and reword README 2023-04-27 22:22 ` [PATCH v2 " Glen Choo via GitGitGadget @ 2023-04-27 22:22 ` Glen Choo via GitGitGadget 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason 2023-04-27 22:22 ` [PATCH v2 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 1 sibling, 1 reply; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-27 22:22 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, SZEDER Gábor, Glen Choo, Glen Choo From: Glen Choo <chooglen@google.com> - Drop "examples" since we actually use the patches. - Drop sentences that could be headings instead Signed-off-by: Glen Choo <chooglen@google.com> --- contrib/coccinelle/README | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index d1daa1f6263..9b28ba1c57a 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -1,7 +1,9 @@ -This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) -semantic patches that might be useful to developers. += coccinelle -There are two types of semantic patches: +This directory provides Coccinelle (http://coccinelle.lip6.fr/) semantic patches +that might be useful to developers. + +== Types of semantic patches * Using the semantic transformation to check for bad patterns in the code; The target 'make coccicheck' is designed to check for these patterns and @@ -42,7 +44,7 @@ There are two types of semantic patches: This allows to expose plans of pending large scale refactorings without impacting the bad pattern checks. -Git-specific tips & things to know about how we run "spatch": +== Git-specific tips & things to know about how we run "spatch": * The "make coccicheck" will piggy-back on "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] cocci: add headings to and reword README 2023-04-27 22:22 ` [PATCH v2 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget @ 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:06 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-05-01 10:53 UTC (permalink / raw) To: Glen Choo via GitGitGadget Cc: git, Taylor Blau, Elijah Newren, SZEDER Gábor, Glen Choo On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote: Re subject: I don't per-se mind the "add headings" formatting change, but doesn't it have headings already? I.e.: > -Git-specific tips & things to know about how we run "spatch": > +== Git-specific tips & things to know about how we run "spatch": > > * The "make coccicheck" will piggy-back on > "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file I think it was clear before that that was a "heading", at least in the sense that it summarized what the indented part that followed was discussing. I think what this is really doing is converting this part of the doc to asciidoc, but is anything actually rendering it as asciidoc? If we are converting it to asciidoc shouldn't the bullet-points be un-indented too? (I'm not sure, but couldn't find a part of our build that actually feeds this through asciidoc, so spot-checking that wasn't trivial...) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] cocci: add headings to and reword README 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason @ 2023-05-01 15:06 ` Junio C Hamano 2023-05-02 19:29 ` Felipe Contreras ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2023-05-01 15:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Glen Choo via GitGitGadget, git, Taylor Blau, Elijah Newren, SZEDER Gábor, Glen Choo Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote: > > Re subject: I don't per-se mind the "add headings" formatting change, > but doesn't it have headings already? I.e.: > >> -Git-specific tips & things to know about how we run "spatch": >> +== Git-specific tips & things to know about how we run "spatch": >> >> * The "make coccicheck" will piggy-back on >> "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file I think "add headings" mostly refers to what the first hunk, that is, the hunk before that one, did. Giving the entire document the title (while removing references to "examples"). As a side effect, the existing two sections ("-Git-specific tips..." we see above is the second one among them) are moved down in the section hierarchy; in other words, I do not think the highlighted part of the patch in your message is the primary change intended. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] cocci: add headings to and reword README 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:06 ` Junio C Hamano @ 2023-05-02 19:29 ` Felipe Contreras 2023-05-02 19:30 ` Felipe Contreras 2023-05-09 17:54 ` Glen Choo 3 siblings, 0 replies; 26+ messages in thread From: Felipe Contreras @ 2023-05-02 19:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Elijah Newren, SZEDER Gábor, Glen Choo Ævar Arnfjörð Bjarmason wrote: > On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote: > > Re subject: I don't per-se mind the "add headings" formatting change, > but doesn't it have headings already? I.e.: > > > -Git-specific tips & things to know about how we run "spatch": > > +== Git-specific tips & things to know about how we run "spatch": > > > > * The "make coccicheck" will piggy-back on > > "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file > > I think it was clear before that that was a "heading", at least in the > sense that it summarized what the indented part that followed was > discussing. > > I think what this is really doing is converting this part of the doc to > asciidoc, but is anything actually rendering it as asciidoc? Personally I write many documents in AsciiDoc format even if I'm not using asciidoc, as I find them easier to read. Moreover, one can always do `:set ft=asciidoc` in vim to see some syntax colors for an easier read. > If we are converting it to asciidoc shouldn't the bullet-points be > un-indented too? (I'm not sure, but couldn't find a part of our build > that actually feeds this through asciidoc, so spot-checking that wasn't > trivial...) You can just do `asciidoctor doc.txt` with any document and it will generate an HTML page. -- Felipe Contreras ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] cocci: add headings to and reword README 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:06 ` Junio C Hamano 2023-05-02 19:29 ` Felipe Contreras @ 2023-05-02 19:30 ` Felipe Contreras 2023-05-09 17:54 ` Glen Choo 3 siblings, 0 replies; 26+ messages in thread From: Felipe Contreras @ 2023-05-02 19:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Elijah Newren, SZEDER Gábor, Glen Choo I fogot to mention: Ævar Arnfjörð Bjarmason wrote: > If we are converting it to asciidoc shouldn't the bullet-points be > un-indented too? I don't think that's necessary. -- Felipe Contreras ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] cocci: add headings to and reword README 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2023-05-02 19:30 ` Felipe Contreras @ 2023-05-09 17:54 ` Glen Choo 3 siblings, 0 replies; 26+ messages in thread From: Glen Choo @ 2023-05-09 17:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Glen Choo via GitGitGadget Cc: git, Taylor Blau, Elijah Newren, SZEDER Gábor, Felipe Contreras, Junio C Hamano Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Apr 27 2023, Glen Choo via GitGitGadget wrote: > > Re subject: I don't per-se mind the "add headings" formatting change, > but doesn't it have headings already? I.e.: > >> -Git-specific tips & things to know about how we run "spatch": >> +== Git-specific tips & things to know about how we run "spatch": >> >> * The "make coccicheck" will piggy-back on >> "COMPUTE_HEADER_DEPENDENCIES". If you've built a given object file > > I think it was clear before that that was a "heading", at least in the > sense that it summarized what the indented part that followed was > discussing. As Junio guessed downthread, I was primarily aiming to heading-ify the other parts of the doc. > I think what this is really doing is converting this part of the doc to > asciidoc, but is anything actually rendering it as asciidoc? And as Felipe mentioned downthread, I chose to author it as asciidoc because I also find structured docs easier to read, and asciidoc seems to be the closest thing to a standardized format we have. You're right that nothing renders this as asciidoc. Thanks, all :) > If we are converting it to asciidoc shouldn't the bullet-points be > un-indented too? (I'm not sure, but couldn't find a part of our build > that actually feeds this through asciidoc, so spot-checking that wasn't > trivial...) Thanks Felipe for checking. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/2] cocci: codify authoring and reviewing practices 2023-04-27 22:22 ` [PATCH v2 " Glen Choo via GitGitGadget 2023-04-27 22:22 ` [PATCH v2 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget @ 2023-04-27 22:22 ` Glen Choo via GitGitGadget 1 sibling, 0 replies; 26+ messages in thread From: Glen Choo via GitGitGadget @ 2023-04-27 22:22 UTC (permalink / raw) To: git Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, Elijah Newren, SZEDER Gábor, Glen Choo, Glen Choo From: Glen Choo <chooglen@google.com> These practices largely reflect what we are already doing on the mailing list, which should help new Coccinelle authors and reviewers get up to speed. Signed-off-by: Glen Choo <chooglen@google.com> --- contrib/coccinelle/README | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index 9b28ba1c57a..055ad0e06a7 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -92,3 +92,33 @@ that might be useful to developers. The absolute times will differ for you, but the relative speedup from caching should be on that order. + +== Authoring and reviewing coccinelle changes + +* When a .cocci is made, both the Git changes and .cocci file should be + reviewed. When reviewing such a change, do your best to understand the .cocci + changes (e.g. by asking the author to explain the change) and be explicit + about your understanding of the changes. This helps us decide whether input + from coccinelle experts is needed or not. If you aren't sure of the cocci + changes, indicate what changes you actively endorse and leave an Acked-by + (instead of Reviewed-by). + +* Authors should consider that reviewers may not be coccinelle experts, thus the + the .cocci changes may not be self-evident. A plain text description of the + changes is strongly encouraged, especially when using more esoteric features + of the language. + +* .cocci rules should target only the problem it is trying to solve; "collateral + damage" is not allowed. Reviewers should look out and flag overly-broad rules. + +* Consider the cost-benefit ratio of .cocci changes. In particular, consider the + effect on the runtime of "make coccicheck", and how often your .cocci check + will catch something valuable. As a rule of thumb, rules that can bail early + if a file doesn't have a particular token will have a small impact on runtime, + and vice-versa. + +* .cocci files used for refactoring should be temporarily kept in-tree to aid + the refactoring of out-of-tree code (e.g. in-flight topics). Periodically + evaluate the cost-benefit ratio to determine when the file should be removed. + For example, consider how many out-of-tree users are left and how much this + slows down "make coccicheck". -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-05-10 22:45 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-12 20:05 [PATCH 0/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 2023-04-12 20:05 ` [PATCH 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget 2023-04-12 21:18 ` Junio C Hamano 2023-04-13 18:37 ` Glen Choo 2023-04-13 18:51 ` Junio C Hamano 2023-04-12 20:05 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget 2023-04-16 7:42 ` SZEDER Gábor 2023-04-19 19:29 ` Glen Choo 2023-04-20 20:53 ` [PATCH] cocci: remove 'unused.cocci' SZEDER Gábor 2023-04-21 2:43 ` Junio C Hamano 2023-05-01 13:27 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:55 ` Junio C Hamano 2023-05-01 17:28 ` Ævar Arnfjörð Bjarmason 2023-05-10 22:45 ` Junio C Hamano 2023-04-16 13:37 ` [PATCH 2/2] cocci: codify authoring and reviewing practices Ævar Arnfjörð Bjarmason 2023-04-19 22:30 ` Glen Choo 2023-04-15 1:27 ` [PATCH 0/2] " Elijah Newren 2023-04-17 16:21 ` Junio C Hamano 2023-04-27 22:22 ` [PATCH v2 " Glen Choo via GitGitGadget 2023-04-27 22:22 ` [PATCH v2 1/2] cocci: add headings to and reword README Glen Choo via GitGitGadget 2023-05-01 10:53 ` Ævar Arnfjörð Bjarmason 2023-05-01 15:06 ` Junio C Hamano 2023-05-02 19:29 ` Felipe Contreras 2023-05-02 19:30 ` Felipe Contreras 2023-05-09 17:54 ` Glen Choo 2023-04-27 22:22 ` [PATCH v2 2/2] cocci: codify authoring and reviewing practices Glen Choo via GitGitGadget
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.