patch, 2 partscocci: 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/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

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

Makes sense.  Will queue.  Thanks.

Junio C Hamano <gitster@pobox.com> writes:

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.

Glen Choo <chooglen@google.com> writes:

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 <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(+)

On Wed, Apr 12, 2023 at 08:05:55PM +0000, Glen Choo via GitGitGadget wrote:

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.

SZEDER Gábor <szeder.dev@gmail.com> writes:

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.

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.

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

SZEDER Gábor <szeder.dev@gmail.com> writes:

Will queue.  Thanks.

On Thu, Apr 20 2023, SZEDER Gábor wrote:

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.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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?

On Mon, May 01 2023, Junio C Hamano wrote:

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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.

On Wed, Apr 12 2023, Glen Choo via GitGitGadget wrote:

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.

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.

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.

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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.

On Wed, Apr 12, 2023 at 1:05 PM Glen Choo via GitGitGadget <gitgitgadget@gmail.com> wrote:

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

Elijah Newren <newren@gmail.com> writes:

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

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

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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.

Ævar Arnfjörð Bjarmason wrote:

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.

I fogot to mention:

Ævar Arnfjörð Bjarmason wrote:

I don't think that's necessary.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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 <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(+)