{"thread":{"id":"63664","subject":"[PATCH] cocci: matching (multiple) identifiers","startedAt":"2025-06-18T17:55:04Z","lastAt":"2025-06-18T19:45:28Z","messageCount":5,"participants":["Junio C Hamano","Collin Funk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520356","messageId":"xmqqbjqlexzd.fsf@gitster.g","threadId":"63664","inReplyTo":null,"subject":"[PATCH] cocci: matching (multiple) identifiers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-18T17:55:02Z","receivedAt":"2025-06-18T17:55:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"make coccicheck\" seems to work OK at GitHub CI using\n\n $ spatch --version\n spatch version 1.1.1 compiled with OCaml version 4.13.1\n OCaml scripting support: yes\n Python scripting support: yes\n Syntax of regular expressions: PCRE\n\nbut not with \n\n $ spatch --version\n spatch version 1.3 compiled with OCaml version 5.3.0\n OCaml scripting support: yes\n Python scripting support: yes\n Syntax of regular expressions: Str\n\nJudging from https://ocaml.org/manual/5.3/api/Str.html, I suspect\nthat this probably is caused by the distinction between BRE vs PCRE.\nAs there is no reasonably clean way to write the multiple choice\nmatches portably between these two pattern languages, let's stop\nusing regexp_constraint and use compare_constraint instead when\nlisting the function names to exclude.\n\nThere are other uses of \"!~\" but they all want to match a single\nsimple token, that should work fine either with BRE or PCRE.\n\nSigned-off-by: Junio C Hamano \n---\n contrib/coccinelle/commit.cocci | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git c/contrib/coccinelle/commit.cocci w/contrib/coccinelle/commit.cocci\nindex af6dd4c20c..c5284604c5 100644\n--- c/contrib/coccinelle/commit.cocci\n+++ w/contrib/coccinelle/commit.cocci\n@@ -25,7 +25,8 @@ expression s;\n // functions, then the recommended transformation will be bogus with\n // repo_get_commit_tree() on the LHS.\n @@\n-identifier f !~ \"^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$\";\n+identifier f != { repo_get_commit_tree, get_commit_tree_in_graph_one,\n+\t\t load_tree_for_commit, set_commit_tree };\n expression c;\n @@\n f(...) {<...\n"},{"id":"520357","messageId":"xmqq4iwcgbzb.fsf@gitster.g","threadId":"63664","inReplyTo":"xmqqbjqlexzd.fsf@gitster.g","subject":"[PATCH] cocci: do not directly access the .d_type member in struct dirent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-18T18:07:20Z","receivedAt":"2025-06-18T18:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In \"struct dirent\", the presence of the .d_type member should not be\nassumed and the code should instead use DTYPE() macro, with possibly\na fallback check to determine the type of the file.\n\nAdd a rule to catch direct access to the .d_type member and use\nDTYPE() macro instead, except in the emulation code paths that work\non platforms that do have the member. This is probably not sufficient\nto notice the lack of necessary fallback code.\n\nSigned-off-by: Junio C Hamano \n---\n contrib/coccinelle/dtype.cocci | 8 ++++++++\n 1 file changed, 8 insertions(+)\n create mode 100644 contrib/coccinelle/dtype.cocci\n\ndiff --git a/contrib/coccinelle/dtype.cocci b/contrib/coccinelle/dtype.cocci\nnew file mode 100644\nindex 0000000000..8fe66fce95\n--- /dev/null\n+++ b/contrib/coccinelle/dtype.cocci\n@@ -0,0 +1,8 @@\n+@@\n+identifier f != { finddata2dirent, precompose_utf8_readdir };\n+struct dirent *E;\n+@@\n+ f(...) {<...\n+- E->d_type\n++ DTYPE(E)\n+ ...>}\n-- \n2.50.0-228-g6e205fdad9\n\n"},{"id":"520359","messageId":"87ldposxyk.fsf@gmail.com","threadId":"63664","inReplyTo":"xmqq4iwcgbzb.fsf@gitster.g","subject":"Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-06-18T18:31:47Z","receivedAt":"2025-06-18T18:31:49Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Junio C Hamano writes:\n\n> In \"struct dirent\", the presence of the .d_type member should not be\n> assumed and the code should instead use DTYPE() macro, with possibly\n> a fallback check to determine the type of the file.\n>\n> Add a rule to catch direct access to the .d_type member and use\n> DTYPE() macro instead, except in the emulation code paths that work\n> on platforms that do have the member. This is probably not sufficient\n> to notice the lack of necessary fallback code.\n>\n> Signed-off-by: Junio C Hamano \n\nThis change looks good to me. Atleast it will catch code that fails to\nbuild on niche platforms, even if it cannot validate existing backup\ncode.\n\nReviewed-by: Collin Funk \n\nYour fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl\non an AIX machine I have access to in order to test (not an\nadministrator on it).\n\nCollin\n\n[1] https://lore.kernel.org/git/xmqqwm98ewsd.fsf@gitster.g/T/#m4fc1f0ddf1730ffb025e37d523035bd9f6cddfae\n"},{"id":"520361","messageId":"xmqqsejwetli.fsf@gitster.g","threadId":"63664","inReplyTo":"87ldposxyk.fsf@gmail.com","subject":"Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-18T19:29:45Z","receivedAt":"2025-06-18T19:29:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Collin Funk writes:\n\n> Junio C Hamano writes:\n>\n>> In \"struct dirent\", the presence of the .d_type member should not be\n>> assumed and the code should instead use DTYPE() macro, with possibly\n>> a fallback check to determine the type of the file.\n>>\n>> Add a rule to catch direct access to the .d_type member and use\n>> DTYPE() macro instead, except in the emulation code paths that work\n>> on platforms that do have the member. This is probably not sufficient\n>> to notice the lack of necessary fallback code.\n>>\n>> Signed-off-by: Junio C Hamano \n>\n> This change looks good to me. Atleast it will catch code that fails to\n> build on niche platforms, even if it cannot validate existing backup\n> code.\n\nI do not think it is necessarily a good idea to allow building a\nbinary that is known to silently misbehave, though.\n\n> Reviewed-by: Collin Funk \n>\n> Your fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl\n> on an AIX machine I have access to in order to test (not an\n> administrator on it).\n\nI do not think you need libcurl if you only want to test the diff\n--no-index change, but anyway, thanks.\n"},{"id":"520364","messageId":"87a564sujt.fsf@gmail.com","threadId":"63664","inReplyTo":"xmqqsejwetli.fsf@gitster.g","subject":"Re: [PATCH] cocci: do not directly access the .d_type member in struct dirent","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2025-06-18T19:45:26Z","receivedAt":"2025-06-18T19:45:28Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Junio C Hamano writes:\n\n>> This change looks good to me. Atleast it will catch code that fails to\n>> build on niche platforms, even if it cannot validate existing backup\n>> code.\n>\n> I do not think it is necessarily a good idea to allow building a\n> binary that is known to silently misbehave, though.\n\nYes, but off the top of my head I cannot think of a great way to check\nthat DTYPE has backup code for systems that do not have d_type.\n\nAt least in the case of 'git diff --no-index' the tests will fail if we\ndo not properly detect a file is a directory. So at least we will likely\nknow upon running 'make test'. Still that requires manual investigation,\nso not perfect, though...\n\n>> Your fix for the 'git diff --no-index' looks correct [1]. I'll build libcurl\n>> on an AIX machine I have access to in order to test (not an\n>> administrator on it).\n>\n> I do not think you need libcurl if you only want to test the diff\n> --no-index change, but anyway, thanks.\n\nI assumed that it was a hard dependency and could not be disabled. Am I\nwrong?\n\nAnyways, it was quick so no worries. I posted my findings [1].\n\nCollin\n\n[1] https://lore.kernel.org/git/87frfwsv4r.fsf@gmail.com/T/#me995c8f49943f0606c236e8e5e9ea8163602e65b\n"}]}