René Scharfe <l....@web.de> writes: >> I'm not sure it's string-list's fault. Many callers (including this one) >> ... > The two modes (dup/nodup) make string_list code tricky. Not sure > how far we'd get with something simpler (e.g. an array of char pointers), > but having the caller do all string allocations would make the code > easier to analyze.
Yes. It probably would have been more sensible if the API did not have two modes (instead, have the caller pass whatever string to be stored, *and* make the caller responsible for freeing them *if* it passed an allocated string). For the push_refs_with_push() patch you sent, another possible fix would be to make cas_options a nodup kind so that the result of strbuf_detach() does not get an extra strdup to be lost when placed in cas_options. With the current string-list API that would not quite work, because freeing done in _release() is tied to the "dup/nodup" ness of the string list. I think there even is a codepath that initializes a string_list as nodup kind, stuffs string in it giving the ownership, and then flips it into dup kind just before calling _release() only to have it free the strings, or something silly/ugly like that. In any case, the patch looks sensible. Thanks for plugging the leaks.
