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

From: Jeff King <peff@peff.net>
Date: May 22, 2025, 06:00 UTC
Message-ID: <20250522060044.GB1135327@coredump.intra.peff.net>
In-Reply-To: <CAOLa=ZTyE9myZqy8UHe=YeZ-diy0Q1UDTBTB3d1XGC09XvshAQ@mail.gmail.com>

On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:

Show 27 quoted lines

> > 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