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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 20, 2016, 08:29 UTC
Message-ID: <CAPig+cTACku01im72Cd7LxYopwwFaXtKYXkxi53GMrTRyZpFGQ@mail.gmail.com>
In-Reply-To: <20160220081010.GA16722@sigill.intra.peff.net>

On Sat, Feb 20, 2016 at 3:10 AM, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> 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.