Re: [PATCH] [Outreachy] patch-ids: fix const correctness
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 13, 2025, 17:12 UTC
- Message-ID
- <xmqq4is23evz.fsf@gitster.g>
- In-Reply-To
- <20251013165320.201333-1-okhuomonajayi54@gmail.com>
Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:
Show 24 quoted lines
> The `patch_id_neq()` function received a pointer to diff options via
> `cmpfn_data` but cast it to a non-const type. This caused a const
> correctness warning and could potentially allow unintended modification
> of read-only data.
>
> Fix this by casting to `const struct diff_options *` instead, removing
> the outdated NEEDSWORK comment in the process.
>
> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
> ---
> patch-ids.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/patch-ids.c b/patch-ids.c
> index a5683b462c..b6b808332f 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -41,8 +41,8 @@ static int patch_id_neq(const void *cmpfn_data,
> const struct hashmap_entry *entry_or_key,
> const void *keydata UNUSED)
> {
> - /* NEEDSWORK: const correctness? */
> - struct diff_options *opt = (void *)cmpfn_data;
> + Trailing whitespace on this line. Remove the entire line instead.
> + const struct diff_options *opt = (void *)cmpfn_data; > struct patch_id *a, *b; > > a = container_of(eptr, struct patch_id, ent);
I do not think this is correct. Have you even compile-tested this patch?
Later in this same function, opt is passed to commit_patch_id() function (twice), which takes non-const "struct diff_options *" that is given to diffcore_std(). And the last function in this callchain has to modify the structure to record various findings (like "did we see any changes in the diff?"), so it cannot be "const" at all.
I think patch_id_neq() that says cmpfn_data is const is the source of the problem, but that function signature is mandated by the hashmap API. I do not know if we can loosen it there in the hashmap API and if so what the argument would be, but thinking about these things is what the NEEDSWORK comment is about ;-).