Re: [PATCH 2/8] builtin/multi-pack-index.c: support --stdin-packs mode

On Sat, Sep 11, 2021 at 12:05:05PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Fri, Sep 10 2021, Taylor Blau wrote:

quoted

+	struct strbuf buf = STRBUF_INIT;
+	while (strbuf_getline(&buf, stdin) != EOF) {
+		string_list_append(to, strbuf_detach(&buf, NULL));

If you strbuf_detach() it...

quoted

+		struct string_list packs = STRING_LIST_INIT_NODUP;

...init the list with NODUP...

quoted

+		string_list_clear(&packs, 0);

...and call string_list_clear() you'll leak memory, since the
string_list will think that someone else is taking care of this memory.

I had some recent musings about how this API is bad, and I've got local
WIP patches to fix it, but in the meantime you need to:

    packs.strdup_strings = 1;

Before calling string_list_clear(). I.e. we didn't strdup(), but during
free() we pretend that we did, because we did, just not in
string_list_append().

Good catch. It's kind of gross, but the result is:

--- 8< ---

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 77488b6b7b..e6cab975e3 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c

@@ -64,7 +64,7 @@ static struct option *add_common_options(struct option *prev)

 static void read_packs_from_stdin(struct string_list *to)
 {
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT_NODUP;
 	while (strbuf_getline(&buf, stdin) != EOF) {
 		string_list_append(to, strbuf_detach(&buf, NULL));
 	}

@@ -107,6 +107,11 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
 		ret = write_midx_file_only(opts.object_dir, &packs,
 					   opts.preferred_pack, opts.flags);

+		/*
+		 * pretend strings are duplicated to free the memory allocated
+		 * by read_packs_from_stdin()
+		 */
+		packs.strdup_strings = 1;
 		string_list_clear(&packs, 0);

 		return ret;