On Fri, May 18, 2018 at 2:33 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxx/
). 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?)
