On Wed, Aug 24 2022, Junio C Hamano wrote: > Derrick Stolee <derrickstolee@xxxxxxxxxx> writes: > >>> We probably need to fix or revert/remove rules we have in >>> unused.cocci that makes bogus "suggestion". >>> >>> https://github.com/git/git/runs/8005321972?check_suite_focus=true >> >> Yes, this is definitely a bogus suggestion. It's probable that it >> is picked up by the newer version of Coccinelle. > > Yes, I think we should tentatively disable the offending one until > we know how to properly "fix" it. I'm seeing this relatively late, it would be nice to get a CC on discussions of of one's code :) This is indeed completely broken, but it's not the unused.cocci rule that's broken. What's happening here is that coccinelle can no longer properly parse the file after the UNUSED() macros were applied to refs.c. Try running with "--verbose-match --verbose-parsing", on "seen". Deleting the UNUSED() from warn_if_dangling_symref() happens to "fix" it, but it's only working as a result of some hack. Coccinelle is running into some unbalanced paren issue, and it happens to balance out with that. I don't think there's any issue here in unused.cocci, it just happens to be the rule that's unlucky enough to fire like that in the face of these parse errors. We should probably coerce coccinelle into stopping in general if it's encountering "BAD:!!!!!" parse errors behind the scenes, as it clearly results in broken logic, but offhand (and from briefly browsing the manpage) I don't know a way to do that. But the fix here isn't to delete unused.cocci, but to hold off on the UNUSEwork D() patches until we figure out how to make coccinelle jive with them. One thing that *would* fix it is to go with the approach I suggested in https://lore.kernel.org/git/220819.868rnk54ju.gmgdl@xxxxxxxxxxxxxxxxxxx/, i.e. to not use an "UNUSED(var)" form, but just "UNUSED". I tried that just now with this hack, which wouldn't even compile with the compiler, but coccinelle is seemingly smart enough to ignore unknown tokens it doesn't know about if they're not introducing parens (i.e. I didn't even have to define UNUSED2). It's also not that it punted out entirely, with this changing refs_verify_refname_available() so that "referent" actually isn't unused for real would have unused.cocci suggest the same removal, so we're managing to fully apply rules to the file: diff --git a/refs.c b/refs.c index 607694c2662..37e7d88920c 100644 --- a/refs.c +++ b/refs.c @@ -442,7 +442,7 @@ struct warn_if_dangling_data { }; static int warn_if_dangling_symref(const char *refname, - const struct object_id *UNUSED(oid), + const struct object_id *oid UNUSED2, int flags, void *cb_data) { struct warn_if_dangling_data *d = cb_data; @@ -982,7 +982,7 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb, } static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid, - const char *UNUSED(email), + const char *email UNUSED2, timestamp_t timestamp, int tz, const char *message, void *cb_data) { @@ -1024,9 +1024,9 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid, return cb->found_it; } -static int read_ref_at_ent_newest(struct object_id *UNUSED(ooid), +static int read_ref_at_ent_newest(struct object_id *ooid UNUSED2, struct object_id *noid, - const char *UNUSED(email), + const char *email UNUSED2, timestamp_t timestamp, int tz, const char *message, void *cb_data) { @@ -1039,7 +1039,7 @@ static int read_ref_at_ent_newest(struct object_id *UNUSED(ooid), } static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid, - const char *UNUSED(email), + const char *email UNUSED2, timestamp_t timestamp, int tz, const char *message, void *cb_data) { @@ -1904,7 +1904,7 @@ struct ref_store_hash_entry char name[FLEX_ARRAY]; }; -static int ref_store_hash_cmp(const void *UNUSED(cmp_data), +static int ref_store_hash_cmp(const void *cmp_data UNUSED2, const struct hashmap_entry *eptr, const struct hashmap_entry *entry_or_key, const void *keydata)
