{"thread":{"id":"7539","subject":"[PATCH 10/13] update delta handling in write_object() for --pack-limit","startedAt":"2007-04-05T22:38:22Z","lastAt":"2007-04-06T00:43:40Z","messageCount":4,"participants":["Dana How","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":13},"messages":[{"id":"38751","messageId":"56b7f5510704051538na4393d7k5e51ed2a511cc86e@mail.gmail.com","threadId":"7539","inReplyTo":null,"subject":"[PATCH 10/13] update delta handling in write_object() for --pack-limit","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-05T22:38:22Z","receivedAt":"2007-04-05T22:38:22Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"---\n builtin-pack-objects.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n\n\nFrom 7ee505accac1675269d1fcfc7a6e3bcf70792a2f Mon Sep 17 00:00:00 2001\nFrom: Dana How \nDate: Thu, 5 Apr 2007 14:11:21 -0700\nSubject: [PATCH 10/13] update delta handling in write_object() for --pack-limit\n\n---\n builtin-pack-objects.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex ccc2d15..a243eed 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -419,13 +419,17 @@ static off_t write_object(struct sha1file *f,\n \t}\n \n \tif (!to_reuse) {\n+\t\tint usable_delta =\t!entry->delta ? 0 :\n+\t\t\t\t\t!offset_limit ? 1 :\n+\t\t\t\t\tentry->delta->no_write ? 0 :\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-- \n1.5.1.rc2.18.g9c88-dirty\n\n"},{"id":"38761","messageId":"7v3b3emmv6.fsf@assigned-by-dhcp.cox.net","threadId":"7539","inReplyTo":"56b7f5510704051538na4393d7k5e51ed2a511cc86e@mail.gmail.com","subject":"Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-05T23:18:21Z","receivedAt":"2007-04-05T23:18:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dana How\" writes:\n\n> Subject: [PATCH 10/13] update delta handling in write_object() for --pack-limit\n>\n> ---\n> builtin-pack-objects.c | 6 +++++-\n> 1 files changed, 5 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index ccc2d15..a243eed 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -419,13 +419,17 @@ static off_t write_object(struct sha1file *f,\n> \t}\n> \n> \tif (!to_reuse) {\n> +\t\tint usable_delta =\t!entry->delta ? 0 :\n> +\t\t\t\t\t!offset_limit ? 1 :\n> +\t\t\t\t\tentry->delta->no_write ? 0 :\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\nThis really needs explanation on *why*, at least in the commit\nlog, and perhaps also in the code as comment.\n\nHere is my attempt to understand that logic (please make\nnecessary corrections).\n\n (1) When there is no delta base found by the earlier\n find_deltas()/try_delta() loop, obviously we cannot write\n deltified so we write plain.\n\n (2) Otherwise if we are not offset limited, then we keep the\n old way of using that delta base.\n\n (3) If the delta base is not in this pack (because of\n offset-limit chomping the pack into two or more), then we\n cannot use it as the base.\n\n (4) If the delta has already been written, we can use it as the\n base for this object, but otherwise we cannot.\n\n(3) makes me wonder if we can still allow it in the thin-pack\ncase by just loosening the condition here.\n\nAlso I need explanation to understand why (4) is needed --\ndoesn't write_one() always write base object out before writing\na deltified object that depends on it?\n"},{"id":"38763","messageId":"56b7f5510704051641s69c9bdf1t8ce75385456bb4da@mail.gmail.com","threadId":"7539","inReplyTo":"7v3b3emmv6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-05T23:41:03Z","receivedAt":"2007-04-05T23:41:03Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/5/07, Junio C Hamano wrote:\n> \"Dana How\" writes:\n> > if (!to_reuse) {\n> > + int usable_delta = !entry->delta ? 0 :\n> > + !offset_limit ? 1 :\n> > + entry->delta->no_write ? 0 :\n> > + entry->delta->offset ? 1 : 0;\n> > buf = read_sha1_file(entry->sha1, &type, &size);\n> > if (!buf)\n> > die(\"unable to read %s\", sha1_to_hex(entry->sha1));\n> > if (size != entry->size)\n> > die(\"object %s size inconsistency (%lu vs %lu)\",\n> > sha1_to_hex(entry->sha1), size, entry->size);\n> > - if (entry->delta) {\n> > + if (usable_delta) {\n> > buf = delta_against(buf, size, entry);\n> > size = entry->delta_size;\n> > obj_type = (allow_ofs_delta && entry->delta->offset) ?\n>\n> This really needs explanation on *why*, at least in the commit\n> log, and perhaps also in the code as comment.\nYes, I need retraining ;-) I'm accustomed to more expensive\ncommits in which the log message describes the objective,\nnot the details. Or, I could have followed the example of the\ncomments on the initialization for \"to_reuse\".\n\n> Here is my attempt to understand that logic (please make\n> necessary corrections).\n>\n> (1) When there is no delta base found by the earlier\n> find_deltas()/try_delta() loop, obviously we cannot write\n> deltified so we write plain.\n>\n> (2) Otherwise if we are not offset limited, then we keep the\n> old way of using that delta base.\n>\n> (3) If the delta base is not in this pack (because of\n> offset-limit chomping the pack into two or more), then we\n> cannot use it as the base.\n>\n> (4) If the delta has already been written, we can use it as the\n> base for this object, but otherwise we cannot.\n>\n> (3) makes me wonder if we can still allow it in the thin-pack\n> case by just loosening the condition here.\nYou are correct. In response to someone's previous question I think\nI said I hadn't handled the --thin-pack cases yet. Do you\nthink these will matter? There is strong sentiment on this list that a pack\nshould be split at the receiver, not the transmitter, so --thin-pack\nand --pack-limit wouldn't be used together. If you think this\ncombination does matter, I'd prefer to add the extra cases\nin a separate follow-on patch if that's OK.\n\n> Also I need explanation to understand why (4) is needed --\n> doesn't write_one() always write base object out before writing\n> a deltified object that depends on it?\nYes, write_one() does that. I was being too cautious,\nsince I had only known (some of) the code for a few hours!\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"},{"id":"38767","messageId":"alpine.LFD.0.98.0704052040170.28181@xanadu.home","threadId":"7539","inReplyTo":"7v3b3emmv6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 10/13] update delta handling in write_object() for --pack-limit","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-06T00:43:40Z","receivedAt":"2007-04-06T00:43:40Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 5 Apr 2007, Junio C Hamano wrote:\n\n> (3) makes me wonder if we can still allow it in the thin-pack\n> case by just loosening the condition here.\n\nI don't think thin packs apply in the sliced pack usage scenario anyway.\n\n\n\n> \n> Also I need explanation to understand why (4) is needed --\n> doesn't write_one() always write base object out before writing\n> a deltified object that depends on it?\n\nYes it does. This is a requirement for OBJ_OFS_DELTA.\n\n\nNicolas\n"}]}