From: Tian Yuchen <a3205153416@gmail.com>
Date: Sun, 08 Mar 2026 14:42:43 GMT
Subject: Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()
Message-ID: <1a1ed5c6-8843-4bd5-9f57-187ef39497c3@gmail.com>
In-Reply-To: <xmqqseaasuph.fsf@gitster.g>

Hi Junio,

On 3/8/26 14:26, Junio C Hamano wrote:

> The fact that `patch_id_neq` receives `const struct hashmap_entry *`
> parameters--which is a requirement of the `hashmap` API--is what is
> clashing with our lazy initialization here. The original code
> handled this by casting to a non-const `struct patch_id *` right at
> the beginning (via `container_of`). While this also "drops" the
> constness, the original is at least more honest about the fact that
> `a` and `b` will be modified.

Oops, This is something I hadn't considered before. I admit I didn't 
think things through carefully enough.

So, we actually find ourselves in this dilemma:

  - The Hashmap API specification mandates that input parameters should 
be const *in principle*.

  - The lazy loading mechanism requires us to write the results into 
memory; otherwise, there will be significant performance loss.

Am I correct?

I find that maintaining the current approach seems the most reasonable 
option. Computing all patch ids before putting objects into the hashmap 
appears to be a move that affects everything else and is not worth the 
effort. On the contrary, slightly breaking the Hashmap API conventions 
seems to be a more *cost-effective* approach...

Or perhaps it would be more reasonable to slightly modify this NEEDSWORK 
flag here?

Will send the next patch shortly.

> If we truly wanted to achieve const-correctness here, we would
> likely need to avoid lazy initialization within the comparison
> function altogether, pre-calculating the full patch ID before it's
> needed for comparison. The NEEDSWORK comment should remain until a
> more fundamental solution is found, or if we should just admit that
> the current lazy evaluation pattern is what we want and document
> that (i.e., add a comment to justify why we strip away the constness
> here). As it stands, this patch doesn't "ensure" const correctness;
> it just masks the violation with an explicit cast at the location
> the pointer is used.

Regards,

Yuchen