{"thread":{"id":"12354","subject":"[PATCH 0/4] enhancements to 'git verify-pack'","startedAt":"2008-02-28T05:25:16Z","lastAt":"2008-03-01T20:25:34Z","messageCount":8,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"70209","messageId":"1204176320-31358-1-git-send-email-nico@cam.org","threadId":"12354","inReplyTo":null,"subject":"[PATCH 0/4] enhancements to 'git verify-pack'","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-28T05:25:16Z","receivedAt":"2008-02-28T05:25:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"The following patches fix a long standing limitation about OBJ_OFS_DELTA\nobjects and verify-pack.\n\n\nNicolas\n"},{"id":"70211","messageId":"1204176320-31358-2-git-send-email-nico@cam.org","threadId":"12354","inReplyTo":"1204176320-31358-1-git-send-email-nico@cam.org","subject":"[PATCH 1/4] factorize revindex code out of builtin-pack-objects.c","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-28T05:25:17Z","receivedAt":"2008-02-28T05:25:17Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"No functional change. This is needed to fix verify-pack in a later patch.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n Makefile               |    5 +-\n builtin-pack-objects.c |  160 +++---------------------------------------------\n pack-revindex.c        |  142 ++++++++++++++++++++++++++++++++++++++++++\n pack-revindex.h        |   12 ++++\n 4 files changed, 167 insertions(+), 152 deletions(-)\n create mode 100644 pack-revindex.c\n create mode 100644 pack-revindex.h\n\ndiff --git a/Makefile b/Makefile\nindex 149343c..530427c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -304,7 +304,8 @@ LIB_H = \\\n \trun-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \\\n \ttree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \\\n \tutf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \\\n-\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h\n+\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h \\\n+\tll-merge.h pack-revindex.h\n \n DIFF_OBJS = \\\n \tdiff.o diff-lib.o diffcore-break.o diffcore-order.o \\\n@@ -328,7 +329,7 @@ LIB_OBJS = \\\n \tcolor.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \\\n \tconvert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \\\n \ttransport.o bundle.o walker.o parse-options.o ws.o archive.o branch.o \\\n-\tll-merge.o alias.o\n+\tll-merge.o alias.o pack-revindex.o\n \n BUILTIN_OBJS = \\\n \tbuiltin-add.o \\\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 1bba6e6..c7834bb 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -8,6 +8,7 @@\n #include \"tree.h\"\n #include \"delta.h\"\n #include \"pack.h\"\n+#include \"pack-revindex.h\"\n #include \"csum-file.h\"\n #include \"tree-walk.h\"\n #include \"diff.h\"\n@@ -93,157 +94,11 @@ static int *object_ix;\n static int object_ix_hashsz;\n \n /*\n- * Pack index for existing packs give us easy access to the offsets into\n- * corresponding pack file where each object's data starts, but the entries\n- * do not store the size of the compressed representation (uncompressed\n- * size is easily available by examining the pack entry header).  It is\n- * also rather expensive to find the sha1 for an object given its offset.\n- *\n- * We build a hashtable of existing packs (pack_revindex), and keep reverse\n- * index here -- pack index file is sorted by object name mapping to offset;\n- * this pack_revindex[].revindex array is a list of offset/index_nr pairs\n- * ordered by offset, so if you know the offset of an object, next offset\n- * is where its packed representation ends and the index_nr can be used to\n- * get the object sha1 from the main index.\n- */\n-struct revindex_entry {\n-\toff_t offset;\n-\tunsigned int nr;\n-};\n-struct pack_revindex {\n-\tstruct packed_git *p;\n-\tstruct revindex_entry *revindex;\n-};\n-static struct  pack_revindex *pack_revindex;\n-static int pack_revindex_hashsz;\n-\n-/*\n  * stats\n  */\n static uint32_t written, written_delta;\n static uint32_t reused, reused_delta;\n \n-static int pack_revindex_ix(struct packed_git *p)\n-{\n-\tunsigned long ui = (unsigned long)p;\n-\tint i;\n-\n-\tui = ui ^ (ui >> 16); /* defeat structure alignment */\n-\ti = (int)(ui % pack_revindex_hashsz);\n-\twhile (pack_revindex[i].p) {\n-\t\tif (pack_revindex[i].p == p)\n-\t\t\treturn i;\n-\t\tif (++i == pack_revindex_hashsz)\n-\t\t\ti = 0;\n-\t}\n-\treturn -1 - i;\n-}\n-\n-static void prepare_pack_ix(void)\n-{\n-\tint num;\n-\tstruct packed_git *p;\n-\tfor (num = 0, p = packed_git; p; p = p->next)\n-\t\tnum++;\n-\tif (!num)\n-\t\treturn;\n-\tpack_revindex_hashsz = num * 11;\n-\tpack_revindex = xcalloc(sizeof(*pack_revindex), pack_revindex_hashsz);\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tnum = pack_revindex_ix(p);\n-\t\tnum = - 1 - num;\n-\t\tpack_revindex[num].p = p;\n-\t}\n-\t/* revindex elements are lazily initialized */\n-}\n-\n-static int cmp_offset(const void *a_, const void *b_)\n-{\n-\tconst struct revindex_entry *a = a_;\n-\tconst struct revindex_entry *b = b_;\n-\treturn (a->offset < b->offset) ? -1 : (a->offset > b->offset) ? 1 : 0;\n-}\n-\n-/*\n- * Ordered list of offsets of objects in the pack.\n- */\n-static void prepare_pack_revindex(struct pack_revindex *rix)\n-{\n-\tstruct packed_git *p = rix->p;\n-\tint num_ent = p->num_objects;\n-\tint i;\n-\tconst char *index = p->index_data;\n-\n-\trix->revindex = xmalloc(sizeof(*rix->revindex) * (num_ent + 1));\n-\tindex += 4 * 256;\n-\n-\tif (p->index_version > 1) {\n-\t\tconst uint32_t *off_32 =\n-\t\t\t(uint32_t *)(index + 8 + p->num_objects * (20 + 4));\n-\t\tconst uint32_t *off_64 = off_32 + p->num_objects;\n-\t\tfor (i = 0; i < num_ent; i++) {\n-\t\t\tuint32_t off = ntohl(*off_32++);\n-\t\t\tif (!(off & 0x80000000)) {\n-\t\t\t\trix->revindex[i].offset = off;\n-\t\t\t} else {\n-\t\t\t\trix->revindex[i].offset =\n-\t\t\t\t\t((uint64_t)ntohl(*off_64++)) << 32;\n-\t\t\t\trix->revindex[i].offset |=\n-\t\t\t\t\tntohl(*off_64++);\n-\t\t\t}\n-\t\t\trix->revindex[i].nr = i;\n-\t\t}\n-\t} else {\n-\t\tfor (i = 0; i < num_ent; i++) {\n-\t\t\tuint32_t hl = *((uint32_t *)(index + 24 * i));\n-\t\t\trix->revindex[i].offset = ntohl(hl);\n-\t\t\trix->revindex[i].nr = i;\n-\t\t}\n-\t}\n-\n-\t/* This knows the pack format -- the 20-byte trailer\n-\t * follows immediately after the last object data.\n-\t */\n-\trix->revindex[num_ent].offset = p->pack_size - 20;\n-\trix->revindex[num_ent].nr = -1;\n-\tqsort(rix->revindex, num_ent, sizeof(*rix->revindex), cmp_offset);\n-}\n-\n-static struct revindex_entry * find_packed_object(struct packed_git *p,\n-\t\t\t\t\t\t  off_t ofs)\n-{\n-\tint num;\n-\tint lo, hi;\n-\tstruct pack_revindex *rix;\n-\tstruct revindex_entry *revindex;\n-\tnum = pack_revindex_ix(p);\n-\tif (num < 0)\n-\t\tdie(\"internal error: pack revindex uninitialized\");\n-\trix = &pack_revindex[num];\n-\tif (!rix->revindex)\n-\t\tprepare_pack_revindex(rix);\n-\trevindex = rix->revindex;\n-\tlo = 0;\n-\thi = p->num_objects + 1;\n-\tdo {\n-\t\tint mi = (lo + hi) / 2;\n-\t\tif (revindex[mi].offset == ofs) {\n-\t\t\treturn revindex + mi;\n-\t\t}\n-\t\telse if (ofs < revindex[mi].offset)\n-\t\t\thi = mi;\n-\t\telse\n-\t\t\tlo = mi + 1;\n-\t} while (lo < hi);\n-\tdie(\"internal error: pack revindex corrupt\");\n-}\n-\n-static const unsigned char *find_packed_object_name(struct packed_git *p,\n-\t\t\t\t\t\t    off_t ofs)\n-{\n-\tstruct revindex_entry *entry = find_packed_object(p, ofs);\n-\treturn nth_packed_object_sha1(p, entry->nr);\n-}\n \n static void *delta_against(void *buf, unsigned long size, struct object_entry *entry)\n {\n@@ -510,7 +365,7 @@ static unsigned long write_object(struct sha1file *f,\n \t\t}\n \t\thdrlen = encode_header(obj_type, entry->size, header);\n \t\toffset = entry->in_pack_offset;\n-\t\trevidx = find_packed_object(p, offset);\n+\t\trevidx = find_pack_revindex(p, offset);\n \t\tdatalen = revidx[1].offset - offset;\n \t\tif (!pack_to_stdout && p->index_version > 1 &&\n \t\t    check_pack_crc(p, &w_curs, offset, datalen, revidx->nr))\n@@ -1162,8 +1017,11 @@ static void check_object(struct object_entry *entry)\n \t\t\t\tdie(\"delta base offset out of bound for %s\",\n \t\t\t\t    sha1_to_hex(entry->idx.sha1));\n \t\t\tofs = entry->in_pack_offset - ofs;\n-\t\t\tif (!no_reuse_delta && !entry->preferred_base)\n-\t\t\t\tbase_ref = find_packed_object_name(p, ofs);\n+\t\t\tif (!no_reuse_delta && !entry->preferred_base) {\n+\t\t\t\tstruct revindex_entry *revidx;\n+\t\t\t\trevidx = find_pack_revindex(p, ofs);\n+\t\t\t\tbase_ref = nth_packed_object_sha1(p, revidx->nr);\n+\t\t\t}\n \t\t\tentry->in_pack_header_size = used + used_0;\n \t\t\tbreak;\n \t\t}\n@@ -1240,9 +1098,11 @@ static void get_object_details(void)\n \t\tsorted_by_offset[i] = objects + i;\n \tqsort(sorted_by_offset, nr_objects, sizeof(*sorted_by_offset), pack_offset_sort);\n \n-\tprepare_pack_ix();\n+\tinit_pack_revindex();\n+\n \tfor (i = 0; i < nr_objects; i++)\n \t\tcheck_object(sorted_by_offset[i]);\n+\n \tfree(sorted_by_offset);\n }\n \ndiff --git a/pack-revindex.c b/pack-revindex.c\nnew file mode 100644\nindex 0000000..cde3f5b\n--- /dev/null\n+++ b/pack-revindex.c\n@@ -0,0 +1,142 @@\n+#include \"cache.h\"\n+#include \"pack-revindex.h\"\n+\n+/*\n+ * Pack index for existing packs give us easy access to the offsets into\n+ * corresponding pack file where each object's data starts, but the entries\n+ * do not store the size of the compressed representation (uncompressed\n+ * size is easily available by examining the pack entry header).  It is\n+ * also rather expensive to find the sha1 for an object given its offset.\n+ *\n+ * We build a hashtable of existing packs (pack_revindex), and keep reverse\n+ * index here -- pack index file is sorted by object name mapping to offset;\n+ * this pack_revindex[].revindex array is a list of offset/index_nr pairs\n+ * ordered by offset, so if you know the offset of an object, next offset\n+ * is where its packed representation ends and the index_nr can be used to\n+ * get the object sha1 from the main index.\n+ */\n+\n+struct pack_revindex {\n+\tstruct packed_git *p;\n+\tstruct revindex_entry *revindex;\n+};\n+\n+static struct  pack_revindex *pack_revindex;\n+static int pack_revindex_hashsz;\n+\n+static int pack_revindex_ix(struct packed_git *p)\n+{\n+\tunsigned long ui = (unsigned long)p;\n+\tint i;\n+\n+\tui = ui ^ (ui >> 16); /* defeat structure alignment */\n+\ti = (int)(ui % pack_revindex_hashsz);\n+\twhile (pack_revindex[i].p) {\n+\t\tif (pack_revindex[i].p == p)\n+\t\t\treturn i;\n+\t\tif (++i == pack_revindex_hashsz)\n+\t\t\ti = 0;\n+\t}\n+\treturn -1 - i;\n+}\n+\n+void init_pack_revindex(void)\n+{\n+\tint num;\n+\tstruct packed_git *p;\n+\n+\tfor (num = 0, p = packed_git; p; p = p->next)\n+\t\tnum++;\n+\tif (!num)\n+\t\treturn;\n+\tpack_revindex_hashsz = num * 11;\n+\tpack_revindex = xcalloc(sizeof(*pack_revindex), pack_revindex_hashsz);\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tnum = pack_revindex_ix(p);\n+\t\tnum = - 1 - num;\n+\t\tpack_revindex[num].p = p;\n+\t}\n+\t/* revindex elements are lazily initialized */\n+}\n+\n+static int cmp_offset(const void *a_, const void *b_)\n+{\n+\tconst struct revindex_entry *a = a_;\n+\tconst struct revindex_entry *b = b_;\n+\treturn (a->offset < b->offset) ? -1 : (a->offset > b->offset) ? 1 : 0;\n+}\n+\n+/*\n+ * Ordered list of offsets of objects in the pack.\n+ */\n+static void create_pack_revindex(struct pack_revindex *rix)\n+{\n+\tstruct packed_git *p = rix->p;\n+\tint num_ent = p->num_objects;\n+\tint i;\n+\tconst char *index = p->index_data;\n+\n+\trix->revindex = xmalloc(sizeof(*rix->revindex) * (num_ent + 1));\n+\tindex += 4 * 256;\n+\n+\tif (p->index_version > 1) {\n+\t\tconst uint32_t *off_32 =\n+\t\t\t(uint32_t *)(index + 8 + p->num_objects * (20 + 4));\n+\t\tconst uint32_t *off_64 = off_32 + p->num_objects;\n+\t\tfor (i = 0; i < num_ent; i++) {\n+\t\t\tuint32_t off = ntohl(*off_32++);\n+\t\t\tif (!(off & 0x80000000)) {\n+\t\t\t\trix->revindex[i].offset = off;\n+\t\t\t} else {\n+\t\t\t\trix->revindex[i].offset =\n+\t\t\t\t\t((uint64_t)ntohl(*off_64++)) << 32;\n+\t\t\t\trix->revindex[i].offset |=\n+\t\t\t\t\tntohl(*off_64++);\n+\t\t\t}\n+\t\t\trix->revindex[i].nr = i;\n+\t\t}\n+\t} else {\n+\t\tfor (i = 0; i < num_ent; i++) {\n+\t\t\tuint32_t hl = *((uint32_t *)(index + 24 * i));\n+\t\t\trix->revindex[i].offset = ntohl(hl);\n+\t\t\trix->revindex[i].nr = i;\n+\t\t}\n+\t}\n+\n+\t/* This knows the pack format -- the 20-byte trailer\n+\t * follows immediately after the last object data.\n+\t */\n+\trix->revindex[num_ent].offset = p->pack_size - 20;\n+\trix->revindex[num_ent].nr = -1;\n+\tqsort(rix->revindex, num_ent, sizeof(*rix->revindex), cmp_offset);\n+}\n+\n+struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)\n+{\n+\tint num;\n+\tint lo, hi;\n+\tstruct pack_revindex *rix;\n+\tstruct revindex_entry *revindex;\n+\n+\tnum = pack_revindex_ix(p);\n+\tif (num < 0)\n+\t\tdie(\"internal error: pack revindex uninitialized\");\n+\n+\trix = &pack_revindex[num];\n+\tif (!rix->revindex)\n+\t\tcreate_pack_revindex(rix);\n+\trevindex = rix->revindex;\n+\n+\tlo = 0;\n+\thi = p->num_objects + 1;\n+\tdo {\n+\t\tint mi = (lo + hi) / 2;\n+\t\tif (revindex[mi].offset == ofs) {\n+\t\t\treturn revindex + mi;\n+\t\t} else if (ofs < revindex[mi].offset)\n+\t\t\thi = mi;\n+\t\telse\n+\t\t\tlo = mi + 1;\n+\t} while (lo < hi);\n+\tdie(\"internal error: pack revindex corrupt\");\n+}\ndiff --git a/pack-revindex.h b/pack-revindex.h\nnew file mode 100644\nindex 0000000..2c51af9\n--- /dev/null\n+++ b/pack-revindex.h\n@@ -0,0 +1,12 @@\n+#ifndef PACK_REVINDEX_H\n+#define PACK_REVINDEX_H\n+\n+struct revindex_entry {\n+\toff_t offset;\n+\tunsigned int nr;\n+};\n+\n+void init_pack_revindex(void);\n+struct revindex_entry * find_pack_revindex(struct packed_git *p, off_t ofs);\n+\n+#endif\n-- \n1.5.4.2.200.g99e75\n"},{"id":"70210","messageId":"1204176320-31358-3-git-send-email-nico@cam.org","threadId":"12354","inReplyTo":"1204176320-31358-2-git-send-email-nico@cam.org","subject":"[PATCH 2/4] make verify_one_pack() a bit less wrong wrt packed_git structure","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-28T05:25:18Z","receivedAt":"2008-02-28T05:25:18Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Simply freeing it is wrong.  There are many things attached to this\nstructure that are not cleaned up.  In practice this doesn't matter much\nsince this happens just before the program exits, but it is still\na bit more \"correct\" to leak it implicitly rather than explicitly.\n\nAnd therefore it is also a good idea to register it with\ninstall_packed_git().  Not only might it have better chance of being\nproperly cleaned up if such functionality is implemented for the general\ncase, but some functions like init_revindex() expect all packed_git\ninstances to be globally accessible.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-verify-pack.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-verify-pack.c b/builtin-verify-pack.c\nindex 4e31c27..4958bbb 100644\n--- a/builtin-verify-pack.c\n+++ b/builtin-verify-pack.c\n@@ -40,8 +40,8 @@ static int verify_one_pack(const char *path, int verbose)\n \tif (!pack)\n \t\treturn error(\"packfile %s not found.\", arg);\n \n+\tinstall_packed_git(pack);\n \terr = verify_pack(pack, verbose);\n-\tfree(pack);\n \n \treturn err;\n }\n-- \n1.5.4.2.200.g99e75\n"},{"id":"70213","messageId":"1204176320-31358-4-git-send-email-nico@cam.org","threadId":"12354","inReplyTo":"1204176320-31358-3-git-send-email-nico@cam.org","subject":"[PATCH 3/4] fix unimplemented packed_object_info_detail() features","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-28T05:25:19Z","receivedAt":"2008-02-28T05:25:19Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Since commit eb32d236df0c16b936b04f0c5402addb61cdb311, there was a TODO\ncomment in packed_object_info_detail() about the SHA1 of base object to\nOBJ_OFS_DELTA objects.  So here it is at last.\n\nWhile at it, providing the actual storage size information as well is now\ntrivial.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n pack-check.c |    3 +++\n sha1_file.c  |   10 +++++++---\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex d7dd62b..ae25685 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"pack.h\"\n+#include \"pack-revindex.h\"\n \n struct idx_entry\n {\n@@ -101,8 +102,10 @@ static int verify_packfile(struct packed_git *p,\n static void show_pack_info(struct packed_git *p)\n {\n \tuint32_t nr_objects, i, chain_histogram[MAX_CHAIN+1];\n+\n \tnr_objects = p->num_objects;\n \tmemset(chain_histogram, 0, sizeof(chain_histogram));\n+\tinit_pack_revindex();\n \n \tfor (i = 0; i < nr_objects; i++) {\n \t\tconst unsigned char *sha1;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1ddb96b..445a871 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -14,6 +14,7 @@\n #include \"tag.h\"\n #include \"tree.h\"\n #include \"refs.h\"\n+#include \"pack-revindex.h\"\n \n #ifndef O_NOATIME\n #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n@@ -1367,11 +1368,15 @@ const char *packed_object_info_detail(struct packed_git *p,\n \tunsigned long dummy;\n \tunsigned char *next_sha1;\n \tenum object_type type;\n+\tstruct revindex_entry *revidx;\n \n \t*delta_chain_length = 0;\n \tcurpos = obj_offset;\n \ttype = unpack_object_header(p, &w_curs, &curpos, size);\n \n+\trevidx = find_pack_revindex(p, obj_offset);\n+\t*store_size = revidx[1].offset - obj_offset;\n+\n \tfor (;;) {\n \t\tswitch (type) {\n \t\tdefault:\n@@ -1381,14 +1386,13 @@ const char *packed_object_info_detail(struct packed_git *p,\n \t\tcase OBJ_TREE:\n \t\tcase OBJ_BLOB:\n \t\tcase OBJ_TAG:\n-\t\t\t*store_size = 0; /* notyet */\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn typename(type);\n \t\tcase OBJ_OFS_DELTA:\n \t\t\tobj_offset = get_delta_base(p, &w_curs, &curpos, type, obj_offset);\n \t\t\tif (*delta_chain_length == 0) {\n-\t\t\t\t/* TODO: find base_sha1 as pointed by curpos */\n-\t\t\t\thashclr(base_sha1);\n+\t\t\t\trevidx = find_pack_revindex(p, obj_offset);\n+\t\t\t\thashcpy(base_sha1, nth_packed_object_sha1(p, revidx->nr));\n \t\t\t}\n \t\t\tbreak;\n \t\tcase OBJ_REF_DELTA:\n-- \n1.5.4.2.200.g99e75\n"},{"id":"70212","messageId":"1204176320-31358-5-git-send-email-nico@cam.org","threadId":"12354","inReplyTo":"1204176320-31358-4-git-send-email-nico@cam.org","subject":"[PATCH 4/4] add storage size output to 'git verify-pack -v'","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-28T05:25:20Z","receivedAt":"2008-02-28T05:25:20Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"This can possibly break external scripts that depend on the previous\noutput, but those script can't possibly be critical to Git usage, and\nfixing them should be trivial.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n Documentation/git-verify-pack.txt |    4 ++--\n contrib/stats/packinfo.pl         |    2 +-\n pack-check.c                      |    8 ++++----\n 3 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt\nindex db019a2..ba2a157 100644\n--- a/Documentation/git-verify-pack.txt\n+++ b/Documentation/git-verify-pack.txt\n@@ -32,11 +32,11 @@ OUTPUT FORMAT\n -------------\n When specifying the -v option the format used is:\n \n-\tSHA1 type size offset-in-packfile\n+\tSHA1 type size size-in-pack-file offset-in-packfile\n \n for objects that are not deltified in the pack, and\n \n-\tSHA1 type size offset-in-packfile depth base-SHA1\n+\tSHA1 type size size-in-packfile offset-in-packfile depth base-SHA1\n \n for objects that are deltified.\n \ndiff --git a/contrib/stats/packinfo.pl b/contrib/stats/packinfo.pl\nindex aab501e..f4a7b62 100755\n--- a/contrib/stats/packinfo.pl\n+++ b/contrib/stats/packinfo.pl\n@@ -93,7 +93,7 @@ my %depths;\n my @depths;\n \n while (<STDIN>) {\n-    my ($sha1, $type, $size, $offset, $depth, $parent) = split(/\\s+/, $_);\n+    my ($sha1, $type, $size, $space, $offset, $depth, $parent) = split(/\\s+/, $_);\n     next unless ($sha1 =~ /^[0-9a-f]{40}$/);\n     $depths{$sha1} = $depth || 0;\n     push(@depths, $depth || 0);\ndiff --git a/pack-check.c b/pack-check.c\nindex ae25685..0f8ad2c 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -128,11 +128,11 @@ static void show_pack_info(struct packed_git *p)\n \t\t\t\t\t\t base_sha1);\n \t\tprintf(\"%s \", sha1_to_hex(sha1));\n \t\tif (!delta_chain_length)\n-\t\t\tprintf(\"%-6s %lu %\"PRIuMAX\"\\n\",\n-\t\t\t       type, size, (uintmax_t)offset);\n+\t\t\tprintf(\"%-6s %lu %lu %\"PRIuMAX\"\\n\",\n+\t\t\t       type, size, store_size, (uintmax_t)offset);\n \t\telse {\n-\t\t\tprintf(\"%-6s %lu %\"PRIuMAX\" %u %s\\n\",\n-\t\t\t       type, size, (uintmax_t)offset,\n+\t\t\tprintf(\"%-6s %lu %lu %\"PRIuMAX\" %u %s\\n\",\n+\t\t\t       type, size, store_size, (uintmax_t)offset,\n \t\t\t       delta_chain_length, sha1_to_hex(base_sha1));\n \t\t\tif (delta_chain_length <= MAX_CHAIN)\n \t\t\t\tchain_histogram[delta_chain_length]++;\n-- \n1.5.4.2.200.g99e75\n"},{"id":"70477","messageId":"7vk5kn7znz.fsf@gitster.siamese.dyndns.org","threadId":"12354","inReplyTo":"1204176320-31358-3-git-send-email-nico@cam.org","subject":"Re: [PATCH 2/4] make verify_one_pack() a bit less wrong wrt packed_git structure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-01T05:59:12Z","receivedAt":"2008-03-01T05:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> ..., but it is still\n> a bit more \"correct\" to leak it implicitly rather than explicitly.\n\nI do not follow this logic to debate which incorrectness is more\ncorrect, but I do not mind the removal of free() there.\n\nI am not sure about the install_packed_git() piece, though.\n\nThis part of the code predates Shawn's windowed mmap and all\nother recent code improvements, but the original motivation of\nnot installing the pack was to make sure that codepaths outside\nverify_packfile() would not see the objects from the pack being\nverified at all.  IOW, the omission originally was intentional.\n\nI just quickly looked at verify_packfile() after applying your\nseries, and it seems that nothing tries to access objects with\nonly their SHA-1 names without explicitly telling which pack to\nread from, so it should still be safe even if we did not install\nthe packed git (iow, the normal codepath would not try to pick\nup objects from the suspect pack that is being validated).\n\nBut it made me feel a bit worried.\n\n\n"},{"id":"70498","messageId":"alpine.LFD.1.00.0803010305230.2899@xanadu.home","threadId":"12354","inReplyTo":"7vk5kn7znz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/4] make verify_one_pack() a bit less wrong wrt packed_git structure","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-03-01T08:18:49Z","receivedAt":"2008-03-01T08:18:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 29 Feb 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > ..., but it is still\n> > a bit more \"correct\" to leak it implicitly rather than explicitly.\n> \n> I do not follow this logic to debate which incorrectness is more\n> correct, but I do not mind the removal of free() there.\n\nFreeing the pack structure here without cleaning the resources it still \nholds is plain wrong.  Not freeing it might be a bit wrong as well, but \nit is inline with the rest of Git which relies on program termination to \nclean everything.\n\n> I am not sure about the install_packed_git() piece, though.\n> \n> This part of the code predates Shawn's windowed mmap and all\n> other recent code improvements, but the original motivation of\n> not installing the pack was to make sure that codepaths outside\n> verify_packfile() would not see the objects from the pack being\n> verified at all.  IOW, the omission originally was intentional.\n> \n> I just quickly looked at verify_packfile() after applying your\n> series, and it seems that nothing tries to access objects with\n> only their SHA-1 names without explicitly telling which pack to\n> read from, so it should still be safe even if we did not install\n> the packed git (iow, the normal codepath would not try to pick\n> up objects from the suspect pack that is being validated).\n> \n> But it made me feel a bit worried.\n\nThe problem is with the patch that follows, which calls init_revindex(). \nMaybe the following patch (on top of the other ones) should bring back \nthat isolation property for the actual verification pass, and \ninstall_packed_git() called only for the verbose output.\n\ndiff --git a/builtin-verify-pack.c b/builtin-verify-pack.c\nindex 4958bbb..cefa4d7 100644\n--- a/builtin-verify-pack.c\n+++ b/builtin-verify-pack.c\n@@ -40,7 +40,6 @@ static int verify_one_pack(const char *path, int verbose)\n \tif (!pack)\n \t\treturn error(\"packfile %s not found.\", arg);\n \n-\tinstall_packed_git(pack);\n \terr = verify_pack(pack, verbose);\n \n \treturn err;\ndiff --git a/pack-check.c b/pack-check.c\nindex 0f8ad2c..32176b2 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -105,6 +105,7 @@ static void show_pack_info(struct packed_git *p)\n \n \tnr_objects = p->num_objects;\n \tmemset(chain_histogram, 0, sizeof(chain_histogram));\n+\tinstall_packed_git(p);\n \tinit_pack_revindex();\n \n \tfor (i = 0; i < nr_objects; i++) {\n"},{"id":"70553","messageId":"7vwsomxkch.fsf@gitster.siamese.dyndns.org","threadId":"12354","inReplyTo":"alpine.LFD.1.00.0803010305230.2899@xanadu.home","subject":"Re: [PATCH 2/4] make verify_one_pack() a bit less wrong wrt packed_git structure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-01T20:25:34Z","receivedAt":"2008-03-01T20:25:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n>> I just quickly looked at verify_packfile() after applying your\n>> series, and it seems that nothing tries to access objects with\n>> only their SHA-1 names without explicitly telling which pack to\n>> read from, so it should still be safe even if we did not install\n>> the packed git (iow, the normal codepath would not try to pick\n>> up objects from the suspect pack that is being validated).\n>> \n>> But it made me feel a bit worried.\n>\n> The problem is with the patch that follows, which calls init_revindex(). \n\nOh, Ok.\n\nI see nobody other than pack-check calls packed_object_info_detail, and\nthat would be the only codepath that would load revindex in-core, so the\nchange would be fine.  I was worried about having to load revindex which\nwould not help anything but info-detail which is pretty much useless for\nmost operations.\n"}]}