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@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html