[PATCH 0/2] cocci: codify authoring and reviewing practices

[PATCH 0/2] cocci: codify authoring and reviewing practices

[Glen Choo via GitGitGadget]

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

[PATCH 1/2] cocci: add headings to and reword README

[Glen Choo via GitGitGadget]

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

Re: [PATCH 1/2] cocci: add headings to and reword README

[Junio C Hamano]

"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.

Re: [PATCH 1/2] cocci: add headings to and reword README

[Glen Choo]

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.

Re: [PATCH 1/2] cocci: add headings to and reword README

[Junio C Hamano]

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.

[PATCH 2/2] cocci: codify authoring and reviewing practices

[Glen Choo via GitGitGadget]

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

Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

[SZEDER Gábor]

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

Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

[Glen Choo]

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?

[PATCH] cocci: remove 'unused.cocci'

[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

Re: [PATCH] cocci: remove 'unused.cocci'

[Junio C Hamano]

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.

Re: [PATCH] cocci: remove 'unused.cocci'

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH] cocci: remove 'unused.cocci'

[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?

Re: [PATCH] cocci: remove 'unused.cocci'

[Ævar Arnfjörð Bjarmason]

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...

Re: [PATCH] cocci: remove 'unused.cocci'

[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.

Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

[Ævar Arnfjörð Bjarmason]

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...

Re: [PATCH 2/2] cocci: codify authoring and reviewing practices

[Glen Choo]

Æ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.

Re: [PATCH 0/2] cocci: codify authoring and reviewing practices

[Elijah Newren]

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.  :-)

Re: [PATCH 0/2] cocci: codify authoring and reviewing practices

[Junio C Hamano]

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.

[PATCH v2 0/2] cocci: codify authoring and reviewing practices

[Glen Choo via GitGitGadget]

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

[PATCH v2 1/2] cocci: add headings to and reword README

[Glen Choo via GitGitGadget]

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

Re: [PATCH v2 1/2] cocci: add headings to and reword README

[Ævar Arnfjörð Bjarmason]

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...)

Re: [PATCH v2 1/2] cocci: add headings to and reword README

[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 "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.

Re: [PATCH v2 1/2] cocci: add headings to and reword README

[Felipe Contreras]

Æ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

Re: [PATCH v2 1/2] cocci: add headings to and reword README

[Felipe Contreras]

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

Re: [PATCH v2 1/2] cocci: add headings to and reword README

[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 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.

[PATCH v2 2/2] cocci: codify authoring and reviewing practices

[Glen Choo via GitGitGadget]

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