Re: [PATCH] revision: hang on to "freed" argv elements

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 01, 2026 at 05:21:20AM -0400, Jeff King wrote:
> On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:
[snip]
> > > +static void mark_argv_for_free(struct rev_info *revs, const char *str)
> > > +{
> > > +	if (!str)
> > > +		return;
> > > +	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> > > +}
> > 
> > Hm. Doesn't this mean that we take ownership of the string and then
> > eventually try to release it when releasing the vector? I wonder whether
> > this could introduce subtle lifetime issues where the caller passes a
> > non-heap-allocated string.
> 
> Yes, that's exactly the point. We are replacing a call to free() with
> one that passes ownership to a strvec which later frees it. If somebody
> is passing a non-heap string along with free_removed_argv_elements, then
> everything was already broken.

Fair.

> > I don't think it's that bad when seeing where we use these. But I feel
> > like hiding this fact by marking the parameter as `const` is a bit of a
> > weird design choice. I'd much rather prefer we force this onto the
> > callers so that they are aware of this, but I haven't seen the end
> > result of that. So maybe it's just too ugly.
> 
> You can see the effect already in the diff. In the preimage all of the
> callers had to cast away const-ness in order to pass the string to
> free(). We could keep doing that here, but since this function has
> exactly one purpose (to free the string we pass it) it seems like a nice
> syntactic convenience to push the cast in here.

Okay, fair enough.

> Though you may want to look at the "2/1" I sent, which pushes the check
> for free_removed_argv_elements into this function. And then the cast and
> that check are side-by-side.

Makes sense, thanks!

Patrick