Re: [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 20, 2023, 06:59 UTC
Message-ID: <xmqqr0mtcosy.fsf@gitster.g>
In-Reply-To: <878r918ps3.fsf@gmail.froward.int.ebiederm.org>

"Eric W. Biederman" <ebiederm@gmail.com> writes:

Show 5 quoted lines

> As the code is written today index_bulk_checkin only accepts blobs.
> Remove the enum object_type parameter and rename index_bulk_checkin to
> index_blob_bulk_checkin, index_stream to index_blob_stream,
> deflate_to_pack to deflate_blob_to_pack, stream_to_pack to
> stream_blobk_to_pack, to make this explicit.

> Not supporting commits, tags, or trees has no downside as it is not
> currently supported now, and commits, tags, and trees being smaller by
> design do not have the problem that the problem that index_bulk_checkin
> was built to solve.

Exactly. The streaming was primarily to help dealing with huge blobs that cannot be held in-core. Of course other parts (like comparing them) of the system would require to hold them in-core so some things may not work for them, but at least it is a start to be able to _hash_ them to store them in the object store and to give them names.

> What is more this is very desiable from the context of the hash function
> transition.

A bit hard to parse; perhaps want a comma before "this"?

> For blob objects it is straight forward to compute multiple hash
> functions during index_bulk_checkin as the object header and content of
> a blob is the same no matter which hash function is being used to
> compute the oid of a blob.

OK.

Show 6 quoted lines

> For commits, tress, and tags the object header and content that need to
> be hashed ard different for different hashes.  Even worse the object
> header can not be known until the size of the content that needs to be
> hashed is known.  The size of the content that needs to be hashed can
> not be known until a complete pass is made through all of the variable
> length entries of the original object.

"tress" -> "trees".  Also a comma after "worse".

> As far as I can tell this extra pass defeats most of the purpose of
> streaming, and it is much easier to implement with in memory buffers.

The purpose of streaming being the ability to hash and compute the object name without having to hold the entirety of the object, I am not sure the above is a good argument. You can run multiple passes by streaming the same data twice if you needed to, and how much easier the implementation may become if you can assume that you can hold everything in-core, what you cannot fit in-core would not fit in-core, so ...

> So if it is needed to write commits, trees, and tags directly to pack
> files writing a separate function to do the would be needed.

But I am OK with this conclusion. As the way to compute the fallback hashes for different types of objects are very different, compared to a single-hash world where as long as you come up with a serialization you have only a single way to hash and name the object. We would end up having separate helper functions per target type anyway, even if we kept a single entry point function like index_stream(). The single entry point function will only be used to just dispatch to type specific ones, so renaming what we have today and making it clear they are for "blobs" does make sense.