# [PATCH] packfile: use extra variable to clarify code in use_pack() 5 messages from 2019-03-13 to 2019-03-14. Participants: Ramsay Jones, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/50736 ## Ramsay Jones, 2019-03-13 21:49 Subject: [PATCH] packfile: use extra variable to clarify code in use_pack() Message-ID: URL: https://gitlist.dev/e/a026ce9f-b825-7fc2-2ad5-c0b00bc508b4%40ramsayjones.plus.com ``` From: Jeff King 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, which 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. Signed-off-by: Ramsay Jones --- Hi Junio, As promised, I am forwarding a 'saved' patch from Jeff, which was a by-product of a long-ago discussion regarding commit 5efde212fc ("zlib.c: use size_t for size", 2018-10-14). I have tested this patch on 'pu' (@6fd68134c8) and directly on top of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib'). However, whilst I have been waiting for the tests to finish, I have been looking at the code and concluded that this does not _have_ to be applied on top of commit 5efde212fc. (I haven't done it, but just tweak the context line to read 'unsigned long *left)' rather than 'size_t *left)' and this should apply cleanly to 'master'. Also, it would have _exactly_ the same effect as the current code! ;-) ). So, dunno. ATB, Ramsay Jones packfile.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packfile.c b/packfile.c index b0efe8cb3d..0e59f929c5 100644 --- a/packfile.c +++ b/packfile.c @@ -622,6 +622,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 @@ -683,10 +684,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 ``` ## Ramsay Jones, 2019-03-13 23:28 Subject: Re: [PATCH] packfile: use extra variable to clarify code in use_pack() Message-ID: <027273d3-be3c-ca38-e13f-9387c14221fb@ramsayjones.plus.com> URL: https://gitlist.dev/e/027273d3-be3c-ca38-e13f-9387c14221fb%40ramsayjones.plus.com In-Reply-To: ``` On 13/03/2019 21:49, Ramsay Jones wrote: > From: Jeff King > > 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, which 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. > > Signed-off-by: Ramsay Jones > --- > > Hi Junio, > > As promised, I am forwarding a 'saved' patch from Jeff, which was > a by-product of a long-ago discussion regarding commit 5efde212fc > ("zlib.c: use size_t for size", 2018-10-14). > > I have tested this patch on 'pu' (@6fd68134c8) and directly on top > of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib'). > > However, whilst I have been waiting for the tests to finish, I have > been looking at the code and concluded that this does not _have_ to > be applied on top of commit 5efde212fc. (I haven't done it, but just > tweak the context line to read 'unsigned long *left)' rather than > 'size_t *left)' and this should apply cleanly to 'master'. Also, it > would have _exactly_ the same effect as the current code! ;-) ). I have now done: $ diff 0001-packfile-use-extra-variable-to-clarify-code-in-use_p.patch ttt.patch 28c28 < size_t *left) --- > unsigned long *left) $ ... this and it applies cleanly to 'master', builds and passes tests. Just FYI. ;-) ATB, Ramsay Jones ``` ## Jeff King, 2019-03-14 00:19 Subject: Re: [PATCH] packfile: use extra variable to clarify code in use_pack() Message-ID: <20190314001940.GD31968@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190314001940.GD31968%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Mar 13, 2019 at 09:49:58PM +0000, Ramsay Jones wrote: > From: Jeff King > [...] > Signed-off-by: Ramsay Jones Signed-off-by: Jeff King Naturally. :) > As promised, I am forwarding a 'saved' patch from Jeff, which was > a by-product of a long-ago discussion regarding commit 5efde212fc > ("zlib.c: use size_t for size", 2018-10-14). > > I have tested this patch on 'pu' (@6fd68134c8) and directly on top > of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib'). > > However, whilst I have been waiting for the tests to finish, I have > been looking at the code and concluded that this does not _have_ to > be applied on top of commit 5efde212fc. (I haven't done it, but just > tweak the context line to read 'unsigned long *left)' rather than > 'size_t *left)' and this should apply cleanly to 'master'. Also, it > would have _exactly_ the same effect as the current code! ;-) ). I think it does apply, though the reasoning in the commit message of "this is OK because 'left' is large enough" becomes a lot more hand-wavy. The patch is not making anything _worse_, certainly, but the fact of the matter is that "left" still might not be big enough, if it is not a size_t. -Peff ``` ## Ramsay Jones, 2019-03-14 00:54 Subject: Re: [PATCH] packfile: use extra variable to clarify code in use_pack() Message-ID: URL: https://gitlist.dev/e/aca43222-bd4f-ec4d-9cbf-bc85623d34fc%40ramsayjones.plus.com In-Reply-To: <20190314001940.GD31968@sigill.intra.peff.net> ``` On 14/03/2019 00:19, Jeff King wrote: > On Wed, Mar 13, 2019 at 09:49:58PM +0000, Ramsay Jones wrote: > >> From: Jeff King >> [...] >> Signed-off-by: Ramsay Jones > > Signed-off-by: Jeff King > > Naturally. :) > >> As promised, I am forwarding a 'saved' patch from Jeff, which was >> a by-product of a long-ago discussion regarding commit 5efde212fc >> ("zlib.c: use size_t for size", 2018-10-14). >> >> I have tested this patch on 'pu' (@6fd68134c8) and directly on top >> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib'). >> >> However, whilst I have been waiting for the tests to finish, I have >> been looking at the code and concluded that this does not _have_ to >> be applied on top of commit 5efde212fc. (I haven't done it, but just >> tweak the context line to read 'unsigned long *left)' rather than >> 'size_t *left)' and this should apply cleanly to 'master'. Also, it >> would have _exactly_ the same effect as the current code! ;-) ). > > I think it does apply, though the reasoning in the commit message of > "this is OK because 'left' is large enough" becomes a lot more > hand-wavy. The patch is not making anything _worse_, certainly, but the > fact of the matter is that "left" still might not be big enough, if it > is not a size_t. Yep, the commit message would have to change (it says 'left' is a size_t), but I think the patch is _still_ an improvement on the existing code, even without s/unsigned long *left/size_t *left/. (ie the code is still 'clarified'). :-D Anyway, it was just an idle FYI while waiting. ;-) ATB, Ramsay Jones ``` ## Junio C Hamano, 2019-03-14 05:38 Subject: Re: [PATCH] packfile: use extra variable to clarify code in use_pack() Message-ID: URL: https://gitlist.dev/e/xmqqa7hy9fse.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190314001940.GD31968@sigill.intra.peff.net> ``` Jeff King writes: > I think it does apply, though the reasoning in the commit message of > "this is OK because 'left' is large enough" becomes a lot more > hand-wavy. The patch is not making anything _worse_, certainly, but the > fact of the matter is that "left" still might not be big enough, if it > is not a size_t. True; it does not make much sense without the actual size_t conversion, I would think. ```