Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together

From: Jeff King <peff@peff.net>
Date: Oct 20, 2023, 07:29 UTC
Message-ID: <20231020072941.GD1642714@coredump.intra.peff.net>
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>

On Thu, Oct 19, 2023 at 01:28:38PM -0400, Taylor Blau wrote:

Show 5 quoted lines

> 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