Re: [PATCH v3 3/3] unpack_trees_options: free messages when done
- From
Elijah Newren <newren@gmail.com>
- Date
- May 18, 2018, 22:30 UTC
- Message-ID
- <CABPp-BFdKFNLHxqt-rbSVPx_cXVG3iyad42qYFbWvP9_2fW2gQ@mail.gmail.com>
- In-Reply-To
- <20180518213333.GB21797@sigill.intra.peff.net>
On Fri, May 18, 2018 at 2:33 PM, Jeff King <peff@peff.net> wrote:
Show 42 quoted lines
> On Fri, May 18, 2018 at 11:23:27PM +0200, Martin Ågren wrote:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 79fd97074e..60293ff536 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -103,6 +103,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>> const char **msgs = opts->msgs;
>> const char *msg;
>>
>> + opts->msgs_to_free.strdup_strings = 0;
>> +
>> [...]
>> +void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
>> +{
>> + opts->msgs_to_free.strdup_strings = 1;
>> + string_list_clear(&opts->msgs_to_free, 0);
>
> I like this string_list approach much better, but it's too bad we have
> to go through these contortions with the strdup flag to get the memory
> ownership right.
>
> If we had a string_list_appendf(), then we could just leave that flag
> alone and this:
>
>> @@ -118,8 +120,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>> ? _("Your local changes to the following files would be overwritten by %s:\n%%s"
>> "Please commit your changes or stash them before you %s.")
>> : _("Your local changes to the following files would be overwritten by %s:\n%%s");
>> - msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
>> - xstrfmt(msg, cmd, cmd);
>> + msg = xstrfmt(msg, cmd, cmd);
>> + msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] = msg;
>> + string_list_append(&opts->msgs_to_free, msg);
>
> would become:
>
> msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOUPTODATE_FILE] =
> string_list_appendf(&opts->msgs_to_free, msg, cmd, cmd)->string;
>
> I don't know if that's worth it or not (I suspect that there are other
> places where appendf would be handy, but I didn't poke around).The strdup_strings=1 immediately before calling string_list_clear() has been used in one other place in merge-recursive.c, and tripped up the reviewer requiring a big code comment to explain it. (See the very end of https://public-inbox.org/git/CABPp-BGh7QTTfu3kgH4KO5DrrXiQjtrNhx_uaQsB6fHXT+9hLQ@mail.gmail.com/ ). So there's already one other place in merge-recursive.c that might benefit from such a change.
A quick search shows about half a dozen other sites throughout the code that are doing something similar:
$ git grep -3 strdup_strings | grep -B 1 string_list_clear bisect.c: refs_for_removal.strdup_strings = 1; bisect.c- string_list_clear(&refs_for_removal, 0); -- builtin/shortlog.c: onelines->strdup_strings = 1; builtin/shortlog.c- string_list_clear(onelines, 0); -- builtin/shortlog.c: log->list.strdup_strings = 1; builtin/shortlog.c- string_list_clear(&log->list, 1); -- mailmap.c: me->namemap.strdup_strings = 1; mailmap.c- string_list_clear_func(&me->namemap, free_mailmap_info); -- mailmap.c: map->strdup_strings = 1; mailmap.c- string_list_clear_func(map, free_mailmap_entry); -- merge-recursive.c: entry->possible_new_dirs.strdup_strings = 1; merge-recursive.c- string_list_clear(&entry->possible_new_dirs, 1); -- revision.c: revs->notes_opt.extra_notes_refs.strdup_strings = 1; revision.c- string_list_clear(&revs->notes_opt.extra_notes_refs, 0);
Maybe someone wants to tackle that as a separate patch series? (Maybe we make it a micro-project for future GSoC'ers?)