Re: [PATCH 2/8] builtin/multi-pack-index.c: support --stdin-packs mode
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Sep 15, 2021, 01:55 UTC
- Message-ID
- <CAPig+cTpdPT6kD9NY2HXgPPvceX=euSGhQXpvKC+WS5-dLxe2A@mail.gmail.com>
- In-Reply-To
- <YUE0u+T/CL8D5WV/@nand.local>
On Tue, Sep 14, 2021 at 7:48 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 25 quoted lines
> 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.