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:
Show 27 quoted lines
>
> 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;
Taylor Blau <me@ttaylorr.com>