# [PATCH] cocci: matching (multiple) identifiers 5 messages from 2025-06-18 to 2025-06-18. Participants: Junio C Hamano, Collin Funk. Thread: https://gitlist.dev/t/63664 ## Junio C Hamano, 2025-06-18 17:55 Subject: [PATCH] cocci: matching (multiple) identifiers Message-ID: URL: https://gitlist.dev/e/xmqqbjqlexzd.fsf%40gitster.g ``` "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 --- 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(...) {<... ``` ## Junio C Hamano, 2025-06-18 18:07 Subject: [PATCH] cocci: do not directly access the .d_type member in struct dirent Message-ID: URL: https://gitlist.dev/e/xmqq4iwcgbzb.fsf%40gitster.g In-Reply-To: ``` 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 --- 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 ``` ## Collin Funk, 2025-06-18 18:31 Subject: Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent Message-ID: <87ldposxyk.fsf@gmail.com> URL: https://gitlist.dev/e/87ldposxyk.fsf%40gmail.com In-Reply-To: ``` Junio C Hamano 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 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 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, 2025-06-18 19:29 Subject: Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent Message-ID: URL: https://gitlist.dev/e/xmqqsejwetli.fsf%40gitster.g In-Reply-To: <87ldposxyk.fsf@gmail.com> ``` Collin Funk writes: > Junio C Hamano 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 > > 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 > > 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, 2025-06-18 19:45 Subject: Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent Message-ID: <87a564sujt.fsf@gmail.com> URL: https://gitlist.dev/e/87a564sujt.fsf%40gmail.com In-Reply-To: ``` Junio C Hamano 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 ```