On Wed, Jun 15, 2016 at 2:47 PM, Pranit Bauva <pranit.ba...@gmail.com> wrote: > On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine <sunsh...@sunshineco.com> > wrote: >> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.ba...@gmail.com> >> wrote: >>> Reimplement `bisect_clean_state` shell function in C and add a >>> `bisect-clean-state` subcommand to `git bisect--helper` to call it from >>> git-bisect.sh . >>> [...] >>> Signed-off-by: Pranit Bauva <pranit.ba...@gmail.com> >>> --- >>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c >>> +static int mark_for_removal(const char *refname, const struct object_id >>> *oid, >>> + int flag, void *cb_data) >>> +{ >>> + struct string_list *refs = cb_data; >>> + char *ref = xstrfmt("refs/bisect/%s", refname); >>> + string_list_append(refs, ref); >>> + return 0; >>> +} >>> + >>> +static int bisect_clean_state(void) >>> +{ >>> + int result = 0; >>> + >>> + /* There may be some refs packed during bisection */ >>> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; >>> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) >>> &refs_for_removal); >>> + string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); >>> + result = delete_refs(&refs_for_removal); >>> + string_list_clear(&refs_for_removal, 0); >> >> This is leaking all the strings added to 'refs_for_removal', isn't it? >> Either you need to loop over the items and free the strings manually, >> or (if it's not too ugly), set 'strdup_strings' before invoking >> string_list_clear(). > > I didn't carefully see that in the function string_list_clear() it > only free()'s the memory if strdup_strings is 1. I think changing > strdup_strings to 1 would be an easy way out but it would make the > code very ugly and non-trivial.
I disagree about it making the code "*very* ugly and non-trivial". It is quite trivial. What I meant by "ugly" was that it may be too intimate with the implementation of string_list. However, since the solution is already used in the codebase, it may be acceptable. For instance, in builtin/fetch.c: /* All names were strdup()ed or strndup()ed */ list.strdup_strings = 1; string_list_clear(&list, 0); which is exactly the approach I was suggesting. You'll find the same pattern in builtin/shortlog.c. > On the other hand, I can initialize > the string as STRING_LIST_INIT_DUP which will automatically set > strdup_strings as 1 and then also free the memory of ref at that point > after the string ref was appended to the list. Personally, I will > prefer the latter one. Meh. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html