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

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

[Junio C Hamano]

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

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

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

[Jeff King]

On Thu, Jun 19, 2025 at 08:35:35AM -0700, Junio C Hamano wrote:

> 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

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

[Carlo Marcelo Arenas Belón]

On Thu, Jun 19, 2025 at 08:35:35AM -0800, Junio C Hamano wrote:
> 
> 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

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

[Eric Sunshine]

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/

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

[Junio C Hamano]

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

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