Patrick Steinhardt <ps@xxxxxx> writes: > On Thu, May 30, 2024 at 12:38:45PM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@xxxxxx> writes: >> >> > Well, we do. Not in `get_fetch_map()`, but in `query_refspecs()`. It >> > does weird stuff where it writes the result into either `src` or `dst` >> > depending on which of these fields is provided by the caller. Which >> > means that one of the fields would typically be a constant, whereas the >> > other one will be allocated. > ... > Well, yes, the pointers can be updated. But that doesn't solve the > underlying problem that the interface is not well-prepared to track > ownership of its inputs and outputs. Oh, absolutely. That is exactly why I wondered why .src and .dst need to be "char *" and not "const char *". If the piece of memory these members point at are never modified via these pointers, marking them as "const char *" would help the compilers help us by catching when we try to break the promise. Sometimes, the "constness of a pointer should help compilers help us avoid modifying a piece of memory we promised not to modify through the pointer" contradicts with the idea of (ab)using the constness to signal ownership, i.e., "const pointers point at something owned by somebody else, but via non-const pointers you always own the memory". I wonder if we can do something to separate these two concerns apart, using a trick similar to what we often use with an extra variable "to_free". Doing so would bloat the refspec_item, but unlike the references themselves, there won't be thousands of them, so it may not be an issue, perhaps? >> And again you are conflating "allocated" with "read-write" here. It >> is often convenient if a variable that points at an allocated string >> is of type "char *" and not "const char *", because you do not cast >> it when calling free(). But if you want to make a structure member >> or a variable that holds an allocated string responsible for >> _owning_ the piece of memory, then you need to consistently have the >> member point at an allocated piece of memory (or NULL), no? What >> this patch does, i.e. prepare an on-stack refspec_str[] array that >> is initialized from a constant string, and have .src member point at >> it, would not make .src freeable. In other words, .src pointing at >> an allocated piece of string "some of the time" alone is not a good >> justification to make it a non-const pointer, I would think. > > That's fair enough. I'm trying to work around a broken interface, but do > not solve the underlying issue.
