threads / patch / 63664

patchcocci: matching (multiple) identifiers

Subject: [PATCH] cocci: matching (multiple) identifiers

## tl;dr

5 messages between Jun 18, 2025 and Jun 18, 2025. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Junio C Hamano· Jun 18, 2025, 17:55 UTC · lore
"make coccicheck" seems to work OK at GitHub CI using
    $ spatch --version
    spatch version 1.1.1 compiled with OCaml version 4.13.1
    OCaml scripting support: yes
    Python scripting support: yes
    Syntax of regular expressions: PCRE
but not with 
    $ spatch --version
    spatch version 1.3 compiled with OCaml version 5.3.0
    OCaml scripting support: yes
    Python scripting support: yes
    Syntax of regular expressions: Str

Judging from https://ocaml.org/manual/5.3/api/Str.html, I suspect that this probably is caused by the distinction between BRE vs PCRE. As there is no reasonably clean way to write the multiple choice matches portably between these two pattern languages, let's stop using regexp_constraint and use compare_constraint instead when listing the function names to exclude.

There are other uses of "!~" but they all want to match a single simple token, that should work fine either with BRE or PCRE.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/coccinelle/commit.cocci | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to diff +2 −1
diff --git c/contrib/coccinelle/commit.cocci w/contrib/coccinelle/commit.cocci
index af6dd4c20c..c5284604c5 100644
--- c/contrib/coccinelle/commit.cocci
+++ w/contrib/coccinelle/commit.cocci
@@ -25,7 +25,8 @@ expression s;
 // functions, then the recommended transformation will be bogus with
 // repo_get_commit_tree() on the LHS.
 @@
-identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$";
+identifier f != { repo_get_commit_tree, get_commit_tree_in_graph_one,
+		  load_tree_for_commit, set_commit_tree };
 expression c;
 @@
   f(...) {<...
Junio C Hamano· Jun 18, 2025, 18:07 UTC · re: Junio C Hamano · lore

[PATCH] cocci: do not directly access the .d_type member in struct dirent

In "struct dirent", the presence of the .d_type member should not be assumed and the code should instead use DTYPE() macro, with possibly a fallback check to determine the type of the file.

Add a rule to catch direct access to the .d_type member and use DTYPE() macro instead, except in the emulation code paths that work on platforms that do have the member. This is probably not sufficient to notice the lack of necessary fallback code.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/coccinelle/dtype.cocci | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 contrib/coccinelle/dtype.cocci
Show changes to contrib/coccinelle/dtype.cocci +8 −0
diff --git a/contrib/coccinelle/dtype.cocci b/contrib/coccinelle/dtype.cocci
new file mode 100644
index 0000000000..8fe66fce95
--- /dev/null
+++ b/contrib/coccinelle/dtype.cocci
@@ -0,0 +1,8 @@
+@@
+identifier f != { finddata2dirent, precompose_utf8_readdir };
+struct dirent *E;
+@@
+  f(...) {<...
+- E->d_type
++ DTYPE(E)
+  ...>}

-- 
2.50.0-228-g6e205fdad9
Collin Funk· Jun 18, 2025, 18:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> In "struct dirent", the presence of the .d_type member should not be
> assumed and the code should instead use DTYPE() macro, with possibly
> a fallback check to determine the type of the file.
>
> Add a rule to catch direct access to the .d_type member and use
> DTYPE() macro instead, except in the emulation code paths that work
> on platforms that do have the member.  This is probably not sufficient
> to notice the lack of necessary fallback code.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

This change looks good to me. Atleast it will catch code that fails to build on niche platforms, even if it cannot validate existing backup code.

Reviewed-by: Collin Funk <collin.funk1@gmail.com>

Your fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl on an AIX machine I have access to in order to test (not an administrator on it).

Collin
[1] https://lore.kernel.org/git/xmqqwm98ewsd.fsf@gitster.g/T/#m4fc1f0ddf1730ffb025e37d523035bd9f6cddfae
Junio C Hamano· Jun 18, 2025, 19:29 UTC · re: Collin Funk · lore

Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent

Collin Funk <collin.funk1@gmail.com> writes:
Show 16 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> In "struct dirent", the presence of the .d_type member should not be
>> assumed and the code should instead use DTYPE() macro, with possibly
>> a fallback check to determine the type of the file.
>>
>> Add a rule to catch direct access to the .d_type member and use
>> DTYPE() macro instead, except in the emulation code paths that work
>> on platforms that do have the member.  This is probably not sufficient
>> to notice the lack of necessary fallback code.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> This change looks good to me. Atleast it will catch code that fails to
> build on niche platforms, even if it cannot validate existing backup
> code.

I do not think it is necessarily a good idea to allow building a binary that is known to silently misbehave, though.

Show 5 quoted lines
> Reviewed-by: Collin Funk <collin.funk1@gmail.com>
>
> Your fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl
> on an AIX machine I have access to in order to test (not an
> administrator on it).

I do not think you need libcurl if you only want to test the diff --no-index change, but anyway, thanks.

Collin Funk· Jun 18, 2025, 19:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
>> This change looks good to me. Atleast it will catch code that fails to
>> build on niche platforms, even if it cannot validate existing backup
>> code.
>
> I do not think it is necessarily a good idea to allow building a
> binary that is known to silently misbehave, though.

Yes, but off the top of my head I cannot think of a great way to check that DTYPE has backup code for systems that do not have d_type.

At least in the case of 'git diff --no-index' the tests will fail if we do not properly detect a file is a directory. So at least we will likely know upon running 'make test'. Still that requires manual investigation, so not perfect, though...

Show 6 quoted lines
>> Your fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl
>> on an AIX machine I have access to in order to test (not an
>> administrator on it).
>
> I do not think you need libcurl if you only want to test the diff
> --no-index change, but anyway, thanks.

I assumed that it was a hard dependency and could not be disabled. Am I wrong?

Anyways, it was quick so no worries. I posted my findings [1].
Collin
[1] https://lore.kernel.org/git/87frfwsv4r.fsf@gmail.com/T/#me995c8f49943f0606c236e8e5e9ea8163602e65b

← back to recent threads