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

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

On Sat, Sep 11, 2021 at 12:25:05PM -0400, Taylor Blau wrote:

quoted

quoted

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:

--- 8< ---

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 77488b6b7b..e6cab975e3 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c

@@ -64,7 +64,7 @@ static struct option *add_common_options(struct option *prev)

 static void read_packs_from_stdin(struct string_list *to)
 {
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT_NODUP;

Did you mean to use STRING_LIST_INIT_NODUP on the string-list
declaration?

Yeah, and I thought that I even mentioned it in [1], but I apparently
sent the contents of my buffer before I saved it. So, I did notice it at
the time, but failed to tell anybody about it!

[1]: https://lore.kernel.org/git/YTzZPti%2FasQwZC%2FD@nand.local/ (local)

quoted

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

 		return ret;

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

(Assuming that you meant `s/&buf/buf.buf/`, but otherwise this makes
sense). There is no need to call strbuf_reset() inside the body of the
loop, since strbuf_getline() calls it via strbuf_getwholeline().

  strbuf_release(&buf);

...But we should still be careful to release the memory used by the
strbuf at the end (which is safe to do, since the whole point is that we
copy each buffer linewise into the string_list).

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

Thanks,
Taylor