cocci: codify authoring and reviewing practices

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/[email protected]

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

From: Glen Choo <[email protected]>

- Drop "examples" since we actually use the patches.
- Drop sentences that could be headings instead

---
 contrib/coccinelle/README | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index d1daa1f6263..9b28ba1c57a 100644
@@ -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

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.

From: Glen Choo <[email protected]>

This isn't set in stone; we expect this to be updated as the project evolves.

---
 contrib/coccinelle/README | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

index 9b28ba1c57a..055e3622e5c 100644
@@ -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

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/[email protected]/

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.

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

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?

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.

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

deleted file mode 100644
index 8294d734ba..0000000000
@@ -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);
-}

2.40.0.573.g2c27013916

Will queue. Thanks.

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/[email protected]/

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/[email protected]/

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.

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

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.

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.

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

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.

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.

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

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.

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.

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

From: Glen Choo <[email protected]>

- Drop "examples" since we actually use the patches.
- Drop sentences that could be headings instead

---
 contrib/coccinelle/README | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index d1daa1f6263..9b28ba1c57a 100644
@@ -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

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

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

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.

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.

You can just do asciidoctor doc.txt with any document and it will generate an HTML page.

Felipe Contreras

I fogot to mention:

I don't think that's necessary.

Felipe Contreras

As Junio guessed downthread, I was primarily aiming to heading-ify the other parts of the doc.

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

Thanks Felipe for checking.

From: Glen Choo <[email protected]>

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.

---
 contrib/coccinelle/README | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

index 9b28ba1c57a..055ad0e06a7 100644
@@ -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