[PATCH] [Outreachy] patch-ids: fix const correctness

[PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

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;
+	
+	const struct diff_options *opt = (void *)cmpfn_data;
 	struct patch_id *a, *b;
 
 	a = container_of(eptr, struct patch_id, ent);
-- 
2.43.0

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Junio C Hamano]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> 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 ;-).

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Thanks, that explains it.

I didn’t compile test before sending my mistake. I see now that opt is
passed into commit_patch_id() (and friends) and those functions modify
the diff_options structure, so making opt const is incorrect. The real
issue is the mismatch introduced by the hashmap API declaring
cmpfn_data as const void *, which is what the NEEDSWORK comment was
flagging.

I’ll revert my local change, run a build and tests, and then think
about safer alternatives (or leave the NEEDSWORK comment in place if
changing the hashmap API isn’t appropriate).
Thanks for the clarification.

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Junio C Hamano]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> I’ll revert my local change, run a build and tests, and then think
> about safer alternatives (or leave the NEEDSWORK comment in place if
> changing the hashmap API isn’t appropriate).
> Thanks for the clarification.

If you can convince readers that changing the hashmap API is not
appropriate, then I would think that would make a great explanation
for a commit that removes the needswork comment without doing
anything else.  "Thinking about const correctness issues around this
code is no longer needed.  The hashmap API is right to insist that
the extra data pointer must be const because ....  Which makes
casting constness away when assigning it to opt, which is what the
code is, is indeed the only reasonable thing to do, and there is no
more change necessary around here."  Of course, such a commit log
message must fill in the "because ..." part with a convincing
argument ;-).

Thanks.

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Hi Junio,

Thanks for explaining! I get it now the NEEDSWORK comment isn’t needed
since the hashmap API is supposed to have cmpfn_data as const. I’ve
removed the comment and didn’t change anything else

Cheers

On Mon, Oct 13, 2025 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:
>
> > I’ll revert my local change, run a build and tests, and then think
> > about safer alternatives (or leave the NEEDSWORK comment in place if
> > changing the hashmap API isn’t appropriate).
> > Thanks for the clarification.
>
> If you can convince readers that changing the hashmap API is not
> appropriate, then I would think that would make a great explanation
> for a commit that removes the needswork comment without doing
> anything else.  "Thinking about const correctness issues around this
> code is no longer needed.  The hashmap API is right to insist that
> the extra data pointer must be const because ....  Which makes
> casting constness away when assigning it to opt, which is what the
> code is, is indeed the only reasonable thing to do, and there is no
> more change necessary around here."  Of course, such a commit log
> message must fill in the "because ..." part with a convincing
> argument ;-).
>
> Thanks.
>
>

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Junio C Hamano]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> Thanks for explaining! I get it now the NEEDSWORK comment isn’t needed
> since the hashmap API is supposed to have cmpfn_data as const. I’ve
> removed the comment and didn’t change anything else

The NEEDSWORK comment is about going even further, starting from
question if hashmap should really be using "const" in the first
place, to sort things out among all the components involved
(including other users of the hashmap API).

A commit that does not do the necessary study and just removes the
needswork comment is simply irresponsible, no?

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Got it, that helps. I'll take a closer look at how const is handled
across the hashmap API before removing the NEEDSWORK comment. Thanks
for the guidance!

By the way, I also sent another patch about clarifying the SHA1 usage
for patch IDs. Would you mind taking a look when you have a moment?


On Mon, Oct 13, 2025 at 7:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:
>
> > Thanks for explaining! I get it now the NEEDSWORK comment isn’t needed
> > since the hashmap API is supposed to have cmpfn_data as const. I’ve
> > removed the comment and didn’t change anything else
>
> The NEEDSWORK comment is about going even further, starting from
> question if hashmap should really be using "const" in the first
> place, to sort things out among all the components involved
> (including other users of the hashmap API).
>
> A commit that does not do the necessary study and just removes the
> needswork comment is simply irresponsible, no?
>

[PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Fix const correctness warning in patch_id_neq() in patch-ids.c.
---
 patch-ids.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/patch-ids.c b/patch-ids.c
index a5683b462c..4a72c2cbe6 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -42,7 +42,7 @@ static int patch_id_neq(const void *cmpfn_data,
 			const void *keydata UNUSED)
 {
 	/* NEEDSWORK: const correctness? */
-	struct diff_options *opt = (void *)cmpfn_data;
+	const struct diff_options *opt = (void *)cmpfn_data;
 	struct patch_id *a, *b;
 
 	a = container_of(eptr, struct patch_id, ent);
-- 
2.43.0

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Kristoffer Haugsbakk]

On Thu, Oct 9, 2025, at 23:44, Okhuomon Ajayi wrote:
> Fix const correctness warning in patch_id_neq() in patch-ids.c.
> ---
>  patch-ids.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/patch-ids.c b/patch-ids.c
> index a5683b462c..4a72c2cbe6 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -42,7 +42,7 @@ static int patch_id_neq(const void *cmpfn_data,
>  			const void *keydata UNUSED)
>  {
>  	/* NEEDSWORK: const correctness? */
> -	struct diff_options *opt = (void *)cmpfn_data;
> +	const struct diff_options *opt = (void *)cmpfn_data;
>  	struct patch_id *a, *b;
> 
>  	a = container_of(eptr, struct patch_id, ent);
> -- 
> 2.43.0

Can’t the comment be removed now, though? If the cmit msg. says “Fix”.

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Thu, Oct 9, 2025, at 23:44, Okhuomon Ajayi wrote:
>> Fix const correctness warning in patch_id_neq() in patch-ids.c.
>> ---
>>  patch-ids.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index a5683b462c..4a72c2cbe6 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -42,7 +42,7 @@ static int patch_id_neq(const void *cmpfn_data,
>>  			const void *keydata UNUSED)
>>  {
>>  	/* NEEDSWORK: const correctness? */
>> -	struct diff_options *opt = (void *)cmpfn_data;
>> +	const struct diff_options *opt = (void *)cmpfn_data;
>>  	struct patch_id *a, *b;
>> 
>>  	a = container_of(eptr, struct patch_id, ent);
>> -- 
>> 2.43.0
>
> Can’t the comment be removed now, though? If the cmit msg. says “Fix”.

And the proposed commit log message should describe what the problem
is, why it matters, and how the updated code is better than the
current state of the code.

[PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Fix const correctness warning in patch_id_neq() in patch-ids.c.

Changes in v2:
- Removed NEEDSWORK comment
---
 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;
+	
+	const struct diff_options *opt = (void *)cmpfn_data;
 	struct patch_id *a, *b;
 
 	a = container_of(eptr, struct patch_id, ent);
-- 
2.43.0

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Agatha Isabelle]

Hello!

Nice patch!

One detail I would add, though, I think the correct way to submit a v2
patch is to prefix it with the `[PATCH v2]` prefix.

See:
https://git-scm.com/docs/SubmittingPatches
https://git-scm.com/docs/MyFirstContribution

On Thu, Oct 09, 2025 at 10:57:20PM +0100, Okhuomon Ajayi wrote:
> Fix const correctness warning in patch_id_neq() in patch-ids.c.
> 

Before the changes, I think there must be the `Signed-off-by:` line.

> Changes in v2:
> - Removed NEEDSWORK comment

And this part of the message, the changelog, I think it's supposed to be
in the email message but not as part of the commit log.

> ---
>  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;
> +	
> +	const struct diff_options *opt = (void *)cmpfn_data;
>  	struct patch_id *a, *b;
>  
>  	a = container_of(eptr, struct patch_id, ent);
> -- 
> 2.43.0
> 

I hope it was helpful.

Best,
Ágatha Isabelle

Re: [PATCH] [Outreachy] patch-ids: fix const correctness

[Okhuomon Ajayi]

Thank you Agatha

On Fri, Oct 10, 2025 at 12:49 AM Agatha Isabelle <code@agatha.dev> wrote:
>
> Hello!
>
> Nice patch!
>
> One detail I would add, though, I think the correct way to submit a v2
> patch is to prefix it with the `[PATCH v2]` prefix.
>
> See:
> https://git-scm.com/docs/SubmittingPatches
> https://git-scm.com/docs/MyFirstContribution
>
> On Thu, Oct 09, 2025 at 10:57:20PM +0100, Okhuomon Ajayi wrote:
> > Fix const correctness warning in patch_id_neq() in patch-ids.c.
> >
>
> Before the changes, I think there must be the `Signed-off-by:` line.
>
> > Changes in v2:
> > - Removed NEEDSWORK comment
>
> And this part of the message, the changelog, I think it's supposed to be
> in the email message but not as part of the commit log.
>
> > ---
> >  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;
> > +
> > +     const struct diff_options *opt = (void *)cmpfn_data;
> >       struct patch_id *a, *b;
> >
> >       a = container_of(eptr, struct patch_id, ent);
> > --
> > 2.43.0
> >
>
> I hope it was helpful.
>
> Best,
> Ágatha Isabelle