On Fri, May 31, 2024 at 03:00:36PM +0200, Patrick Steinhardt wrote: > 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. > > > > Yes, <src, dst> is used as a pair of <key, value> to query one with > > the other (i.e. "where does this one go?" "where does this one come > > from?"). > > > > But we are not talking about const-ness of the member (iow, once you > > point a string with the member, you cannot repoint the pointer to > > another string), but we are talking about const-ness of the string > > that is pointed by the member (iow, not "char const *src" but "const > > char *src"), no? If I ask "I have this src, where does it go?" with > > a refspec element filled with src, the dst member may need to be > > updated to point at the string that is the answer of the query, but > > that still can be done with "const char *src, *dst", can't it? That > > was what I was wondering. > > 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. > > The root problem is that `struct refspec_item` is being abused for > multiple purposes. One of these purposes it to store parsed refspec > items. Another purpose is to actually serve as a query. The requirements > for those two purposes regarding the lifetime of associated strings is > different: > > - When using it for storage purposes the strings should be owned by > the structure itself. Consequently, the lifetime should be managed > by the structure too, namely via `refspec_item_clear()`. > > - When using it for parsing purposes the input string is owned by the > caller, whereas the output string is owned by the callee and then > handed over to the caller. Here we cannot rely on the structure > itself to manage the lifetimes of both strings, at least not in the > current form. > > Now of course, we could amend `struct refspec_item` to grow two > additional fields `src_to_free` and `dst_to_free`. But that only doubles > down on this misdesigned interface. > > The proper solution in my opinion is to detangle those two usecases and > split them out into two independent interfaces. > > > 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. > > Patrick In any case, I have now invested some more time into the individual sites where I was converting to use on-stack strings. In many of the cases we were indeed able to avoid the issue altogether without too much of a hassle. The end result is more pleasing indeed, I'd say. Patrick
Attachment:
signature.asc
Description: PGP signature
