Re: [PATCH v2 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`

[Taylor Blau <me@ttaylorr.com>]

On Tue, Oct 17, 2023 at 07:18:04PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >  bulk-checkin.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  bulk-checkin.h |   4 ++
> >  2 files changed, 122 insertions(+)
>
> Unlike the previous four, which were very clear refactoring to
> create reusable helper functions, this step leaves a bad aftertaste
> after reading twice, and I think what is disturbing is that many new
> lines are pretty much literally copied from stream_blob_to_pack().
>
> I wonder if we can introduce an "input" source abstraction, that
> replaces "fd" and "size" (and "path" for error reporting) parameters
> to the stream_blob_to_pack(), so that the bulk of the implementation
> of stream_blob_to_pack() can call its .read() method to read bytes
> up to "size" from such an abstracted interface?  That would be a
> good sized first half of this change.  Then in the second half, you
> can add another "input" source that works with in-core "buf" and
> "size", whose .read() method will merely be a memcpy().

Thanks, I like this idea. I had initially avoided it in the first couple
of rounds, because the abstraction felt clunky and involved an
unnecessary extra memcpy().

But having applied your suggestion here, I think that the price is well
worth the result, which is that `stream_blob_to_pack()` does not have to
be implemented twice with very subtle differences.

Thanks again for the suggestion, I'm really pleased with how it came
out. Reroll coming shortly...

Thanks,
Taylor