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

From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH 2/8] builtin/multi-pack-index.c: support --stdin-packs mode
Date: Sat, 11 Sep 2021 12:25:05 -0400	[thread overview]
Message-ID: <YTzYYfBGqG87O5mN@nand.local> (raw)
In-Reply-To: <87ee9va0ie.fsf@evledraar.gmail.com>

On Sat, Sep 11, 2021 at 12:05:05PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Sep 10 2021, Taylor Blau wrote:
>
> > +	struct strbuf buf = STRBUF_INIT;
> > +	while (strbuf_getline(&buf, stdin) != EOF) {
> > +		string_list_append(to, strbuf_detach(&buf, NULL));
>
> If you strbuf_detach() it...
>
>
> > +		struct string_list packs = STRING_LIST_INIT_NODUP;
>
> ...init the list with NODUP...
>
> > +		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;
