On Fri, Oct 12, 2018 at 10:38:45PM -0400, Jeff King wrote:
> So right now let's imagine that off_t is 64-bit, and "unsigned long" is
> 32-bit (e.g., 32-bit system, or an IL32P64 model like Windows). We'll
> repeatedly ask use_pack() for a window, and it will tell us how many
> bytes we have in "avail". So even as a 32-bit value, that just means
> we'll process chunks smaller than 4GB, and this is correct (or at least
> this part of it -- hold on). But we can still process the whole "len"
> given by the off_t eventually.
So this "hold on" was because I thought I had found another bug in
use_pack(), but I actually think it's OK.
In use_pack(), we do this:
if (left)
*left = win->len - xsize_t(offset);
where win->len is a size_t. Before this patch, "left" is a pointer to
unsigned long. So that has the usual broken-on-Windows mismatch. This
patch makes it a size_t, which is good.
But what's up with that xsize_t(offset)? We'll complain about truncation
_before_ we do any offset subtraction. So at first glance, I thought
this meant we were broken for larger-than-4GB packs on 32-bit systems
when trying to read past the 4GB mark.
But no, right before that we have this line:
offset -= win->offset;
So offset is in fact no longer its original meaning of "offset into the
packfile", but is now an offset of the specific request into the window
we found.
So I think it's correct, but it sure confused me. I wonder if another
variable might help, like:
size_t offset_in_window;
...
/*
* We know this difference will fit in a size_t, because our mmap
* window by * definition can be no larger than a size_t.
*/
offset_in_window = xsize_t(offset - win->offset);
if (left)
*left = win->len - offset_in_window;
return win->base + offset_in_window;
I dunno. Maybe it is overkill.
-Peff
