* [PATCH] cocci: matching (multiple) identifiers
@ 2025-06-18 17:55 Junio C Hamano
2025-06-18 18:07 ` [PATCH] cocci: do not directly access the .d_type member in struct dirent Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2025-06-18 17:55 UTC (permalink / raw)
To: git
"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(-)
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(...) {<...
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] cocci: do not directly access the .d_type member in struct dirent
2025-06-18 17:55 [PATCH] cocci: matching (multiple) identifiers Junio C Hamano
@ 2025-06-18 18:07 ` Junio C Hamano
2025-06-18 18:31 ` Collin Funk
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2025-06-18 18:07 UTC (permalink / raw)
To: git; +Cc: Carlo Marcelo Arenas Belón, Jacob Keller
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
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent
2025-06-18 18:07 ` [PATCH] cocci: do not directly access the .d_type member in struct dirent Junio C Hamano
@ 2025-06-18 18:31 ` Collin Funk
2025-06-18 19:29 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Collin Funk @ 2025-06-18 18:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Carlo Marcelo Arenas Belón, Jacob Keller
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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent
2025-06-18 18:31 ` Collin Funk
@ 2025-06-18 19:29 ` Junio C Hamano
2025-06-18 19:45 ` Collin Funk
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2025-06-18 19:29 UTC (permalink / raw)
To: Collin Funk; +Cc: git, Carlo Marcelo Arenas Belón, Jacob Keller
Collin Funk <collin.funk1@gmail•com> writes:
> 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.
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent
2025-06-18 19:29 ` Junio C Hamano
@ 2025-06-18 19:45 ` Collin Funk
0 siblings, 0 replies; 5+ messages in thread
From: Collin Funk @ 2025-06-18 19:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Carlo Marcelo Arenas Belón, Jacob Keller
Junio C Hamano <gitster@pobox•com> writes:
>> 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...
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-18 19:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-18 17:55 [PATCH] cocci: matching (multiple) identifiers Junio C Hamano
2025-06-18 18:07 ` [PATCH] cocci: do not directly access the .d_type member in struct dirent Junio C Hamano
2025-06-18 18:31 ` Collin Funk
2025-06-18 19:29 ` Junio C Hamano
2025-06-18 19:45 ` Collin Funk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox