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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Sep 11, 2021 at 10:08:44PM -0400, Eric Sunshine wrote:
> On Sat, Sep 11, 2021 at 12:25 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Sat, Sep 11, 2021 at 12:05:05PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > > 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:
> >
> >  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);
>
> An alternative is to do this:
>
>     struct strbuf buf = STRBUF_INIT;
>     ...
>     while (...) {
>         string_list_append_nodup(to, strbuf_detach(&buf, NULL));
>         ...
>     }
>     ...
>     string_list_clear(&packs, 0);
>
> That is, use string_list_append_nodup(), which is documented as
> existing precisely for this sort of use-case:
>
>     Like string_list_append(), except string is never copied.  When
>     list->strdup_strings is set, this function can be used to hand
>     ownership of a malloc()ed string to list without making an extra
>     copy.
>
> (I mention this only for completeness but don't care strongly which
> approach is used.)

Thanks for a great suggestion; I do prefer using the existing APIs where
possible, and it seems like string_list_append_nodup() was designed
exactly for this case.

Thanks,
Taylor