Re: [PATCH v2 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

From: Jeff King <peff@peff.net>
Date: Jun 24, 2021, 17:14 UTC
Message-ID: <YNS9Yz+EtVzI+Ui7@coredump.intra.peff.net>
In-Reply-To: <cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com>

On Mon, Jun 21, 2021 at 05:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This v2 addresses an omission noted by Andrei Rybak[1]. I didn't
> factor out the "name" into a variable in 2/3 for ease of reading
> 3/3. That's now done.

This all looks OK to me. I left a few small comments on the patches themselves.

The UNLEAK() thing I suggested for patch 1 does make that patch much smaller and easier to read, but I suspect makes patch 3 harder (i.e., you are reusing the "cleanup" sections there to do the bundle header release. That could _also_ get UNLEAKed, but at some point it becomes more clear to actually clean up after ourselves, and I think patch 3 probably crosses that point). So I'm OK to ignore that.

I would prefer to see the "die()" thing I mentioned there addressed, as well as the ternary thing from patch 3. But neither of them is incorrect as-is; it's just a style/preference thing.

-Peff