Re: [PATCH v2] patch-ids: document intentional const-casting in patch_id_neq()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2026, 00:26 UTC
Message-ID: <xmqqh5qp97bd.fsf@gitster.g>
In-Reply-To: <20260308150203.86299-1-cat@malon.dev>

Tian Yuchen <cat@malon.dev> writes:

Show 9 quoted lines

> +	/*
> +	 * We drop the 'const' modifier here intentionally.
> +	 *
> +	 * The hashmap API requires us to treat the entries as const.
> +	 * However, to avoid performance regression, we lazily compute
> +	 * the patch IDs inside this comparison function. This fundamentally
> +	 * requires us to mutate the 'struct patch_id'. Therefore, we use
> +	 * container_of() to cast away the constness from the hashmap_entry.
> +	 */

Is that a "performance regression", I have to wonder? We would regress relative to what by doing what?

Is the lazy evaluation avoiding unnecessary work?

If we are going to pass _all_ the objects in the hashmap to this comparator function eventually _anyway_, then the total cost of computing patch IDs to all of them in the hashmap would not change with or without lazy computation, but if we are currently getting away without having to compute for all, but only computing for the ones we pass to this function, then lazy evaluation is clearly a win. I do not offhand know which of the above two is the case, but we need to know that before we can touch the NEEDSWORK comment, I think.

The lazy computation comes from b3dfeebb (rebase: avoid computing unnecessary patch IDs, 2016-07-29), even though the "const correctness?" comment is a bit newer than that.

So it seems that we indeed are avoiding unnecessary work without this patch. We'd encounter "performance regression" only if we stop avoiding unnecessary work, so I am afraid that the phrasing used in the patch is somewhat confusing.

    Even though eptr and entry_or_key are const, we want to lazily
    compute their .patch_id members; see b3dfeebb (rebase: avoid
    computing unnecessary patch IDs, 2016-07-29), so cast the
    constness away with container_of().

or something, perhaps?

>  	struct diff_options *opt = (void *)cmpfn_data;
>  	struct patch_id *a, *b;