{"thread":{"id":"7574","subject":"[PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","startedAt":"2007-04-08T23:24:04Z","lastAt":"2007-04-09T20:09:44Z","messageCount":10,"participants":["Dana How","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"38893","messageId":"46197994.70009@gmail.com","threadId":"7574","inReplyTo":null,"subject":"[PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-08T23:24:04Z","receivedAt":"2007-04-08T23:24:04Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\nAccept new 'limit' argument and check against it\nbefore each group of writes. Update delta usability rules\nfor possibility of delta base being in a previously-\nwritten pack. Inline sha1write_compressed() so we know\nthe exact size of the written data when it needs to be compressed.\n\nSigned-off-by: Dana How \n---\n builtin-pack-objects.c | 66 ++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 58 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 7ab0712..9530008 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -373,7 +373,8 @@ static int revalidate_loose_object(struct object_entry *entry,\n }\n \n static off_t write_object(struct sha1file *f,\n-\t\t\t\t struct object_entry *entry)\n+\t\t\t\t struct object_entry *entry,\n+\t\t\t\t unsigned long limit)\n {\n \tunsigned long size;\n \tenum object_type type;\n@@ -407,6 +408,10 @@ static off_t write_object(struct sha1file *f,\n \t\t\tif (revalidate_loose_object(entry, map, mapsize))\n \t\t\t\tdie(\"corrupt loose object %s\",\n \t\t\t\t sha1_to_hex(entry->sha1));\n+\t\t\tif ( limit && mapsize + 20 >= limit ) {\n+\t\t\t\tmunmap(map, mapsize);\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tsha1write(f, map, mapsize);\n \t\t\tmunmap(map, mapsize);\n \t\t\twritten++;\n@@ -418,24 +423,51 @@ static off_t write_object(struct sha1file *f,\n \t}\n \n \tif (!to_reuse) {\n+\t\tz_stream stream;\n+\t\tunsigned long maxsize;\n+\t\tvoid *out;\n+\t\t\t\t\t/* no if no delta */\n+\t\tint usable_delta =\t!entry->delta ? 0 :\n+\t\t\t\t\t/* yes if unlimited packfile */\n+\t\t\t\t\t!offset_limit ? 1 :\n+\t\t\t\t\t/* no if base written to previous pack */\n+\t\t\t\t\tentry->delta->prev_pack ? 0 :\n+\t\t\t\t\t/* otherwise double-check written to this\n+\t\t\t\t\t * pack, like we do below\n+\t\t\t\t\t */\n+\t\t\t\t\tentry->delta->offset ? 1 : 0;\n \t\tbuf = read_sha1_file(entry->sha1, &type, &size);\n \t\tif (!buf)\n \t\t\tdie(\"unable to read %s\", sha1_to_hex(entry->sha1));\n \t\tif (size != entry->size)\n \t\t\tdie(\"object %s size inconsistency (%lu vs %lu)\",\n \t\t\t sha1_to_hex(entry->sha1), size, entry->size);\n-\t\tif (entry->delta) {\n+\t\tif (usable_delta) {\n \t\t\tbuf = delta_against(buf, size, entry);\n \t\t\tsize = entry->delta_size;\n \t\t\tobj_type = (allow_ofs_delta && entry->delta->offset) ?\n \t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n \t\t}\n+\t\t/* compress the data to store and put compressed length in datalen */\n+\t\tmemset(&stream, 0, sizeof(stream));\n+\t\tdeflateInit(&stream, zlib_compression_level);\n+\t\tmaxsize = deflateBound(&stream, size);\n+\t\tout = xmalloc(maxsize);\n+\t\t/* Compress it */\n+\t\tstream.next_in = buf;\n+\t\tstream.avail_in = size;\n+\t\tstream.next_out = out;\n+\t\tstream.avail_out = maxsize;\n+\t\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n+\t\t\t/* nothing */;\n+\t\tdeflateEnd(&stream);\n+\t\tdatalen = stream.total_out;\n+\t\tdeflateEnd(&stream);\n \t\t/*\n \t\t * The object header is a byte of 'type' followed by zero or\n \t\t * more bytes of length.\n \t\t */\n \t\thdrlen = encode_header(obj_type, size, header);\n-\t\tsha1write(f, header, hdrlen);\n \n \t\tif (obj_type == OBJ_OFS_DELTA) {\n \t\t\t/*\n@@ -448,6 +480,12 @@ static off_t write_object(struct sha1file *f,\n \t\t\theader[pos] = ofs & 127;\n \t\t\twhile (ofs >>= 7)\n \t\t\t\theader[--pos] = 128 | (--ofs & 127);\n+\t\t\tif ( limit && hdrlen + sizeof(header) - pos + datalen + 20 >= limit ) {\n+\t\t\t\tfree(out);\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, header + pos, sizeof(header) - pos);\n \t\t\thdrlen += sizeof(header) - pos;\n \t\t} else if (obj_type == OBJ_REF_DELTA) {\n@@ -455,10 +493,17 @@ static off_t write_object(struct sha1file *f,\n \t\t\t * Deltas with a base reference contain\n \t\t\t * an additional 20 bytes for the base sha1.\n \t\t\t */\n+\t\t\tif ( limit && hdrlen + 20 + datalen + 20 >= limit ) {\n+\t\t\t\tfree(out);\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, entry->delta->sha1, 20);\n \t\t\thdrlen += 20;\n \t\t}\n-\t\tdatalen = sha1write_compressed(f, buf, size);\n+\t\tsha1write(f, out, datalen);\n+\t\tfree(out);\n \t\tfree(buf);\n \t}\n \telse {\n@@ -472,23 +517,28 @@ static off_t write_object(struct sha1file *f,\n \t\t\treused_delta++;\n \t\t}\n \t\thdrlen = encode_header(obj_type, entry->size, header);\n-\t\tsha1write(f, header, hdrlen);\n+\t\tdatalen = find_packed_object_size(p, entry->in_pack_offset)\n+\t\t\t\t- entry->in_pack_header_size;\n \t\tif (obj_type == OBJ_OFS_DELTA) {\n \t\t\toff_t ofs = entry->offset - entry->delta->offset;\n \t\t\tunsigned pos = sizeof(header) - 1;\n \t\t\theader[pos] = ofs & 127;\n \t\t\twhile (ofs >>= 7)\n \t\t\t\theader[--pos] = 128 | (--ofs & 127);\n+\t\t\tif ( limit && hdrlen + sizeof(header) - pos + datalen + 20 >= limit )\n+\t\t\t\treturn 0;\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, header + pos, sizeof(header) - pos);\n \t\t\thdrlen += sizeof(header) - pos;\n \t\t} else if (obj_type == OBJ_REF_DELTA) {\n+\t\t\tif ( limit && hdrlen + 20 + datalen + 20 >= limit )\n+\t\t\t\treturn 0;\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, entry->delta->sha1, 20);\n \t\t\thdrlen += 20;\n \t\t}\n \n \t\toffset = entry->in_pack_offset + entry->in_pack_header_size;\n-\t\tdatalen = find_packed_object_size(p, entry->in_pack_offset)\n-\t\t\t\t- entry->in_pack_header_size;\n \t\tif (!pack_to_stdout && check_pack_inflate(p, &w_curs,\n \t\t\t\toffset, datalen, entry->size))\n \t\t\tdie(\"corrupt delta in pack %s\", sha1_to_hex(entry->sha1));\n@@ -515,7 +565,7 @@ static off_t write_one(struct sha1file *f,\n \tif (e->delta)\n \t\toffset = write_one(f, e->delta, offset);\n \te->offset = offset;\n-\treturn offset + write_object(f, e);\n+\treturn offset + write_object(f, e, 0);\n }\n \n /*\n-- \n1.5.1.89.g8abf0\n"},{"id":"38899","messageId":"7v4pnq8ld4.fsf@assigned-by-dhcp.cox.net","threadId":"7574","inReplyTo":"46197994.70009@gmail.com","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-09T00:03:35Z","receivedAt":"2007-04-09T00:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dana How writes:\n\n> Accept new 'limit' argument and check against it\n> before each group of writes. Update delta usability rules\n> for possibility of delta base being in a previously-\n> written pack. Inline sha1write_compressed() so we know\n> the exact size of the written data when it needs to be compressed.\n>\n> Signed-off-by: Dana How \n\nMy first reaction of open-coding sha1write_compressed() was\n\"Ugh\", but as you are removing the only user of that function,\nmaybe this is not as bad as it looks.\n\n> @@ -407,6 +408,10 @@ static off_t write_object(struct sha1file *f,\n> \t\t\tif (revalidate_loose_object(entry, map, mapsize))\n> \t\t\t\tdie(\"corrupt loose object %s\",\n> \t\t\t\t sha1_to_hex(entry->sha1));\n> +\t\t\tif ( limit && mapsize + 20 >= limit ) {\n> +\t\t\t\tmunmap(map, mapsize);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n\nOur coding style guideline is similar to that of the kernel. No\nSP after '(' or before ')' in conditionals.\n"},{"id":"38900","messageId":"7vr6qu768r.fsf@assigned-by-dhcp.cox.net","threadId":"7574","inReplyTo":"7v4pnq8ld4.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-09T00:15:32Z","receivedAt":"2007-04-09T00:15:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dana How writes:\n>\n>> Accept new 'limit' argument and check against it\n>> before each group of writes. Update delta usability rules\n>> for possibility of delta base being in a previously-\n>> written pack. Inline sha1write_compressed() so we know\n>> the exact size of the written data when it needs to be compressed.\n>>\n>> Signed-off-by: Dana How \n>\n> My first reaction of open-coding sha1write_compressed() was\n> \"Ugh\", but as you are removing the only user of that function,\n> maybe this is not as bad as it looks.\n\nHaving said that, I suspect that for other possible users of\nthat function we might have later, it would be a better\ninterface to add an optional 'limit' and 'prelude' to\nsha1write_compressed(). The function would write prelude\nfollowed by the compressed payload, only if they fit the limit.\n\nThen your write_object() would prepare the header (depending on\nthe type, the object header, ofs-delta header or ref-delta\nheader) in header[] but would not cal sha1write() itself.\nInstead it would send header[] in as prelude, *buf as the\npayload, with an appropriate limit.\n"},{"id":"38902","messageId":"alpine.LFD.0.98.0704082034060.28181@xanadu.home","threadId":"7574","inReplyTo":"46197994.70009@gmail.com","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T00:40:05Z","receivedAt":"2007-04-09T00:40:05Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 8 Apr 2007, Dana How wrote:\n\n> \n> Accept new 'limit' argument and check against it\n> before each group of writes. Update delta usability rules\n> for possibility of delta base being in a previously-\n> written pack. Inline sha1write_compressed() so we know\n> the exact size of the written data when it needs to be compressed.\n\nIt would be cleaner if the compression code was made into a function of \nits own I think.\n\n> @@ -448,6 +480,12 @@ static off_t write_object(struct sha1file *f,\n> \t\t\theader[pos] = ofs & 127;\n> \t\t\twhile (ofs >>= 7)\n> \t\t\t\theader[--pos] = 128 | (--ofs & 127);\n> +\t\t\tif ( limit && hdrlen + sizeof(header) - pos + datalen + 20 >= limit ) {\n> +\t\t\t\tfree(out);\n> +\t\t\t\tfree(buf);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t\tsha1write(f, header, hdrlen);\n> \t\t\tsha1write(f, header + pos, sizeof(header) - pos);\n\nThe above looks rather buggy to me.\n\n\nNicolas\n"},{"id":"38903","messageId":"alpine.LFD.0.98.0704082227580.28181@xanadu.home","threadId":"7574","inReplyTo":"7vr6qu768r.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T02:28:56Z","receivedAt":"2007-04-09T02:28:56Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 8 Apr 2007, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Dana How writes:\n> >\n> >> Accept new 'limit' argument and check against it\n> >> before each group of writes. Update delta usability rules\n> >> for possibility of delta base being in a previously-\n> >> written pack. Inline sha1write_compressed() so we know\n> >> the exact size of the written data when it needs to be compressed.\n> >>\n> >> Signed-off-by: Dana How \n> >\n> > My first reaction of open-coding sha1write_compressed() was\n> > \"Ugh\", but as you are removing the only user of that function,\n> > maybe this is not as bad as it looks.\n> \n> Having said that, I suspect that for other possible users of\n> that function we might have later, it would be a better\n> interface to add an optional 'limit' and 'prelude' to\n> sha1write_compressed(). The function would write prelude\n> followed by the compressed payload, only if they fit the limit.\n\nI'd wait for those possible future users to show up before doing such \nthing though.\n\n\nNicolas\n"},{"id":"38964","messageId":"56b7f5510704091151l70cc74b1la82e0c5a67d6885f@mail.gmail.com","threadId":"7574","inReplyTo":"alpine.LFD.0.98.0704082034060.28181@xanadu.home","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-09T18:51:55Z","receivedAt":"2007-04-09T18:51:55Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/8/07, Nicolas Pitre wrote:\n> > @@ -448,6 +480,12 @@ static off_t write_object(struct sha1file *f,\n> > header[pos] = ofs & 127;\n> > while (ofs >>= 7)\n> > header[--pos] = 128 | (--ofs & 127);\n> > + if ( limit && hdrlen + sizeof(header) - pos + datalen + 20 >= limit ) {\n> > + free(out);\n> > + free(buf);\n> > + return 0;\n> > + }\n> > + sha1write(f, header, hdrlen);\n> > sha1write(f, header + pos, sizeof(header) - pos);\n>\n> The above looks rather buggy to me.\n\nOK, can you be more specific?\n\nSo far from my re-inspection I don't see any error in the snippet above.\nTwo lines before it, however, we have the line\n off_t ofs = entry->offset - entry->delta->offset;\nand this only works if entry->offset is set in write_one() *before*\ncalling write_object(). I will make that change.\n\nThanks!\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38953","messageId":"alpine.LFD.0.98.0704091455100.28181@xanadu.home","threadId":"7574","inReplyTo":"56b7f5510704091151l70cc74b1la82e0c5a67d6885f@mail.gmail.com","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T18:59:18Z","receivedAt":"2007-04-09T18:59:18Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 9 Apr 2007, Dana How wrote:\n\n> On 4/8/07, Nicolas Pitre wrote:\n> > > @@ -448,6 +480,12 @@ static off_t write_object(struct sha1file *f,\n> > > header[pos] = ofs & 127;\n> > > while (ofs >>= 7)\n> > > header[--pos] = 128 | (--ofs & 127);\n> > > + if ( limit && hdrlen + sizeof(header) - pos +\n> > datalen + 20 >= limit ) {\n> > > + free(out);\n> > > + free(buf);\n> > > + return 0;\n> > > + }\n> > > + sha1write(f, header, hdrlen);\n> > > sha1write(f, header + pos, sizeof(header) - pos);\n> > \n> > The above looks rather buggy to me.\n> \n> OK, can you be more specific?\n\nYou're writing the content of the array 'header' twice in a row. Sure \nthe second time it is header + pos but it is still the result of an \noperation that used to put data into 'header' after the first content \nwas already written out. Right now it looks like the first write might \ncontain clobbered data.\n\n\nNicolas\n"},{"id":"38951","messageId":"56b7f5510704091220w69529e72n2f982c097c4e26a0@mail.gmail.com","threadId":"7574","inReplyTo":"alpine.LFD.0.98.0704091455100.28181@xanadu.home","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-09T19:20:26Z","receivedAt":"2007-04-09T19:20:26Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/9/07, Nicolas Pitre wrote:\n> On Mon, 9 Apr 2007, Dana How wrote:\n> > On 4/8/07, Nicolas Pitre wrote:\n> > > > + sha1write(f, header, hdrlen);\n> > > > sha1write(f, header + pos, sizeof(header) - pos);\n> > >\n> > > The above looks rather buggy to me.\n> >\n> > OK, can you be more specific?\n>\n> You're writing the content of the array 'header' twice in a row. Sure\n> the second time it is header + pos but it is still the result of an\n> operation that used to put data into 'header' after the first content\n> was already written out. Right now it looks like the first write might\n> contain clobbered data.\nThanks, I'll take care of that.\n\nFor testing, I've been using git-fsck/git-verify-pack/git-unpack-objects .\nThe only bugs they ever caught were (1) offset wraparound in .idx\nbefore I started this patchset (with a very bad error message) and\n(2) I failed to flush out the buffer before changing packs.\nEverything else has been detected manually (not always by me). Hmm.\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38950","messageId":"alpine.LFD.0.98.0704091523410.28181@xanadu.home","threadId":"7574","inReplyTo":"56b7f5510704091220w69529e72n2f982c097c4e26a0@mail.gmail.com","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-09T19:25:06Z","receivedAt":"2007-04-09T19:25:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 9 Apr 2007, Dana How wrote:\n\n> For testing, I've been using git-fsck/git-verify-pack/git-unpack-objects .\n> The only bugs they ever caught were (1) offset wraparound in .idx\n> before I started this patchset (with a very bad error message) and\n> (2) I failed to flush out the buffer before changing packs.\n> Everything else has been detected manually (not always by me). Hmm.\n\nMake sure you always run 'make test' as well when you're done.\n\n\nNicolas\n"},{"id":"38947","messageId":"7v3b39z4vr.fsf@assigned-by-dhcp.cox.net","threadId":"7574","inReplyTo":"56b7f5510704091220w69529e72n2f982c097c4e26a0@mail.gmail.com","subject":"Re: [PATCH 5/8] get-repack --max-pack-size: write_object() takes 'limit' arg","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-09T20:09:44Z","receivedAt":"2007-04-09T20:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dana How\" writes:\n\n> For testing, I've been using git-fsck/git-verify-pack/git-unpack-objects .\n\nAmong the above, verify-pack does not do much more than what the\nnormal object reading path does already these days. Also\nunpack-objects (especially with -n) omits a lot of object\nintegrity checks, and not very appropriate test.\n\nWe have some tests for packfiles in t/ (make test) but I would\nnot be surprised if we do not have enough. Most of the existing\ntests are done on loose objects the tests themselves produce\nwithout repacking, because their focus is not about packs and\nthey test whatever they are interested in, assuming that the\nlowest level object layer is correctly functioning.\n"}]}