Re: [PATCH v3 5/8] odb: teach read_blob_entry to use size_t

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 2, 2021, 15:10 UTC
Message-ID: <nycvar.QRO.7.76.6.2111021601360.56@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqwnlvfmjr.fsf@gitster.g>

Hi Junio,

On Fri, 29 Oct 2021, Junio C Hamano wrote:

Show 15 quoted lines

> "Matt Cooper via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> > +void *read_blob_entry(const struct cache_entry *ce, size_t *size)
> >  {
> >  	enum object_type type;
> > -	void *blob_data = read_object_file(&ce->oid, &type, size);
> > +	unsigned long ul;
> > +	void *blob_data = read_object_file(&ce->oid, &type, &ul);
> >
> > +	*size = ul;
>
> It is a bit curious place to draw the line; we want to make sure
> that blob_entry can hold huge data, but in this step we do not mind
> read_object_file() is not capable of going full 64-bit?

Indeed that is the case. The consideration here is: how large of a patch series do I want to take this late in the cycle?

Here is the call tree (for full details, look no further than https://github.com/git-for-windows/git/pull/3487#issuecomment-950727616):

    read_object_file()
        repo_read_object_file()
            read_object_file_extended()
                read_object()
                    oid_object_info_extended()
                        do_oid_object_info_extended()
                            loose_object_info()
                                parse_loose_header_extended()
                            packed_object_info()
                                cache_or_unpack_entry()
                                    unpack_entry()
                                        unpack_object_header()
                                           unpack_object_header_buffer()
                                get_size_from_delta()
                                    get_delta_hdr_size()

All three leaves have code that needs to be adjusted to use `size_t` instead of `unsigned long`, and all of the other functions in that call tree need to be adjusted for that. Some of the callers do not even pass an `unsigned long` pointer around, but instead a pointer to `struct object_info` (which, you guessed it, also has an `unsigned long` that should have been a `size_t` from the beginning).

This is too big a change I am willing to work on, let alone accept, this late in the cycle.

Sure, it would fix the scenario where clean/smudge filters are not even involved (read: `git add`ing a 5GB file _directly_). But the potential for bugs to hide!

> I guess I'll see soon enough why by reading later steps.  I can see
> that for the purpose of making write_entry() aware of the size_t,
> this is necessary at the minimum.

I fear that you won't see more about this in the later steps ;-)

Ciao, Dscho