Re: [PATCH 02/19] global: assign non-const strings as required

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3945 bytes --]

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@pks.im> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]