{"thread":{"id":"30536","subject":"[PATCH 1/2] index-pack: hash non-delta objects while reading from stream","startedAt":"2012-05-16T12:50:29Z","lastAt":"2012-05-24T13:55:44Z","messageCount":16,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"191588","messageId":"1337172630-23679-1-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":null,"subject":"[PATCH 1/2] index-pack: hash non-delta objects while reading from stream","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-16T12:50:29Z","receivedAt":"2012-05-16T12:50:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Preparatory step for large blob support.\n\n builtin/index-pack.c |   43 ++++++++++++++++++++++++++++++++-----------\n 1 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dc2cfe6..ccb0214 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -384,30 +384,55 @@ static void unlink_base_data(struct base_data *c)\n \tfree_base_data(c);\n }\n \n-static void *unpack_entry_data(unsigned long offset, unsigned long size)\n+static int is_delta_type(enum object_type type)\n+{\n+\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n+}\n+\n+static void *unpack_entry_data(unsigned long offset, unsigned long size,\n+\t\t\t       enum object_type type, unsigned char *sha1)\n {\n \tint status;\n \tgit_zstream stream;\n \tvoid *buf = xmalloc(size);\n+\tgit_SHA_CTX c;\n+\tchar hdr[32];\n+\tint hdrlen;\n+\tunsigned char *last_out;\n+\n+\tif (!is_delta_type(type)) {\n+\t\thdrlen = sprintf(hdr, \"%s %lu\", typename(type), size)+1;\n+\t\tgit_SHA1_Init(&c);\n+\t\tgit_SHA1_Update(&c, hdr, hdrlen);\n+\t} else\n+\t\tsha1 = NULL;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n \tstream.next_out = buf;\n \tstream.avail_out = size;\n+\tlast_out = buf;\n \n \tdo {\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = input_len;\n \t\tstatus = git_inflate(&stream, 0);\n \t\tuse(input_len - stream.avail_in);\n+\t\tif (sha1)\n+\t\t\tgit_SHA1_Update(&c, last_out, stream.next_out - last_out);\n+\t\tlast_out = stream.next_out;\n \t} while (status == Z_OK);\n \tif (stream.total_out != size || status != Z_STREAM_END)\n \t\tbad_object(offset, _(\"inflate returned %d\"), status);\n \tgit_inflate_end(&stream);\n+\tif (sha1)\n+\t\tgit_SHA1_Final(sha1, &c);\n \treturn buf;\n }\n \n-static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_base)\n+static void *unpack_raw_entry(struct object_entry *obj,\n+\t\t\t      union delta_base *delta_base,\n+\t\t\t      unsigned char *sha1)\n {\n \tunsigned char *p;\n \tunsigned long size, c;\n@@ -467,7 +492,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_\n \t}\n \tobj->hdr_size = consumed_bytes - obj->idx.offset;\n \n-\tdata = unpack_entry_data(obj->idx.offset, obj->size);\n+\tdata = unpack_entry_data(obj->idx.offset, obj->size, obj->type, sha1);\n \tobj->idx.crc32 = input_crc32;\n \treturn data;\n }\n@@ -569,9 +594,8 @@ static void find_delta_children(const union delta_base *base,\n }\n \n static void sha1_object(const void *data, unsigned long size,\n-\t\t\tenum object_type type, unsigned char *sha1)\n+\t\t\tenum object_type type, const unsigned char *sha1)\n {\n-\thash_sha1_file(data, size, typename(type), sha1);\n \tread_lock();\n \tif (has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n@@ -627,11 +651,6 @@ static void sha1_object(const void *data, unsigned long size,\n \t}\n }\n \n-static int is_delta_type(enum object_type type)\n-{\n-\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n-}\n-\n /*\n  * This function is part of find_unresolved_deltas(). There are two\n  * walkers going in the opposite ways.\n@@ -711,6 +730,8 @@ static void resolve_delta(struct object_entry *delta_obj,\n \tfree(delta_data);\n \tif (!result->data)\n \t\tbad_object(delta_obj->idx.offset, _(\"failed to apply delta\"));\n+\thash_sha1_file(result->data, result->size,\n+\t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n \tsha1_object(result->data, result->size, delta_obj->real_type,\n \t\t    delta_obj->idx.sha1);\n \tcounter_lock();\n@@ -851,7 +872,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *obj = &objects[i];\n-\t\tvoid *data = unpack_raw_entry(obj, &delta->base);\n+\t\tvoid *data = unpack_raw_entry(obj, &delta->base, obj->idx.sha1);\n \t\tobj->real_type = obj->type;\n \t\tif (is_delta_type(obj->type)) {\n \t\t\tnr_deltas++;\n-- \n1.7.8.36.g69ee2\n"},{"id":"191589","messageId":"1337172630-23679-2-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337172630-23679-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] index-pack: use streaming interface on large blobs (most of the time)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-16T12:50:30Z","receivedAt":"2012-05-16T12:50:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"unpack_raw_entry() will not allocate and return decompressed blobs if\nthey are larger than core.bigFileThreshold.\n\nThe blob content is needed by sha1_object() in some cases. When we do\nneed the blob content, we put it back in core with\nget_data_from_pack(). However we rarely need that in pratice.\n\nThe first case is when we find an in-repo blob with the same SHA-1. We\nneed to do collision test, byte-on-byte. Normally (e.g. in\nfetch/pull/clone) this does not happen because git avoid to send\nobjects that client already has.\n\nThe other case is when --strict is specified and the object in\nquestion is not a blob, which can't happen in reality becase we deal\nwith large _blobs_ here.\n\nNote: --verify (or git-verify-pack) a pack from current repository\nwill trigger collision test on every object in the pack, which\neffectively disables this patch. This could be easily worked around by\nsetting GIT_DIR to an imaginary place with no packs.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/index-pack.c |   52 ++++++++++++++++++++++++++++++++++++++++++-------\n t/t1050-large.sh     |    5 ++++\n 2 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex ccb0214..1b790df 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -392,9 +392,10 @@ static int is_delta_type(enum object_type type)\n static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\t\t       enum object_type type, unsigned char *sha1)\n {\n+\tstatic char fixed_buf[8192];\n \tint status;\n \tgit_zstream stream;\n-\tvoid *buf = xmalloc(size);\n+\tvoid *buf;\n \tgit_SHA_CTX c;\n \tchar hdr[32];\n \tint hdrlen;\n@@ -406,11 +407,15 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\tgit_SHA1_Update(&c, hdr, hdrlen);\n \t} else\n \t\tsha1 = NULL;\n+\tif (type == OBJ_BLOB && size > big_file_threshold)\n+\t\tbuf = fixed_buf;\n+\telse\n+\t\tbuf = xmalloc(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n \tstream.next_out = buf;\n-\tstream.avail_out = size;\n+\tstream.avail_out = buf == fixed_buf ? sizeof(fixed_buf) : size;\n \tlast_out = buf;\n \n \tdo {\n@@ -420,6 +425,10 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\tuse(input_len - stream.avail_in);\n \t\tif (sha1)\n \t\t\tgit_SHA1_Update(&c, last_out, stream.next_out - last_out);\n+\t\tif (buf == fixed_buf) {\n+\t\t\tstream.next_out = buf;\n+\t\t\tstream.avail_out = sizeof(fixed_buf);\n+\t\t}\n \t\tlast_out = stream.next_out;\n \t} while (status == Z_OK);\n \tif (stream.total_out != size || status != Z_STREAM_END)\n@@ -427,7 +436,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \tgit_inflate_end(&stream);\n \tif (sha1)\n \t\tgit_SHA1_Final(sha1, &c);\n-\treturn buf;\n+\treturn buf == fixed_buf ? NULL : buf;\n }\n \n static void *unpack_raw_entry(struct object_entry *obj,\n@@ -593,14 +602,21 @@ static void find_delta_children(const union delta_base *base,\n \t*last_index = last;\n }\n \n-static void sha1_object(const void *data, unsigned long size,\n-\t\t\tenum object_type type, const unsigned char *sha1)\n+static void sha1_object(const void *data, struct object_entry *obj_entry,\n+\t\t\tunsigned long size, enum object_type type,\n+\t\t\tconst unsigned char *sha1)\n {\n+\tvoid *new_data = NULL;\n+\n+\tassert(data || obj_entry);\n+\n \tread_lock();\n \tif (has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n+\t\tif (!data)\n+\t\t\tdata = new_data = get_data_from_pack(obj_entry);\n \t\thas_data = read_sha1_file(sha1, &has_type, &has_size);\n \t\tread_unlock();\n \t\tif (!has_data)\n@@ -625,6 +641,9 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t\tint eaten;\n \t\t\tvoid *buf = (void *) data;\n \n+\t\t\tif (!buf)\n+\t\t\t\tbuf = new_data = get_data_from_pack(obj_entry);\n+\n \t\t\t/*\n \t\t\t * we do not need to free the memory here, as the\n \t\t\t * buf is deleted by the caller.\n@@ -649,6 +668,8 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t}\n \t\tread_unlock();\n \t}\n+\n+\tfree(new_data);\n }\n \n /*\n@@ -732,7 +753,7 @@ static void resolve_delta(struct object_entry *delta_obj,\n \t\tbad_object(delta_obj->idx.offset, _(\"failed to apply delta\"));\n \thash_sha1_file(result->data, result->size,\n \t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n-\tsha1_object(result->data, result->size, delta_obj->real_type,\n+\tsha1_object(result->data, NULL, result->size, delta_obj->real_type,\n \t\t    delta_obj->idx.sha1);\n \tcounter_lock();\n \tnr_resolved_deltas++;\n@@ -862,7 +883,7 @@ static void *threaded_second_pass(void *data)\n  */\n static void parse_pack_objects(unsigned char *sha1)\n {\n-\tint i;\n+\tint i, nr_delays = 0;\n \tstruct delta_entry *delta = deltas;\n \tstruct stat st;\n \n@@ -878,8 +899,12 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\tnr_deltas++;\n \t\t\tdelta->obj_no = i;\n \t\t\tdelta++;\n+\t\t} else if (!data) {\n+\t\t\t/* large blobs, check later */\n+\t\t\tobj->real_type = OBJ_BAD;\n+\t\t\tnr_delays++;\n \t\t} else\n-\t\t\tsha1_object(data, obj->size, obj->type, obj->idx.sha1);\n+\t\t\tsha1_object(data, NULL, obj->size, obj->type, obj->idx.sha1);\n \t\tfree(data);\n \t\tdisplay_progress(progress, i+1);\n \t}\n@@ -899,6 +924,17 @@ static void parse_pack_objects(unsigned char *sha1)\n \tif (S_ISREG(st.st_mode) &&\n \t\t\tlseek(input_fd, 0, SEEK_CUR) - input_len != st.st_size)\n \t\tdie(_(\"pack has junk at the end\"));\n+\n+\tfor (i = 0; i < nr_objects; i++) {\n+\t\tstruct object_entry *obj = &objects[i];\n+\t\tif (obj->real_type != OBJ_BAD)\n+\t\t\tcontinue;\n+\t\tobj->real_type = obj->type;\n+\t\tsha1_object(NULL, obj, obj->size, obj->type, obj->idx.sha1);\n+\t\tnr_delays--;\n+\t}\n+\tif (nr_delays)\n+\t\tdie(_(\"confusion beyond insanity in parse_pack_objects()\"));\n }\n \n /*\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 55ed955..3f80688 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -130,6 +130,11 @@ test_expect_success 'git-show a large file' '\n \n '\n \n+test_expect_success 'index-pack' '\n+\tgit clone file://\"`pwd`\"/.git foo &&\n+\tGIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack\n+'\n+\n test_expect_success 'repack' '\n \tgit repack -ad\n '\n-- \n1.7.8.36.g69ee2\n"},{"id":"191683","messageId":"xmqq4nrd2l1c.fsf@junio.mtv.corp.google.com","threadId":"30536","inReplyTo":"1337172630-23679-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] index-pack: hash non-delta objects while reading from stream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-18T22:15:27Z","receivedAt":"2012-05-18T22:15:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> +static void *unpack_entry_data(unsigned long offset, unsigned long size,\n> +\t\t\t       enum object_type type, unsigned char *sha1)\n>  {\n>  \tint status;\n>  \tgit_zstream stream;\n>  \tvoid *buf = xmalloc(size);\n> +\tgit_SHA_CTX c;\n> +\tchar hdr[32];\n> +\tint hdrlen;\n> +\tunsigned char *last_out;\n> +\n> +\tif (!is_delta_type(type)) {\n> +\t\thdrlen = sprintf(hdr, \"%s %lu\", typename(type), size)+1;\n\ns/+/ & /;\n\n> +\t\tgit_SHA1_Init(&c);\n> +\t\tgit_SHA1_Update(&c, hdr, hdrlen);\n> +\t} else\n> +\t\tsha1 = NULL;\n> ...\n>  \tmemset(&stream, 0, sizeof(stream));\n>  \tgit_inflate_init(&stream);\n>  \tstream.next_out = buf;\n>  \tstream.avail_out = size;\n> +\tlast_out = buf;\n>  \n>  \tdo {\n>  \t\tstream.next_in = fill(1);\n>  \t\tstream.avail_in = input_len;\n>  \t\tstatus = git_inflate(&stream, 0);\n>  \t\tuse(input_len - stream.avail_in);\n> +\t\tif (sha1)\n> +\t\t\tgit_SHA1_Update(&c, last_out, stream.next_out - last_out);\n> +\t\tlast_out = stream.next_out;\n>  \t} while (status == Z_OK);\n\nShouldn't \"last_out\" be scoped inside the loop, perhaps like this,\n\n\tdo {\n\t\tunsigned char *boo = stream.next_out;\n\t\tstream.next_in = fill(1);\n\t\tstream.avail_in = input_len;\n\t\tstatus = git_inflate(&stream, 0);\n\t\tuse(input_len - stream.avail_in);\n\t\tif (sha1)\n\t\t\tgit_SHA1_Update(&c, boo, stream.next_out - boo);\n\t} while (status == Z_OK);\n\ninstead of leaking it to the whole function?\n\n> @@ -711,6 +730,8 @@ static void resolve_delta(struct object_entry *delta_obj,\n>  \tfree(delta_data);\n>  \tif (!result->data)\n>  \t\tbad_object(delta_obj->idx.offset, _(\"failed to apply delta\"));\n> +\thash_sha1_file(result->data, result->size,\n> +\t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n>  \tsha1_object(result->data, result->size, delta_obj->real_type,\n>  \t\t    delta_obj->idx.sha1);\n>  \tcounter_lock();\n\nThe call-chain after your patch looks like this:\n\n    parse_pack_objects()\n    . for each incoming object \n      -> unpack_raw_entry()\n\t . check type\n         -> unpack_entry_data()\n            . check type\n            . unpack to obtain the data while hashing\n\t      if the type can be hashed.\n      . check type\n      . if the data is complete, let sha1_object() check it\n\nThe updated sha1_object() no longer hashes but the caller is responsible\nfor giving a correct hash value for the data, so you added a call to\nhash_sha1_file() to the other caller (i.e resolve_delta()) of the\nfunction.\n\nThe flow of the resulting code feels somewhat iffy that two functions\nthat are very far apart in the callchain have to coordinate among\nthemselves to ensure that the object data is always hashed once: plain\nones in unpack_entry_data() and deltified ones in resolve_delta().  It\nsmells like inviting future bugs when we introduce new in-pack types\nthat are neither delta or plain.\n\nOverall it looks correct.  Thanks.\n"},{"id":"191684","messageId":"xmqqzk95168w.fsf@junio.mtv.corp.google.com","threadId":"30536","inReplyTo":"1337172630-23679-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] index-pack: use streaming interface on large blobs (most of the time)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-18T22:20:15Z","receivedAt":"2012-05-18T22:20:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> unpack_raw_entry() will not allocate and return decompressed blobs if\n> they are larger than core.bigFileThreshold.\n>\n> The blob content is needed by sha1_object() in some cases. When we do\n> need the blob content, we put it back in core with\n> get_data_from_pack(). However we rarely need that in pratice.\n>\n> The first case is when we find an in-repo blob with the same SHA-1. We\n> need to do collision test, byte-on-byte. Normally (e.g. in\n> fetch/pull/clone) this does not happen because git avoid to send\n> objects that client already has.\n\nPerhaps the codepath that performs the byte-for-byte comparison can be\ntaught to stream from the received pack data and whatever was already\nin the repository, using the streaming interface?  That way you do not\nhave to hold all of the both objects at the same time in core, no?\n"},{"id":"191721","messageId":"CACsJy8CypvcS3sTn1isREYzApN273PcKnMJBpWBrOaO2ZaSLSw@mail.gmail.com","threadId":"30536","inReplyTo":"xmqqzk95168w.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 2/2] index-pack: use streaming interface on large blobs (most of the time)","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-19T05:31:57Z","receivedAt":"2012-05-19T05:31:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 19, 2012 at 5:20 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> unpack_raw_entry() will not allocate and return decompressed blobs if\n>> they are larger than core.bigFileThreshold.\n>>\n>> The blob content is needed by sha1_object() in some cases. When we do\n>> need the blob content, we put it back in core with\n>> get_data_from_pack(). However we rarely need that in pratice.\n>>\n>> The first case is when we find an in-repo blob with the same SHA-1. We\n>> need to do collision test, byte-on-byte. Normally (e.g. in\n>> fetch/pull/clone) this does not happen because git avoid to send\n>> objects that client already has.\n>\n> Perhaps the codepath that performs the byte-for-byte comparison can be\n> taught to stream from the received pack data and whatever was already\n> in the repository, using the streaming interface?  That way you do not\n> have to hold all of the both objects at the same time in core, no?\n\nSure. But that does not happen until you are attacked. Maybe later.\n\nverify-pack'ing also runs into this, but that's another issue and\nshould be fixed separately (hopefully by verify-pack users).\n-- \nDuy\n"},{"id":"191981","messageId":"1337782191-10091-1-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337172630-23679-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/6] index-pack: hash non-delta objects while reading from stream","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:46Z","receivedAt":"2012-05-23T14:09:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n fix spaces around \"+ 1\" operator in unpack_entry_data and last_out's scope.\n\n builtin/index-pack.c | 41 ++++++++++++++++++++++++++++++-----------\n 1 file changed, 30 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dc2cfe6..a744856 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -384,11 +384,27 @@ static void unlink_base_data(struct base_data *c)\n \tfree_base_data(c);\n }\n \n-static void *unpack_entry_data(unsigned long offset, unsigned long size)\n+static int is_delta_type(enum object_type type)\n+{\n+\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n+}\n+\n+static void *unpack_entry_data(unsigned long offset, unsigned long size,\n+\t\t\t       enum object_type type, unsigned char *sha1)\n {\n \tint status;\n \tgit_zstream stream;\n \tvoid *buf = xmalloc(size);\n+\tgit_SHA_CTX c;\n+\tchar hdr[32];\n+\tint hdrlen;\n+\n+\tif (!is_delta_type(type)) {\n+\t\thdrlen = sprintf(hdr, \"%s %lu\", typename(type), size) + 1;\n+\t\tgit_SHA1_Init(&c);\n+\t\tgit_SHA1_Update(&c, hdr, hdrlen);\n+\t} else\n+\t\tsha1 = NULL;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -396,18 +412,25 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)\n \tstream.avail_out = size;\n \n \tdo {\n+\t\tunsigned char *last_out = stream.next_out;\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = input_len;\n \t\tstatus = git_inflate(&stream, 0);\n \t\tuse(input_len - stream.avail_in);\n+\t\tif (sha1)\n+\t\t\tgit_SHA1_Update(&c, last_out, stream.next_out - last_out);\n \t} while (status == Z_OK);\n \tif (stream.total_out != size || status != Z_STREAM_END)\n \t\tbad_object(offset, _(\"inflate returned %d\"), status);\n \tgit_inflate_end(&stream);\n+\tif (sha1)\n+\t\tgit_SHA1_Final(sha1, &c);\n \treturn buf;\n }\n \n-static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_base)\n+static void *unpack_raw_entry(struct object_entry *obj,\n+\t\t\t      union delta_base *delta_base,\n+\t\t\t      unsigned char *sha1)\n {\n \tunsigned char *p;\n \tunsigned long size, c;\n@@ -467,7 +490,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_\n \t}\n \tobj->hdr_size = consumed_bytes - obj->idx.offset;\n \n-\tdata = unpack_entry_data(obj->idx.offset, obj->size);\n+\tdata = unpack_entry_data(obj->idx.offset, obj->size, obj->type, sha1);\n \tobj->idx.crc32 = input_crc32;\n \treturn data;\n }\n@@ -569,9 +592,8 @@ static void find_delta_children(const union delta_base *base,\n }\n \n static void sha1_object(const void *data, unsigned long size,\n-\t\t\tenum object_type type, unsigned char *sha1)\n+\t\t\tenum object_type type, const unsigned char *sha1)\n {\n-\thash_sha1_file(data, size, typename(type), sha1);\n \tread_lock();\n \tif (has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n@@ -627,11 +649,6 @@ static void sha1_object(const void *data, unsigned long size,\n \t}\n }\n \n-static int is_delta_type(enum object_type type)\n-{\n-\treturn (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);\n-}\n-\n /*\n  * This function is part of find_unresolved_deltas(). There are two\n  * walkers going in the opposite ways.\n@@ -711,6 +728,8 @@ static void resolve_delta(struct object_entry *delta_obj,\n \tfree(delta_data);\n \tif (!result->data)\n \t\tbad_object(delta_obj->idx.offset, _(\"failed to apply delta\"));\n+\thash_sha1_file(result->data, result->size,\n+\t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n \tsha1_object(result->data, result->size, delta_obj->real_type,\n \t\t    delta_obj->idx.sha1);\n \tcounter_lock();\n@@ -851,7 +870,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\t\tnr_objects);\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tstruct object_entry *obj = &objects[i];\n-\t\tvoid *data = unpack_raw_entry(obj, &delta->base);\n+\t\tvoid *data = unpack_raw_entry(obj, &delta->base, obj->idx.sha1);\n \t\tobj->real_type = obj->type;\n \t\tif (is_delta_type(obj->type)) {\n \t\t\tnr_deltas++;\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191982","messageId":"1337782191-10091-2-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/6] index-pack: use streaming interface on large blobs (most of the time)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:47Z","receivedAt":"2012-05-23T14:09:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"unpack_raw_entry() will not allocate and return decompressed blobs if\nthey are larger than core.bigFileThreshold. sha1_object() may not be\ncalled on those objects because there's no actual content.\n\nsha1_object() is called later on those objects, where we can safely\nuse get_data_from_pack() to retrieve blob content for checking.\nHowever we always do that when we definitely need the blob\ncontent. And we often don't.\n\nThere are two cases when we may need object content. The first case is\nwhen we find an in-repo blob with the same SHA-1. We need to do\ncollision test, byte-on-byte. If this test is on, the blob must be\nloaded on memory (i.e. no streaming). Normally (e.g. in\nfetch/pull/clone) this does not happen because git avoid to send\nobjects that client already has.\n\nThe other case is when --strict is specified and the object in\nquestion is not a blob, which can't happen in reality becase we deal\nwith large _blobs_ here.\n\nNote: --verify (or git-verify-pack) a pack from current repository\nwill trigger collision test on every object in the pack, which\neffectively disables this patch. This could be easily worked around by\nsetting GIT_DIR to an imaginary place with no packs.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/index-pack.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n t/t1050-large.sh     |  5 +++++\n 2 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a744856..c7c2b88 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -392,9 +392,10 @@ static int is_delta_type(enum object_type type)\n static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\t\t       enum object_type type, unsigned char *sha1)\n {\n+\tstatic char fixed_buf[8192];\n \tint status;\n \tgit_zstream stream;\n-\tvoid *buf = xmalloc(size);\n+\tvoid *buf;\n \tgit_SHA_CTX c;\n \tchar hdr[32];\n \tint hdrlen;\n@@ -405,11 +406,15 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\tgit_SHA1_Update(&c, hdr, hdrlen);\n \t} else\n \t\tsha1 = NULL;\n+\tif (type == OBJ_BLOB && size > big_file_threshold)\n+\t\tbuf = fixed_buf;\n+\telse\n+\t\tbuf = xmalloc(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n \tstream.next_out = buf;\n-\tstream.avail_out = size;\n+\tstream.avail_out = buf == fixed_buf ? sizeof(fixed_buf) : size;\n \n \tdo {\n \t\tunsigned char *last_out = stream.next_out;\n@@ -419,13 +424,17 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\tuse(input_len - stream.avail_in);\n \t\tif (sha1)\n \t\t\tgit_SHA1_Update(&c, last_out, stream.next_out - last_out);\n+\t\tif (buf == fixed_buf) {\n+\t\t\tstream.next_out = buf;\n+\t\t\tstream.avail_out = sizeof(fixed_buf);\n+\t\t}\n \t} while (status == Z_OK);\n \tif (stream.total_out != size || status != Z_STREAM_END)\n \t\tbad_object(offset, _(\"inflate returned %d\"), status);\n \tgit_inflate_end(&stream);\n \tif (sha1)\n \t\tgit_SHA1_Final(sha1, &c);\n-\treturn buf;\n+\treturn buf == fixed_buf ? NULL : buf;\n }\n \n static void *unpack_raw_entry(struct object_entry *obj,\n@@ -591,14 +600,21 @@ static void find_delta_children(const union delta_base *base,\n \t*last_index = last;\n }\n \n-static void sha1_object(const void *data, unsigned long size,\n-\t\t\tenum object_type type, const unsigned char *sha1)\n+static void sha1_object(const void *data, struct object_entry *obj_entry,\n+\t\t\tunsigned long size, enum object_type type,\n+\t\t\tconst unsigned char *sha1)\n {\n+\tvoid *new_data = NULL;\n+\n+\tassert(data || obj_entry);\n+\n \tread_lock();\n \tif (has_sha1_file(sha1)) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n+\t\tif (!data)\n+\t\t\tdata = new_data = get_data_from_pack(obj_entry);\n \t\thas_data = read_sha1_file(sha1, &has_type, &has_size);\n \t\tread_unlock();\n \t\tif (!has_data)\n@@ -623,6 +639,9 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t\tint eaten;\n \t\t\tvoid *buf = (void *) data;\n \n+\t\t\tif (!buf)\n+\t\t\t\tbuf = new_data = get_data_from_pack(obj_entry);\n+\n \t\t\t/*\n \t\t\t * we do not need to free the memory here, as the\n \t\t\t * buf is deleted by the caller.\n@@ -647,6 +666,8 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t}\n \t\tread_unlock();\n \t}\n+\n+\tfree(new_data);\n }\n \n /*\n@@ -730,7 +751,7 @@ static void resolve_delta(struct object_entry *delta_obj,\n \t\tbad_object(delta_obj->idx.offset, _(\"failed to apply delta\"));\n \thash_sha1_file(result->data, result->size,\n \t\t       typename(delta_obj->real_type), delta_obj->idx.sha1);\n-\tsha1_object(result->data, result->size, delta_obj->real_type,\n+\tsha1_object(result->data, NULL, result->size, delta_obj->real_type,\n \t\t    delta_obj->idx.sha1);\n \tcounter_lock();\n \tnr_resolved_deltas++;\n@@ -860,7 +881,7 @@ static void *threaded_second_pass(void *data)\n  */\n static void parse_pack_objects(unsigned char *sha1)\n {\n-\tint i;\n+\tint i, nr_delays = 0;\n \tstruct delta_entry *delta = deltas;\n \tstruct stat st;\n \n@@ -876,8 +897,12 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\tnr_deltas++;\n \t\t\tdelta->obj_no = i;\n \t\t\tdelta++;\n+\t\t} else if (!data) {\n+\t\t\t/* large blobs, check later */\n+\t\t\tobj->real_type = OBJ_BAD;\n+\t\t\tnr_delays++;\n \t\t} else\n-\t\t\tsha1_object(data, obj->size, obj->type, obj->idx.sha1);\n+\t\t\tsha1_object(data, NULL, obj->size, obj->type, obj->idx.sha1);\n \t\tfree(data);\n \t\tdisplay_progress(progress, i+1);\n \t}\n@@ -897,6 +922,17 @@ static void parse_pack_objects(unsigned char *sha1)\n \tif (S_ISREG(st.st_mode) &&\n \t\t\tlseek(input_fd, 0, SEEK_CUR) - input_len != st.st_size)\n \t\tdie(_(\"pack has junk at the end\"));\n+\n+\tfor (i = 0; i < nr_objects; i++) {\n+\t\tstruct object_entry *obj = &objects[i];\n+\t\tif (obj->real_type != OBJ_BAD)\n+\t\t\tcontinue;\n+\t\tobj->real_type = obj->type;\n+\t\tsha1_object(NULL, obj, obj->size, obj->type, obj->idx.sha1);\n+\t\tnr_delays--;\n+\t}\n+\tif (nr_delays)\n+\t\tdie(_(\"confusion beyond insanity in parse_pack_objects()\"));\n }\n \n /*\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 55ed955..3f80688 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -130,6 +130,11 @@ test_expect_success 'git-show a large file' '\n \n '\n \n+test_expect_success 'index-pack' '\n+\tgit clone file://\"`pwd`\"/.git foo &&\n+\tGIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack\n+'\n+\n test_expect_success 'repack' '\n \tgit repack -ad\n '\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191983","messageId":"1337782191-10091-3-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/6] index-pack: factor out unpack core from get_data_from_pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:48Z","receivedAt":"2012-05-23T14:09:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This allows caller to consume large inflated object with a fixed\namount of memory.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n for streaming collision test..\n\n builtin/index-pack.c | 27 ++++++++++++++++++++++++---\n 1 file changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex c7c2b88..9129299 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -504,7 +504,9 @@ static void *unpack_raw_entry(struct object_entry *obj,\n \treturn data;\n }\n \n-static void *get_data_from_pack(struct object_entry *obj)\n+static void *unpack_data(struct object_entry *obj,\n+\t\t\t int (*consume)(const unsigned char *, unsigned long, void *),\n+\t\t\t void *cb_data)\n {\n \toff_t from = obj[0].idx.offset + obj[0].hdr_size;\n \tunsigned long len = obj[1].idx.offset - from;\n@@ -512,15 +514,16 @@ static void *get_data_from_pack(struct object_entry *obj)\n \tgit_zstream stream;\n \tint status;\n \n-\tdata = xmalloc(obj->size);\n+\tdata = xmalloc(consume ? 64*1024 : obj->size);\n \tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n \tstream.next_out = data;\n-\tstream.avail_out = obj->size;\n+\tstream.avail_out = consume ? 64*1024 : obj->size;\n \n \tdo {\n+\t\tunsigned char *last_out = stream.next_out;\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n \t\tn = pread(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n@@ -535,6 +538,15 @@ static void *get_data_from_pack(struct object_entry *obj)\n \t\tstream.next_in = inbuf;\n \t\tstream.avail_in = n;\n \t\tstatus = git_inflate(&stream, 0);\n+\t\tif (consume) {\n+\t\t\tif (consume(last_out, stream.next_out - last_out, cb_data)) {\n+\t\t\t\tfree(inbuf);\n+\t\t\t\tfree(data);\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\tstream.next_out = data;\n+\t\t\tstream.avail_out = 64*1024;\n+\t\t}\n \t} while (len && status == Z_OK && !stream.avail_in);\n \n \t/* This has been inflated OK when first encountered, so... */\n@@ -543,9 +555,18 @@ static void *get_data_from_pack(struct object_entry *obj)\n \n \tgit_inflate_end(&stream);\n \tfree(inbuf);\n+\tif (consume) {\n+\t\tfree(data);\n+\t\tdata = NULL;\n+\t}\n \treturn data;\n }\n \n+static void *get_data_from_pack(struct object_entry *obj)\n+{\n+\treturn unpack_data(obj, NULL, NULL);\n+}\n+\n static int compare_delta_bases(const union delta_base *base1,\n \t\t\t       const union delta_base *base2,\n \t\t\t       enum object_type type1,\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191984","messageId":"1337782191-10091-4-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 4/6] index-pack: use streaming interface for collision test on large blobs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:49Z","receivedAt":"2012-05-23T14:09:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n the use of sha1_object_info() instead of has_sha1_file() is the cause\n of 6/6 as it is called on non-existent objects too.\n\n builtin/index-pack.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++++---\n t/t5300-pack-object.sh |  5 +++\n 2 files changed, 83 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9129299..05b1d35 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -9,6 +9,7 @@\n #include \"progress.h\"\n #include \"fsck.h\"\n #include \"exec_cmd.h\"\n+#include \"streaming.h\"\n #include \"thread-utils.h\"\n \n static const char index_pack_usage[] =\n@@ -621,21 +622,94 @@ static void find_delta_children(const union delta_base *base,\n \t*last_index = last;\n }\n \n+struct compare_data {\n+\tstruct object_entry *entry;\n+\tstruct git_istream *st;\n+\tunsigned char *buf;\n+\tunsigned long buf_size;\n+};\n+\n+static int compare_objects(const unsigned char *buf, unsigned long size,\n+\t\t\t   void *cb_data)\n+{\n+\tstruct compare_data *data = cb_data;\n+\n+\tif (data->buf_size < size) {\n+\t\tfree(data->buf);\n+\t\tdata->buf = xmalloc(size);\n+\t\tdata->buf_size = size;\n+\t}\n+\n+\twhile (size) {\n+\t\tssize_t len = read_istream(data->st, data->buf, size);\n+\t\tif (len == 0)\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tif (len < 0)\n+\t\t\tdie(_(\"unable to read %s\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tif (memcmp(buf, data->buf, len))\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tsize -= len;\n+\t\tbuf += len;\n+\t}\n+\treturn 0;\n+}\n+\n+static int check_collison(struct object_entry *entry)\n+{\n+\tstruct compare_data data;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\tif (entry->size <= big_file_threshold || entry->type != OBJ_BLOB)\n+\t\treturn -1;\n+\n+\tmemset(&data, 0, sizeof(data));\n+\tdata.entry = entry;\n+\tdata.st = open_istream(entry->idx.sha1, &type, &size, NULL);\n+\tif (!data.st)\n+\t\treturn -1;\n+\tif (size != entry->size || type != entry->type)\n+\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t    sha1_to_hex(entry->idx.sha1));\n+\tunpack_data(entry, compare_objects, &data);\n+\tclose_istream(data.st);\n+\tfree(data.buf);\n+\treturn 0;\n+}\n+\n static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tunsigned long size, enum object_type type,\n \t\t\tconst unsigned char *sha1)\n {\n \tvoid *new_data = NULL;\n+\tint collision_test_needed = 1;\n+\tenum object_type has_type;\n+\tunsigned long has_size;\n \n \tassert(data || obj_entry);\n \n \tread_lock();\n-\tif (has_sha1_file(sha1)) {\n+\thas_type = sha1_object_info(sha1, &has_size);\n+\tread_unlock();\n+\n+\tif (has_type < 0)\n+\t\tcollision_test_needed = 0;\n+\tif (collision_test_needed && !data) {\n+\t\tread_lock();\n+\t\tif (!check_collison(obj_entry))\n+\t\t\tcollision_test_needed = 0;\n+\t\tread_unlock();\n+\t}\n+\tif (collision_test_needed) {\n \t\tvoid *has_data;\n-\t\tenum object_type has_type;\n-\t\tunsigned long has_size;\n+\t\tif (has_type != type || has_size != size)\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"), sha1_to_hex(sha1));\n \t\tif (!data)\n \t\t\tdata = new_data = get_data_from_pack(obj_entry);\n+\t\tread_lock();\n \t\thas_data = read_sha1_file(sha1, &has_type, &has_size);\n \t\tread_unlock();\n \t\tif (!has_data)\n@@ -644,8 +718,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t    memcmp(data, has_data, size) != 0)\n \t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"), sha1_to_hex(sha1));\n \t\tfree(has_data);\n-\t} else\n-\t\tread_unlock();\n+\t}\n \n \tif (strict) {\n \t\tread_lock();\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d9d856b..300ed91 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -418,4 +418,9 @@ test_expect_success \\\n     'test_must_fail git index-pack -o bad.idx test-3.pack 2>msg &&\n      grep \"SHA1 COLLISION FOUND\" msg'\n \n+test_expect_success \\\n+    'make sure index-pack detects the SHA1 collision (large blobs)' \\\n+    'test_must_fail git -c core.bigfilethreshold=1 index-pack -o bad.idx test-3.pack 2>msg &&\n+     grep \"SHA1 COLLISION FOUND\" msg'\n+\n test_done\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191985","messageId":"1337782191-10091-5-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 5/6] index-pack: avoid collision test when verifying in-repo pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:50Z","receivedAt":"2012-05-23T14:09:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n not part of the original goal, but while i'm touching this code..\n\n builtin/index-pack.c | 31 +++++++++++++++++++++++++++++--\n 1 file changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 05b1d35..641edb5 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -78,6 +78,7 @@ static int nr_threads;\n static int from_stdin;\n static int strict;\n static int verbose;\n+static int verify;\n \n static struct progress *progress;\n \n@@ -688,11 +689,13 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \tint collision_test_needed = 1;\n \tenum object_type has_type;\n \tunsigned long has_size;\n+\tstruct object_info oi;\n \n \tassert(data || obj_entry);\n \n+\toi.sizep = &has_size;\n \tread_lock();\n-\thas_type = sha1_object_info(sha1, &has_size);\n+\thas_type = sha1_object_info_extended(sha1, &oi);\n \tread_unlock();\n \n \tif (has_type < 0)\n@@ -703,6 +706,30 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tcollision_test_needed = 0;\n \t\tread_unlock();\n \t}\n+\tread_lock();\n+\tif (collision_test_needed && oi.whence == OI_PACKED &&\n+\t    verify && !from_stdin &&\n+\t    is_pack_valid(oi.u.packed.pack)) {\n+\t\tstatic int good_stats = -1;\n+\t\tstatic struct stat st1, st2;\n+\n+\t\tgood_stats = good_stats == -1 &&\n+\t\t\tfstat_is_reliable() &&\n+\t\t\t!fstat(pack_fd, &st2) &&\n+\t\t\t!stat(oi.u.packed.pack->pack_name, &st1);\n+\t\tread_unlock();\n+\n+\t\t/*\n+\t\t * Windows builds do not support inode info. Inode\n+\t\t * comparison would be wrong. Luckily in all Windows\n+\t\t * builds set UNRELIABLE_FSTAT and good_stats would\n+\t\t * always be 0, so we never get to inode comparison\n+\t\t * test.\n+\t\t */\n+\t\tif (good_stats && st1.st_ino == st2.st_ino)\n+\t\t\tcollision_test_needed = 0;\n+\t} else\n+\t\tread_unlock();\n \tif (collision_test_needed) {\n \t\tvoid *has_data;\n \t\tif (has_type != type || has_size != size)\n@@ -1459,7 +1486,7 @@ static void show_pack_info(int stat_only)\n \n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, stat = 0;\n+\tint i, fix_thin_pack = 0, stat_only = 0, stat = 0;\n \tconst char *curr_pack, *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191986","messageId":"1337782191-10091-6-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 6/6] sha1_loose_object_info: do not complain out loud on non-existent objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:09:51Z","receivedAt":"2012-05-23T14:09:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n makes sense to me, but I might have overlooked something\n\n sha1_file.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3c4f165..520b41e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2133,6 +2133,8 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n \tgit_zstream stream;\n \tchar hdr[32];\n \n+\tif (!has_loose_object(sha1))\n+\t\treturn -1;\n \tmap = map_sha1_file(sha1, &mapsize);\n \tif (!map)\n \t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n-- \n1.7.10.2.549.g9354186\n"},{"id":"191987","messageId":"CACsJy8A5Xv8_KY2z4eQvm2e=3e=KjDJyVM+-d0dmXntHJ-6efg@mail.gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 6/6] sha1_loose_object_info: do not complain out loud on non-existent objects","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-23T14:24:12Z","receivedAt":"2012-05-23T14:24:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 23, 2012 at 9:09 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  makes sense to me, but I might have overlooked something\n\nwhile it's still making sense for me, i think it's more logical  to\nmove the check to the caller, where \"entry in pack?\" check is also\ndone.\n\n>\n>  sha1_file.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 3c4f165..520b41e 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2133,6 +2133,8 @@ static int sha1_loose_object_info(const unsigned char *sha1, unsigned long *size\n>        git_zstream stream;\n>        char hdr[32];\n>\n> +       if (!has_loose_object(sha1))\n> +               return -1;\n>        map = map_sha1_file(sha1, &mapsize);\n>        if (!map)\n>                return error(\"unable to find %s\", sha1_to_hex(sha1));\n> --\n> 1.7.10.2.549.g9354186\n>\n\n\n\n-- \nDuy\n"},{"id":"191997","messageId":"7vvcjm28ff.fsf@alter.siamese.dyndns.org","threadId":"30536","inReplyTo":"CACsJy8A5Xv8_KY2z4eQvm2e=3e=KjDJyVM+-d0dmXntHJ-6efg@mail.gmail.com","subject":"Re: [PATCH v2 6/6] sha1_loose_object_info: do not complain out loud on non-existent objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T16:01:24Z","receivedAt":"2012-05-23T16:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Wed, May 23, 2012 at 9:09 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>> makes sense to me, but I might have overlooked something\n>\n> while it's still making sense for me, i think it's more logical  to\n> move the check to the caller, where \"entry in pack?\" check is also\n> done.\n\nI think most of the callers of sha1_object_info_extended() are using this\nfunction, saying \"We expect this object to exist somewhere, perhaps in\npack or perhaps in a loose form, and trying to see what it is\", and they\nrely on the first error message \"unable to find\" to be issued.\n\nSo in that sense, I do not see how this patch makes any sense at all.\nCare to point out a codepath where we throw a random 20 bytes at it in\norder to see if an object with the given object name exists?  That would\nbe the only case where \"unable to find\" might be an unwanted error\nmessage.\n"},{"id":"191998","messageId":"7vr4ua28bd.fsf@alter.siamese.dyndns.org","threadId":"30536","inReplyTo":"1337782191-10091-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 4/6] index-pack: use streaming interface for collision test on large blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T16:03:50Z","receivedAt":"2012-05-23T16:03:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  the use of sha1_object_info() instead of has_sha1_file() is the cause\n>  of 6/6 as it is called on non-existent objects too.\n\nI think it is wrong to call object-info before knowing if it even exists.\n"},{"id":"192065","messageId":"CACsJy8CpqR4WGoYqNpUESigqTgHr0cL4gK2t7sCcb8P5StX3-w@mail.gmail.com","threadId":"30536","inReplyTo":"7vvcjm28ff.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 6/6] sha1_loose_object_info: do not complain out loud on non-existent objects","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-24T13:12:30Z","receivedAt":"2012-05-24T13:12:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 23, 2012 at 11:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> while it's still making sense for me, i think it's more logical  to\n>> move the check to the caller, where \"entry in pack?\" check is also\n>> done.\n>\n> I think most of the callers of sha1_object_info_extended() are using this\n> function, saying \"We expect this object to exist somewhere, perhaps in\n> pack or perhaps in a loose form, and trying to see what it is\", and they\n> rely on the first error message \"unable to find\" to be issued.\n\nhmm.. if you see it from that angle, yes it makes sense\n\n> So in that sense, I do not see how this patch makes any sense at all.\n> Care to point out a codepath where we throw a random 20 bytes at it in\n> order to see if an object with the given object name exists?  That would\n> be the only case where \"unable to find\" might be an unwanted error\n> message.\n\npacked_delta_info(), fast-import (I think) and cat-file do not check\nfor object existence before calling this.\n-- \nDuy\n"},{"id":"192068","messageId":"1337867744-24704-1-git-send-email-pclouds@gmail.com","threadId":"30536","inReplyTo":"1337782191-10091-4-git-send-email-pclouds@gmail.com","subject":"[PATCH v2.1 4/6] index-pack: use streaming interface for collision test on large blobs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-24T13:55:44Z","receivedAt":"2012-05-24T13:55:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When putting whole objects in core is unavoidable, try match object\ntype and size first before actually inflating.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n The bypassing has_sha1_object() and doing sha1_object_info() directly mainly to\n help avoid unnecessary collision tesst when user run this command on\n an in-repo pack. But I realized there's no sure way to check if the\n given pack (especiall with --stdin) is in repo. So I'll drop 5/6 and\n 6/6. People just have to set GIT_DIR=/nowhere.\n\n builtin/index-pack.c   | 82 +++++++++++++++++++++++++++++++++++++++++++++++---\n t/t5300-pack-object.sh |  5 +++\n 2 files changed, 82 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9129299..8b5c1eb 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -9,6 +9,7 @@\n #include \"progress.h\"\n #include \"fsck.h\"\n #include \"exec_cmd.h\"\n+#include \"streaming.h\"\n #include \"thread-utils.h\"\n \n static const char index_pack_usage[] =\n@@ -621,31 +622,102 @@ static void find_delta_children(const union delta_base *base,\n \t*last_index = last;\n }\n \n+struct compare_data {\n+\tstruct object_entry *entry;\n+\tstruct git_istream *st;\n+\tunsigned char *buf;\n+\tunsigned long buf_size;\n+};\n+\n+static int compare_objects(const unsigned char *buf, unsigned long size,\n+\t\t\t   void *cb_data)\n+{\n+\tstruct compare_data *data = cb_data;\n+\n+\tif (data->buf_size < size) {\n+\t\tfree(data->buf);\n+\t\tdata->buf = xmalloc(size);\n+\t\tdata->buf_size = size;\n+\t}\n+\n+\twhile (size) {\n+\t\tssize_t len = read_istream(data->st, data->buf, size);\n+\t\tif (len == 0)\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tif (len < 0)\n+\t\t\tdie(_(\"unable to read %s\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tif (memcmp(buf, data->buf, len))\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t\t    sha1_to_hex(data->entry->idx.sha1));\n+\t\tsize -= len;\n+\t\tbuf += len;\n+\t}\n+\treturn 0;\n+}\n+\n+static int check_collison(struct object_entry *entry)\n+{\n+\tstruct compare_data data;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\tif (entry->size <= big_file_threshold || entry->type != OBJ_BLOB)\n+\t\treturn -1;\n+\n+\tmemset(&data, 0, sizeof(data));\n+\tdata.entry = entry;\n+\tdata.st = open_istream(entry->idx.sha1, &type, &size, NULL);\n+\tif (!data.st)\n+\t\treturn -1;\n+\tif (size != entry->size || type != entry->type)\n+\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"),\n+\t\t    sha1_to_hex(entry->idx.sha1));\n+\tunpack_data(entry, compare_objects, &data);\n+\tclose_istream(data.st);\n+\tfree(data.buf);\n+\treturn 0;\n+}\n+\n static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tunsigned long size, enum object_type type,\n \t\t\tconst unsigned char *sha1)\n {\n \tvoid *new_data = NULL;\n+\tint collision_test_needed;\n \n \tassert(data || obj_entry);\n \n \tread_lock();\n-\tif (has_sha1_file(sha1)) {\n+\tcollision_test_needed = has_sha1_file(sha1);\n+\tread_unlock();\n+\n+\tif (collision_test_needed && !data) {\n+\t\tread_lock();\n+\t\tif (!check_collison(obj_entry))\n+\t\t\tcollision_test_needed = 0;\n+\t\tread_unlock();\n+\t}\n+\tif (collision_test_needed) {\n \t\tvoid *has_data;\n \t\tenum object_type has_type;\n \t\tunsigned long has_size;\n-\t\tif (!data)\n-\t\t\tdata = new_data = get_data_from_pack(obj_entry);\n+\t\tread_lock();\n+\t\thas_type = sha1_object_info(sha1, &has_size);\n+\t\tif (has_type != type || has_size != size)\n+\t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"), sha1_to_hex(sha1));\n \t\thas_data = read_sha1_file(sha1, &has_type, &has_size);\n \t\tread_unlock();\n+\t\tif (!data)\n+\t\t\tdata = new_data = get_data_from_pack(obj_entry);\n \t\tif (!has_data)\n \t\t\tdie(_(\"cannot read existing object %s\"), sha1_to_hex(sha1));\n \t\tif (size != has_size || type != has_type ||\n \t\t    memcmp(data, has_data, size) != 0)\n \t\t\tdie(_(\"SHA1 COLLISION FOUND WITH %s !\"), sha1_to_hex(sha1));\n \t\tfree(has_data);\n-\t} else\n-\t\tread_unlock();\n+\t}\n \n \tif (strict) {\n \t\tread_lock();\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d9d856b..300ed91 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -418,4 +418,9 @@ test_expect_success \\\n     'test_must_fail git index-pack -o bad.idx test-3.pack 2>msg &&\n      grep \"SHA1 COLLISION FOUND\" msg'\n \n+test_expect_success \\\n+    'make sure index-pack detects the SHA1 collision (large blobs)' \\\n+    'test_must_fail git -c core.bigfilethreshold=1 index-pack -o bad.idx test-3.pack 2>msg &&\n+     grep \"SHA1 COLLISION FOUND\" msg'\n+\n test_done\n-- \n1.7.10.2.549.g9354186\n"}]}