Re: [PATCH] patch-ids: achieve const correctness in patch_id_neq()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2026, 06:26 UTC
Message-ID: <xmqqseaasuph.fsf@gitster.g>
In-Reply-To: <20260308043131.77782-1-a3205153416@gmail.com>

Tian Yuchen <a3205153416@gmail.com> writes:

Show 10 quoted lines

> The implementation of the 'contain_of' macro in 'patch_id_neq()' is:
>
> 	#define container_of(ptr, type, member) \
> 		((type *) ((char *)(ptr) - offsetof(type, member)))
>
> Here, 'type' is passed as a raw type with no const information.
> Consequently, const correctness cannot be guaranteed here, resulting
> in an eight-year-long NEEDSWORK comment.
>
> Use explicit casting (struct object_id *) to ensure const correctness.

The "NEEDSWORK: const correctness?" comment in patch-ids.c:patch_id_neq() is indeed about the fact that this function modifies its arguments `eptr` and `entry_or_key` (via the `patch_id` pointers `a` and `b`) to lazily compute the full patch ID.

I am afraid, however, that this patch may not be moving in the right direction. By marking `a` and `b` as `const struct patch_id *`, you are telling the compiler and the reader that the objects they point to will not be modified. But then you immediately cast that constness away when calling `commit_patch_id()`:

> -	    commit_patch_id(a->commit, opt, &a->patch_id, 0))
> +	    commit_patch_id(a->commit, opt, (struct object_id *)&a->patch_id, 0))

I wonder if this is actually any better than the original code. If an object is marked `const`, it is generally supposed to be immutable. Casting away the constness of a member to pass it to a function that is expected to write its findings into that memory region sounds wrong to me.

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.

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.