{"thread":{"id":"36806","subject":"[PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","startedAt":"2014-06-01T11:07:21Z","lastAt":"2014-06-03T06:29:51Z","messageCount":6,"participants":["René Scharfe","Jeff King","David Kastrup","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243092","messageId":"538B0969.9080409@web.de","threadId":"36806","inReplyTo":null,"subject":"[PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-01T11:07:21Z","receivedAt":"2014-06-01T11:07:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Whenever the hash table becomes too small then its size is increased,\nthe original part (and the added space) is zerod out using memset(),\nand the table is rebuilt from scratch.\n\nSimplify this proceess by returning the old memory using free() and\nallocating the new buffer using xcalloc(), which already clears the\nbuffer for us. That way we avoid copying the old hash table contents\nneedlessly inside xrealloc().\n\nWhile at it, use the first array member with sizeof instead of a\nspecific type. The old code used uint32_t and int, while index is\nactually an array of int32_t. Their sizes are the same basically\neverywhere, so it's not actually a problem, but the new code is\ncleaner and doesn't have to be touched should the type be changed.\n\nSigned-off-by: Rene Scharfe \n---\n pack-objects.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-objects.c b/pack-objects.c\nindex d01d851..4f36c32 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -47,8 +47,8 @@ static void rehash_objects(struct packing_data *pdata)\n \tif (pdata->index_size < 1024)\n \t\tpdata->index_size = 1024;\n \n-\tpdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);\n-\tmemset(pdata->index, 0, sizeof(int) * pdata->index_size);\n+\tfree(pdata->index);\n+\tpdata->index = xcalloc(pdata->index_size, sizeof(*pdata->index));\n \n \tentry = pdata->objects;\n \n-- \n2.0.0\n"},{"id":"243152","messageId":"20140602194246.GD2510@sigill.intra.peff.net","threadId":"36806","inReplyTo":"538B0969.9080409@web.de","subject":"Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-02T19:42:46Z","receivedAt":"2014-06-02T19:42:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 01, 2014 at 01:07:21PM +0200, René Scharfe wrote:\n\n> Whenever the hash table becomes too small then its size is increased,\n> the original part (and the added space) is zerod out using memset(),\n> and the table is rebuilt from scratch.\n> \n> Simplify this proceess by returning the old memory using free() and\n> allocating the new buffer using xcalloc(), which already clears the\n> buffer for us. That way we avoid copying the old hash table contents\n> needlessly inside xrealloc().\n> \n> While at it, use the first array member with sizeof instead of a\n> specific type. The old code used uint32_t and int, while index is\n> actually an array of int32_t. Their sizes are the same basically\n> everywhere, so it's not actually a problem, but the new code is\n> cleaner and doesn't have to be touched should the type be changed.\n> \n> Signed-off-by: Rene Scharfe \n\nLooks good to me.\n\nBTW, the code does git-blame to Vicent's 2834bc2 (which I also worked\non), but actually originated in 7a979d9 (Thin pack - create packfile\nwith missing delta base., 2006-02-19). Not that it matters, but I was\njust surprised since the code you are changing did not seem familiar to\nme. I guess there was just too much refactoring during the code movement\nfor git-blame to pass along the blame in this case.\n\n-Peff\n"},{"id":"243158","messageId":"878upf9h9v.fsf@fencepost.gnu.org","threadId":"36806","inReplyTo":"20140602194246.GD2510@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-06-02T20:40:44Z","receivedAt":"2014-06-02T20:40:44Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jun 01, 2014 at 01:07:21PM +0200, René Scharfe wrote:\n>\n>> Whenever the hash table becomes too small then its size is increased,\n>> the original part (and the added space) is zerod out using memset(),\n>> and the table is rebuilt from scratch.\n>> \n>> Simplify this proceess by returning the old memory using free() and\n>> allocating the new buffer using xcalloc(), which already clears the\n>> buffer for us. That way we avoid copying the old hash table contents\n>> needlessly inside xrealloc().\n>> \n>> While at it, use the first array member with sizeof instead of a\n>> specific type. The old code used uint32_t and int, while index is\n>> actually an array of int32_t. Their sizes are the same basically\n>> everywhere, so it's not actually a problem, but the new code is\n>> cleaner and doesn't have to be touched should the type be changed.\n>> \n>> Signed-off-by: Rene Scharfe \n>\n> Looks good to me.\n>\n> BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked\n> on), but actually originated in 7a979d9 (Thin pack - create packfile\n> with missing delta base., 2006-02-19). Not that it matters, but I was\n> just surprised since the code you are changing did not seem familiar to\n> me. I guess there was just too much refactoring during the code movement\n> for git-blame to pass along the blame in this case.\n\nWithout -M, \"too much refactoring\" for git-blame may just be moving a\nfunction to a different place in the same file.\n\n-- \nDavid Kastrup\n"},{"id":"243167","messageId":"20140602215911.GA4612@sigill.intra.peff.net","threadId":"36806","inReplyTo":"878upf9h9v.fsf@fencepost.gnu.org","subject":"Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-02T21:59:11Z","receivedAt":"2014-06-02T21:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 02, 2014 at 10:40:44PM +0200, David Kastrup wrote:\n\n> > BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked\n> > on), but actually originated in 7a979d9 (Thin pack - create packfile\n> > with missing delta base., 2006-02-19). Not that it matters, but I was\n> > just surprised since the code you are changing did not seem familiar to\n> > me. I guess there was just too much refactoring during the code movement\n> > for git-blame to pass along the blame in this case.\n> \n> Without -M, \"too much refactoring\" for git-blame may just be moving a\n> function to a different place in the same file.\n\nI tried \"git blame -M -C -C -C pack-objects.c\" but couldn't get anything\nbut the whole thing blamed to 2834bc2.\n\n-Peff\n"},{"id":"243171","messageId":"xmqqbnua3o4n.fsf@gitster.dls.corp.google.com","threadId":"36806","inReplyTo":"20140602215911.GA4612@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-02T23:09:12Z","receivedAt":"2014-06-02T23:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jun 02, 2014 at 10:40:44PM +0200, David Kastrup wrote:\n>\n>> > BTW, the code does git-blame to Vicent's 2834bc2 (which I also worked\n>> > on), but actually originated in 7a979d9 (Thin pack - create packfile\n>> > with missing delta base., 2006-02-19). Not that it matters, but I was\n>> > just surprised since the code you are changing did not seem familiar to\n>> > me. I guess there was just too much refactoring during the code movement\n>> > for git-blame to pass along the blame in this case.\n>> \n>> Without -M, \"too much refactoring\" for git-blame may just be moving a\n>> function to a different place in the same file.\n>\n> I tried \"git blame -M -C -C -C pack-objects.c\" but couldn't get anything\n> but the whole thing blamed to 2834bc2.\n\nAre you two being a bit too unreasonable, or trying to be fanciful\nand funny and I am not getting the humor?\n\nHere is the relevant part of what 2834bc27 (pack-objects: refactor\nthe packing list, 2013-10-24) removes from builtin/pack-objects.c:\n\n-\tobject_ix = xrealloc(object_ix, sizeof(int) * object_ix_hashsz);\n-\tmemset(object_ix, 0, sizeof(int) * object_ix_hashsz);\n\nAnd here is how the same rehash is done in pack-objects.c at the\ntoplevel in the new code:\n\n+\tpdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);\n+\tmemset(pdata->index, 0, sizeof(int) * pdata->index_size);\n\nSurely, the code structure may be similar, but the similarity ends\nthere. These lines are not equivalent even under the \"-w\" option.\n"},{"id":"243178","messageId":"20140603062951.GA26150@sigill.intra.peff.net","threadId":"36806","inReplyTo":"xmqqbnua3o4n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pack-objects: use free()+xcalloc() instead of xrealloc()+memset()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-03T06:29:51Z","receivedAt":"2014-06-03T06:29:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 02, 2014 at 04:09:12PM -0700, Junio C Hamano wrote:\n\n> > I tried \"git blame -M -C -C -C pack-objects.c\" but couldn't get anything\n> > but the whole thing blamed to 2834bc2.\n> \n> Are you two being a bit too unreasonable, or trying to be fanciful\n> and funny and I am not getting the humor?\n\nWe are being too unreasonable.\n\n> Here is the relevant part of what 2834bc27 (pack-objects: refactor\n> the packing list, 2013-10-24) removes from builtin/pack-objects.c:\n> \n> -\tobject_ix = xrealloc(object_ix, sizeof(int) * object_ix_hashsz);\n> -\tmemset(object_ix, 0, sizeof(int) * object_ix_hashsz);\n> \n> And here is how the same rehash is done in pack-objects.c at the\n> toplevel in the new code:\n> \n> +\tpdata->index = xrealloc(pdata->index, sizeof(uint32_t) * pdata->index_size);\n> +\tmemset(pdata->index, 0, sizeof(int) * pdata->index_size);\n> \n> Surely, the code structure may be similar, but the similarity ends\n> there. These lines are not equivalent even under the \"-w\" option.\n\nYes, I did not expect these particular lines to get blamed, but I\nthought some of the surrounding function would (which could lead to a\nparent-blame to find the true origin). Skimming the diff, it looked like\nsome of them made it through unscathed. But they didn't.\n\nRunning:\n\n git show 2834bc2 |\n perl -lne '\n /^-(.*)/ and $del{$1}++;\n print \"$.: $_\" if /^\\+(.*)/ && $del{$1};\n '\n\nshows that there are only a handful of interesting lines that survived\ncompletely intact, and typically not more than one line in a row. The\nbig exceptions are the bits that made it into pack-objects.h, and a \"git\nblame\" there does find the code movement.\n\nSo I think everything is operating as expected.\n\n-Peff\n"}]}