On Fri, May 18, 2018 at 2:33 PM, Jeff King [off-list ref] wrote:

On Fri, May 18, 2018 at 11:23:27PM +0200, Martin Ågren wrote:

quoted

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:

quoted

@@ -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?)