{"thread":{"id":"26618","subject":"[PATCH 0/5] Using index-pack in place of verify-pack","startedAt":"2011-02-28T09:49:38Z","lastAt":"2011-02-28T19:50:02Z","messageCount":15,"participants":["Junio C Hamano","Sverre Rabbelier","Jeff King","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"162415","messageId":"1298886583-30965-1-git-send-email-gitster@pobox.com","threadId":"26618","inReplyTo":null,"subject":"[PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T09:49:38Z","receivedAt":"2011-02-28T09:49:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The verify-pack command walks objects in a pack for contents verification\nin a very naive way, namely, in the object name order. It is inefficient\ncompared to the way index-pack does, which verifies contents of the\nobjects in the order delta-chain is applied. As the result, the former\ncommand would end up reconstituting the same intermediate object in a\ndelta chain number of times while the latter would recreate an object only\nonce.\n\nThis series, which is still a WIP, is to teach index-pack \"--verify\"\noption, in order to eventually replace verify-pack with it.\n\nThe basic idea, which comes from Shawn Pearce, is to run index-pack on a\npackfile being verified, and compare the resulting .idx file with the\nexisting one, and make sure they match.\n\nJunio C Hamano (5):\n index-pack: group the delta-base array entries also by type\n write_idx_file: introduce a struct to hold idx customization options\n index-pack: --verify\n write_idx_file: need_large_offset() helper function\n index-pack --verify: read anomalous offsets from v2 idx file\n\n builtin/index-pack.c | 178 ++++++++++++++++++++++++++++++++++++++----------\n builtin/pack-objects.c | 20 +++---\n csum-file.c | 46 ++++++++++++-\n csum-file.h | 2 +\n fast-import.c | 10 ++-\n pack-write.c | 82 ++++++++++++++++------\n pack.h | 23 +++++-\n t/t5302-pack-index.sh | 18 +++++\n 8 files changed, 301 insertions(+), 78 deletions(-)\n"},{"id":"162416","messageId":"1298886583-30965-2-git-send-email-gitster@pobox.com","threadId":"26618","inReplyTo":"1298886583-30965-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/5] index-pack: group the delta-base array entries also by type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T09:49:39Z","receivedAt":"2011-02-28T09:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Entries in the delta_base array are only grouped by the bytepattern in\nthe delta_base union, some of which have 20-byte object name of the base\nobject (i.e. base for REF_DELTA objects), while others have sizeof(off_t)\nbytes followed by enough NULs to fill 20-byte. The loops to iterate\nthrough a range inside this array still needs to inspect the type of the\ndelta, and skip over false hits.\n\nGroup the entries also by type to eliminate the potential of false hits.\n\nSigned-off-by: Junio C Hamano \n---\n * This is a resend of an earlier patch we already discussed; it is not a\n wrong thing to do per-se, but is somewhat difficult to trigger. You\n would need to come up with an object name that consists of almost all\n NULs and other bytes that happen to be the same as an offset of some\n other object.\n\n builtin/index-pack.c | 61 ++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 40 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8dc5c0b..1b5d83a 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -391,7 +391,18 @@ static void *get_data_from_pack(struct object_entry *obj)\n \treturn data;\n }\n \n-static int find_delta(const union delta_base *base)\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+\t\t\t enum object_type type2)\n+{\n+\tint cmp = type1 - type2;\n+\tif (cmp)\n+\t\treturn cmp;\n+\treturn memcmp(base1, base2, UNION_BASE_SZ);\n+}\n+\n+static int find_delta(const union delta_base *base, enum object_type type)\n {\n \tint first = 0, last = nr_deltas;\n \n@@ -400,7 +411,8 @@ static int find_delta(const union delta_base *base)\n struct delta_entry *delta = &deltas[next];\n int cmp;\n \n- cmp = memcmp(base, &delta->base, UNION_BASE_SZ);\n+\t\tcmp = compare_delta_bases(base, &delta->base,\n+\t\t\t\t\t type, objects[delta->obj_no].type);\n if (!cmp)\n return next;\n if (cmp < 0) {\n@@ -413,9 +425,10 @@ static int find_delta(const union delta_base *base)\n }\n \n static void find_delta_children(const union delta_base *base,\n-\t\t\t\tint *first_index, int *last_index)\n+\t\t\t\tint *first_index, int *last_index,\n+\t\t\t\tenum object_type type)\n {\n-\tint first = find_delta(base);\n+\tint first = find_delta(base, type);\n \tint last = first;\n \tint end = nr_deltas - 1;\n \n@@ -543,11 +556,13 @@ static void find_unresolved_deltas(struct base_data *base,\n \t\tunion delta_base base_spec;\n \n \t\thashcpy(base_spec.sha1, base->obj->idx.sha1);\n-\t\tfind_delta_children(&base_spec, &ref_first, &ref_last);\n+\t\tfind_delta_children(&base_spec,\n+\t\t\t\t &ref_first, &ref_last, OBJ_REF_DELTA);\n \n \t\tmemset(&base_spec, 0, sizeof(base_spec));\n \t\tbase_spec.offset = base->obj->idx.offset;\n-\t\tfind_delta_children(&base_spec, &ofs_first, &ofs_last);\n+\t\tfind_delta_children(&base_spec,\n+\t\t\t\t &ofs_first, &ofs_last, OBJ_OFS_DELTA);\n \t}\n \n \tif (ref_last == -1 && ofs_last == -1) {\n@@ -559,24 +574,24 @@ static void find_unresolved_deltas(struct base_data *base,\n \n \tfor (i = ref_first; i <= ref_last; i++) {\n \t\tstruct object_entry *child = objects + deltas[i].obj_no;\n-\t\tif (child->real_type == OBJ_REF_DELTA) {\n-\t\t\tstruct base_data result;\n-\t\t\tresolve_delta(child, base, &result);\n-\t\t\tif (i == ref_last && ofs_last == -1)\n-\t\t\t\tfree_base_data(base);\n-\t\t\tfind_unresolved_deltas(&result, base);\n-\t\t}\n+\t\tstruct base_data result;\n+\n+\t\tassert(child->real_type == OBJ_REF_DELTA);\n+\t\tresolve_delta(child, base, &result);\n+\t\tif (i == ref_last && ofs_last == -1)\n+\t\t\tfree_base_data(base);\n+\t\tfind_unresolved_deltas(&result, base);\n \t}\n \n \tfor (i = ofs_first; i <= ofs_last; i++) {\n \t\tstruct object_entry *child = objects + deltas[i].obj_no;\n-\t\tif (child->real_type == OBJ_OFS_DELTA) {\n-\t\t\tstruct base_data result;\n-\t\t\tresolve_delta(child, base, &result);\n-\t\t\tif (i == ofs_last)\n-\t\t\t\tfree_base_data(base);\n-\t\t\tfind_unresolved_deltas(&result, base);\n-\t\t}\n+\t\tstruct base_data result;\n+\n+\t\tassert(child->real_type == OBJ_OFS_DELTA);\n+\t\tresolve_delta(child, base, &result);\n+\t\tif (i == ofs_last)\n+\t\t\tfree_base_data(base);\n+\t\tfind_unresolved_deltas(&result, base);\n \t}\n \n \tunlink_base_data(base);\n@@ -586,7 +601,11 @@ static int compare_delta_entry(const void *a, const void *b)\n {\n \tconst struct delta_entry *delta_a = a;\n \tconst struct delta_entry *delta_b = b;\n-\treturn memcmp(&delta_a->base, &delta_b->base, UNION_BASE_SZ);\n+\n+\t/* group by type (ref vs ofs) and then by value (sha-1 or offset) */\n+\treturn compare_delta_bases(&delta_a->base, &delta_b->base,\n+\t\t\t\t objects[delta_a->obj_no].type,\n+\t\t\t\t objects[delta_b->obj_no].type);\n }\n \n /* Parse all objects and return the pack content SHA1 hash */\n-- \n1.7.4.1.249.g4aa72\n"},{"id":"162417","messageId":"1298886583-30965-3-git-send-email-gitster@pobox.com","threadId":"26618","inReplyTo":"1298886583-30965-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/5] write_idx_file: introduce a struct to hold idx customization options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T09:49:40Z","receivedAt":"2011-02-28T09:49:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remove two globals, pack_idx_default version and pack_idx_off32_limit,\nand place them in a pack_idx_option structure. Allow callers to pass\nit to write_idx_file() as a parameter.\n\nAdjust all callers to the API change.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 23 +++++++++++++----------\n builtin/pack-objects.c | 20 +++++++++++---------\n fast-import.c | 10 ++++++----\n pack-write.c | 17 +++++++++++------\n pack.h | 11 +++++++----\n 5 files changed, 48 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1b5d83a..4df6818 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -880,11 +880,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \n static int git_index_pack_config(const char *k, const char *v, void *cb)\n {\n+\tstruct pack_idx_option *opts = cb;\n+\n \tif (!strcmp(k, \"pack.indexversion\")) {\n-\t\tpack_idx_default_version = git_config_int(k, v);\n-\t\tif (pack_idx_default_version > 2)\n-\t\t\tdie(\"bad pack.indexversion=%\"PRIu32,\n-\t\t\t\tpack_idx_default_version);\n+\t\topts->version = git_config_int(k, v);\n+\t\tif (opts->version > 2)\n+\t\t\tdie(\"bad pack.indexversion=%\"PRIu32, opts->version);\n \t\treturn 0;\n \t}\n \treturn git_default_config(k, v, cb);\n@@ -898,6 +899,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n \tstruct pack_idx_entry **idx_objects;\n+\tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -905,7 +907,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \n \tread_replace_refs = 0;\n \n-\tgit_config(git_index_pack_config, NULL);\n+\treset_pack_idx_option(&opts);\n+\tgit_config(git_index_pack_config, &opts);\n \tif (prefix && chdir(prefix))\n \t\tdie(\"Cannot come back to cwd\");\n \n@@ -944,12 +947,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tindex_name = argv[++i];\n \t\t\t} else if (!prefixcmp(arg, \"--index-version=\")) {\n \t\t\t\tchar *c;\n-\t\t\t\tpack_idx_default_version = strtoul(arg + 16, &c, 10);\n-\t\t\t\tif (pack_idx_default_version > 2)\n+\t\t\t\topts.version = strtoul(arg + 16, &c, 10);\n+\t\t\t\tif (opts.version > 2)\n \t\t\t\t\tdie(\"bad %s\", arg);\n \t\t\t\tif (*c == ',')\n-\t\t\t\t\tpack_idx_off32_limit = strtoul(c+1, &c, 0);\n-\t\t\t\tif (*c || pack_idx_off32_limit & 0x80000000)\n+\t\t\t\t\topts.off32_limit = strtoul(c+1, &c, 0);\n+\t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(\"bad %s\", arg);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n@@ -1032,7 +1035,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n \tfor (i = 0; i < nr_objects; i++)\n \t\tidx_objects[i] = &objects[i].idx;\n-\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, pack_sha1);\n+\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);\n \tfree(idx_objects);\n \n \tfinal(pack_name, curr_pack,\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b0503b2..dc471b7 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -70,6 +70,7 @@ static int local;\n static int incremental;\n static int ignore_packed_keep;\n static int allow_ofs_delta;\n+static struct pack_idx_option pack_idx_opts;\n static const char *base_name;\n static int progress = 1;\n static int window = 10;\n@@ -493,8 +494,8 @@ static void write_pack_file(void)\n \t\t\tconst char *idx_tmp_name;\n \t\t\tchar tmpname[PATH_MAX];\n \n-\t\t\tidx_tmp_name = write_idx_file(NULL, written_list,\n-\t\t\t\t\t\t nr_written, sha1);\n+\t\t\tidx_tmp_name = write_idx_file(NULL, written_list, nr_written,\n+\t\t\t\t\t\t &pack_idx_opts, sha1);\n \n \t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n \t\t\t\t base_name, sha1_to_hex(sha1));\n@@ -1880,10 +1881,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.indexversion\")) {\n-\t\tpack_idx_default_version = git_config_int(k, v);\n-\t\tif (pack_idx_default_version > 2)\n+\t\tpack_idx_opts.version = git_config_int(k, v);\n+\t\tif (pack_idx_opts.version > 2)\n \t\t\tdie(\"bad pack.indexversion=%\"PRIu32,\n-\t\t\t\tpack_idx_default_version);\n+\t\t\t pack_idx_opts.version);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.packsizelimit\")) {\n@@ -2130,6 +2131,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \trp_av[1] = \"--objects\"; /* --thin will make it --objects-edge */\n \trp_ac = 2;\n \n+\treset_pack_idx_option(&pack_idx_opts);\n \tgit_config(git_pack_config, NULL);\n \tif (!pack_compression_seen && core_compression_seen)\n \t\tpack_compression_level = core_compression_level;\n@@ -2274,12 +2276,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!prefixcmp(arg, \"--index-version=\")) {\n \t\t\tchar *c;\n-\t\t\tpack_idx_default_version = strtoul(arg + 16, &c, 10);\n-\t\t\tif (pack_idx_default_version > 2)\n+\t\t\tpack_idx_opts.version = strtoul(arg + 16, &c, 10);\n+\t\t\tif (pack_idx_opts.version > 2)\n \t\t\t\tdie(\"bad %s\", arg);\n \t\t\tif (*c == ',')\n-\t\t\t\tpack_idx_off32_limit = strtoul(c+1, &c, 0);\n-\t\t\tif (*c || pack_idx_off32_limit & 0x80000000)\n+\t\t\t\tpack_idx_opts.off32_limit = strtoul(c+1, &c, 0);\n+\t\t\tif (*c || pack_idx_opts.off32_limit & 0x80000000)\n \t\t\t\tdie(\"bad %s\", arg);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/fast-import.c b/fast-import.c\nindex 3886a1b..91e936d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -315,6 +315,7 @@ static unsigned int atom_cnt;\n static struct atom_str **atom_table;\n \n /* The .pack file being generated */\n+static struct pack_idx_option pack_idx_opts;\n static unsigned int pack_id;\n static struct sha1file *pack_file;\n static struct packed_git *pack_data;\n@@ -905,7 +906,7 @@ static const char *create_index(void)\n \tif (c != last)\n \t\tdie(\"internal consistency error creating the index\");\n \n-\ttmpfile = write_idx_file(NULL, idx, object_count, pack_data->sha1);\n+\ttmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);\n \tfree(idx);\n \treturn tmpfile;\n }\n@@ -3055,10 +3056,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.indexversion\")) {\n-\t\tpack_idx_default_version = git_config_int(k, v);\n-\t\tif (pack_idx_default_version > 2)\n+\t\tpack_idx_opts.version = git_config_int(k, v);\n+\t\tif (pack_idx_opts.version > 2)\n \t\t\tdie(\"bad pack.indexversion=%\"PRIu32,\n-\t\t\t pack_idx_default_version);\n+\t\t\t pack_idx_opts.version);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.packsizelimit\")) {\n@@ -3116,6 +3117,7 @@ int main(int argc, const char **argv)\n \t\tusage(fast_import_usage);\n \n \tsetup_git_directory();\n+\treset_pack_idx_option(&pack_idx_opts);\n \tgit_config(git_pack_config, NULL);\n \tif (!pack_compression_seen && core_compression_seen)\n \t\tpack_compression_level = core_compression_level;\ndiff --git a/pack-write.c b/pack-write.c\nindex a905ca4..f739a0f 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -2,8 +2,12 @@\n #include \"pack.h\"\n #include \"csum-file.h\"\n \n-uint32_t pack_idx_default_version = 2;\n-uint32_t pack_idx_off32_limit = 0x7fffffff;\n+void reset_pack_idx_option(struct pack_idx_option *opts)\n+{\n+\tmemset(opts, 0, sizeof(*opts));\n+\topts->version = 2;\n+\topts->off32_limit = 0x7fffffff;\n+}\n \n static int sha1_compare(const void *_a, const void *_b)\n {\n@@ -18,7 +22,8 @@ static int sha1_compare(const void *_a, const void *_b)\n * will be sorted by SHA1 on exit.\n */\n const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,\n-\t\t\t int nr_objects, unsigned char *sha1)\n+\t\t\t int nr_objects, const struct pack_idx_option *opts,\n+\t\t\t unsigned char *sha1)\n {\n \tstruct sha1file *f;\n \tstruct pack_idx_entry **sorted_by_sha, **list, **last;\n@@ -55,7 +60,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \tf = sha1fd(fd, index_name);\n \n \t/* if last object's offset is >= 2^31 we should use index V2 */\n-\tindex_version = (last_obj_offset >> 31) ? 2 : pack_idx_default_version;\n+\tindex_version = (last_obj_offset >> 31) ? 2 : opts->version;\n \n \t/* index versions 2 and above need a header */\n \tif (index_version >= 2) {\n@@ -115,7 +120,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\tlist = sorted_by_sha;\n \t\tfor (i = 0; i < nr_objects; i++) {\n \t\t\tstruct pack_idx_entry *obj = *list++;\n-\t\t\tuint32_t offset = (obj->offset <= pack_idx_off32_limit) ?\n+\t\t\tuint32_t offset = (obj->offset <= opts->off32_limit) ?\n \t\t\t\tobj->offset : (0x80000000 | nr_large_offset++);\n \t\t\toffset = htonl(offset);\n \t\t\tsha1write(f, &offset, 4);\n@@ -126,7 +131,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\twhile (nr_large_offset) {\n \t\t\tstruct pack_idx_entry *obj = *list++;\n \t\t\tuint64_t offset = obj->offset;\n-\t\t\tif (offset > pack_idx_off32_limit) {\n+\t\t\tif (offset > opts->off32_limit) {\n \t\t\t\tuint32_t split[2];\n \t\t\t\tsplit[0] = htonl(offset >> 32);\n \t\t\t\tsplit[1] = htonl(offset & 0xffffffff);\ndiff --git a/pack.h b/pack.h\nindex bb27576..953f57e 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -34,9 +34,12 @@ struct pack_header {\n */\n #define PACK_IDX_SIGNATURE 0xff744f63\t/* \"\\377tOc\" */\n \n-/* These may be overridden by command-line parameters */\n-extern uint32_t pack_idx_default_version;\n-extern uint32_t pack_idx_off32_limit;\n+struct pack_idx_option {\n+\tuint32_t version;\n+\tuint32_t off32_limit;\n+};\n+\n+extern void reset_pack_idx_option(struct pack_idx_option *);\n \n /*\n * Packed object index header\n@@ -55,7 +58,7 @@ struct pack_idx_entry {\n \toff_t offset;\n };\n \n-extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n+extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *);\n-- \n1.7.4.1.249.g4aa72\n"},{"id":"162418","messageId":"1298886583-30965-4-git-send-email-gitster@pobox.com","threadId":"26618","inReplyTo":"1298886583-30965-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/5] index-pack: --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T09:49:41Z","receivedAt":"2011-02-28T09:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Given an existing .pack file and the .idx file that describes it,\nthis new mode of operation reads and re-index the packfile and makes\nsure the existing .idx file matches the result byte-for-byte.\n\nAll the objects in the .pack file are validated during this operation as\nwell. Unlike verify-pack, which visits each object described in the .idx\nfile in the SHA-1 order, index-pack efficiently exploits the delta-chain\nto avoid rebuilding the objects that are used as the base of deltified\nobjects over and over again while validating the objects. This should\nresult in much quicker verification of the .pack file and its .idx file.\n\nThis version however cannot verify a .pack/.idx pair with a handcrafted v2\nindex that uses 64-bit offset representation for offsets that would fit\nwithin 31-bit. You can create such an .idx file by giving a custom offset\nto --index-version option to the command.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This does not make much of a difference in a small project like git.git\n itself, especially when the repository is packed with not very big depth\n value.\n\n builtin/index-pack.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n csum-file.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-\n csum-file.h | 2 ++\n pack-write.c | 26 ++++++++++++++++----------\n pack.h | 4 ++++\n t/t5302-pack-index.sh | 18 ++++++++++++++++++\n 6 files changed, 125 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 4df6818..24a9a16 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -11,7 +11,7 @@\n #include \"exec_cmd.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [ --keep | --keep= ] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry\n {\n@@ -891,9 +891,32 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)\n \treturn git_default_config(k, v, cb);\n }\n \n+static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)\n+{\n+\tstruct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1);\n+\n+\tif (!p)\n+\t\tdie(\"Cannot open existing pack file '%s'\", pack_name);\n+\tif (open_pack_index(p))\n+\t\tdie(\"Cannot open existing pack idx file for '%s'\", pack_name);\n+\n+\t/* Read the attributes from the existing idx file */\n+\topts->version = p->index_version;\n+\n+\t/*\n+\t * Get rid of the idx file as we do not need it anymore.\n+\t * NEEDSWORK: extract this bit from free_pack_by_name() in\n+\t * sha1_file.c, perhaps? It shouldn't matter very much as we\n+\t * know we haven't installed this pack (hence we never have\n+\t * read anything from it).\n+\t */\n+\tclose_pack_index(p);\n+\tfree(p);\n+}\n+\n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, fix_thin_pack = 0;\n+\tint i, fix_thin_pack = 0, verify = 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@@ -922,6 +945,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--strict\")) {\n \t\t\t\tstrict = 1;\n+\t\t\t} else if (!strcmp(arg, \"--verify\")) {\n+\t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--keep\")) {\n \t\t\t\tkeep_msg = \"\";\n \t\t\t} else if (!prefixcmp(arg, \"--keep=\")) {\n@@ -988,6 +1013,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tstrcpy(keep_name_buf + len - 5, \".keep\");\n \t\tkeep_name = keep_name_buf;\n \t}\n+\tif (verify) {\n+\t\tif (!index_name)\n+\t\t\tdie(\"--verify with no packfile name given\");\n+\t\tread_idx_option(&opts, index_name);\n+\t\topts.flags |= WRITE_IDX_VERIFY;\n+\t}\n \n \tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\n@@ -1038,10 +1069,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);\n \tfree(idx_objects);\n \n-\tfinal(pack_name, curr_pack,\n-\t\tindex_name, curr_index,\n-\t\tkeep_name, keep_msg,\n-\t\tpack_sha1);\n+\tif (!verify)\n+\t\tfinal(pack_name, curr_pack,\n+\t\t index_name, curr_index,\n+\t\t keep_name, keep_msg,\n+\t\t pack_sha1);\n+\telse\n+\t\tclose(input_fd);\n \tfree(objects);\n \tfree(index_name_buf);\n \tfree(keep_name_buf);\ndiff --git a/csum-file.c b/csum-file.c\nindex 4d50cc5..f70e3dd 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -11,8 +11,20 @@\n #include \"progress.h\"\n #include \"csum-file.h\"\n \n-static void flush(struct sha1file *f, void * buf, unsigned int count)\n+static void flush(struct sha1file *f, void *buf, unsigned int count)\n {\n+\tif (0 <= f->check_fd && count) {\n+\t\tunsigned char check_buffer[8192];\n+\t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n+\n+\t\tif (ret < 0)\n+\t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n+\t\tif (ret < count)\n+\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n+\t\tif (memcmp(buf, check_buffer, count))\n+\t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n+\t}\n+\n \tfor (;;) {\n \t\tint ret = xwrite(f->fd, buf, count);\n \t\tif (ret > 0) {\n@@ -59,6 +71,17 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n \t\tfd = 0;\n \t} else\n \t\tfd = f->fd;\n+\tif (0 <= f->check_fd) {\n+\t\tchar discard;\n+\t\tint cnt = read_in_full(f->check_fd, &discard, 1);\n+\t\tif (cnt < 0)\n+\t\t\tdie_errno(\"%s: error when reading the tail of sha1 file\",\n+\t\t\t\t f->name);\n+\t\tif (cnt)\n+\t\t\tdie(\"%s: sha1 file has trailing garbage\", f->name);\n+\t\tif (close(f->check_fd))\n+\t\t\tdie_errno(\"%s: sha1 file error on close\", f->name);\n+\t}\n \tfree(f);\n \treturn fd;\n }\n@@ -101,10 +124,31 @@ struct sha1file *sha1fd(int fd, const char *name)\n \treturn sha1fd_throughput(fd, name, NULL);\n }\n \n+struct sha1file *sha1fd_check(const char *name)\n+{\n+\tint sink, check;\n+\tstruct sha1file *f;\n+\n+\tsink = open(\"/dev/null\", O_WRONLY);\n+\tif (sink < 0)\n+\t\treturn NULL;\n+\tcheck = open(name, O_RDONLY);\n+\tif (check < 0) {\n+\t\tint saved_errno = errno;\n+\t\tclose(sink);\n+\t\terrno = saved_errno;\n+\t\treturn NULL;\n+\t}\n+\tf = sha1fd(sink, name);\n+\tf->check_fd = check;\n+\treturn f;\n+}\n+\n struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp)\n {\n \tstruct sha1file *f = xmalloc(sizeof(*f));\n \tf->fd = fd;\n+\tf->check_fd = -1;\n \tf->offset = 0;\n \tf->total = 0;\n \tf->tp = tp;\ndiff --git a/csum-file.h b/csum-file.h\nindex 294add2..6a7967c 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -6,6 +6,7 @@ struct progress;\n /* A SHA1-protected file */\n struct sha1file {\n \tint fd;\n+\tint check_fd;\n \tunsigned int offset;\n \tgit_SHA_CTX ctx;\n \toff_t total;\n@@ -21,6 +22,7 @@ struct sha1file {\n #define CSUM_FSYNC\t2\n \n extern struct sha1file *sha1fd(int fd, const char *name);\n+extern struct sha1file *sha1fd_check(const char *name);\n extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);\n extern int sha1close(struct sha1file *, unsigned char *, unsigned int);\n extern int sha1write(struct sha1file *, void *, unsigned int);\ndiff --git a/pack-write.c b/pack-write.c\nindex f739a0f..16529c3 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -47,17 +47,22 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \telse\n \t\tsorted_by_sha = list = last = NULL;\n \n-\tif (!index_name) {\n-\t\tstatic char tmpfile[PATH_MAX];\n-\t\tfd = odb_mkstemp(tmpfile, sizeof(tmpfile), \"pack/tmp_idx_XXXXXX\");\n-\t\tindex_name = xstrdup(tmpfile);\n+\tif (opts->flags & WRITE_IDX_VERIFY) {\n+\t\tassert(index_name);\n+\t\tf = sha1fd_check(index_name);\n \t} else {\n-\t\tunlink(index_name);\n-\t\tfd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\tif (!index_name) {\n+\t\t\tstatic char tmpfile[PATH_MAX];\n+\t\t\tfd = odb_mkstemp(tmpfile, sizeof(tmpfile), \"pack/tmp_idx_XXXXXX\");\n+\t\t\tindex_name = xstrdup(tmpfile);\n+\t\t} else {\n+\t\t\tunlink(index_name);\n+\t\t\tfd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\t}\n+\t\tif (fd < 0)\n+\t\t\tdie_errno(\"unable to create '%s'\", index_name);\n+\t\tf = sha1fd(fd, index_name);\n \t}\n-\tif (fd < 0)\n-\t\tdie_errno(\"unable to create '%s'\", index_name);\n-\tf = sha1fd(fd, index_name);\n \n \t/* if last object's offset is >= 2^31 we should use index V2 */\n \tindex_version = (last_obj_offset >> 31) ? 2 : opts->version;\n@@ -142,7 +147,8 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t}\n \n \tsha1write(f, sha1, 20);\n-\tsha1close(f, NULL, CSUM_FSYNC);\n+\tsha1close(f, NULL, ((opts->flags & WRITE_IDX_VERIFY)\n+\t\t\t ? CSUM_CLOSE : CSUM_FSYNC));\n \tgit_SHA1_Final(sha1, &ctx);\n \treturn index_name;\n }\ndiff --git a/pack.h b/pack.h\nindex 953f57e..dddafdd 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -35,6 +35,10 @@ struct pack_header {\n #define PACK_IDX_SIGNATURE 0xff744f63\t/* \"\\377tOc\" */\n \n struct pack_idx_option {\n+\tunsigned flags;\n+\t/* flag bits */\n+#define WRITE_IDX_VERIFY 01\n+\n \tuint32_t version;\n \tuint32_t off32_limit;\n };\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex b34ea93..7c5fa03 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -65,6 +65,14 @@ test_expect_success \\\n 'cmp \"test-1-${pack1}.idx\" \"1.idx\" &&\n cmp \"test-2-${pack2}.idx\" \"2.idx\"'\n \n+test_expect_success 'index-pack --verify on index version 1' '\n+\tgit index-pack --verify \"test-1-${pack1}.pack\"\n+'\n+\n+test_expect_success 'index-pack --verify on index version 2' '\n+\tgit index-pack --verify \"test-2-${pack2}.pack\"\n+'\n+\n test_expect_success \\\n 'index v2: force some 64-bit offsets with pack-objects' \\\n 'pack3=$(git pack-objects --index-version=2,0x40000 test-3 \n---\n\n * This incidentally \"fixes\" the initial determination of index_version.\n Earlier it didn't take the off32-limit given from the command line into\n account when inspecting the offset of the last object in the pack, but\n this version does. As off32-limit is primarily a debugging feature,\n this does not matter much, but as long as we are touching the\n surrounding code, it would be better to fix it.\n\n pack-write.c | 29 +++++++++++++++++++----------\n 1 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 16529c3..92e7eef 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -16,6 +16,11 @@ static int sha1_compare(const void *_a, const void *_b)\n \treturn hashcmp(a->sha1, b->sha1);\n }\n \n+static int need_large_offset(off_t offset, const struct pack_idx_option *opts)\n+{\n+\treturn (offset >> 31) || (opts->off32_limit < offset);\n+}\n+\n /*\n * On entry *sha1 contains the pack content SHA1 hash, on exit it is\n * the SHA1 hash of sorted object names. The objects array passed in\n@@ -65,7 +70,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t}\n \n \t/* if last object's offset is >= 2^31 we should use index V2 */\n-\tindex_version = (last_obj_offset >> 31) ? 2 : opts->version;\n+\tindex_version = need_large_offset(last_obj_offset, opts) ? 2 : opts->version;\n \n \t/* index versions 2 and above need a header */\n \tif (index_version >= 2) {\n@@ -125,8 +130,11 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\tlist = sorted_by_sha;\n \t\tfor (i = 0; i < nr_objects; i++) {\n \t\t\tstruct pack_idx_entry *obj = *list++;\n-\t\t\tuint32_t offset = (obj->offset <= opts->off32_limit) ?\n-\t\t\t\tobj->offset : (0x80000000 | nr_large_offset++);\n+\t\t\tuint32_t offset;\n+\n+\t\t\toffset = (need_large_offset(obj->offset, opts)\n+\t\t\t\t ? (0x80000000 | nr_large_offset++)\n+\t\t\t\t : obj->offset);\n \t\t\toffset = htonl(offset);\n \t\t\tsha1write(f, &offset, 4);\n \t\t}\n@@ -136,13 +144,14 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\twhile (nr_large_offset) {\n \t\t\tstruct pack_idx_entry *obj = *list++;\n \t\t\tuint64_t offset = obj->offset;\n-\t\t\tif (offset > opts->off32_limit) {\n-\t\t\t\tuint32_t split[2];\n-\t\t\t\tsplit[0] = htonl(offset >> 32);\n-\t\t\t\tsplit[1] = htonl(offset & 0xffffffff);\n-\t\t\t\tsha1write(f, split, 8);\n-\t\t\t\tnr_large_offset--;\n-\t\t\t}\n+\t\t\tuint32_t split[2];\n+\n+\t\t\tif (!need_large_offset(offset, opts))\n+\t\t\t\tcontinue;\n+\t\t\tsplit[0] = htonl(offset >> 32);\n+\t\t\tsplit[1] = htonl(offset & 0xffffffff);\n+\t\t\tsha1write(f, split, 8);\n+\t\t\tnr_large_offset--;\n \t\t}\n \t}\n \n-- \n1.7.4.1.249.g4aa72\n"},{"id":"162420","messageId":"1298886583-30965-6-git-send-email-gitster@pobox.com","threadId":"26618","inReplyTo":"1298886583-30965-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/5] index-pack --verify: read anomalous offsets from v2 idx file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T09:49:43Z","receivedAt":"2011-02-28T09:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A pack v2 .idx file usually records offset using 64-bit representation\nonly when the offset does not fit within 31-bit, but you can handcraft\nyour .idx file to record smaller offset using 64-bit, storing all zero\nin the upper 4-byte. By inspecting the original idx file when running\nindex-pack --verify, encode such low offsets that do not need to be in\n64-bit but are encoded using 64-bit just like the original idx file so\nthat we can still validate the pack/idx pair by comparing the idx file\nrecomputed with the original.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This \"fixes\" the shortcoming of 3/5 that breaks on a handcrafted .idx\n in a way that does not even depend on the off32_limit logic.\n\n builtin/index-pack.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n pack-write.c | 18 +++++++++++++++++-\n pack.h | 8 ++++++++\n t/t5302-pack-index.sh | 2 +-\n 4 files changed, 74 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 24a9a16..513fbbc 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -891,6 +891,51 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)\n \treturn git_default_config(k, v, cb);\n }\n \n+static int cmp_uint32(const void *a_, const void *b_)\n+{\n+\tuint32_t a = *((uint32_t *)a_);\n+\tuint32_t b = *((uint32_t *)b_);\n+\n+\treturn (a < b) ? -1 : (a != b);\n+}\n+\n+static void read_v2_anomalous_offsets(struct packed_git *p,\n+\t\t\t\t struct pack_idx_option *opts)\n+{\n+\tconst uint32_t *idx1, *idx2;\n+\tuint32_t i;\n+\n+\t/* The address of the 4-byte offset table */\n+\tidx1 = (((const uint32_t *)p->index_data)\n+\t\t+ 2 /* 8-byte header */\n+\t\t+ 256 /* fan out */\n+\t\t+ 5 * p->num_objects /* 20-byte SHA-1 table */\n+\t\t+ p->num_objects /* CRC32 table */\n+\t\t);\n+\n+\t/* The address of the 8-byte offset table */\n+\tidx2 = idx1 + p->num_objects;\n+\n+\tfor (i = 0; i < p->num_objects; i++) {\n+\t\tuint32_t off = ntohl(idx1[i]);\n+\t\tif (!(off & 0x80000000))\n+\t\t\tcontinue;\n+\t\toff = off & 0x7fffffff;\n+\t\tif (idx2[off * 2])\n+\t\t\tcontinue;\n+\t\t/*\n+\t\t * The real offset is ntohl(idx2[off * 2]) in high 4\n+\t\t * octets, and ntohl(idx2[off * 2 + 1]) in low 4\n+\t\t * octets. But idx2[off * 2] is Zero!!!\n+\t\t */\n+\t\tALLOC_GROW(opts->anomaly, opts->anomaly_nr + 1, opts->anomaly_alloc);\n+\t\topts->anomaly[opts->anomaly_nr++] = ntohl(idx2[off * 2 + 1]);\n+\t}\n+\n+\tif (1 < opts->anomaly_nr)\n+\t\tqsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);\n+}\n+\n static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)\n {\n \tstruct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1);\n@@ -903,6 +948,9 @@ static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)\n \t/* Read the attributes from the existing idx file */\n \topts->version = p->index_version;\n \n+\tif (opts->version == 2)\n+\t\tread_v2_anomalous_offsets(p, opts);\n+\n \t/*\n \t * Get rid of the idx file as we do not need it anymore.\n \t * NEEDSWORK: extract this bit from free_pack_by_name() in\ndiff --git a/pack-write.c b/pack-write.c\nindex 92e7eef..9cd3bfb 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -16,9 +16,25 @@ static int sha1_compare(const void *_a, const void *_b)\n \treturn hashcmp(a->sha1, b->sha1);\n }\n \n+static int cmp_uint32(const void *a_, const void *b_)\n+{\n+\tuint32_t a = *((uint32_t *)a_);\n+\tuint32_t b = *((uint32_t *)b_);\n+\n+\treturn (a < b) ? -1 : (a != b);\n+}\n+\n static int need_large_offset(off_t offset, const struct pack_idx_option *opts)\n {\n-\treturn (offset >> 31) || (opts->off32_limit < offset);\n+\tuint32_t ofsval;\n+\n+\tif ((offset >> 31) || (opts->off32_limit < offset))\n+\t\treturn 1;\n+\tif (!opts->anomaly_nr)\n+\t\treturn 0;\n+\tofsval = offset;\n+\treturn !!bsearch(&ofsval, opts->anomaly, opts->anomaly_nr,\n+\t\t\t sizeof(ofsval), cmp_uint32);\n }\n \n /*\ndiff --git a/pack.h b/pack.h\nindex dddafdd..722a54e 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -41,6 +41,14 @@ struct pack_idx_option {\n \n \tuint32_t version;\n \tuint32_t off32_limit;\n+\n+\t/*\n+\t * List of offsets that would fit within off32_limit but\n+\t * need to be written out as 64-bit entity for byte-for-byte\n+\t * verification.\n+\t */\n+\tint anomaly_alloc, anomaly_nr;\n+\tuint32_t *anomaly;\n };\n \n extern void reset_pack_idx_option(struct pack_idx_option *);\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 7c5fa03..76bcaca 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -107,7 +107,7 @@ test_expect_success OFF64_T 'index-pack --verify on 64-bit offset v2 (cheat)' '\n \tgit index-pack --verify --index-version=2,0x40000 \"test-3-${pack3}.pack\"\n '\n \n-test_expect_failure OFF64_T 'index-pack --verify on 64-bit offset v2' '\n+test_expect_success OFF64_T 'index-pack --verify on 64-bit offset v2' '\n \tgit index-pack --verify \"test-3-${pack3}.pack\"\n '\n \n-- \n1.7.4.1.249.g4aa72\n"},{"id":"162436","messageId":"AANLkTimrO6NqB0pXcfz=dAzA5fxz9ZizLyW7YV4Jan6S@mail.gmail.com","threadId":"26618","inReplyTo":"1298886583-30965-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-28T13:07:20Z","receivedAt":"2011-02-28T13:07:20Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Feb 28, 2011 at 10:49, Junio C Hamano wrote:\n> The basic idea, which comes from Shawn Pearce, is to run index-pack on a\n> packfile being verified, and compare the resulting .idx file with the\n> existing one, and make sure they match.\n\nDo you have any timings, on say, the kernel repo?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162447","messageId":"7vfwr8jfsl.fsf@alter.siamese.dyndns.org","threadId":"26618","inReplyTo":"AANLkTimrO6NqB0pXcfz=dAzA5fxz9ZizLyW7YV4Jan6S@mail.gmail.com","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T16:16:10Z","receivedAt":"2011-02-28T16:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Mon, Feb 28, 2011 at 10:49, Junio C Hamano wrote:\n>> The basic idea, which comes from Shawn Pearce, is to run index-pack on a\n>> packfile being verified, and compare the resulting .idx file with the\n>> existing one, and make sure they match.\n>\n> Do you have any timings, on say, the kernel repo?\n\nNot yet; the code has just become in good enough shape to be built and\nmeasured by anybody interested --- didn't I say WIP somewhere?\n"},{"id":"162449","messageId":"AANLkTikTPVf1fZiTxsmLe-MCWVQY4YBGen5Bj_Yn0SSz@mail.gmail.com","threadId":"26618","inReplyTo":"7vfwr8jfsl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-28T16:46:19Z","receivedAt":"2011-02-28T16:46:19Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Feb 28, 2011 at 17:16, Junio C Hamano wrote:\n> Not yet; the code has just become in good enough shape to be built and\n> measured by anybody interested --- didn't I say WIP somewhere?\n\nYes, but I figured that we'd want to see if it's worth pursuing at\nall, e.g., whether we should try and get this in decent shape or not?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162451","messageId":"20110228165827.GB24915@sigill.intra.peff.net","threadId":"26618","inReplyTo":"AANLkTikTPVf1fZiTxsmLe-MCWVQY4YBGen5Bj_Yn0SSz@mail.gmail.com","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-02-28T16:58:27Z","receivedAt":"2011-02-28T16:58:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 28, 2011 at 05:46:19PM +0100, Sverre Rabbelier wrote:\n\n> On Mon, Feb 28, 2011 at 17:16, Junio C Hamano wrote:\n> > Not yet; the code has just become in good enough shape to be built and\n> > measured by anybody interested --- didn't I say WIP somewhere?\n> \n> Yes, but I figured that we'd want to see if it's worth pursuing at\n> all, e.g., whether we should try and get this in decent shape or not?\n\nI'm not at all interested in this topic, but in the time it took the two\nof you to write your emails, I did this:\n\n $ cd linux-2.6\n\n $ time git verify-pack \\\n .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.idx\n real 2m37.338s\n user 2m35.874s\n sys 0m1.348s\n\n $ time git index-pack --verify \\\n .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.pack\n real 1m37.208s\n user 1m36.106s\n sys 0m1.048s\n\n\nOK, with the CPU time it probably took longer than your emails. But I\nate some ice cream while it computed. :)\n\n-Peff\n"},{"id":"162452","messageId":"AANLkTinCYnvhZ49HgTWVNObQimRVwb+K6Pu3djE2RBFN@mail.gmail.com","threadId":"26618","inReplyTo":"20110228165827.GB24915@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-02-28T17:01:32Z","receivedAt":"2011-02-28T17:01:32Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Feb 28, 2011 at 17:58, Jeff King wrote:\n>  $ time git verify-pack \\\n>      .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.idx\n>  real    2m37.338s\n>  user    2m35.874s\n>  sys     0m1.348s\n>\n>  $ time git index-pack --verify \\\n>      .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.pack\n>  real    1m37.208s\n>  user    1m36.106s\n>  sys     0m1.048s\n\nWow, impressive results, a full minute shaved off.\n\n> OK, with the CPU time it probably took longer than your emails. But I\n> ate some ice cream while it computed. :)\n\nYou lucky man ;)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"162453","messageId":"20110228172455.GA17278@spearce.org","threadId":"26618","inReplyTo":"20110228165827.GB24915@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T17:25:32Z","receivedAt":"2011-02-28T17:25:32Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jeff King wrote:\n> $ cd linux-2.6\n> \n> $ time git verify-pack \\\n> .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.idx\n> real 2m37.338s\n> user 2m35.874s\n> sys 0m1.348s\n> \n> $ time git index-pack --verify \\\n> .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.pack\n> real 1m37.208s\n> user 1m36.106s\n> sys 0m1.048s\n\nNot the improvement I had hoped. Your verify-pack ran much more\nquickly than the fsck I was seeing that led me to suggest this to\nJunio a few weeks ago. But saving a minute is still pretty good.\n\nThat index-pack run is basically what we get during clone, so\nI don't really expect to improve on that. (However I have been\ncontemplating that the locality within the pack isn't optimal and\nwe can do better placement.)\n\n-- \nShawn.\n"},{"id":"162456","messageId":"20110228174857.GB17278@spearce.org","threadId":"26618","inReplyTo":"1298886583-30965-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/5] index-pack: --verify","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-02-28T17:48:57Z","receivedAt":"2011-02-28T17:48:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Given an existing .pack file and the .idx file that describes it,\n> this new mode of operation reads and re-index the packfile and makes\n> sure the existing .idx file matches the result byte-for-byte.\n> \n> All the objects in the .pack file are validated during this operation as\n> well. Unlike verify-pack, which visits each object described in the .idx\n> file in the SHA-1 order, index-pack efficiently exploits the delta-chain\n> to avoid rebuilding the objects that are used as the base of deltified\n> objects over and over again while validating the objects. This should\n> result in much quicker verification of the .pack file and its .idx file.\n> \n> This version however cannot verify a .pack/.idx pair with a handcrafted v2\n> index that uses 64-bit offset representation for offsets that would fit\n> within 31-bit. You can create such an .idx file by giving a custom offset\n> to --index-version option to the command.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * This does not make much of a difference in a small project like git.git\n> itself, especially when the repository is packed with not very big depth\n> value.\n\nHowever it is useful on larger repositories, like linux-2.6. I'd like\nto see this series cleaned up and submitted, because as Peff's tests\nshows, it shaves 1 minute off the linux-2.6 fsck.\n\nSkim reading the code it mostly looked OK, though the NEEDSWORK\nstuff has to be cleaned up. And I wonder if the series cannot be\nflipped around a bit to put the 4/5 earlier and try to avoid a\nstage where `index-pack --verify` doesn't do the right thing on\nthe hand-rolled lower 32 bit index limit.\n\nIn this patch I'm not happy about csum-file having this check_fd\npart of its contents. But its probably the shortest way to inject\nvalidation into index-pack without butchering a large part of its\nindex generation function.\n\n-- \nShawn.\n"},{"id":"162462","messageId":"7v7hckhtwj.fsf@alter.siamese.dyndns.org","threadId":"26618","inReplyTo":"20110228174857.GB17278@spearce.org","subject":"Re: [PATCH 3/5] index-pack: --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T18:54:20Z","receivedAt":"2011-02-28T18:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Skim reading the code it mostly looked OK, though the NEEDSWORK\n> stuff has to be cleaned up.\n\nYeah, the part that needs working is about thinking if such a clean-up\n(i.e. extracting small piece of code from free-pack-by-name to make an\nofficial API function to free storage for a packed_git while closing the\npack_index and its associated resource) is necessary. There are other\nremaining tasks such as integrating this into verify_pack() function,\nwhich in turn requires to teach index-pack to produce the delta-chain\nhistograms (i.e. \"verify-pack --verbose\"). Also the codepath needs to be\ntaught about the plan for narrow clones---there are places that assume\neverything referred to from objects need to exist. These have higher\npriority than this NEEDSWORK comment.\n\n> ... And I wonder if the series cannot be\n> flipped around a bit to put the 4/5 earlier and try to avoid a\n> stage where `index-pack --verify` doesn't do the right thing on\n> the hand-rolled lower 32 bit index limit.\n\nOh, I fully agree that 4/5 should be squashed into this in the final\nround. I preserved the progression of thought in this WIP as it\nillustrated pitfalls to avoid rather nicely (iow, hoping to show others a\nBCP of building a series by example). The separation would also hopefully\nhave made the series easier to review.\n\n> In this patch I'm not happy about csum-file having this check_fd\n> part of its contents. But its probably the shortest way to inject\n> validation into index-pack without butchering a large part of its\n> index generation function.\n\nYeah, I am only 70% happy about the approach myself. Patches and\nimprovements are welcome, of course.\n"},{"id":"162465","messageId":"7v39n8hrbp.fsf@alter.siamese.dyndns.org","threadId":"26618","inReplyTo":"20110228165827.GB24915@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] Using index-pack in place of verify-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-28T19:50:02Z","receivedAt":"2011-02-28T19:50:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not at all interested in this topic, but in the time it took the two\n> of you to write your emails, I did this:\n>\n> $ cd linux-2.6\n>\n> $ time git verify-pack \\\n> .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.idx\n> real 2m37.338s\n> user 2m35.874s\n> sys 0m1.348s\n>\n> $ time git index-pack --verify \\\n> .git/objects/pack/pack-36c50f6082df404c26a699f2702946e0cce6208f.pack\n> real 1m37.208s\n> user 1m36.106s\n> sys 0m1.048s\n>\n>\n> OK, with the CPU time it probably took longer than your emails. But I\n> ate some ice cream while it computed. :)\n\nI guess my box is showing its age. It is 2m34s vs 3m25s for the kernel\nrepository packed with depth=50 (the default).\n\nA pack created with insanely deep depth of course shows larger difference\n(2m31s vs 4m38s for the same repository repacked with depth=8192).\n"}]}