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

On Tue, Sep 14, 2021 at 7:48 PM Taylor Blau [off-list ref] wrote:

On Tue, Sep 14, 2021 at 03:02:38PM -0400, Jeff King wrote:

quoted

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);

quoted

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.