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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	Git List <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 2/8] builtin/multi-pack-index.c: support --stdin-packs mode
Date: Sun, 12 Sep 2021 17:15:18 +0200	[thread overview]
Message-ID: <87bl4x94ux.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YT1kDVBHtPhxc6Wk@nand.local>

On Sat, Sep 11 2021, Taylor Blau wrote:

> 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.

I think Eric's suggestion is better in this case, maybe all cases.

For what it's worth the alternate WIP approach I was hinting at is some
version of this:
https://lore.kernel.org/git/87bl6kq631.fsf@evledraar.gmail.com/

I might change my mind, but I think ultimately running with move of the
string_list_append_nodup() approach Eric points out is probably the
right thing to do.

I.e. it's the difference between declaring that a string list is "dup'd"
upfront, and whenever we insert into it we make sure that's the case one
way or another. Then when we clear we don't need to pass any options.

It does mean that instead of extra clear functions we need to have
*_nodup() versions of all the utility functions for this to work,
e.g. in trying to convert this now there's no
string_list_insert_nodup(), which would be needed.

Or maybe the whole approch of the string_list API is just a dead-end in
API design, i.e. it shouldn't have any "dup" mode, just nodup, if you
need something dup'd you xstrdup()-it.