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
