Message ID | e561b83f-cf1c-eef8-7651-8519ce105491@ramsayjones.plus.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC/PATCH] packfile: use extra variable to clarify code in use_pack() | expand |
On 12/03/2019 16:55, Ramsay Jones wrote: > From: Jeff King <peff@peff.net> > > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > --- > > Hi Jeff, > > I recently tried (yet again) to tidy up some old branches. When I get > around to doing a 'git gc; git fsck' I always take a quick look at > the 'dangling' commits, just before a 'git gc --prune=now'. > > I had no recollection of this commit, from last October, but a quick > look at the ML archive found this [1] discussion. I obviously thought > it was worth saving this thought of yours. ;-) So, having deleted this > already, I did a quick 'format-patch' to see if anyone thinks it is > worth applying. > > [1] https://public-inbox.org/git/20181013024624.GB15595@sigill.intra.peff.net/#t > 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. Ah, well, sorry for the noise! ATB, Ramsay Jones
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@peff.net> > > > > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> 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. :) > 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. :) -Peff
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@peff.net> >>> >>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > > 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
diff --git a/packfile.c b/packfile.c index 013294aec7..2f81ec9345 100644 --- a/packfile.c +++ b/packfile.c @@ -588,6 +588,7 @@ unsigned char *use_pack(struct packed_git *p, size_t *left) { struct pack_window *win = *w_cursor; + size_t offset_in_window; /* Since packfiles end in a hash of their content and it's * pointless to ask for an offset into the middle of that @@ -649,10 +650,14 @@ unsigned char *use_pack(struct packed_git *p, win->inuse_cnt++; *w_cursor = win; } - offset -= win->offset; + /* + * 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 - xsize_t(offset); - return win->base + offset; + *left = win->len - offset_in_window; + return win->base + offset_in_window; } void unuse_pack(struct pack_window **w_cursor)