[RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

[RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

[Ramsay Jones]

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

Thanks!

ATB,
Ramsay Jones


 packfile.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

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)
-- 
2.21.0

Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

[Ramsay Jones]

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

Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

[Jeff King]

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

Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

[Ramsay Jones]

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