{"thread":{"id":"1043","subject":"[PATCH] assorted delta code cleanup","startedAt":"2005-06-29T06:49:56Z","lastAt":"2005-06-29T07:32:11Z","messageCount":3,"participants":["Nicolas Pitre","Matthias Urlichs","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"5407","messageId":"Pine.LNX.4.63.0506290246130.1667@localhost.localdomain","threadId":"1043","inReplyTo":null,"subject":"[PATCH] assorted delta code cleanup","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2005-06-29T06:49:56Z","receivedAt":"2005-06-29T06:49:56Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"\nThis is a wrap-up patch including all the cleanups I've done to the\ndelta code and its usage.  The most important change is the \nfactorization of the delta header handling code.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n\ndiff --git a/count-delta.c b/count-delta.c\n--- a/count-delta.c\n+++ b/count-delta.c\n@@ -6,23 +6,9 @@\n #include <stdlib.h>\n #include <string.h>\n #include <limits.h>\n+#include \"delta.h\"\n #include \"count-delta.h\"\n \n-static unsigned long get_hdr_size(const unsigned char **datap)\n-{\n-\tconst unsigned char *data = *datap;\n-\tunsigned char cmd = *data++;\n-\tunsigned long size = cmd & ~0x80;\n-\tint i = 7;\n-\twhile (cmd & 0x80) {\n-\t\tcmd = *data++;\n-\t\tsize |= (cmd & ~0x80) << i;\n-\t\ti += 7;\n-\t}\n-\t*datap = data;\n-\treturn size;\n-}\n-\n /*\n  * NOTE.  We do not _interpret_ delta fully.  As an approximation, we\n  * just count the number of bytes that are copied from the source, and\n@@ -44,15 +30,14 @@ int count_delta(void *delta_buf, unsigne\n \tunsigned char cmd;\n \tunsigned long src_size, dst_size, out;\n \n-\t/* the smallest delta size possible is 4 bytes */\n-\tif (delta_size < 4)\n+\tif (delta_size < DELTA_SIZE_MIN)\n \t\treturn -1;\n \n \tdata = delta_buf;\n \ttop = delta_buf + delta_size;\n \n-\tsrc_size = get_hdr_size(&data);\n-\tdst_size = get_hdr_size(&data);\n+\tsrc_size = get_delta_hdr_size(&data);\n+\tdst_size = get_delta_hdr_size(&data);\n \n \tadded_literal = copied_from_source = out = 0;\n \twhile (data < top) {\ndiff --git a/delta.h b/delta.h\n--- a/delta.h\n+++ b/delta.h\n@@ -9,4 +9,26 @@ extern void *patch_delta(void *src_buf, \n \t\t\t void *delta_buf, unsigned long delta_size,\n \t\t\t unsigned long *dst_size);\n \n+/* the smallest possible delta size is 4 bytes */\n+#define DELTA_SIZE_MIN\t4\n+\n+/*\n+ * This must be called twice on the delta data buffer, first to get the\n+ * expected reference buffer size, and again to get the result buffer size.\n+ */\n+static inline unsigned long get_delta_hdr_size(const unsigned char **datap)\n+{\n+\tconst unsigned char *data = *datap;\n+\tunsigned char cmd = *data++;\n+\tunsigned long size = cmd & ~0x80;\n+\tint i = 7;\n+\twhile (cmd & 0x80) {\n+\t\tcmd = *data++;\n+\t\tsize |= (cmd & ~0x80) << i;\n+\t\ti += 7;\n+\t}\n+\t*datap = data;\n+\treturn size;\n+}\n+\n #endif\ndiff --git a/diff-delta.c b/diff-delta.c\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -306,12 +306,13 @@ void *diff_delta(void *from_buf, unsigne\n \t\t\t*orig = i;\n \t\t}\n \n-\t\t/* next time around the largest possible output is 1 + 4 + 3 */\n \t\tif (max_size && outpos > max_size) {\n \t\t\tfree(out);\n \t\t\tdelta_cleanup(&bdf);\n \t\t\treturn NULL;\n \t\t}\n+\n+\t\t/* next time around the largest possible output is 1 + 4 + 3 */\n \t\tif (outpos > outsize - 8) {\n \t\t\tvoid *tmp = out;\n \t\t\toutsize = outsize * 3 / 2;\ndiff --git a/pack-objects.c b/pack-objects.c\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -34,7 +34,7 @@ static void *delta_against(void *buf, un\n \tif (!otherbuf)\n \t\tdie(\"unable to read %s\", sha1_to_hex(entry->delta->sha1));\n         delta_buf = diff_delta(otherbuf, othersize,\n-\t\t\t       buf, size, &delta_size, ~0UL);\n+\t\t\t       buf, size, &delta_size, 0);\n         if (!delta_buf || delta_size != entry->delta_size)\n         \tdie(\"delta size changed\");\n         free(buf);\ndiff --git a/patch-delta.c b/patch-delta.c\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -20,36 +20,20 @@ void *patch_delta(void *src_buf, unsigne\n \tconst unsigned char *data, *top;\n \tunsigned char *dst_buf, *out, cmd;\n \tunsigned long size;\n-\tint i;\n \n-\t/* the smallest delta size possible is 4 bytes */\n-\tif (delta_size < 4)\n+\tif (delta_size < DELTA_SIZE_MIN)\n \t\treturn NULL;\n \n \tdata = delta_buf;\n \ttop = delta_buf + delta_size;\n \n \t/* make sure the orig file size matches what we expect */\n-\tcmd = *data++;\n-\tsize = cmd & ~0x80;\n-\ti = 7;\n-\twhile (cmd & 0x80) {\n-\t\tcmd = *data++;\n-\t\tsize |= (cmd & ~0x80) << i;\n-\t\ti += 7;\n-\t}\n+\tsize = get_delta_hdr_size(&data);\n \tif (size != src_size)\n \t\treturn NULL;\n \n \t/* now the result size */\n-\tcmd = *data++;\n-\tsize = cmd & ~0x80;\n-\ti = 7;\n-\twhile (cmd & 0x80) {\n-\t\tcmd = *data++;\n-\t\tsize |= (cmd & ~0x80) << i;\n-\t\ti += 7;\n-\t}\n+\tsize = get_delta_hdr_size(&data);\n \tdst_buf = malloc(size);\n \tif (!dst_buf)\n \t\treturn NULL;\ndiff --git a/sha1_file.c b/sha1_file.c\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -592,22 +592,6 @@ void * unpack_sha1_file(void *map, unsig\n \treturn unpack_sha1_rest(&stream, hdr, *size);\n }\n \n-static unsigned long parse_delta_size(unsigned char **p)\n-{\n-\tunsigned char c;\n-\tunsigned long size = 0;\n-\tunsigned shift = 0;\n-\tunsigned char *data = *p;\n-\n-\tdo {\n-\t\tc = *data++;\n-\t\tsize += (c & 0x7f) << shift;\n-\t\tshift += 7;\n-\t} while (c & 0x80);\n-\t*p = data;\n-\treturn size;\n-}\n-\n static int packed_delta_info(unsigned char *base_sha1,\n \t\t\t     unsigned long delta_size,\n \t\t\t     unsigned long left,\n@@ -645,11 +629,12 @@ static int packed_delta_info(unsigned ch\n \t * the result size.  Verify the base size while we are at it.\n \t */\n \tdata = delta_head;\n-\tverify_base_size = parse_delta_size(&data);\n-\tresult_size = parse_delta_size(&data);\n+\tverify_base_size = get_delta_hdr_size(&data);\n \tif (verify_base_size != base_size)\n \t\tdie(\"delta base size mismatch\");\n \n+\t/* Read the result size */\n+\tresult_size = get_delta_hdr_size(&data);\n \t*sizep = result_size;\n \treturn 0;\n }\n"},{"id":"5408","messageId":"pan.2005.06.29.07.10.16.473252@smurf.noris.de","threadId":"1043","inReplyTo":"Pine.LNX.4.63.0506290246130.1667@localhost.localdomain","subject":"Re: [PATCH] assorted delta code cleanup","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-06-29T07:10:20Z","receivedAt":"2005-06-29T07:10:20Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Nicolas Pitre wrote:\n\n> This is a wrap-up patch including all the cleanups I've done to the\n> delta code and its usage. \n\nFWIW, I think Linus said he'd prefer a do/while loop. In fact this old\ncode ...\n\n> -static unsigned long parse_delta_size(unsigned char **p)\n> -{\n> -\tunsigned char c;\n> -\tunsigned long size = 0;\n> -\tunsigned shift = 0;\n> -\tunsigned char *data = *p;\n> -\n> -\tdo {\n> -\t\tc = *data++;\n> -\t\tsize += (c & 0x7f) << shift;\n> -\t\tshift += 7;\n> -\t} while (c & 0x80);\n> -\t*p = data;\n> -\treturn size;\n> -}\n> -\n\n... is IMHO much more readable than this ...\n\n> +static inline unsigned long get_delta_hdr_size(const unsigned char **datap)\n> +{\n> +\tconst unsigned char *data = *datap;\n> +\tunsigned char cmd = *data++;\n> +\tunsigned long size = cmd & ~0x80;\n> +\tint i = 7;\n> +\twhile (cmd & 0x80) {\n> +\t\tcmd = *data++;\n> +\t\tsize |= (cmd & ~0x80) << i;\n> +\t\ti += 7;\n> +\t}\n> +\t*datap = data;\n> +\treturn size;\n> +}\n> +\n\n(except that the first += should be |= ).\n\nYeah, I know, it's nitpicking. Regard it as a testimony to the code's\nquality that I didn't see worse. ;-)\n\nNB, it might be a good idea to exit with an error if the shift exceeds 31.\n\n-- \nMatthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nWhen it's fall in New York, the air smells as if someone's been frying\ngoats in it, and if you are keen to breathe the best plan is to open a\nwindow and stick your head in a building.\n\t\t-- The Hitch Hiker's Guide to the Galaxy\n"},{"id":"5410","messageId":"7vpsu5ehz8.fsf@assigned-by-dhcp.cox.net","threadId":"1043","inReplyTo":"Pine.LNX.4.63.0506290246130.1667@localhost.localdomain","subject":"[PATCH] Fixlets on top of Nico's clean-up.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-06-29T07:32:11Z","receivedAt":"2005-06-29T07:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Do we care about a corner case where delta_size is not smaller\nthan 4 bytes, *datap passed to get_delta_hdr_size() points at\nsome bytes before a mmaped page boundary, and all the bytes til\nthe end of that page have high-bit set?  The said inline\nfunction would step over the page boundary and would presumably\nsegfault/sigbus.\n\nOther than that, with the attached patch on top of it, I find\nyour clean-up very pleasant to read.  Please consider it acked.\n\n------------\nIf we prefer 0 as maxsize for diff_delta() to say \"unlimited\",\nlet's be consistent about it.\n\nThis patch also fixes type mismatch in a call to get_delta_hdr_size() \nfrom packed_delta_info().\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n diffcore-break.c |    2 +-\n sha1_file.c      |   11 ++++-------\n test-delta.c     |    2 +-\n 3 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -65,7 +65,7 @@ static int should_break(struct diff_file\n \n \tdelta = diff_delta(src->data, src->size,\n \t\t\t   dst->data, dst->size,\n-\t\t\t   &delta_size, ~0UL);\n+\t\t\t   &delta_size, 0);\n \n \t/* Estimate the edit size by interpreting delta. */\n \tif (count_delta(delta, delta_size,\ndiff --git a/sha1_file.c b/sha1_file.c\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -598,9 +598,9 @@ static int packed_delta_info(unsigned ch\n \t\t\t     char *type,\n \t\t\t     unsigned long *sizep)\n {\n-\tunsigned char *data;\n+\tconst unsigned char *data;\n \tunsigned char delta_head[64];\n-\tunsigned long data_size, result_size, base_size, verify_base_size;\n+\tunsigned long result_size, base_size, verify_base_size;\n \tz_stream stream;\n \tint st;\n \n@@ -609,13 +609,10 @@ static int packed_delta_info(unsigned ch\n \tif (sha1_object_info(base_sha1, type, &base_size))\n \t\tdie(\"cannot get info for delta-pack base\");\n \n-\tdata = base_sha1 + 20;\n-\tdata_size = left - 20;\n-\n \tmemset(&stream, 0, sizeof(stream));\n \n-\tstream.next_in = data;\n-\tstream.avail_in = data_size;\n+\tdata = stream.next_in = base_sha1 + 20;\n+\tstream.avail_in = left - 20;\n \tstream.next_out = delta_head;\n \tstream.avail_out = sizeof(delta_head);\n \ndiff --git a/test-delta.c b/test-delta.c\n--- a/test-delta.c\n+++ b/test-delta.c\n@@ -61,7 +61,7 @@ int main(int argc, char *argv[])\n \tif (argv[1][1] == 'd')\n \t\tout_buf = diff_delta(from_buf, from_size,\n \t\t\t\t     data_buf, data_size,\n-\t\t\t\t     &out_size, ~0UL);\n+\t\t\t\t     &out_size, 0);\n \telse\n \t\tout_buf = patch_delta(from_buf, from_size,\n \t\t\t\t      data_buf, data_size,\n------------------------------------------------\n"}]}