{"thread":{"id":"65164","subject":"[PATCH] patch-ids: achieve const correctness in patch_id_neq()","startedAt":"2026-03-08T04:31:59Z","lastAt":"2026-03-09T06:52:08Z","messageCount":7,"participants":["Tian Yuchen","Junio C Hamano","cat@malon.dev"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538187","messageId":"20260308043131.77782-1-a3205153416@gmail.com","threadId":"65164","inReplyTo":null,"subject":"[PATCH] patch-ids: achieve const correctness in patch_id_neq()","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-03-08T04:31:31Z","receivedAt":"2026-03-08T04:31:59Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The implementation of the 'contain_of' macro in 'patch_id_neq()' is:\n\n\t#define container_of(ptr, type, member) \\\n\t\t((type *) ((char *)(ptr) - offsetof(type, member)))\n\nHere, 'type' is passed as a raw type with no const information.\nConsequently, const correctness cannot be guaranteed here, resulting\nin an eight-year-long NEEDSWORK comment.\n\nUse explicit casting (struct object_id *) to ensure const correctness.\n\nSigned-off-by: Tian Yuchen <a3205153416@gmail.com>\n---\n patch-ids.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex a5683b462c..e2d29e9dbb 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -41,19 +41,18 @@ 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-\tstruct patch_id *a, *b;\n+\tstruct diff_options *opt = (struct diff_options *)cmpfn_data;\n+\tconst struct patch_id *a, *b;\n \n-\ta = container_of(eptr, struct patch_id, ent);\n-\tb = container_of(entry_or_key, struct patch_id, ent);\n+\ta = container_of(eptr, const struct patch_id, ent);\n+\tb = container_of(entry_or_key, const struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t    commit_patch_id(a->commit, opt, &a->patch_id, 0))\n+\t    commit_patch_id(a->commit, opt, (struct object_id *)&a->patch_id, 0))\n \t\treturn error(\"Could not get patch ID for %s\",\n \t\t\toid_to_hex(&a->commit->object.oid));\n \tif (is_null_oid(&b->patch_id) &&\n-\t    commit_patch_id(b->commit, opt, &b->patch_id, 0))\n+\t    commit_patch_id(b->commit, opt, (struct object_id *)&b->patch_id, 0))\n \t\treturn error(\"Could not get patch ID for %s\",\n \t\t\toid_to_hex(&b->commit->object.oid));\n \treturn !oideq(&a->patch_id, &b->patch_id);\n-- \n2.43.0\n\n"},{"id":"538191","messageId":"xmqqseaasuph.fsf@gitster.g","threadId":"65164","inReplyTo":"20260308043131.77782-1-a3205153416@gmail.com","subject":"Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-08T06:26:18Z","receivedAt":"2026-03-08T06:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen <a3205153416@gmail.com> writes:\n\n> The implementation of the 'contain_of' macro in 'patch_id_neq()' is:\n>\n> \t#define container_of(ptr, type, member) \\\n> \t\t((type *) ((char *)(ptr) - offsetof(type, member)))\n>\n> Here, 'type' is passed as a raw type with no const information.\n> Consequently, const correctness cannot be guaranteed here, resulting\n> in an eight-year-long NEEDSWORK comment.\n>\n> Use explicit casting (struct object_id *) to ensure const correctness.\n\nThe \"NEEDSWORK: const correctness?\" comment in patch-ids.c:patch_id_neq()\nis indeed about the fact that this function modifies its arguments `eptr`\nand `entry_or_key` (via the `patch_id` pointers `a` and `b`) to lazily\ncompute the full patch ID.\n\nI am afraid, however, that this patch may not be moving in the right\ndirection. By marking `a` and `b` as `const struct patch_id *`, you are\ntelling the compiler and the reader that the objects they point to will\nnot be modified. But then you immediately cast that constness away when\ncalling `commit_patch_id()`:\n\n> -\t    commit_patch_id(a->commit, opt, &a->patch_id, 0))\n> +\t    commit_patch_id(a->commit, opt, (struct object_id *)&a->patch_id, 0))\n\nI wonder if this is actually any better than the original code. If an\nobject is marked `const`, it is generally supposed to be immutable.\nCasting away the constness of a member to pass it to a function that is\nexpected to write its findings into that memory region sounds wrong to me.\n\nThe fact that `patch_id_neq` receives `const struct hashmap_entry *`\nparameters--which is a requirement of the `hashmap` API--is what is\nclashing with our lazy initialization here. The original code\nhandled this by casting to a non-const `struct patch_id *` right at\nthe beginning (via `container_of`). While this also \"drops\" the\nconstness, the original is at least more honest about the fact that\n`a` and `b` will be modified.\n\nIf we truly wanted to achieve const-correctness here, we would\nlikely need to avoid lazy initialization within the comparison\nfunction altogether, pre-calculating the full patch ID before it's\nneeded for comparison. The NEEDSWORK comment should remain until a\nmore fundamental solution is found, or if we should just admit that\nthe current lazy evaluation pattern is what we want and document\nthat (i.e., add a comment to justify why we strip away the constness\nhere). As it stands, this patch doesn't \"ensure\" const correctness;\nit just masks the violation with an explicit cast at the location\nthe pointer is used.\n"},{"id":"538202","messageId":"1a1ed5c6-8843-4bd5-9f57-187ef39497c3@gmail.com","threadId":"65164","inReplyTo":"xmqqseaasuph.fsf@gitster.g","subject":"Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-03-08T14:42:43Z","receivedAt":"2026-03-08T14:42:47Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\nOn 3/8/26 14:26, Junio C Hamano wrote:\n\n> The fact that `patch_id_neq` receives `const struct hashmap_entry *`\n> parameters--which is a requirement of the `hashmap` API--is what is\n> clashing with our lazy initialization here. The original code\n> handled this by casting to a non-const `struct patch_id *` right at\n> the beginning (via `container_of`). While this also \"drops\" the\n> constness, the original is at least more honest about the fact that\n> `a` and `b` will be modified.\n\nOops, This is something I hadn't considered before. I admit I didn't \nthink things through carefully enough.\n\nSo, we actually find ourselves in this dilemma:\n\n  - The Hashmap API specification mandates that input parameters should \nbe const *in principle*.\n\n  - The lazy loading mechanism requires us to write the results into \nmemory; otherwise, there will be significant performance loss.\n\nAm I correct?\n\nI find that maintaining the current approach seems the most reasonable \noption. Computing all patch ids before putting objects into the hashmap \nappears to be a move that affects everything else and is not worth the \neffort. On the contrary, slightly breaking the Hashmap API conventions \nseems to be a more *cost-effective* approach...\n\nOr perhaps it would be more reasonable to slightly modify this NEEDSWORK \nflag here?\n\nWill send the next patch shortly.\n\n> If we truly wanted to achieve const-correctness here, we would\n> likely need to avoid lazy initialization within the comparison\n> function altogether, pre-calculating the full patch ID before it's\n> needed for comparison. The NEEDSWORK comment should remain until a\n> more fundamental solution is found, or if we should just admit that\n> the current lazy evaluation pattern is what we want and document\n> that (i.e., add a comment to justify why we strip away the constness\n> here). As it stands, this patch doesn't \"ensure\" const correctness;\n> it just masks the violation with an explicit cast at the location\n> the pointer is used.\n\nRegards,\n\nYuchen\n"},{"id":"538203","messageId":"20260308150203.86299-1-cat@malon.dev","threadId":"65164","inReplyTo":"20260308043131.77782-1-a3205153416@gmail.com","subject":"[PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-08T15:02:03Z","receivedAt":"2026-03-08T15:02:22Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The hashmap API requires the comparison function to take const pointers.\nHowever, patch_id_neq() uses lazy evaluation to compute patch IDs on\ndemand.\n\nPre-calculating all patch IDs to achieve true const correctness would\nintroduce an unacceptable performance penalty.\n\nRemove the eight-year-old \"NEEDSWORK\" comment and formally document\nthis intentional design trade-off.\n\nSigned-off-by: Tian Yuchen <cat@malon.dev>\n---\n patch-ids.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex a5683b462c..35e6a974f1 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -41,7 +41,15 @@ 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+\t/*\n+\t * We drop the 'const' modifier here intentionally.\n+\t *\n+\t * The hashmap API requires us to treat the entries as const.\n+\t * However, to avoid performance regression, we lazily compute\n+\t * the patch IDs inside this comparison function. This fundamentally\n+\t * requires us to mutate the 'struct patch_id'. Therefore, we use\n+\t * container_of() to cast away the constness from the hashmap_entry.\n+\t */\n \tstruct diff_options *opt = (void *)cmpfn_data;\n \tstruct patch_id *a, *b;\n \n-- \n2.43.0\n\n"},{"id":"538228","messageId":"xmqqh5qp97bd.fsf@gitster.g","threadId":"65164","inReplyTo":"20260308150203.86299-1-cat@malon.dev","subject":"Re: [PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-09T00:26:30Z","receivedAt":"2026-03-09T00:26:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tian Yuchen <cat@malon.dev> writes:\n\n> +\t/*\n> +\t * We drop the 'const' modifier here intentionally.\n> +\t *\n> +\t * The hashmap API requires us to treat the entries as const.\n> +\t * However, to avoid performance regression, we lazily compute\n> +\t * the patch IDs inside this comparison function. This fundamentally\n> +\t * requires us to mutate the 'struct patch_id'. Therefore, we use\n> +\t * container_of() to cast away the constness from the hashmap_entry.\n> +\t */\n\nIs that a \"performance regression\", I have to wonder?  We would\nregress relative to what by doing what?\n\nIs the lazy evaluation avoiding unnecessary work?\n\nIf we are going to pass _all_ the objects in the hashmap to this\ncomparator function eventually _anyway_, then the total cost of\ncomputing patch IDs to all of them in the hashmap would not change\nwith or without lazy computation, but if we are currently getting\naway without having to compute for all, but only computing for the\nones we pass to this function, then lazy evaluation is clearly a\nwin.  I do not offhand know which of the above two is the case, but\nwe need to know that before we can touch the NEEDSWORK comment, I\nthink.\n\nThe lazy computation comes from b3dfeebb (rebase: avoid computing\nunnecessary patch IDs, 2016-07-29), even though the \"const\ncorrectness?\" comment is a bit newer than that.\n\nSo it seems that we indeed are avoiding unnecessary work without\nthis patch.  We'd encounter \"performance regression\" only if we stop\navoiding unnecessary work, so I am afraid that the phrasing used in\nthe patch is somewhat confusing.\n\n    Even though eptr and entry_or_key are const, we want to lazily\n    compute their .patch_id members; see b3dfeebb (rebase: avoid\n    computing unnecessary patch IDs, 2016-07-29), so cast the\n    constness away with container_of().\n\nor something, perhaps?\n\n>  \tstruct diff_options *opt = (void *)cmpfn_data;\n>  \tstruct patch_id *a, *b;\n"},{"id":"538251","messageId":"4d93dbf55e141460989f21edad24440d@purelymail.com","threadId":"65164","inReplyTo":"xmqqh5qp97bd.fsf@gitster.g","subject":"Re: [PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()","fromName":"","fromEmail":"cat@malon.dev","sentAt":"2026-03-09T06:39:25Z","receivedAt":"2026-03-09T06:39:34Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"Hi Junio,\n\n> Is that a \"performance regression\", I have to wonder?  We would\n> regress relative to what by doing what?\n> \n> Is the lazy evaluation avoiding unnecessary work?\n> \n> If we are going to pass _all_ the objects in the hashmap to this\n> comparator function eventually _anyway_, then the total cost of\n> computing patch IDs to all of them in the hashmap would not change\n> with or without lazy computation, but if we are currently getting\n> away without having to compute for all, but only computing for the\n> ones we pass to this function, then lazy evaluation is clearly a\n> win.  I do not offhand know which of the above two is the case, but\n> we need to know that before we can touch the NEEDSWORK comment, I\n> think.\n> \n> The lazy computation comes from b3dfeebb (rebase: avoid computing\n> unnecessary patch IDs, 2016-07-29), even though the \"const\n> correctness?\" comment is a bit newer than that.\n> \n> So it seems that we indeed are avoiding unnecessary work without\n> this patch.  We'd encounter \"performance regression\" only if we stop\n> avoiding unnecessary work, so I am afraid that the phrasing used in\n> the patch is somewhat confusing.\n\nYou're right. Avoiding unnecessary work is indeed a more fundamental\nreason than preventing performance regression.\n\n>     Even though eptr and entry_or_key are const, we want to lazily\n>     compute their .patch_id members; see b3dfeebb (rebase: avoid\n>     computing unnecessary patch IDs, 2016-07-29), so cast the\n>     constness away with container_of().\n> \n> or something, perhaps?\n> \n>>  \tstruct diff_options *opt = (void *)cmpfn_data;\n>>  \tstruct patch_id *a, *b;\n\nI will incorporate your suggested phrasing and reference to the \nhistorical\ncommit in v3.\n\nRegards,\n\nYuchen\n"},{"id":"538252","messageId":"20260309065140.108644-1-cat@malon.dev","threadId":"65164","inReplyTo":"20260308150203.86299-1-cat@malon.dev","subject":"[PATCH v3] patch-ids: document intentional const-casting in patch_id_neq()","fromName":"Tian Yuchen","fromEmail":"cat@malon.dev","sentAt":"2026-03-09T06:51:40Z","receivedAt":"2026-03-09T06:52:08Z","isPatch":true,"sender":{"key":"cat@malon.dev","avatar":"https://avatars.githubusercontent.com/u/232002048?v=4"},"body":"The hashmap API requires the comparison function to take const pointers.\nHowever, patch_id_neq() uses lazy evaluation to compute patch IDs on\ndemand. As established in b3dfeebb (rebase: avoid computing unnecessary\npatch IDs, 2016-07-29), this avoids unnecessary work since not all\nobjects in the hashmap will eventually be compared.\n\nRemove the ten-year-old \"NEEDSWORK\" comment and formally document\nthis intentional design trade-off.\n\nSigned-off-by: Tian Yuchen <cat@malon.dev>\n---\n patch-ids.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex a5683b462c..1fbc88cbec 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -41,7 +41,14 @@ 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+\t/*\n+\t * We drop the 'const' modifier here intentionally.\n+\t *\n+\t * Even though eptr and entry_or_key are const, we want to\n+\t * lazily compute their .patch_id members; see b3dfeebb (rebase:\n+\t * avoid computing unnecessary patch IDs, 2016-07-29). So we cast\n+\t * the constness away with container_of().\n+\t */\n \tstruct diff_options *opt = (void *)cmpfn_data;\n \tstruct patch_id *a, *b;\n \n-- \n2.43.0\n\n"}]}