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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: "Jeff King" <peff@peff.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Git List" <git@vger.kernel.org>
Subject: Re: [PATCH 2/8] builtin/multi-pack-index.c: support --stdin-packs mode
Date: Tue, 14 Sep 2021 21:55:47 -0400	[thread overview]
Message-ID: <CAPig+cTpdPT6kD9NY2HXgPPvceX=euSGhQXpvKC+WS5-dLxe2A@mail.gmail.com> (raw)
In-Reply-To: <YUE0u+T/CL8D5WV/@nand.local>

On Tue, Sep 14, 2021 at 7:48 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Tue, Sep 14, 2021 at 03:02:38PM -0400, Jeff King wrote:
> > I think the root of the problem here is the non-idiomatic use of
> > strbuf_getline(). The usual thing (and in fact the thing done by the
> > quite-similar code in read_packs_list_from_stdin() in pack-objects.c)
> > is not to detach, because strbuf_getline() will reset the buffer each
> > time. I.e.:
> >
> >   struct string_list to = STRING_LIST_INIT_DUP;
> >   ...
> >   struct strbuf buf = STRBUF
> >   while (strbuf_getline(&buf, stdin) != EOF)
> >       string_list_append(to, &buf);
>
> > That avoids any clever string-list allocation games. The number of heap
> > allocations is about the same (one strbuf and N list items, versus N
> > strbufs and 0 list items). There's a little extra copying (from the
> > strbuf into the list items), but the strings themselves are more
> > efficiently allocated (strbuf may over-allocate, and we lock in
> > that choice forever by handing over the string).
> >
> > Not that efficiency probably matters either way for this spot. I'd do it
> > this way because it's simple and idiomatic for our code base.
>
> More than anything, I'm persuaded by that (though I was quite partial to
> Eric's suggestion, which I found very clever).

For what it's worth, Peff's suggestion seems best of all those
presented thus far.