On 12/03/2019 20:26, Jeff King wrote:
> On Tue, Mar 12, 2019 at 05:39:13PM +0000, Ramsay Jones wrote:
>
>> On 12/03/2019 16:55, Ramsay Jones wrote:
>>> From: Jeff King <peff@xxxxxxxx>
>>>
>>> Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
>
> Could definitely use a commit message. I think it's something like:
>
> We use the "offset" variable for two purposes. It's the offset into
> the packfile that the caller provides us (which is rightly an off_t,
> since we might have a packfile much larger than memory). But later we
> also use it as the offset within a given mmap'd window, and that
> window cannot be larger than a size_t.
>
> For the second use, the fact that we have an off_t leads to some
> confusion when we assign it to the "left" variable, is a size_t. It is
> in fact correct (because our earlier "offset -= win->offset" means we
> must be within the pack window), but using a separate variable of the
> right type makes that much more obvious.
>
> You'll note that I snuck in the assumption that "left" is a size_t,
> which as you noted is not quite valid yet. :)
Looks good to me! :-D
>> Heh, of course I should have tried applying on top of today's
>> codebase before sending it out! :(
>>
>> Having just done so, it quickly showed that this patch assumes
>> that the 'left' parameter to use_pack() has been changed from
>> an 'unsigned long *' to an 'size_t *' as part of the series
>> that was being discussed in the above link.
>
> Yep. Until then, I do not think there is much point (and in fact I'd
> suspect this code behaves incorrectly on Windows, where "unsigned long"
> is too short; hopefully they clamp pack windows to 4GB by default
> there, which would work around it).
>
> But I would be very happy if you wanted to resurrect the "left" patch
> and then do this on top. :)
It actually applies on top of the 'pu' branch, because the 'left'
patch is commit 5efde212fc ("zlib.c: use size_t for size", 2018-10-18).
If you recall, this was going to be just the first step in a series
of patches to replace 'unsigned long' as the type used for various
'size' or 'length' variables.
So, if I add the above commit message, it could apply on top of the
current 'mk/use-size-t-in-zlib' branch.
I may not get to that tonight (busy with something else), but I can
hopefully do that tomorrow.
ATB,
Ramsay Jones
