{"thread":{"id":"34956","subject":"[PATCH np/pack-v4] index-pack: tighten object type check based on pack version","startedAt":"2013-09-18T12:25:29Z","lastAt":"2013-09-18T14:39:07Z","messageCount":2,"participants":["Nguyễn Thái Ngọc Duy","Nicolas Pitre"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"227834","messageId":"1379507129-27151-1-git-send-email-pclouds@gmail.com","threadId":"34956","inReplyTo":null,"subject":"[PATCH np/pack-v4] index-pack: tighten object type check based on pack version","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-18T12:25:29Z","receivedAt":"2013-09-18T12:25:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In pack version 4, ref-delta technically could be used to compress any\nobjects including commits and trees (both canonical and v4). But it\ndoes not make sense to do so. It can only lead to performance\ndegradation. Catch those packers.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I could now verify that pack-objects does not compress commits nor\n trees using ref-delta in v4. But perhaps these are a bit too strict?\n Maybe downgrade from die() to warning() and still accept the pack?\n\n builtin/index-pack.c | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex fbf97f0..e4ecf69 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -788,6 +788,12 @@ static void *unpack_raw_entry(struct object_entry *obj,\n \tcase OBJ_BLOB:\n \tcase OBJ_TAG:\n \t\tbreak;\n+\n+\t/*\n+\t * OBJ_PV4_* are all greater than 7, which is the limit of\n+\t * field \"type\" in pack v2. So we do not really need 'if\n+\t * (!packv4) die(\"wrong type\");' here.\n+\t */\n \tcase OBJ_PV4_COMMIT:\n \t\tobj->real_type = OBJ_COMMIT;\n \t\tbreak;\n@@ -1288,6 +1294,16 @@ static void resolve_delta(struct object_entry *delta_obj,\n \thash_sha1_file(result->data, result->size,\n \t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n \tcheck_against_sha1table(delta_obj->idx.sha1);\n+\tif (packv4 &&\n+\t    delta_obj->type == OBJ_REF_DELTA &&\n+\t    delta_obj->real_type == OBJ_TREE)\n+\t\tbad_object(delta_obj->idx.offset,\n+\t\t\t   _(\"ref-delta on a tree is not supported in pack version 4\"));\n+\tif (packv4 &&\n+\t    delta_obj->type == OBJ_REF_DELTA &&\n+\t    delta_obj->real_type == OBJ_COMMIT)\n+\t\tbad_object(delta_obj->idx.offset,\n+\t\t\t   _(\"ref-delta on a commit is not supported in pack version 4\"));\n \tsha1_object(result->data, NULL, result->size, delta_obj->real_type,\n \t\t    delta_obj->idx.sha1);\n \tcounter_lock();\n-- \n1.8.2.83.gc99314b\n"},{"id":"227838","messageId":"alpine.LFD.2.03.1309181024290.20709@syhkavp.arg","threadId":"34956","inReplyTo":"1379507129-27151-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH np/pack-v4] index-pack: tighten object type check based on pack version","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2013-09-18T14:39:07Z","receivedAt":"2013-09-18T14:39:07Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 18 Sep 2013, Nguyễn Thái Ngọc Duy wrote:\n\n> In pack version 4, ref-delta technically could be used to compress any\n> objects including commits and trees (both canonical and v4). But it\n> does not make sense to do so. It can only lead to performance\n> degradation. Catch those packers.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  I could now verify that pack-objects does not compress commits nor\n>  trees using ref-delta in v4. But perhaps these are a bit too strict?\n>  Maybe downgrade from die() to warning() and still accept the pack?\n\nEven then...  There is a difference between an \"invalid\" pack and a \n\"suboptimal\" one.  I don't think we should complain when presented with \nsuboptimal encoding if it is still valid and there is no problem \nactually processing the data correctly.  You never know when this \nalternative encoding, even if suboptimal, might be handy.\n\nRobustness principle: Be conservative in what you send, be liberal in \nwhat you accept.\n\n\nNicolas\n"}]}