On Thu, Oct 19, 2023 at 01:28:38PM -0400, Taylor Blau wrote: > This is a new round that is significantly simplified thanks to > another very helpful suggestion[1] from Junio. By factoring out a common > "deflate object to pack" that takes an abstract bulk_checkin_source as a > parameter, all of the earlier refactorings can be dropped since we > retain only a single caller instead of multiple. FWIW, I gave this a read-over and did not see anything wrong (on the contrary, I think the new abstractions make it quite easy to follow). Two thoughts that occurred to me while reading it (but neither actionable for your series): - Having not worked with the bulk-checkin code much before, I thought it only put in one blob per pack, and thus you'd have to teach it to handle multiple objects, too. But fortunately I was wrong, and it handles this case already. (I mention this mainly because we had talked about it off-list a few weeks ago, and some of my confusion may have seeped into my comments then). - I did briefly wonder if we need this SOURCE abstraction at all. The file source assumes we can seek() and read(), so it must be a regular file. We could probably just mmap() it and treat it as in-core, too (this is what index_fd() and index_path() do under the hood!). But it would probably be a disservice to any platforms that do not support mmap, as the fallback is to read into a full buffer (and the whole point of the bulk checkin code is to avoid that). -Peff