{"thread":{"id":"50736","subject":"[PATCH] packfile: use extra variable to clarify code in use_pack()","startedAt":"2019-03-13T21:50:04Z","lastAt":"2019-03-14T05:38:35Z","messageCount":5,"participants":["Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371405","messageId":"a026ce9f-b825-7fc2-2ad5-c0b00bc508b4@ramsayjones.plus.com","threadId":"50736","inReplyTo":null,"subject":"[PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-13T21:49:58Z","receivedAt":"2019-03-13T21:50:04Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"From: Jeff King \n\nWe use the \"offset\" variable for two purposes. It's the offset into\nthe packfile that the caller provides us (which is rightly an off_t,\nsince we might have a packfile much larger than memory). But later we\nalso use it as the offset within a given mmap'd window, and that\nwindow cannot be larger than a size_t.\n\nFor the second use, the fact that we have an off_t leads to some\nconfusion when we assign it to the \"left\" variable, which is a size_t.\nIt is in fact correct (because our earlier \"offset -= win->offset\" means\nwe must be within the pack window), but using a separate variable of the\nright type makes that much more obvious.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nAs promised, I am forwarding a 'saved' patch from Jeff, which was\na by-product of a long-ago discussion regarding commit 5efde212fc\n(\"zlib.c: use size_t for size\", 2018-10-14).\n\nI have tested this patch on 'pu' (@6fd68134c8) and directly on top\nof commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').\n\nHowever, whilst I have been waiting for the tests to finish, I have\nbeen looking at the code and concluded that this does not _have_ to\nbe applied on top of commit 5efde212fc. (I haven't done it, but just\ntweak the context line to read 'unsigned long *left)' rather than\n'size_t *left)' and this should apply cleanly to 'master'. Also, it\nwould have _exactly_ the same effect as the current code! ;-) ).\n\nSo, dunno.\n\nATB,\nRamsay Jones\n\n\n packfile.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex b0efe8cb3d..0e59f929c5 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -622,6 +622,7 @@ unsigned char *use_pack(struct packed_git *p,\n \t\tsize_t *left)\n {\n \tstruct pack_window *win = *w_cursor;\n+\tsize_t offset_in_window;\n \n \t/* Since packfiles end in a hash of their content and it's\n \t * pointless to ask for an offset into the middle of that\n@@ -683,10 +684,14 @@ unsigned char *use_pack(struct packed_git *p,\n \t\twin->inuse_cnt++;\n \t\t*w_cursor = win;\n \t}\n-\toffset -= win->offset;\n+\t/*\n+\t * We know this difference will fit in a size_t, because our mmap\n+\t * window by definition can be no larger than a size_t.\n+\t */\n+\toffset_in_window = xsize_t(offset - win->offset);\n \tif (left)\n-\t\t*left = win->len - xsize_t(offset);\n-\treturn win->base + offset;\n+\t\t*left = win->len - offset_in_window;\n+\treturn win->base + offset_in_window;\n }\n \n void unuse_pack(struct pack_window **w_cursor)\n-- \n2.21.0\n"},{"id":"371416","messageId":"027273d3-be3c-ca38-e13f-9387c14221fb@ramsayjones.plus.com","threadId":"50736","inReplyTo":"a026ce9f-b825-7fc2-2ad5-c0b00bc508b4@ramsayjones.plus.com","subject":"Re: [PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-13T23:28:02Z","receivedAt":"2019-03-13T23:28:07Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/03/2019 21:49, Ramsay Jones wrote:\n> From: Jeff King \n> \n> We use the \"offset\" variable for two purposes. It's the offset into\n> the packfile that the caller provides us (which is rightly an off_t,\n> since we might have a packfile much larger than memory). But later we\n> also use it as the offset within a given mmap'd window, and that\n> window cannot be larger than a size_t.\n> \n> For the second use, the fact that we have an off_t leads to some\n> confusion when we assign it to the \"left\" variable, which is a size_t.\n> It is in fact correct (because our earlier \"offset -= win->offset\" means\n> we must be within the pack window), but using a separate variable of the\n> right type makes that much more obvious.\n> \n> Signed-off-by: Ramsay Jones \n> ---\n> \n> Hi Junio,\n> \n> As promised, I am forwarding a 'saved' patch from Jeff, which was\n> a by-product of a long-ago discussion regarding commit 5efde212fc\n> (\"zlib.c: use size_t for size\", 2018-10-14).\n> \n> I have tested this patch on 'pu' (@6fd68134c8) and directly on top\n> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').\n> \n> However, whilst I have been waiting for the tests to finish, I have\n> been looking at the code and concluded that this does not _have_ to\n> be applied on top of commit 5efde212fc. (I haven't done it, but just\n> tweak the context line to read 'unsigned long *left)' rather than\n> 'size_t *left)' and this should apply cleanly to 'master'. Also, it\n> would have _exactly_ the same effect as the current code! ;-) ).\n\nI have now done:\n\n $ diff 0001-packfile-use-extra-variable-to-clarify-code-in-use_p.patch ttt.patch\n 28c28\n < \t\tsize_t *left)\n ---\n > \t\tunsigned long *left)\n $ \n\n... this and it applies cleanly to 'master', builds and passes tests.\n\nJust FYI. ;-)\n\nATB,\nRamsay Jones\n\n"},{"id":"371431","messageId":"20190314001940.GD31968@sigill.intra.peff.net","threadId":"50736","inReplyTo":"a026ce9f-b825-7fc2-2ad5-c0b00bc508b4@ramsayjones.plus.com","subject":"Re: [PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:19:40Z","receivedAt":"2019-03-14T00:21:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 09:49:58PM +0000, Ramsay Jones wrote:\n\n> From: Jeff King \n> [...]\n> Signed-off-by: Ramsay Jones \n\nSigned-off-by: Jeff King \n\nNaturally. :)\n\n> As promised, I am forwarding a 'saved' patch from Jeff, which was\n> a by-product of a long-ago discussion regarding commit 5efde212fc\n> (\"zlib.c: use size_t for size\", 2018-10-14).\n> \n> I have tested this patch on 'pu' (@6fd68134c8) and directly on top\n> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').\n> \n> However, whilst I have been waiting for the tests to finish, I have\n> been looking at the code and concluded that this does not _have_ to\n> be applied on top of commit 5efde212fc. (I haven't done it, but just\n> tweak the context line to read 'unsigned long *left)' rather than\n> 'size_t *left)' and this should apply cleanly to 'master'. Also, it\n> would have _exactly_ the same effect as the current code! ;-) ).\n\nI think it does apply, though the reasoning in the commit message of\n\"this is OK because 'left' is large enough\" becomes a lot more\nhand-wavy. The patch is not making anything _worse_, certainly, but the\nfact of the matter is that \"left\" still might not be big enough, if it\nis not a size_t.\n\n-Peff\n"},{"id":"371438","messageId":"aca43222-bd4f-ec4d-9cbf-bc85623d34fc@ramsayjones.plus.com","threadId":"50736","inReplyTo":"20190314001940.GD31968@sigill.intra.peff.net","subject":"Re: [PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-14T00:54:48Z","receivedAt":"2019-03-14T00:54:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/03/2019 00:19, Jeff King wrote:\n> On Wed, Mar 13, 2019 at 09:49:58PM +0000, Ramsay Jones wrote:\n> \n>> From: Jeff King \n>> [...]\n>> Signed-off-by: Ramsay Jones \n> \n> Signed-off-by: Jeff King \n> \n> Naturally. :)\n> \n>> As promised, I am forwarding a 'saved' patch from Jeff, which was\n>> a by-product of a long-ago discussion regarding commit 5efde212fc\n>> (\"zlib.c: use size_t for size\", 2018-10-14).\n>>\n>> I have tested this patch on 'pu' (@6fd68134c8) and directly on top\n>> of commit 5efde212fc. (see branch 'mk/use-size-t-in-zlib').\n>>\n>> However, whilst I have been waiting for the tests to finish, I have\n>> been looking at the code and concluded that this does not _have_ to\n>> be applied on top of commit 5efde212fc. (I haven't done it, but just\n>> tweak the context line to read 'unsigned long *left)' rather than\n>> 'size_t *left)' and this should apply cleanly to 'master'. Also, it\n>> would have _exactly_ the same effect as the current code! ;-) ).\n> \n> I think it does apply, though the reasoning in the commit message of\n> \"this is OK because 'left' is large enough\" becomes a lot more\n> hand-wavy. The patch is not making anything _worse_, certainly, but the\n> fact of the matter is that \"left\" still might not be big enough, if it\n> is not a size_t.\n\nYep, the commit message would have to change (it says 'left' is\na size_t), but I think the patch is _still_ an improvement on\nthe existing code, even without s/unsigned long *left/size_t *left/.\n(ie the code is still 'clarified'). :-D\n\nAnyway, it was just an idle FYI while waiting. ;-)\n\nATB,\nRamsay Jones\n"},{"id":"371459","messageId":"xmqqa7hy9fse.fsf@gitster-ct.c.googlers.com","threadId":"50736","inReplyTo":"20190314001940.GD31968@sigill.intra.peff.net","subject":"Re: [PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T05:38:25Z","receivedAt":"2019-03-14T05:38:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think it does apply, though the reasoning in the commit message of\n> \"this is OK because 'left' is large enough\" becomes a lot more\n> hand-wavy. The patch is not making anything _worse_, certainly, but the\n> fact of the matter is that \"left\" still might not be big enough, if it\n> is not a size_t.\n\nTrue; it does not make much sense without the actual size_t\nconversion, I would think.\n"}]}