threads / rfc / 63672

RFC patchcoccicheck: fail "make" when it fails

Subject: [RFC PATCH] coccicheck: fail "make" when it fails

## tl;dr

6 messages between Jun 19, 2025 and Jun 23, 2025. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Junio C Hamano· Jun 19, 2025, 15:35 UTC · lore

With "make coccicheck", we generate contrib/coccinelle/*.cocci.patch files that contain changes suggested by semantic patches, but "make" succeeds. Admittedly, not many developers may run "make coccicheck" in the first place, but it makes it harder to notice when they do run it after they introduced an iffy piece of code.

Check that the resulting cocci.patch files are all empty.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)
Show changes to diff +2 −0
diff --git c/Makefile w/Makefile
index 70d1543b6b..2c17868a24 100644
--- c/Makefile
+++ w/Makefile
@@ -3475,8 +3475,10 @@ coccicheck-test: $(COCCI_TEST_RES_GEN)
 coccicheck: coccicheck-test
 ifdef SPATCH_CONCAT_COCCI
 coccicheck: contrib/coccinelle/ALL.cocci.patch
+	! test -s contrib/coccinelle/ALL.cocci.patch
 else
 coccicheck: $(COCCICHECK_PATCHES_INTREE)
+	test $$(cat $(COCCICHECK_PATCHES_INTREE) | wc -c) = 0
 endif
 
 # See contrib/coccinelle/README
Jeff King· Jun 21, 2025, 05:16 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] coccicheck: fail "make" when it fails

On Thu, Jun 19, 2025 at 08:35:35AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> With "make coccicheck", we generate contrib/coccinelle/*.cocci.patch
> files that contain changes suggested by semantic patches, but "make"
> succeeds.  Admittedly, not many developers may run "make coccicheck"
> in the first place, but it makes it harder to notice when they do
> run it after they introduced an iffy piece of code.

I admit that I do not run coccicheck very often. But I have definitely been surprised by it not triggering a make failure when there are changes to be made. So I'd be in favor of this patch.

-Peff
Carlo Marcelo Arenas Belón· Jun 21, 2025, 06:29 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] coccicheck: fail "make" when it fails

On Thu, Jun 19, 2025 at 08:35:35AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> 
> diff --git c/Makefile w/Makefile
> index 70d1543b6b..2c17868a24 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -3475,8 +3475,10 @@ coccicheck-test: $(COCCI_TEST_RES_GEN)
>  coccicheck: coccicheck-test
>  ifdef SPATCH_CONCAT_COCCI
>  coccicheck: contrib/coccinelle/ALL.cocci.patch
> +	! test -s contrib/coccinelle/ALL.cocci.patch
>  else
>  coccicheck: $(COCCICHECK_PATCHES_INTREE)
> +	test $$(cat $(COCCICHECK_PATCHES_INTREE) | wc -c) = 0

Definitely a nitpick, but I think using `-eq` instead of `=` and might even come handy from a portability point of view.

Just like Peff, agree would be a big improvement to have this changed for usability.

Carlo
Eric Sunshine· Jun 21, 2025, 08:09 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [RFC PATCH] coccicheck: fail "make" when it fails

On Sat, Jun 21, 2025 at 2:29 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 6 quoted lines
> On Thu, Jun 19, 2025 at 08:35:35AM -0800, Junio C Hamano wrote:
> >  coccicheck: $(COCCICHECK_PATCHES_INTREE)
> > +     test $$(cat $(COCCICHECK_PATCHES_INTREE) | wc -c) = 0
>
> Definitely a nitpick, but I think using `-eq` instead of `=`
> and might even come handy from a portability point of view.

Junio has previously expressed[*] a preference for `=` over `-eq` in this sort of situation.

[*]: https://lore.kernel.org/git/7v8v3m50ds.fsf@alter.siamese.dyndns.org/
Junio C Hamano· Jun 22, 2025, 04:18 UTC · re: Eric Sunshine · lore

Re: [RFC PATCH] coccicheck: fail "make" when it fails

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 13 quoted lines
> On Sat, Jun 21, 2025 at 2:29 AM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
>> On Thu, Jun 19, 2025 at 08:35:35AM -0800, Junio C Hamano wrote:
>> >  coccicheck: $(COCCICHECK_PATCHES_INTREE)
>> > +     test $$(cat $(COCCICHECK_PATCHES_INTREE) | wc -c) = 0
>>
>> Definitely a nitpick, but I think using `-eq` instead of `=`
>> and might even come handy from a portability point of view.
>
> Junio has previously expressed[*] a preference for `=` over `-eq` in
> this sort of situation.
>
> [*]: https://lore.kernel.org/git/7v8v3m50ds.fsf@alter.siamese.dyndns.org/
Heh, you have an infinite more memory than I have ;-)

That one is 12 years ago or so, isn't it? Not that I changed my stance.

Having said that, I suspect
    ! grep -q ^ $(COCCICHECK_PATCHES_INTREE)

might have been easier to understand. We do not want any file with even a single line in it, and "grep -q" knows to short-circuit (i.e. if the first file in the COCCICHECK_PATCHES_INTREE list is not empty, it does not even bother to open the rest of the files).

Junio C Hamano· Jun 23, 2025, 16:14 UTC · re: Junio C Hamano · lore

[PATCH v2] coccicheck: fail "make" when it fails

With "make coccicheck", we generate contrib/coccinelle/*.cocci.patch files that contain changes suggested by semantic patches, but "make" succeeds. Admittedly, not many developers may run "make coccicheck" in the first place, but it makes it harder to notice when they do run it after they introduced an iffy piece of code.

Check that the resulting cocci.patch files are all empty.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to Makefile +5 −2
diff --git a/Makefile b/Makefile
index 97e8385b66..8f1e9424a7 100644
--- a/Makefile
+++ b/Makefile
@@ -3422,11 +3422,14 @@ endif
 coccicheck-test: $(COCCI_TEST_RES_GEN)
 
 coccicheck: coccicheck-test
+
 ifdef SPATCH_CONCAT_COCCI
-coccicheck: contrib/coccinelle/ALL.cocci.patch
+COCCICHECK_PATCH_MUST_BE_EMPTY_FILES = contrib/coccinelle/ALL.cocci.patch
 else
-coccicheck: $(COCCICHECK_PATCHES_INTREE)
+COCCICHECK_PATCH_MUST_BE_EMPTY_FILES = $(COCCICHECK_PATCHES_INTREE)
 endif
+coccicheck: $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES)
+	! grep -q ^ $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES) /dev/null
 
 # See contrib/coccinelle/README
 coccicheck-pending: coccicheck-test

Range-diff against v1:
1:  163a63f2cf ! 1:  dee873e931 coccicheck: fail "make" when it fails
    @@ Commit message
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## Makefile ##
    -@@ Makefile: coccicheck-test: $(COCCI_TEST_RES_GEN)
    +@@ Makefile: endif
    + coccicheck-test: $(COCCI_TEST_RES_GEN)
    + 
      coccicheck: coccicheck-test
    ++
      ifdef SPATCH_CONCAT_COCCI
    - coccicheck: contrib/coccinelle/ALL.cocci.patch
    -+	! test -s contrib/coccinelle/ALL.cocci.patch
    +-coccicheck: contrib/coccinelle/ALL.cocci.patch
    ++COCCICHECK_PATCH_MUST_BE_EMPTY_FILES = contrib/coccinelle/ALL.cocci.patch
      else
    - coccicheck: $(COCCICHECK_PATCHES_INTREE)
    -+	test $$(cat $(COCCICHECK_PATCHES_INTREE) | wc -c) = 0
    +-coccicheck: $(COCCICHECK_PATCHES_INTREE)
    ++COCCICHECK_PATCH_MUST_BE_EMPTY_FILES = $(COCCICHECK_PATCHES_INTREE)
      endif
    ++coccicheck: $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES)
    ++	! grep -q ^ $(COCCICHECK_PATCH_MUST_BE_EMPTY_FILES) /dev/null
      
      # See contrib/coccinelle/README
    + coccicheck-pending: coccicheck-test

-- 
2.50.0-243-g67b26bc0ed

← back to recent threads