Re: [PATCH 06/21] convert manual allocations to argv_array

[Jeff King <peff@peff.net>]

On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:

> On Sat, Feb 20, 2016 at 3:10 AM, Jeff King <peff@peff.net> wrote:
> > On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
> >> > +               /* argv strings are now owned by pathspec */
> >> > +               paths.argc = 0;
> >> > +               argv_array_clear(&paths);
> >>
> >> This overly intimate knowledge of the internal implementation of
> >> argv_array_clear() is rather ugly.
> >
> > Yep, I agree. Suggestions?
> >
> > We can just leak the array of "char *". This function is called only
> > once per program invocation, and that's unlikely to change.
> >
> > I guess we can make an argv_array_detach_strings() function. Or maybe
> > even just argv_array_detach() would be less gross, and then this
> > function could manually free the array but not the strings themselves.
> 
> The latter is what I was thinking, and I agree that
> argv_array_detach() would be less gross than
> argv_array_detach_strings(), however, it also feels a bit wrong since
> this sort of ownership transfer is kind of out of scope for
> argv_array.
> 
> I wonder if a simple "dup'ing" string_list would be more suitable for
> this case. You'd have to append the NULL item manually with
> string_list_append_nodup(), and string_list_clear() would then be the
> correct way to dispose of the list without intimate knowledge of its
> implementation and no need for an API extension.

A string_list doesn't just store pointers; it's a struct with a util
field. So you can't pass it to things expecting a "const char **".

I think argv_array_detach() is the least-bad thing here. It matches
strbuf_detach() to say "you now own the storage" (as opposed to just
peeking at argv.argv, which we should do only in a read-only way).

-Peff