Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jun 15, 2016 at 2:47 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@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.bauva@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.