{"thread":{"id":"64308","subject":"[PATCH] [Outreachy] patch-ids: fix const correctness","startedAt":"2025-10-13T16:53:46Z","lastAt":"2025-10-13T21:55:42Z","messageCount":7,"participants":["Okhuomon Ajayi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528641","messageId":"20251013165320.201333-1-okhuomonajayi54@gmail.com","threadId":"64308","inReplyTo":null,"subject":"[PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-13T16:53:20Z","receivedAt":"2025-10-13T16:53:46Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"The `patch_id_neq()` function received a pointer to diff options via\n`cmpfn_data` but cast it to a non-const type. This caused a const\ncorrectness warning and could potentially allow unintended modification\nof read-only data.\n\nFix this by casting to `const struct diff_options *` instead, removing\nthe outdated NEEDSWORK comment in the process.\n\nSigned-off-by: Okhuomon Ajayi \n---\n patch-ids.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex a5683b462c..b6b808332f 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -41,8 +41,8 @@ static int patch_id_neq(const void *cmpfn_data,\n \t\t\tconst struct hashmap_entry *entry_or_key,\n \t\t\tconst void *keydata UNUSED)\n {\n-\t/* NEEDSWORK: const correctness? */\n-\tstruct diff_options *opt = (void *)cmpfn_data;\n+\t\n+\tconst struct diff_options *opt = (void *)cmpfn_data;\n \tstruct patch_id *a, *b;\n \n \ta = container_of(eptr, struct patch_id, ent);\n-- \n2.43.0\n\n"},{"id":"528643","messageId":"xmqq4is23evz.fsf@gitster.g","threadId":"64308","inReplyTo":"20251013165320.201333-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-13T17:12:00Z","receivedAt":"2025-10-13T17:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Okhuomon Ajayi writes:\n\n> The `patch_id_neq()` function received a pointer to diff options via\n> `cmpfn_data` but cast it to a non-const type. This caused a const\n> correctness warning and could potentially allow unintended modification\n> of read-only data.\n>\n> Fix this by casting to `const struct diff_options *` instead, removing\n> the outdated NEEDSWORK comment in the process.\n>\n> Signed-off-by: Okhuomon Ajayi \n> ---\n> patch-ids.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/patch-ids.c b/patch-ids.c\n> index a5683b462c..b6b808332f 100644\n> --- a/patch-ids.c\n> +++ b/patch-ids.c\n> @@ -41,8 +41,8 @@ static int patch_id_neq(const void *cmpfn_data,\n> \t\t\tconst struct hashmap_entry *entry_or_key,\n> \t\t\tconst void *keydata UNUSED)\n> {\n> -\t/* NEEDSWORK: const correctness? */\n> -\tstruct diff_options *opt = (void *)cmpfn_data;\n> +\t\n\nTrailing whitespace on this line. Remove the entire line instead.\n\n> +\tconst struct diff_options *opt = (void *)cmpfn_data;\n> \tstruct patch_id *a, *b;\n> \n> \ta = container_of(eptr, struct patch_id, ent);\n\nI do not think this is correct. Have you even compile-tested this\npatch?\n\nLater in this same function, opt is passed to commit_patch_id()\nfunction (twice), which takes non-const \"struct diff_options *\" that\nis given to diffcore_std(). And the last function in this callchain\nhas to modify the structure to record various findings (like \"did we\nsee any changes in the diff?\"), so it cannot be \"const\" at all.\n\nI think patch_id_neq() that says cmpfn_data is const is the source\nof the problem, but that function signature is mandated by the\nhashmap API. I do not know if we can loosen it there in the hashmap\nAPI and if so what the argument would be, but thinking about these\nthings is what the NEEDSWORK comment is about ;-).\n\n\n\n"},{"id":"528644","messageId":"CAFpMFfBXhfy7ecBzR-cnGViivQG3AHGrQ00vSTnVY6OdxZPSLg@mail.gmail.com","threadId":"64308","inReplyTo":"xmqq4is23evz.fsf@gitster.g","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-13T17:22:24Z","receivedAt":"2025-10-13T17:22:36Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Thanks, that explains it.\n\nI didn’t compile test before sending my mistake. I see now that opt is\npassed into commit_patch_id() (and friends) and those functions modify\nthe diff_options structure, so making opt const is incorrect. The real\nissue is the mismatch introduced by the hashmap API declaring\ncmpfn_data as const void *, which is what the NEEDSWORK comment was\nflagging.\n\nI’ll revert my local change, run a build and tests, and then think\nabout safer alternatives (or leave the NEEDSWORK comment in place if\nchanging the hashmap API isn’t appropriate).\nThanks for the clarification.\n"},{"id":"528645","messageId":"xmqqzf9u1zix.fsf@gitster.g","threadId":"64308","inReplyTo":"CAFpMFfBXhfy7ecBzR-cnGViivQG3AHGrQ00vSTnVY6OdxZPSLg@mail.gmail.com","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-13T17:29:10Z","receivedAt":"2025-10-13T17:29:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Okhuomon Ajayi writes:\n\n> I’ll revert my local change, run a build and tests, and then think\n> about safer alternatives (or leave the NEEDSWORK comment in place if\n> changing the hashmap API isn’t appropriate).\n> Thanks for the clarification.\n\nIf you can convince readers that changing the hashmap API is not\nappropriate, then I would think that would make a great explanation\nfor a commit that removes the needswork comment without doing\nanything else. \"Thinking about const correctness issues around this\ncode is no longer needed. The hashmap API is right to insist that\nthe extra data pointer must be const because .... Which makes\ncasting constness away when assigning it to opt, which is what the\ncode is, is indeed the only reasonable thing to do, and there is no\nmore change necessary around here.\" Of course, such a commit log\nmessage must fill in the \"because ...\" part with a convincing\nargument ;-).\n\nThanks.\n\n\n"},{"id":"528649","messageId":"CAFpMFfAHA8OfVXKVVSSAQ5p+B8ngT3p54on1HpM+n2qs3P1rHA@mail.gmail.com","threadId":"64308","inReplyTo":"xmqqzf9u1zix.fsf@gitster.g","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-13T18:14:47Z","receivedAt":"2025-10-13T18:14:59Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Hi Junio,\n\nThanks for explaining! I get it now the NEEDSWORK comment isn’t needed\nsince the hashmap API is supposed to have cmpfn_data as const. I’ve\nremoved the comment and didn’t change anything else\n\nCheers\n\nOn Mon, Oct 13, 2025 at 6:29 PM Junio C Hamano wrote:\n>\n> Okhuomon Ajayi writes:\n>\n> > I’ll revert my local change, run a build and tests, and then think\n> > about safer alternatives (or leave the NEEDSWORK comment in place if\n> > changing the hashmap API isn’t appropriate).\n> > Thanks for the clarification.\n>\n> If you can convince readers that changing the hashmap API is not\n> appropriate, then I would think that would make a great explanation\n> for a commit that removes the needswork comment without doing\n> anything else. \"Thinking about const correctness issues around this\n> code is no longer needed. The hashmap API is right to insist that\n> the extra data pointer must be const because .... Which makes\n> casting constness away when assigning it to opt, which is what the\n> code is, is indeed the only reasonable thing to do, and there is no\n> more change necessary around here.\" Of course, such a commit log\n> message must fill in the \"because ...\" part with a convincing\n> argument ;-).\n>\n> Thanks.\n>\n>\n"},{"id":"528658","messageId":"xmqqo6qa1wjg.fsf@gitster.g","threadId":"64308","inReplyTo":"CAFpMFfAHA8OfVXKVVSSAQ5p+B8ngT3p54on1HpM+n2qs3P1rHA@mail.gmail.com","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-13T18:33:39Z","receivedAt":"2025-10-13T18:33:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Okhuomon Ajayi writes:\n\n> Thanks for explaining! I get it now the NEEDSWORK comment isn’t needed\n> since the hashmap API is supposed to have cmpfn_data as const. I’ve\n> removed the comment and didn’t change anything else\n\nThe NEEDSWORK comment is about going even further, starting from\nquestion if hashmap should really be using \"const\" in the first\nplace, to sort things out among all the components involved\n(including other users of the hashmap API).\n\nA commit that does not do the necessary study and just removes the\nneedswork comment is simply irresponsible, no?\n\n"},{"id":"528670","messageId":"CAFpMFfCXy_R1iHmDDo3Zr4rhCpVukSqSsdZ+ycEfj=_6Q45vAw@mail.gmail.com","threadId":"64308","inReplyTo":"xmqqo6qa1wjg.fsf@gitster.g","subject":"Re: [PATCH] [Outreachy] patch-ids: fix const correctness","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-13T21:55:29Z","receivedAt":"2025-10-13T21:55:42Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Got it, that helps. I'll take a closer look at how const is handled\nacross the hashmap API before removing the NEEDSWORK comment. Thanks\nfor the guidance!\n\nBy the way, I also sent another patch about clarifying the SHA1 usage\nfor patch IDs. Would you mind taking a look when you have a moment?\n\n\nOn Mon, Oct 13, 2025 at 7:33 PM Junio C Hamano wrote:\n>\n> Okhuomon Ajayi writes:\n>\n> > Thanks for explaining! I get it now the NEEDSWORK comment isn’t needed\n> > since the hashmap API is supposed to have cmpfn_data as const. I’ve\n> > removed the comment and didn’t change anything else\n>\n> The NEEDSWORK comment is about going even further, starting from\n> question if hashmap should really be using \"const\" in the first\n> place, to sort things out among all the components involved\n> (including other users of the hashmap API).\n>\n> A commit that does not do the necessary study and just removes the\n> needswork comment is simply irresponsible, no?\n>\n"}]}