Re: [PATCH v2 02/17] chunk-format: create chunk format write API

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2021, 19:25 UTC
Message-ID: <xmqqim761flz.fsf@gitster.c.googlers.com>
In-Reply-To: <d5097b51-e096-16b3-5b7b-2c7dd1cbe918@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 7 quoted lines

>> But a silent type casting from size_t to uint64_t done silently by
>> assignment bothers me. 
>
> Does this bother you only because its part of the external interface?
> If I understand correctly, uint64_t will always be at least as big
> as size_t, so this doesn't need any protections for overflow or
> anything. Is there something I should be doing before casting?

I am OK to use uint64_t on the caller-facing side, as long as we explicitly declare that uint64_t ought to be large enough for everybody. Struct members and variables that are closer to the on-disk data need to be of sized type to avoid repeating the pain caused by our early "unsigned long ought to be large enough for everybody" attitude, but it is nicer to be working with more abstract types in the layer higher up. And if there is a risk of truncation in either direction, we should be defensive. That's all.

Show 11 quoted lines

>>> +int write_chunkfile(struct chunkfile *cf, void *data)
>>> +{
>>> +	int i;
>>> +	size_t cur_offset = cf->f->offset + cf->f->total;
>> 
>> That ought to be off_t, as it is a seek position inside a file
>> (struct hashfile.total is already off_t).
>
> I can use off_t for the other offsets in this computation, but
> cur_offset will be used in hashwrite_be64(), so maybe it is best
> to use uint64_t here?

As I discovered in the later parts, I think off_t makes less sense than size_t in the context of this design, so size_t is fine, as long as we keep the "users of chunkfile API work on a mmapped region of contiguous memory" (which I think is OK). uint64_t is also fine, as long as this one is an internal implementation detail; iow, callers of the API do not have to be responsible for casting their more abstract and platform neutral types down to these fixed-sized types even if we choose to use uint64_t here.

Show 6 quoted lines

> The current chunk format API makes the same assumption (ToC comes
> first) but could be adjusted later to let this part of the method
> dynamically compute the chunk sizes and fill a ToC at the end. The
> way to modify this API would be to add a 'flags' parameter.
>
> So far, this has not been necessary, but might be in the future.

Yup, and I am happy with the current design for now. Thanks for clarifying the thinking behind it.