Re: [PATCH v3 0/3]

From: Jeff King <peff@peff.net>
Date: Jun 30, 2021, 17:34 UTC
Message-ID: <YNyrDxUO1PlGJvCn@coredump.intra.peff.net>
In-Reply-To: <cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com>

On Wed, Jun 30, 2021 at 04:06:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> Refactor the bundle API to use the string_list API instead of its own
> version of a similar API. See [1] for v2.
> 
> Addresses comments by Jeff King about us being too overzelous in
> trying not to leak memory (the 'die_no_repo' is gone), and other
> flow/style comments of his.
> 
> I also added a bundle_header_init() function for use in transport.c,
> and noticed a redundant call to string_list_clear() there.

Thanks, all three look good to me.

As an aside, having to have a separate bundle_header_init() and BUNDLE_HEADER_INIT is annoying (because they both must be kept up to date with each other), but quite common in our code base. I wonder if writing:

  void bundle_header_init(struct bundle_header *header)
  {
	struct bundle_header blank = BUNDLE_HEADER_INIT;
	memcpy(header, &blank, sizeof(*header));
  }

would let a smart enough compiler just init "header" in place without the extra copy (the performance of a single bundle_header almost certainly doesn't matter, but it might for other types).

Just musing. ;)

-Peff