Re: [PATCH] am: don't pass strvec to apply_parse_options()

From: Jeff King <peff@peff.net>
Date: Dec 17, 2022, 21:53 UTC
Message-ID: <Y546bgdvYIkfwTlv@coredump.intra.peff.net>
In-Reply-To: <91c5120d-bbd4-8aa5-a205-d1f5387a7f19@web.de>

On Sat, Dec 17, 2022 at 05:07:12PM +0100, René Scharfe wrote:

Show 15 quoted lines

> > If we are just re-ordering argv, though, it feels like this could still
> > work with a strvec. Right now a strvec with "nr" items will free items 0
> > through nr-1, assuming that v[nr] is its NULL invariant. But it could
> > free v[nr], too, in case the NULL was swapped into an earlier position.
> >
> > It's a little weird already, because that swap has violated the
> > invariant, so trying to strvec_push() onto it would cause confusing
> > results. But if the general use case is to pass the strvec to
> > parse_options(), get reordered, and then clear() it, it should work. If
> > you wanted to get really fancy, push() et al could double-check the
> > invariant and BUG().
> 
> Yes, parse_options() and strvec are not fitting perfectly.  Changing the
> former to reorder the elements and keeping them all would require
> checking that all callers use the return value.  Feels like a lot of work.

I think we're already munging the strvec arrays in the option parser, though. I'm just suggesting that parse_options() swap arguments to the end instead of overwriting a NULL (actually, I'm not even sure it doesn't do that already), and strvec_clear() checking the final element. The end state is not necessarily safe, but it's no worse than what we have today.

That said...

Show 7 quoted lines

> A variant that takes a strvec and removes and frees parsed items from it
> would be clean, but would require refactoring indirect callers like
> apply_parse_options() users.  Busywork.
> 
> Making a shallow copy to give to parse_options() in callers that currently
> pass a strvec directly or indirectly seems like the simplest solution to
> me for now.

Yes, I thought your original patch actually got to the root of the problem. strvec owns the array and its elements, and parse-options wants to munge the array itself (but not the elements). Making a shallow copy is eliminates the conflict over ownership.

-Peff