{"thread":{"id":"50719","subject":"[RFC/PATCH] packfile: use extra variable to clarify code in use_pack()","startedAt":"2019-03-12T16:55:24Z","lastAt":"2019-03-12T22:15:29Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371273","messageId":"e561b83f-cf1c-eef8-7651-8519ce105491@ramsayjones.plus.com","threadId":"50719","inReplyTo":null,"subject":"[RFC/PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-12T16:55:16Z","receivedAt":"2019-03-12T16:55:24Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"From: Jeff King \n\nSigned-off-by: Ramsay Jones \n---\n\nHi Jeff,\n\nI recently tried (yet again) to tidy up some old branches. When I get\naround to doing a 'git gc; git fsck' I always take a quick look at\nthe 'dangling' commits, just before a 'git gc --prune=now'.\n\nI had no recollection of this commit, from last October, but a quick\nlook at the ML archive found this [1] discussion. I obviously thought\nit was worth saving this thought of yours. ;-) So, having deleted this\nalready, I did a quick 'format-patch' to see if anyone thinks it is\nworth applying.\n\n[1] https://public-inbox.org/git/20181013024624.GB15595@sigill.intra.peff.net/#t\n\nThanks!\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 013294aec7..2f81ec9345 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -588,6 +588,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@@ -649,10 +650,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":"371278","messageId":"b4106d4b-5f9e-4577-c8ed-641df33d4fb5@ramsayjones.plus.com","threadId":"50719","inReplyTo":"e561b83f-cf1c-eef8-7651-8519ce105491@ramsayjones.plus.com","subject":"Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-12T17:39:13Z","receivedAt":"2019-03-12T17:39:18Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/03/2019 16:55, Ramsay Jones wrote:\n> From: Jeff King \n> \n> Signed-off-by: Ramsay Jones \n> ---\n> \n> Hi Jeff,\n> \n> I recently tried (yet again) to tidy up some old branches. When I get\n> around to doing a 'git gc; git fsck' I always take a quick look at\n> the 'dangling' commits, just before a 'git gc --prune=now'.\n> \n> I had no recollection of this commit, from last October, but a quick\n> look at the ML archive found this [1] discussion. I obviously thought\n> it was worth saving this thought of yours. ;-) So, having deleted this\n> already, I did a quick 'format-patch' to see if anyone thinks it is\n> worth applying.\n> \n> [1] https://public-inbox.org/git/20181013024624.GB15595@sigill.intra.peff.net/#t\n> \n\nHeh, of course I should have tried applying on top of today's\ncodebase before sending it out! :(\n\nHaving just done so, it quickly showed that this patch assumes\nthat the 'left' parameter to use_pack() has been changed from\nan 'unsigned long *' to an 'size_t *' as part of the series\nthat was being discussed in the above link.\n\nAh, well, sorry for the noise!\n\nATB,\nRamsay Jones\n\n"},{"id":"371295","messageId":"20190312202605.GA24350@sigill.intra.peff.net","threadId":"50719","inReplyTo":"b4106d4b-5f9e-4577-c8ed-641df33d4fb5@ramsayjones.plus.com","subject":"Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T20:26:06Z","receivedAt":"2019-03-12T20:26:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2019 at 05:39:13PM +0000, Ramsay Jones wrote:\n\n> On 12/03/2019 16:55, Ramsay Jones wrote:\n> > From: Jeff King \n> > \n> > Signed-off-by: Ramsay Jones \n\nCould definitely use a commit message. I think it's something like:\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, is a size_t. It is\n in fact correct (because our earlier \"offset -= win->offset\" means we\n must be within the pack window), but using a separate variable of the\n right type makes that much more obvious.\n\nYou'll note that I snuck in the assumption that \"left\" is a size_t,\nwhich as you noted is not quite valid yet. :)\n\n> Heh, of course I should have tried applying on top of today's\n> codebase before sending it out! :(\n> \n> Having just done so, it quickly showed that this patch assumes\n> that the 'left' parameter to use_pack() has been changed from\n> an 'unsigned long *' to an 'size_t *' as part of the series\n> that was being discussed in the above link.\n\nYep. Until then, I do not think there is much point (and in fact I'd\nsuspect this code behaves incorrectly on Windows, where \"unsigned long\"\nis too short; hopefully they clamp pack windows to 4GB by default\nthere, which would work around it).\n\nBut I would be very happy if you wanted to resurrect the \"left\" patch\nand then do this on top. :)\n\n-Peff\n"},{"id":"371304","messageId":"1fbd29f2-3385-58d8-e69d-d7df80e214db@ramsayjones.plus.com","threadId":"50719","inReplyTo":"20190312202605.GA24350@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-03-12T22:15:25Z","receivedAt":"2019-03-12T22:15:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/03/2019 20:26, Jeff King wrote:\n> On Tue, Mar 12, 2019 at 05:39:13PM +0000, Ramsay Jones wrote:\n> \n>> On 12/03/2019 16:55, Ramsay Jones wrote:\n>>> From: Jeff King \n>>>\n>>> Signed-off-by: Ramsay Jones \n> \n> Could definitely use a commit message. I think it's something like:\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, is a size_t. It is\n> in fact correct (because our earlier \"offset -= win->offset\" means we\n> must be within the pack window), but using a separate variable of the\n> right type makes that much more obvious.\n> \n> You'll note that I snuck in the assumption that \"left\" is a size_t,\n> which as you noted is not quite valid yet. :)\n\nLooks good to me! :-D\n\n>> Heh, of course I should have tried applying on top of today's\n>> codebase before sending it out! :(\n>>\n>> Having just done so, it quickly showed that this patch assumes\n>> that the 'left' parameter to use_pack() has been changed from\n>> an 'unsigned long *' to an 'size_t *' as part of the series\n>> that was being discussed in the above link.\n> \n> Yep. Until then, I do not think there is much point (and in fact I'd\n> suspect this code behaves incorrectly on Windows, where \"unsigned long\"\n> is too short; hopefully they clamp pack windows to 4GB by default\n> there, which would work around it).\n> \n> But I would be very happy if you wanted to resurrect the \"left\" patch\n> and then do this on top. :)\n\nIt actually applies on top of the 'pu' branch, because the 'left'\npatch is commit 5efde212fc (\"zlib.c: use size_t for size\", 2018-10-18).\n\nIf you recall, this was going to be just the first step in a series\nof patches to replace 'unsigned long' as the type used for various\n'size' or 'length' variables.\n\nSo, if I add the above commit message, it could apply on top of the\ncurrent 'mk/use-size-t-in-zlib' branch.\n\nI may not get to that tonight (busy with something else), but I can\nhopefully do that tomorrow.\n\nATB,\nRamsay Jones\n\n"}]}