From: Philip Oakley <philipoakley@iee.email>
Date: Sun, 20 Oct 2019 11:26:01 GMT
Subject: Re: [PATCH v2 9/9] pack-objects: improve partial packfile reuse
Message-ID: <ac89d8ed-5a57-6297-5fc2-cf562c8da812@iee.email>
In-Reply-To: <20191019232322.GB32408@sigill.intra.peff.net>

On 20/10/2019 00:23, Jeff King wrote:
> On Sat, Oct 19, 2019 at 09:20:11PM +0200, Christian Couder wrote:
>
>>>> +static void write_reused_pack_one(size_t pos, struct hashfile *out,
>>>> +                               struct pack_window **w_curs)
>>>> +{
>>>> +     off_t offset, next, cur;
>>>> +     enum object_type type;
>>>> +     unsigned long size;
>>> Is this a mem_sized size or a counter for less that 4GiB items?
>> What I can see is that `&size` is passed as the last argument to
>> unpack_object_header() below. And unpack_object_header() is defined in
>> packfile.h like this:
>>
>> int unpack_object_header(struct packed_git *, struct pack_window **,
>> off_t *, unsigned long *);
>>
>> since at least 336226c259 (packfile.h: drop extern from function
>> declarations, 2019-04-05)
>>
>> So fixing this, if it needs to be fixed, should probably be part of a
>> separate topic fixing unpack_object_header().
> Yeah, this one definitely should be moved to whatever we used to
> represent object sizes in the future (size_t, or I guess off_t if we
> really want to handle huge objects on 32-bit systems too). But
> definitely it shouldn't happen in this series, and I don't think anybody
> interested in the other topic (converting the integer type for object
> sizes) needs to keep tabs on it. When they convert
> unpack_object_header(), the compiler will complain because of passing
> it as a pointer (the more insidious ones will be where we return an
> unsigned long to represent an object type, and somebody will have to
> look into every caller).
>
Thanks, I'll add that one to my list of size values that I should check 
when rebasing my current series.

If I understand correctly gcc is no longer detecting those size 
conversions, but clang has -Wshorten-64-to-32 but I've still to check if 
it catches some of these conversion issues (esp when int (32) is 
extended to 64 bit size_t, rather than being 64 bit in the first place)

Philip