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
Attachment:
signature.asc
Description: PGP signature
