Re: [PATCH v3 0/4] fetch/receive: use batched reference updates

> On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:
>
>> > Not an issue with this series at all, but one thing I wondered is if
>> > it makes sense to change the type of strmap_get/strmap_put to deal
>> > with "const void *".  That way, it would not be necessary to cast
>> > away the constness like so:
>> >
>> >>     -+	strmap_put(failed_refs, refname, ref_transaction_error_msg(err));
>> >>     ++	strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
>> >
>> > without harming the other side, namely
>> >
>> >>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com
>> >>      +		if (reported_error)
>> >>      +			cmd->error_string = reported_error;
>> >>      +		else if (strmap_contains(&failed_refs, cmd->ref_name))
>> >>     -+			cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
>> >>     ++			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
>> >
>> > this piece of code.
>> >
>> > It may not make sense, and even if it did, of course, it is totally
>> > outside of this series.
>> >
>> > Thanks.
>>
>> It definitely does, The only other typecast I did find for `strmap_put`
>> was within 'strmap.h'. Nevertheless, I think it makes sense to make that
>> change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.
>
> I'm not sure that is a good idea. Even though strmap does not touch the
> void data pointer itself, it is accessible to the callers, and we do not
> know if they stored const data or not, or how they plan to access it.
>
> If we store a "const void *" pointer and returned that via strmap_get(),
> then there will be callers who need to cast away the constness.
>
> If we store and return a "void *" pointer as we do now, but accept a
> const pointer via strmap_put(), then we're casting away potentially
> important const-ness without the caller even seeing it. I think it's
> safer for the client to do the cast explicitly (since they are the ones
> who know how they plan to use it).
>

> I don't think we can really represent what we want in C's type system.
> But if we wanted a safe(r) interface that didn't involve type-casting,
> we might be able to do something like:
>
>   - the strmap stores an extra flag for "the data pointer is const",
>     which can be set by strmap_init(). (It is tempting to replace
>     strmap_clear()'s free_entries parameter by checking this flag, but
>     the two are not always going to be exactly the same).
>
>   - introduce strmap_get_const() and strmap_put_const() which give and
>     receive const pointers
>
>   - in the const functions, BUG() if the "pointers are const" flag is
>     not set
>
> But it feels gross, and it only gives runtime checking, not
> compile-time. What we really want are generics that can carry the type
> annotation for a particular instantiated map. That is probably pretty
> easy in most modern languages, but not really in C without horrible
> macros. :)
>
> -Peff