{"thread":{"id":"3278","subject":"[PATCH] remove delta-against-self bit","startedAt":"2006-02-09T22:50:04Z","lastAt":"2006-02-10T17:19:05Z","messageCount":6,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"15812","messageId":"Pine.LNX.4.64.0602091736500.5397@localhost.localdomain","threadId":"3278","inReplyTo":null,"subject":"[PATCH] remove delta-against-self bit","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-02-09T22:50:04Z","receivedAt":"2006-02-09T22:50:04Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"After experimenting with code to add the ability to encode a delta \nagainst part of the deltified file, it turns out that resulting packs \nare _bigger_ than when this ability is not used.  The raw delta output \nmight be smaller, but it doesn't compress as well using gzip with a \nnegative net saving on average.\n\nSaid bit would in fact be more useful to allow for encoding the copying \nof chunks larger than 64KB providing more savings with large files.  \nThis will correspond to packs version 3.\n\nWhile the current code still produces packs version 2, it is made future \nproof so pack versions 2 and 3 are accepted.  Any pack version 2 are \ncompatible with version 3 since the redefined bit was never used before.  \nWhen enough time has passed, code to use that bit to produce version 3 \npacks could be added.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n\n---\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex 98c27be..c0e1311 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -44,16 +44,15 @@ void *patch_delta(void *src_buf, unsigne\n \t\tcmd = *data++;\n \t\tif (cmd & 0x80) {\n \t\t\tunsigned long cp_off = 0, cp_size = 0;\n-\t\t\tconst unsigned char *buf;\n \t\t\tif (cmd & 0x01) cp_off = *data++;\n \t\t\tif (cmd & 0x02) cp_off |= (*data++ << 8);\n \t\t\tif (cmd & 0x04) cp_off |= (*data++ << 16);\n \t\t\tif (cmd & 0x08) cp_off |= (*data++ << 24);\n \t\t\tif (cmd & 0x10) cp_size = *data++;\n \t\t\tif (cmd & 0x20) cp_size |= (*data++ << 8);\n+\t\t\tif (cmd & 0x40) cp_size |= (*data++ << 16);\n \t\t\tif (cp_size == 0) cp_size = 0x10000;\n-\t\t\tbuf = (cmd & 0x40) ? dst_buf : src_buf;\n-\t\t\tmemcpy(out, buf + cp_off, cp_size);\n+\t\t\tmemcpy(out, src_buf + cp_off, cp_size);\n \t\t\tout += cp_size;\n \t\t} else {\n \t\t\tmemcpy(out, data, cmd);\ndiff --git a/pack.h b/pack.h\nindex 657deaa..9dafa2b 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -21,6 +21,7 @@ enum object_type {\n  */\n #define PACK_SIGNATURE 0x5041434b\t/* \"PACK\" */\n #define PACK_VERSION 2\n+#define pack_version_ok(v) ((v) == htonl(2) || (v) == htonl(3))\n struct pack_header {\n \tunsigned int hdr_signature;\n \tunsigned int hdr_version;\ndiff --git a/index-pack.c b/index-pack.c\nindex 541d7bc..babe34b 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -68,9 +68,9 @@ static void parse_pack_header(void)\n \thdr = (void *)pack_base;\n \tif (hdr->hdr_signature != htonl(PACK_SIGNATURE))\n \t\tdie(\"packfile '%s' signature mismatch\", pack_name);\n-\tif (hdr->hdr_version != htonl(PACK_VERSION))\n-\t\tdie(\"packfile '%s' version %d different from ours %d\",\n-\t\t    pack_name, ntohl(hdr->hdr_version), PACK_VERSION);\n+\tif (!pack_version_ok(hdr->hdr_version))\n+\t\tdie(\"packfile '%s' version %d unsupported\",\n+\t\t    pack_name, ntohl(hdr->hdr_version));\n \n \tnr_objects = ntohl(hdr->hdr_entries);\n \ndiff --git a/pack-check.c b/pack-check.c\nindex 511f294..67a7ecd 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -16,9 +16,9 @@ static int verify_packfile(struct packed\n \thdr = p->pack_base;\n \tif (hdr->hdr_signature != htonl(PACK_SIGNATURE))\n \t\treturn error(\"Packfile %s signature mismatch\", p->pack_name);\n-\tif (hdr->hdr_version != htonl(PACK_VERSION))\n-\t\treturn error(\"Packfile version %d different from ours %d\",\n-\t\t\t     ntohl(hdr->hdr_version), PACK_VERSION);\n+\tif (!pack_version_ok(hdr->hdr_version))\n+\t\treturn error(\"Packfile version %d unsupported\",\n+\t\t\t     ntohl(hdr->hdr_version));\n \tnr_objects = ntohl(hdr->hdr_entries);\n \tif (num_packed_objects(p) != nr_objects)\n \t\treturn error(\"Packfile claims to have %d objects, \"\ndiff --git a/pack-objects.c b/pack-objects.c\ndiff --git a/unpack-objects.c b/unpack-objects.c\nindex 4b5b5cb..815a1b3 100644\n--- a/unpack-objects.c\n+++ b/unpack-objects.c\n@@ -246,13 +246,12 @@ static void unpack_all(void)\n {\n \tint i;\n \tstruct pack_header *hdr = fill(sizeof(struct pack_header));\n-\tunsigned version = ntohl(hdr->hdr_version);\n \tunsigned nr_objects = ntohl(hdr->hdr_entries);\n \n \tif (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n-\tif (version != PACK_VERSION)\n-\t\tdie(\"unable to handle pack file version %d\", version);\n+\tif (!pack_version_ok(hdr->hdr_version))\n+\t\tdie(\"unknown pack file version %d\", ntohl(hdr->hdr_version));\n \tfprintf(stderr, \"Unpacking %d objects\\n\", nr_objects);\n \n \tuse(sizeof(struct pack_header));\n"},{"id":"15821","messageId":"7v7j846qco.fsf@assigned-by-dhcp.cox.net","threadId":"3278","inReplyTo":"Pine.LNX.4.64.0602091736500.5397@localhost.localdomain","subject":"Re: [PATCH] remove delta-against-self bit","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-09T23:53:59Z","receivedAt":"2006-02-09T23:53:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> Said bit would in fact be more useful to allow for encoding the copying \n> of chunks larger than 64KB providing more savings with large files.  \n> This will correspond to packs version 3.\n>\n> While the current code still produces packs version 2, it is made future \n> proof so pack versions 2 and 3 are accepted.  Any pack version 2 are \n> compatible with version 3 since the redefined bit was never used before.  \n> When enough time has passed, code to use that bit to produce version 3 \n> packs could be added.\n\nI agree with the general direction and this futureproofing is a\ngood thing to have before 1.2.0, I think.\n\nThe bit is however _already_ looked at by the count_delta(),\nto assess the extent of damage, IIRC.  Should we be\nfutureproofing that bit as well?\n"},{"id":"15831","messageId":"7vbqxg3tdd.fsf_-_@assigned-by-dhcp.cox.net","threadId":"3278","inReplyTo":"7v7j846qco.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] count-delta.c: Match the delta data semantics change in version 3.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-10T01:17:02Z","receivedAt":"2006-02-10T01:17:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> I agree with the general direction and this futureproofing is a\n> good thing to have before 1.2.0, I think.\n>\n> The bit is however _already_ looked at by the count_delta(),\n> to assess the extent of damage, IIRC.  Should we be\n> futureproofing that bit as well?\n\nSomething like this?\n\n-- >8 --\nThis matches the count_delta() logic to the change previous\ncommit introduces to patch_delta().\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n count-delta.c |    7 ++-----\n 1 files changed, 2 insertions(+), 5 deletions(-)\n\n6cb2807fd27b9e970a9b012b925deab99f480d61\ndiff --git a/count-delta.c b/count-delta.c\nindex 7559ff6..978a60c 100644\n--- a/count-delta.c\n+++ b/count-delta.c\n@@ -50,13 +50,10 @@ int count_delta(void *delta_buf, unsigne\n \t\t\tif (cmd & 0x08) cp_off |= (*data++ << 24);\n \t\t\tif (cmd & 0x10) cp_size = *data++;\n \t\t\tif (cmd & 0x20) cp_size |= (*data++ << 8);\n+\t\t\tif (cmd & 0x40) cp_size |= (*data++ << 16);\n \t\t\tif (cp_size == 0) cp_size = 0x10000;\n \n-\t\t\tif (cmd & 0x40)\n-\t\t\t\t/* copy from dst */\n-\t\t\t\t;\n-\t\t\telse\n-\t\t\t\tcopied_from_source += cp_size;\n+\t\t\tcopied_from_source += cp_size;\n \t\t\tout += cp_size;\n \t\t} else {\n \t\t\t/* write literal into dst */\n-- \n1.1.6.gce16\n"},{"id":"15833","messageId":"Pine.LNX.4.64.0602092053200.5397@localhost.localdomain","threadId":"3278","inReplyTo":"7vbqxg3tdd.fsf_-_@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] count-delta.c: Match the delta data semantics change in version 3.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-02-10T01:54:31Z","receivedAt":"2006-02-10T01:54:31Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 9 Feb 2006, Junio C Hamano wrote:\n\n> Junio C Hamano <junkio@cox.net> writes:\n> \n> > I agree with the general direction and this futureproofing is a\n> > good thing to have before 1.2.0, I think.\n> >\n> > The bit is however _already_ looked at by the count_delta(),\n> > to assess the extent of damage, IIRC.  Should we be\n> > futureproofing that bit as well?\n> \n> Something like this?\n\nRight.\n\n\nNicolas\n"},{"id":"15893","messageId":"Pine.LNX.4.64.0602101017420.5397@localhost.localdomain","threadId":"3278","inReplyTo":"Pine.LNX.4.64.0602092053200.5397@localhost.localdomain","subject":"Re: [PATCH] count-delta.c: Match the delta data semantics change in version 3.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-02-10T15:20:40Z","receivedAt":"2006-02-10T15:20:40Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 9 Feb 2006, Nicolas Pitre wrote:\n\n> On Thu, 9 Feb 2006, Junio C Hamano wrote:\n> \n> > Junio C Hamano <junkio@cox.net> writes:\n> > \n> > > The bit is however _already_ looked at by the count_delta(),\n> > > to assess the extent of damage, IIRC.  Should we be\n> > > futureproofing that bit as well?\n> > \n> > Something like this?\n> \n> Right.\n\nI'm not sure to fully understand what you meant in the comment below. \nbut if it was related to the removed code, maybe this patch would make \nsense:\n\ndiff --git a/count-delta.c b/count-delta.c\nindex 978a60c..058a2aa 100644\n--- a/count-delta.c\n+++ b/count-delta.c\n@@ -16,11 +16,7 @@\n  *\n  * Number of bytes that are _not_ copied from the source is deletion,\n  * and number of inserted literal bytes are addition, so sum of them\n- * is the extent of damage.  xdelta can express an edit that copies\n- * data inside of the destination which originally came from the\n- * source.  We do not count that in the following routine, so we are\n- * undercounting the source material that remains in the final output\n- * that way.\n+ * is the extent of damage.\n  */\n int count_delta(void *delta_buf, unsigned long delta_size,\n \t\tunsigned long *src_copied, unsigned long *literal_added)\n"},{"id":"15885","messageId":"7vlkwjw2ra.fsf@assigned-by-dhcp.cox.net","threadId":"3278","inReplyTo":"Pine.LNX.4.64.0602101017420.5397@localhost.localdomain","subject":"Re: [PATCH] count-delta.c: Match the delta data semantics change in version 3.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-10T17:19:05Z","receivedAt":"2006-02-10T17:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"You are right about that comment.  Thanks.\n"}]}