{"thread":{"id":"54205","subject":"[PATCH] blame.c: replace instance of !oidcmp for oideq","startedAt":"2020-09-07T17:17:38Z","lastAt":"2020-09-09T20:43:51Z","messageCount":14,"participants":["Edmundo Carmona Antoranz","Derrick Stolee","Jeff King","Jeff Smith","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"405129","messageId":"20200907171639.766547-1-eantoranz@gmail.com","threadId":"54205","inReplyTo":null,"subject":"[PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-09-07T17:16:39Z","receivedAt":"2020-09-07T17:17:38Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"---\n blame.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/blame.c b/blame.c\nindex 1be1cd82a2..b475bfa1c0 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1353,8 +1353,8 @@ static struct blame_origin *find_origin(struct repository *r,\n \telse {\n \t\tint compute_diff = 1;\n \t\tif (origin->commit->parents &&\n-\t\t !oidcmp(&parent->object.oid,\n-\t\t\t &origin->commit->parents->item->object.oid))\n+\t\t oideq(&parent->object.oid,\n+\t\t\t &origin->commit->parents->item->object.oid))\n \t\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n \n \t\tif (compute_diff)\n-- \n2.28.0\n\n"},{"id":"405130","messageId":"CAOc6etY1Wdfbg5D-CHSg3zOtGWko+de=HEAnRfgVO_QfK9w07Q@mail.gmail.com","threadId":"54205","inReplyTo":"20200907171639.766547-1-eantoranz@gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-09-07T17:21:21Z","receivedAt":"2020-09-07T17:21:47Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Mon, Sep 7, 2020 at 11:16 AM Edmundo Carmona Antoranz\n wrote:\nBlamed the wrong branch. I should have looped Derrick instead of Jeff.\nSorry about that.\n"},{"id":"405160","messageId":"CAOc6etZ=yurfKsX_Pnx4stnMnBfj=2CggT8hzCueO5yk6banbg@mail.gmail.com","threadId":"54205","inReplyTo":"CAOc6etY1Wdfbg5D-CHSg3zOtGWko+de=HEAnRfgVO_QfK9w07Q@mail.gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-09-08T13:55:24Z","receivedAt":"2020-09-08T16:42:40Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Mon, Sep 7, 2020 at 11:21 AM Edmundo Carmona Antoranz\n wrote:\n> Blamed the wrong branch. I should have looped Derrick instead of Jeff.\n> Sorry about that.\n\nI realized I didn't sign it off. Should I send it again? Or given that\nit's an almost 1-liner, it's ok?\nIf I send it again, I will provide just a little more context about\nhaving the !oidcmp calls replaced\nfor oideq in previous versions.\n"},{"id":"405173","messageId":"ce94b41f-e829-d7ca-a5f5-e41748caea81@gmail.com","threadId":"54205","inReplyTo":"20200907171639.766547-1-eantoranz@gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-08T19:07:34Z","receivedAt":"2020-09-08T19:07:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/7/2020 1:16 PM, Edmundo Carmona Antoranz wrote:\n> ---\n\nPlease include sign-off. I saw you reported your intention there\nin another message, but it's probably best to just send it again.\n\nThis message could also mention 14438c4 (introduce hasheq() and\noideq(), 2018-08-28) which introduced oideq().\n\nThis use of !oidcmp() was introduced by 0906ac2b (blame: use\nchanged-path Bloom filters, 2020-04-16). My bad. There is no\ngood reason to introduce this use since it is well after the\noideq() method was introduced.\n\n> @@ -1353,8 +1353,8 @@ static struct blame_origin *find_origin(struct repository *r,\n> \telse {\n> \t\tint compute_diff = 1;\n> \t\tif (origin->commit->parents &&\n> -\t\t !oidcmp(&parent->object.oid,\n> -\t\t\t &origin->commit->parents->item->object.oid))\n> +\t\t oideq(&parent->object.oid,\n> +\t\t\t &origin->commit->parents->item->object.oid))\n> \t\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n\nThe code itself looks correct.\n\nThanks,\n-Stolee\n"},{"id":"405233","messageId":"20200909091149.GB2496536@coredump.intra.peff.net","threadId":"54205","inReplyTo":"ce94b41f-e829-d7ca-a5f5-e41748caea81@gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-09T09:11:49Z","receivedAt":"2020-09-09T09:12:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 08, 2020 at 03:07:34PM -0400, Derrick Stolee wrote:\n\n> This message could also mention 14438c4 (introduce hasheq() and\n> oideq(), 2018-08-28) which introduced oideq().\n> \n> This use of !oidcmp() was introduced by 0906ac2b (blame: use\n> changed-path Bloom filters, 2020-04-16). My bad. There is no\n> good reason to introduce this use since it is well after the\n> oideq() method was introduced.\n> \n> > @@ -1353,8 +1353,8 @@ static struct blame_origin *find_origin(struct repository *r,\n> > \telse {\n> > \t\tint compute_diff = 1;\n> > \t\tif (origin->commit->parents &&\n> > -\t\t !oidcmp(&parent->object.oid,\n> > -\t\t\t &origin->commit->parents->item->object.oid))\n> > +\t\t oideq(&parent->object.oid,\n> > +\t\t\t &origin->commit->parents->item->object.oid))\n> > \t\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n> \n> The code itself looks correct.\n\nYeah, it looks obviously correct. I am puzzled why \"make coccicheck\"\ndoesn't find this, though. +cc René, as my favorite target for\ncoccinelle nerd-snipes. :)\n\n(But clearly we should make the change with or without figuring out the\ncoccinelle part).\n\n-Peff\n"},{"id":"405251","messageId":"CAOc6etZS3mGxsPPh25XFi2-qR0TNzq0Gx1NrydgQwmHbsjxejA@mail.gmail.com","threadId":"54205","inReplyTo":"20200909091149.GB2496536@coredump.intra.peff.net","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2020-09-09T14:00:57Z","receivedAt":"2020-09-09T16:46:37Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Sep 9, 2020 at 3:11 AM Jeff King wrote:\n>\n> Yeah, it looks obviously correct. I am puzzled why \"make coccicheck\"\n> doesn't find this, though. +cc René, as my favorite target for\n> coccinelle nerd-snipes. :)\n>\n\nI added this to contrib/coccinelle/object_id.cocci in v2.27.0\n\n@@\nidentifier f != oideq;\nexpression E1, E2;\n@@\n- !oidcmp(E1, E2)\n+ oideq(E1, E2)\n\nAnd it found it:\n\n$ cat contrib/coccinelle/object_id.cocci.patch\ndiff -u -p a/blame.c b/blame.c\n--- a/blame.c\n+++ b/blame.c\n@@ -1352,8 +1352,7 @@ static struct blame_origin *find_origin(\n else {\n int compute_diff = 1;\n if (origin->commit->parents &&\n- !oidcmp(&parent->object.oid,\n- &origin->commit->parents->item->object.oid))\n+ oideq(&parent->object.oid,\n&origin->commit->parents->item->object.oid))\n compute_diff = maybe_changed_path(r, origin, bd);\n\n if (compute_diff)\n\n\nDo I need to add more things into the coccinelle definition so that it\nis more restrictive in terms of the\nexpression we are hunting down?\n"},{"id":"405265","messageId":"CAPX7N=5wRZbwWzzKmqyQGswJDFDf-DWq56pBPMSx4=fEuisskg@mail.gmail.com","threadId":"54205","inReplyTo":"CAOc6etZS3mGxsPPh25XFi2-qR0TNzq0Gx1NrydgQwmHbsjxejA@mail.gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Jeff Smith","fromEmail":"whydoubt@gmail.com","sentAt":"2020-09-09T17:48:13Z","receivedAt":"2020-09-09T17:48:36Z","isPatch":true,"sender":{"key":"whydoubt@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2654252?v=4"},"body":"I haven't had a chance to look at the cocci script, but I did have one\nthought...\n\nDerrick pointed out, 14438c4 added both oideq and hasheq.\nIt might be good to have a similar check for hasheq, if there is not\none already.\n\nOn Wed, Sep 9, 2020 at 9:01 AM Edmundo Carmona Antoranz\n wrote:\n>\n> On Wed, Sep 9, 2020 at 3:11 AM Jeff King wrote:\n> >\n> > Yeah, it looks obviously correct. I am puzzled why \"make coccicheck\"\n> > doesn't find this, though. +cc René, as my favorite target for\n> > coccinelle nerd-snipes. :)\n> >\n>\n> I added this to contrib/coccinelle/object_id.cocci in v2.27.0\n>\n> @@\n> identifier f != oideq;\n> expression E1, E2;\n> @@\n> - !oidcmp(E1, E2)\n> + oideq(E1, E2)\n>\n> And it found it:\n>\n> $ cat contrib/coccinelle/object_id.cocci.patch\n> diff -u -p a/blame.c b/blame.c\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -1352,8 +1352,7 @@ static struct blame_origin *find_origin(\n> else {\n> int compute_diff = 1;\n> if (origin->commit->parents &&\n> - !oidcmp(&parent->object.oid,\n> - &origin->commit->parents->item->object.oid))\n> + oideq(&parent->object.oid,\n> &origin->commit->parents->item->object.oid))\n> compute_diff = maybe_changed_path(r, origin, bd);\n>\n> if (compute_diff)\n>\n>\n> Do I need to add more things into the coccinelle definition so that it\n> is more restrictive in terms of the\n> expression we are hunting down?\n"},{"id":"405272","messageId":"20200909191345.GA2511547@coredump.intra.peff.net","threadId":"54205","inReplyTo":"CAOc6etZS3mGxsPPh25XFi2-qR0TNzq0Gx1NrydgQwmHbsjxejA@mail.gmail.com","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-09T19:13:45Z","receivedAt":"2020-09-09T19:13:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 09, 2020 at 08:00:57AM -0600, Edmundo Carmona Antoranz wrote:\n\n> On Wed, Sep 9, 2020 at 3:11 AM Jeff King wrote:\n> >\n> > Yeah, it looks obviously correct. I am puzzled why \"make coccicheck\"\n> > doesn't find this, though. +cc René, as my favorite target for\n> > coccinelle nerd-snipes. :)\n> >\n> \n> I added this to contrib/coccinelle/object_id.cocci in v2.27.0\n> \n> @@\n> identifier f != oideq;\n> expression E1, E2;\n> @@\n> - !oidcmp(E1, E2)\n> + oideq(E1, E2)\n> \n> And it found it:\n\nInteresting. The existing rule is:\n\n struct object_id *OIDPTR1;\n struct object_id *OIDPTR2;\n @@\n - oidcmp(OIDPTR1, OIDPTR2) == 0\n + oideq(OIDPTR1, OIDPTR2)\n\nThe \"== 0\" part looks like it might be significant, but it's not.\nCoccinelle knows that \"!foo\" is the same as \"foo == 0\" (and you can\nconfirm by tweaking it).\n\nThe addition of \"identifer f != oideq\" here isn't necessary (we don't\neven define an \"f\" in the semantic patch part). And anyway, we use\nhasheq() inside oideq(), so no need to override the rule there.\n\nSo the relevant part is probably that our existing rule specifies the\nexact type, whereas your rule allows any expression.\n\nAnd indeed, if I do this, it works:\n\ndiff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\nindex ddf4f22bd7..62a6cee0eb 100644\n--- a/contrib/coccinelle/object_id.cocci\n+++ b/contrib/coccinelle/object_id.cocci\n@@ -55,8 +55,8 @@ struct object_id OID;\n + oidcmp(&OID, OIDPTR)\n \n @@\n-struct object_id *OIDPTR1;\n-struct object_id *OIDPTR2;\n+expression OIDPTR1;\n+expression OIDPTR2;\n @@\n - oidcmp(OIDPTR1, OIDPTR2) == 0\n + oideq(OIDPTR1, OIDPTR2)\n\nWhich really _seems_ like a bug in coccinelle, unless I am missing\nsomething. Because both of those parameters look like object_id pointers\n(and the compiler would be complaining if it were not the case). But I\nalso wonder if giving the specific types in the coccinelle rule is\nbuying us anything. If you passed two void pointers or ints or whatever\nto !oidcmp(), we'd still want to rewrite it as oideq().\n\n-Peff\n"},{"id":"405274","messageId":"20200909191746.GA2514794@coredump.intra.peff.net","threadId":"54205","inReplyTo":"20200909191345.GA2511547@coredump.intra.peff.net","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-09T19:17:46Z","receivedAt":"2020-09-09T19:17:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 09, 2020 at 03:13:46PM -0400, Jeff King wrote:\n\n> Which really _seems_ like a bug in coccinelle, unless I am missing\n> something. Because both of those parameters look like object_id pointers\n> (and the compiler would be complaining if it were not the case). But I\n> also wonder if giving the specific types in the coccinelle rule is\n> buying us anything. If you passed two void pointers or ints or whatever\n> to !oidcmp(), we'd still want to rewrite it as oideq().\n\nAnd indeed, just blindly swapping out \"struct object_id\" for\n\"expression\" in the coccinelle file (patch below), shows another spot\nthat was missed:\n\ndiff -u -p a/packfile.c b/packfile.c\n--- a/packfile.c\n+++ b/packfile.c\n@@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const\n \tp->mtime = st.st_mtime;\n \tif (path_len < the_hash_algo->hexsz ||\n \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n-\t\thashclr(p->hash);\n+\t\toidclr(p);\n \treturn p;\n }\n \n\nMaybe it's worth being looser in our cocci patch definitions. I'm having\ntrouble thinking of a downside...\n\n-Peff\n\n-- >8 --\nHere's the patch to loosen object_id.cocci. Perhaps we'd want to do the\nsame in other files.\n\ndiff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\nindex ddf4f22bd7..738c60923e 100644\n--- a/contrib/coccinelle/object_id.cocci\n+++ b/contrib/coccinelle/object_id.cocci\n@@ -1,62 +1,62 @@\n @@\n-struct object_id OID;\n+expression OID;\n @@\n - is_null_sha1(OID.hash)\n + is_null_oid(&OID)\n \n @@\n-struct object_id *OIDPTR;\n+expression *OIDPTR;\n @@\n - is_null_sha1(OIDPTR->hash)\n + is_null_oid(OIDPTR)\n \n @@\n-struct object_id OID;\n+expression OID;\n @@\n - hashclr(OID.hash)\n + oidclr(&OID)\n \n @@\n identifier f != oidclr;\n-struct object_id *OIDPTR;\n+expression *OIDPTR;\n @@\n f(...) {<...\n - hashclr(OIDPTR->hash)\n + oidclr(OIDPTR)\n ...>}\n \n @@\n-struct object_id OID1, OID2;\n+expression OID1, OID2;\n @@\n - hashcmp(OID1.hash, OID2.hash)\n + oidcmp(&OID1, &OID2)\n \n @@\n identifier f != oidcmp;\n-struct object_id *OIDPTR1, OIDPTR2;\n+expression *OIDPTR1, OIDPTR2;\n @@\n f(...) {<...\n - hashcmp(OIDPTR1->hash, OIDPTR2->hash)\n + oidcmp(OIDPTR1, OIDPTR2)\n ...>}\n \n @@\n-struct object_id *OIDPTR;\n-struct object_id OID;\n+expression *OIDPTR;\n+expression OID;\n @@\n - hashcmp(OIDPTR->hash, OID.hash)\n + oidcmp(OIDPTR, &OID)\n \n @@\n-struct object_id *OIDPTR;\n-struct object_id OID;\n+expression *OIDPTR;\n+expression OID;\n @@\n - hashcmp(OID.hash, OIDPTR->hash)\n + oidcmp(&OID, OIDPTR)\n \n @@\n-struct object_id *OIDPTR1;\n-struct object_id *OIDPTR2;\n+expression OIDPTR1;\n+expression OIDPTR2;\n @@\n - oidcmp(OIDPTR1, OIDPTR2) == 0\n + oideq(OIDPTR1, OIDPTR2)\n@@ -71,8 +71,8 @@ expression E1, E2;\n ...>}\n \n @@\n-struct object_id *OIDPTR1;\n-struct object_id *OIDPTR2;\n+expression *OIDPTR1;\n+expression *OIDPTR2;\n @@\n - oidcmp(OIDPTR1, OIDPTR2) != 0\n + !oideq(OIDPTR1, OIDPTR2)\n"},{"id":"405279","messageId":"0773b560-b456-fc88-42d7-f214246ddd1b@web.de","threadId":"54205","inReplyTo":"20200909191746.GA2514794@coredump.intra.peff.net","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-09T19:54:55Z","receivedAt":"2020-09-09T19:55:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.09.20 um 21:17 schrieb Jeff King:\n> On Wed, Sep 09, 2020 at 03:13:46PM -0400, Jeff King wrote:\n>\n>> Which really _seems_ like a bug in coccinelle, unless I am missing\n>> something. Because both of those parameters look like object_id pointers\n>> (and the compiler would be complaining if it were not the case). But I\n>> also wonder if giving the specific types in the coccinelle rule is\n>> buying us anything. If you passed two void pointers or ints or whatever\n>> to !oidcmp(), we'd still want to rewrite it as oideq().\n\nRight, using expressions for such a like-for-like transformation is safe\nand practical in the sense that it won't break correct code, and broken\ncode will be flagged by the compiler.\n\n>\n> And indeed, just blindly swapping out \"struct object_id\" for\n> \"expression\" in the coccinelle file (patch below), shows another spot\n> that was missed:\n>\n> diff -u -p a/packfile.c b/packfile.c\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const\n> \tp->mtime = st.st_mtime;\n> \tif (path_len < the_hash_algo->hexsz ||\n> \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n> -\t\thashclr(p->hash);\n> +\t\toidclr(p);\n> \treturn p;\n> }\n>\n>\n> Maybe it's worth being looser in our cocci patch definitions. I'm having\n> trouble thinking of a downside...\n\nFor transformations that change the type as in the example above we\nshould insist on getting the right one, otherwise we might introduce\nbugs -- like in the example above. p points to a struct packed_git and\nnot to a struct object_id, so this introduces a type mismatch.\n\nWe better make sure our semantic patches are safe, otherwise we have to\ncheck all conversions very carefully, and then we might be better off\ndoing them manually..\n\nRené\n"},{"id":"405281","messageId":"20200909195803.GA2515827@coredump.intra.peff.net","threadId":"54205","inReplyTo":"0773b560-b456-fc88-42d7-f214246ddd1b@web.de","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-09T19:58:03Z","receivedAt":"2020-09-09T19:58:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 09, 2020 at 09:54:55PM +0200, René Scharfe wrote:\n\n> > diff -u -p a/packfile.c b/packfile.c\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const\n> > \tp->mtime = st.st_mtime;\n> > \tif (path_len < the_hash_algo->hexsz ||\n> > \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n> > -\t\thashclr(p->hash);\n> > +\t\toidclr(p);\n> > \treturn p;\n> > }\n> >\n> >\n> > Maybe it's worth being looser in our cocci patch definitions. I'm having\n> > trouble thinking of a downside...\n> \n> For transformations that change the type as in the example above we\n> should insist on getting the right one, otherwise we might introduce\n> bugs -- like in the example above. p points to a struct packed_git and\n> not to a struct object_id, so this introduces a type mismatch.\n\nHeh. You'd think that I would have applied that patch and run \"make\". Or\neven read it carefully.\n\nThanks for pointing that out. I guess now we have a real example of a\ndownside (the compiler _would_ still catch it, but it means \"make\ncoccicheck\" is useless if it's repeatedly suggesting a bad\ntransformation).\n\n-Peff\n"},{"id":"405282","messageId":"xmqqa6xyvhtx.fsf@gitster.c.googlers.com","threadId":"54205","inReplyTo":"0773b560-b456-fc88-42d7-f214246ddd1b@web.de","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-09T20:03:06Z","receivedAt":"2020-09-09T20:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> diff -u -p a/packfile.c b/packfile.c\n>> --- a/packfile.c\n>> +++ b/packfile.c\n>> @@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const\n>> \tp->mtime = st.st_mtime;\n>> \tif (path_len < the_hash_algo->hexsz ||\n>> \t get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))\n>> -\t\thashclr(p->hash);\n>> +\t\toidclr(p);\n>> \treturn p;\n>> }\n>>\n>>\n>> Maybe it's worth being looser in our cocci patch definitions. I'm having\n>> trouble thinking of a downside...\n>\n> For transformations that change the type as in the example above we\n> should insist on getting the right one, otherwise we might introduce\n> bugs -- like in the example above. p points to a struct packed_git and\n> not to a struct object_id, so this introduces a type mismatch.\n\n;-) A good counter-example.\n\n> We better make sure our semantic patches are safe, otherwise we have to\n> check all conversions very carefully, and then we might be better off\n> doing them manually..\n\nYes, that is a sensible suggestion.\n\n"},{"id":"405283","messageId":"xmqq5z8mvhou.fsf@gitster.c.googlers.com","threadId":"54205","inReplyTo":"20200909191746.GA2514794@coredump.intra.peff.net","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-09T20:06:09Z","receivedAt":"2020-09-09T20:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@\n> -struct object_id *OIDPTR1;\n> -struct object_id *OIDPTR2;\n> +expression OIDPTR1;\n> +expression OIDPTR2;\n> @@\n> - oidcmp(OIDPTR1, OIDPTR2) == 0\n> + oideq(OIDPTR1, OIDPTR2)\n> @@ -71,8 +71,8 @@ expression E1, E2;\n> ...>}\n> \n> @@\n> -struct object_id *OIDPTR1;\n> -struct object_id *OIDPTR2;\n> +expression *OIDPTR1;\n> +expression *OIDPTR2;\n> @@\n> - oidcmp(OIDPTR1, OIDPTR2) != 0\n> + !oideq(OIDPTR1, OIDPTR2)\n\nWith an extra insight from the counter-example Réne pointed out in\nyour message, I think the above two are safe but all the others are\nunsafe.\n\n"},{"id":"405288","messageId":"84ce64d0-996a-33f6-53be-e47120da81a7@web.de","threadId":"54205","inReplyTo":"20200909191345.GA2511547@coredump.intra.peff.net","subject":"Re: [PATCH] blame.c: replace instance of !oidcmp for oideq","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-09T20:43:38Z","receivedAt":"2020-09-09T20:43:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.09.20 um 21:13 schrieb Jeff King:\n> On Wed, Sep 09, 2020 at 08:00:57AM -0600, Edmundo Carmona Antoranz wrote:\n>\n>> On Wed, Sep 9, 2020 at 3:11 AM Jeff King wrote:\n>>>\n>>> Yeah, it looks obviously correct. I am puzzled why \"make coccicheck\"\n>>> doesn't find this, though. +cc René, as my favorite target for\n>>> coccinelle nerd-snipes. :)\n>>>\n>>\n>> I added this to contrib/coccinelle/object_id.cocci in v2.27.0\n>>\n>> @@\n>> identifier f != oideq;\n>> expression E1, E2;\n>> @@\n>> - !oidcmp(E1, E2)\n>> + oideq(E1, E2)\n>>\n>> And it found it:\n>\n> Interesting. The existing rule is:\n>\n> struct object_id *OIDPTR1;\n> struct object_id *OIDPTR2;\n> @@\n> - oidcmp(OIDPTR1, OIDPTR2) == 0\n> + oideq(OIDPTR1, OIDPTR2)\n>\n> The \"== 0\" part looks like it might be significant, but it's not.\n> Coccinelle knows that \"!foo\" is the same as \"foo == 0\" (and you can\n> confirm by tweaking it).\n\nIt is significant in the sense that \"x == 0\" in the semantic patch also\nmatches \"!x\" in the code, but \"!x\" in the semantic patch doesn't match\n\"x == 0\". That's because coccinelle has this isomorphism built in\n(in /usr/lib/coccinelle/standard.iso on my machine):\n\nExpression\n@ not_int1 @\nint X;\n@@\n !X => X == 0\n\nIt's a one-way isomorphism (i.e. a rule that says that certain\nexpressions have the same meaning). So we should use \"x == 0\" over \"!x\"\nin semantic patches to cover both cases.\n\n> So the relevant part is probably that our existing rule specifies the\n> exact type, whereas your rule allows any expression.\n>\n> And indeed, if I do this, it works:\n>\n> diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci\n> index ddf4f22bd7..62a6cee0eb 100644\n> --- a/contrib/coccinelle/object_id.cocci\n> +++ b/contrib/coccinelle/object_id.cocci\n> @@ -55,8 +55,8 @@ struct object_id OID;\n> + oidcmp(&OID, OIDPTR)\n>\n> @@\n> -struct object_id *OIDPTR1;\n> -struct object_id *OIDPTR2;\n> +expression OIDPTR1;\n> +expression OIDPTR2;\n> @@\n> - oidcmp(OIDPTR1, OIDPTR2) == 0\n> + oideq(OIDPTR1, OIDPTR2)\n>\n> Which really _seems_ like a bug in coccinelle, unless I am missing\n> something. Because both of those parameters look like object_id pointers\n> (and the compiler would be complaining if it were not the case).\nYes, seems it looks like coccinelle gives up trying to determine the\ntype of these things.\n\nAnd while this one here matches the example in blame.c:\n\n@@\nexpression A, B;\n@@\n- 0 == oidcmp(A, B)\n+ oideq(A, B)\n\n... and this one does as well:\n\n@@\nexpression A, B;\n@@\n- !oidcmp\n+ oideq\n (A, B)\n\n... the following one doesn't:\n\n@@\nexpression A, B;\n@@\n- 0 == oidcmp\n+ oideq\n (A, B)\n\n... and neither does this one:\n\n@@\nexpression A, B;\n@@\n- oidcmp\n+ oideq\n (A, B)\n- == 0\n\nSo it helps to try some variants in the hope to bypass some of the\nrestrictions/bugs/misunderstandings. O_o\n\nRené\n"}]}